[PATCH v4 6/8] drm/xe: Cache data about user-visible engines
Lucas De Marchi
lucas.demarchi at intel.com
Thu May 16 19:52:01 UTC 2024
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 at intel.com>
>
>Just a few questions below, otherwise this looks good as is:
>
>Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa 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(>_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.
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
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
>>
More information about the dri-devel
mailing list