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

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

On Thu, May 16, 2024 at 02:52:01PM -0500, Lucas De Marchi wrote:
>On Thu, May 16, 2024 at 11:33:54AM GMT, Umesh Nerlige Ramappa wrote:
>>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.
>
>that's why it's an assert. user_engines.mask should always be a
>subset of info.engine_mask, otherwise something went terribly wrong.
>
>>
>>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.
>
>no, what I'm trying to assert is that user_engines.mask never has an
>engine that is not present in info.engine_mask. Example:
>
>	engine_mask       == 0b01
>	user_engines.mask == 0b11
>
>That should never happen and it should fail the assert.

oh, my bad, the assert looks correct.
>
>I decided to add the assert because I'm not deriving the
>user_engines.mask directly from the mask, but indirectly. Early on probe
>we setup the mask and create the hw_engine instances and we are
>calculating the user_engines.mask from there. I just wanted to make sure
>we don't screw up something in the middle that causes issues.
>
>>
>>>+}
>>>+
>>>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.
>
>it can be set via sysfs, but it blocks changing it if there are clients.
>For with display, it's easier to check by loading the driver with
>enable_display=0. Trying that on a DG2:
>
>	# modprobe xe enable_display=0
>	# exec 3<> /dev/dri/card1
>	# tail -n4 /proc/self/fdinfo/3
>	drm-cycles-bcs: 0
>	drm-total-cycles-bcs:   37728138157
>	drm-cycles-ccs: 0
>	drm-total-cycles-ccs:   37728138157
>	#
>	# exec 3<&-
>	# echo 2 > /sys/devices/pci0000:00/0000:00:01.0/0000:01:00.0/0000:02:01.0/0000:03:00.0/tile0/gt0/ccs_mode
>	# exec 3<> /dev/dri/card1
>	# tail -n4 /proc/self/fdinfo/3
>	drm-total-cycles-bcs:   38260910526
>	drm-cycles-ccs: 0
>	drm-total-cycles-ccs:   38260910526
>	drm-engine-capacity-ccs:        2

makes sense, thanks,

Umesh
>
>thanks
>Lucas De Marchi
>
>>
>>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
>>>