Quoting Jackie Li (2018-02-09 01:03:53)
> On CNL A0 and Gen9, there's a hardware restriction that requires the
> available GuC WOPCM size to be larger than or equal to HuC firmware size.
>
> This patch adds new verfication code to ensure the available GuC WOPCM size
> to be larger than or equal to HuC firmware size on both Gen9 and CNL A0.
>
> v6:
> - Extended HuC FW size check against GuC WOPCM size to all
> Gen9 and CNL A0 platforms
>
> v7:
> - Fixed patch format issues
>
> v8:
> - Renamed variables and functions to avoid ambiguity (Joonas)
> - Updated commit message and comments to be more comprehensive (Sagar)
>
> v9:
> - Moved code that is not related to restriction check into a separate
> patch and updated the commit message accordingly (Sagar/Michal)
> - Avoided to call uc_get_fw_size for better layer isolation (Michal)
>
> Cc: Michal Wajdeczko <michal.wajdec...@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kam...@intel.com>
> Cc: John Spotswood <john.a.spotsw...@intel.com>
> Cc: Jeff McGee <jeff.mc...@intel.com>
> Cc: Chris Wilson <ch...@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahti...@linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kam...@intel.com> (v8)
> Signed-off-by: Jackie Li <yaodong...@intel.com>
<SNIP>
> -static inline int gen9_guc_wopcm_size_check(struct intel_guc_wopcm
> *guc_wopcm)
> +static inline int guc_wopcm_check_huc_fw_size(struct intel_guc_wopcm
> *guc_wopcm,
> + u32 huc_fw_size)
You can abbreviate here as there are local funcs, "check_huc_fw_fits" is
enough.
> +{
> + /*
> + * On Gen9 & CNL A0, hardware requires the total available GuC WOPCM
> + * size to be larger than or equal to HuC firmware size. Otherwise,
> + * firmware uploading would fail.
> + */
> + if (unlikely(guc_wopcm->size - GUC_WOPCM_RESERVED < huc_fw_size))
> + return -E2BIG;
Again, it is overkill to try to educate branch predictor when this is
called once during boot, just do the check without unlikely() wrapping
:)
> +
> + return 0;
> +}
> +
> +static inline int gen9_guc_wopcm_size_check(struct intel_guc_wopcm
> *guc_wopcm,
> + u32 huc_fw_size)
Abbreviate to gen9_check_dword_gap or something, don't bring the huc_fw_size
here.
> {
> u32 guc_wopcm_start;
> u32 delta;
> @@ -58,16 +73,19 @@ static inline int gen9_guc_wopcm_size_check(struct
> intel_guc_wopcm *guc_wopcm)
> if (unlikely(delta < sizeof(u32)))
> return -E2BIG;
>
> - return 0;
> + return guc_wopcm_check_huc_fw_size(guc_wopcm, huc_fw_size);
> }
>
> -static inline int guc_wopcm_size_check(struct intel_guc *guc)
> +static inline int guc_wopcm_size_check(struct intel_guc *guc, u32
> huc_fw_size)
> {
> struct drm_i915_private *i915 = guc_to_i915(guc);
> struct intel_guc_wopcm *guc_wopcm = &guc->wopcm;
int err = 0;
>
> if (IS_GEN9(i915))
> - return gen9_guc_wopcm_size_check(guc_wopcm);
> + return gen9_guc_wopcm_size_check(guc_wopcm, huc_fw_size);
if (..)
err = gen9_check_dword_gap(guc_wopcm);
if (err)
goto err;
> +
> + if (IS_CNL_REVID(i915, CNL_REVID_A0, CNL_REVID_A0))
> + return guc_wopcm_check_huc_fw_size(guc_wopcm, huc_fw_size);
>
if (IS_GEN9(i915) || IS_CNL_REVID(...))
err = check_huc_fw_fit(...)
out:
return err;
This will better isolate the "two bugs", and huc_fw_size only goes to one
of the checks, which makes the picture clearer.
> return 0;
> }
> @@ -131,7 +149,7 @@ int intel_guc_wopcm_init(struct intel_guc_wopcm
> *guc_wopcm, u32 guc_fw_size,
> guc->wopcm.top = top;
>
> /* Check platform specific restrictions */
> - err = guc_wopcm_size_check(guc);
> + err = guc_wopcm_size_check(guc, huc_fw_size);
s/guc_wopcm_size_check/size_checks/ as we're doing multiple (that's
the only information the comment carries). Then drop the comment.
Regards, Joonas
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
