[PATCH v4 6/8] drm/xe: Cache data about user-visible engines

[Cavitt, Jonathan]

-----Original Message-----
From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Lucas De Marchi
Sent: Wednesday, May 15, 2024 2:43 PM
To: intel-xe at lists.freedesktop.org
Cc: Tvrtko Ursulin <tursulin at ursulin.net>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa at intel.com>; dri-devel at lists.freedesktop.org; De Marchi, Lucas <lucas.demarchi at intel.com>
Subject: [PATCH v4 6/8] drm/xe: Cache data about user-visible engines
> 
> gt->info.engine_mask used to indicate the available engines, but that
> is not always true anymore: some engines are reserved to kernel and some
> may be exposed as a single engine (e.g. with ccs_mode).
> 
> Runtime changes only happen when no clients exist, so it's safe to cache
> the list of engines in the gt and update that when it's needed. This
> will help implementing per client engine utilization so this (mostly
> constant) information doesn't need to be re-calculated on every query.
> 
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>

I have a minor nit lower down, but nothing worth blocking over:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

> ---
>  drivers/gpu/drm/xe/xe_gt.c          | 23 +++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_gt.h          | 13 +++++++++++++
>  drivers/gpu/drm/xe/xe_gt_ccs_mode.c |  1 +
>  drivers/gpu/drm/xe/xe_gt_types.h    | 21 ++++++++++++++++++++-
>  4 files changed, 57 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index e69a03ddd255..5194a3d38e76 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -560,9 +560,32 @@ int xe_gt_init(struct xe_gt *gt)
>  	if (err)
>  		return err;
>  
> +	xe_gt_record_user_engines(gt);
> +
>  	return drmm_add_action_or_reset(&gt_to_xe(gt)->drm, gt_fini, gt);
>  }
>  
> +void xe_gt_record_user_engines(struct xe_gt *gt)

On one hand, I think it might look better to put
xe_gt_record_user_engines above xe_gt_init.  On the
other hand, I recognize that it's not strictly necessary to
make this change because xe_gt_record_user_engines
is declared in xe_gt.h, so this request would just be for
style reasons.

Feel free to disregard, this is just a suggestion.
-Jonathan Cavitt

> +{
> +	struct xe_hw_engine *hwe;
> +	enum xe_hw_engine_id id;
> +
> +	gt->user_engines.mask = 0;
> +	memset(gt->user_engines.instances_per_class, 0,
> +	       sizeof(gt->user_engines.instances_per_class));
> +
> +	for_each_hw_engine(hwe, gt, id) {
> +		if (xe_hw_engine_is_reserved(hwe))
> +			continue;
> +
> +		gt->user_engines.mask |= BIT_ULL(id);
> +		gt->user_engines.instances_per_class[hwe->class]++;
> +	}
> +
> +	xe_gt_assert(gt, (gt->user_engines.mask | gt->info.engine_mask)
> +		     == gt->info.engine_mask);
> +}
> +
>  static int do_gt_reset(struct xe_gt *gt)
>  {
>  	int err;
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index 8474c50b1b30..ad3fd31e0a41 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -38,6 +38,19 @@ int xe_gt_init_hwconfig(struct xe_gt *gt);
>  int xe_gt_init_early(struct xe_gt *gt);
>  int xe_gt_init(struct xe_gt *gt);
>  int xe_gt_record_default_lrcs(struct xe_gt *gt);
> +
> +/**
> + * @xe_gt_record_user_engines - save data related to engines available to
> + * usersapce
> + * @gt: GT structure
> + *
> + * Walk the available HW engines from gt->info.engine_mask and calculate data
> + * related to those engines that may be used by userspace. To be used whenever
> + * available engines change in runtime (e.g. with ccs_mode) or during
> + * initialization
> + */
> +void xe_gt_record_user_engines(struct xe_gt *gt);
> +
>  void xe_gt_suspend_prepare(struct xe_gt *gt);
>  int xe_gt_suspend(struct xe_gt *gt);
>  int xe_gt_resume(struct xe_gt *gt);
> diff --git a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> index a34c9a24dafc..c36218f4f6c8 100644
> --- a/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> +++ b/drivers/gpu/drm/xe/xe_gt_ccs_mode.c
> @@ -134,6 +134,7 @@ ccs_mode_store(struct device *kdev, struct device_attribute *attr,
>  	if (gt->ccs_mode != num_engines) {
>  		xe_gt_info(gt, "Setting compute mode to %d\n", num_engines);
>  		gt->ccs_mode = num_engines;
> +		xe_gt_record_user_engines(gt);
>  		xe_gt_reset_async(gt);
>  	}
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_types.h b/drivers/gpu/drm/xe/xe_gt_types.h
> index 5a114fc9dde7..aaf2951749a6 100644
> --- a/drivers/gpu/drm/xe/xe_gt_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_types.h
> @@ -112,7 +112,11 @@ struct xe_gt {
>  		enum xe_gt_type type;
>  		/** @info.reference_clock: clock frequency */
>  		u32 reference_clock;
> -		/** @info.engine_mask: mask of engines present on GT */
> +		/**
> +		 * @info.engine_mask: mask of engines present on GT. Some of
> +		 * them may be reserved in runtime and not available for user.
> +		 * See @user_engines.mask
> +		 */
>  		u64 engine_mask;
>  		/** @info.gmdid: raw GMD_ID value from hardware */
>  		u32 gmdid;
> @@ -365,6 +369,21 @@ struct xe_gt {
>  		/** @wa_active.oob: bitmap with active OOB workaroudns */
>  		unsigned long *oob;
>  	} wa_active;
> +
> +	/** @user_engines: engines present in GT and available to userspace */
> +	struct {
> +		/**
> +		 * @mask: like @info->engine_mask, but take in consideration
> +		 * only engines available to userspace
> +		 */
> +		u64 mask;
> +
> +		/**
> +		 * @instances_per_class: aggregate per class the number of
> +		 * engines available to userspace
> +		 */
> +		u8 instances_per_class[XE_ENGINE_CLASS_MAX];
> +	} user_engines;
>  };
>  
>  #endif
> -- 
> 2.43.0
> 
>