Re: [Intel-gfx] [PATCH v2 1/7] drm/i915/huc: only load HuC on GTs that have VCS engines

["Iddamsetty, Aravind" <aravind.iddamsetty@intel.com>]

On 13-10-2022 05:33, Daniele Ceraolo Spurio wrote:
> On MTL the primary GT doesn't have any media capabilities, so no video
> engines and no HuC. We must therefore skip the HuC fetch and load on
> that specific case. Given that other multi-GT platforms might have HuC
> on the primary GT, we can't just check for that and it is easier to
> instead check for the lack of VCS engines.
> 
> Based on code from Aravind Iddamsetty
> 
> v2: clarify which engine_mask is used for each GT and why (Tvrtko)
> 
> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Aravind Iddamsetty <aravind.iddamsetty@intel.com>
> Cc: John Harrison <john.c.harrison@intel.com>
> Cc: Alan Previn <alan.previn.teres.alexis@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Reviewed-by: Aravind Iddamsetty <aravind.iddamsetty@intel.com> #v1
> ---
>  drivers/gpu/drm/i915/gt/uc/intel_huc.c | 29 ++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h        |  9 +++++---
>  2 files changed, 35 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> index 4d1cc383b681..ca170ea3426c 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> @@ -203,12 +203,41 @@ void intel_huc_unregister_gsc_notifier(struct intel_huc *huc, struct bus_type *b
>  	huc->delayed_load.nb.notifier_call = NULL;
>  }
>  
> +static bool vcs_supported(struct intel_gt *gt)
> +{
> +	intel_engine_mask_t mask = gt->info.engine_mask;
> +
> +	/*
> +	 * We reach here from i915_driver_early_probe for the primary GT before
> +	 * its engine mask is set, so we use the device info engine mask for it;
> +	 * this means we're not taking VCS fusing into account, but if the
> +	 * primary GT supports VCS engines we expect at least one of them to
> +	 * remain unfused so we're fine.
> +	 * For other GTs we expect the GT-specific mask to be set before we
> +	 * call this function.
> +	 */
Comment sounds good to me. as the rest of the change is same as v1, You
can have my r-b for this.

Thanks,
Aravind.
> +	GEM_BUG_ON(!gt_is_root(gt) && !gt->info.engine_mask);
> +
> +	if (gt_is_root(gt))
> +		mask = RUNTIME_INFO(gt->i915)->platform_engine_mask;
> +	else
> +		mask = gt->info.engine_mask;
> +
> +	return __ENGINE_INSTANCES_MASK(mask, VCS0, I915_MAX_VCS);
> +}
> +
>  void intel_huc_init_early(struct intel_huc *huc)
>  {
>  	struct drm_i915_private *i915 = huc_to_gt(huc)->i915;
> +	struct intel_gt *gt = huc_to_gt(huc);
>  
>  	intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC);
>  
> +	if (!vcs_supported(gt)) {
> +		intel_uc_fw_change_status(&huc->fw, INTEL_UC_FIRMWARE_NOT_SUPPORTED);
> +		return;
> +	}
> +
>  	if (GRAPHICS_VER(i915) >= 11) {
>  		huc->status.reg = GEN11_HUC_KERNEL_LOAD_INFO;
>  		huc->status.mask = HUC_LOAD_SUCCESSFUL;
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 90ed8e6db2fe..90a347140e90 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -776,12 +776,15 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define __HAS_ENGINE(engine_mask, id) ((engine_mask) & BIT(id))
>  #define HAS_ENGINE(gt, id) __HAS_ENGINE((gt)->info.engine_mask, id)
>  
> -#define ENGINE_INSTANCES_MASK(gt, first, count) ({		\
> +#define __ENGINE_INSTANCES_MASK(mask, first, count) ({			\
>  	unsigned int first__ = (first);					\
>  	unsigned int count__ = (count);					\
> -	((gt)->info.engine_mask &						\
> -	 GENMASK(first__ + count__ - 1, first__)) >> first__;		\
> +	((mask) & GENMASK(first__ + count__ - 1, first__)) >> first__;	\
>  })
> +
> +#define ENGINE_INSTANCES_MASK(gt, first, count) \
> +	__ENGINE_INSTANCES_MASK((gt)->info.engine_mask, first, count)
> +
>  #define RCS_MASK(gt) \
>  	ENGINE_INSTANCES_MASK(gt, RCS0, I915_MAX_RCS)
>  #define BCS_MASK(gt) \