Re: [PATCH v9 5/7] drm/i915/guc: Add HuC firmware size related restriction for Gen9 and CNL A0

[Joonas Lahtinen <joonas.lahtinen@linux.intel.com>]

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.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: John Spotswood <john.a.spotswood@intel.com>
> Cc: Jeff McGee <jeff.mcgee@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com> (v8)
> Signed-off-by: Jackie Li <yaodong.li@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