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

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Wed, May 15, 2024 at 02:42:56PM -0700, Lucas De Marchi wrote:
>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@intel.com>

Just a few questions below, otherwise this looks good as is:

Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@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)
>+{
>+	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);

I am not seeing a place where user_engines.mask is not a subset of 
info.engine_mask in the driver, so the above check will always be true.

Did you mean to do and & instead of | above? That might make sense since 
then you are making sure that the user_engines are a subset of 
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

After the driver loads, do we expect ccs_mode to change dynamically 
based on some criteria OR is it a one time configuration at driver load?

If former, can you provide an example where ccs_mode would change 
dynamically, just curious.

Regards,
Umesh

>+ * 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
>