| Message ID | 20240515214258.59209-7-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/xe: Per client usage | expand |
-----Original Message----- From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Lucas De Marchi Sent: Wednesday, May 15, 2024 2:43 PM To: intel-xe@lists.freedesktop.org Cc: Tvrtko Ursulin <tursulin@ursulin.net>; Nerlige Ramappa, Umesh <umesh.nerlige.ramappa@intel.com>; dri-devel@lists.freedesktop.org; De Marchi, Lucas <lucas.demarchi@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@intel.com> I have a minor nit lower down, but nothing worth blocking over: Reviewed-by: Jonathan Cavitt <jonathan.cavitt@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) 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 > >
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(>_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 >
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(>_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 >>
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(>_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 >>>
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); +} + 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
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> --- 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(-)