[PATCH v4 6/8] drm/xe: Cache data about user-visible engines
Umesh Nerlige Ramappa
umesh.nerlige.ramappa at intel.com
Thu May 16 22:56:34 UTC 2024
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 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.
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
>>>
More information about the dri-devel
mailing list