| Message ID | 20230217005850.2511422-9-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Thu, Feb 16, 2023 at 04:58:49PM -0800, Umesh Nerlige Ramappa wrote: >Current implementation of perf defaults to render and configures the >default OAG unit. Since there are more OA units on newer hardware, allow >user to pass engine class and instance to program specific OA units. > >UMD specific changes for GPUvis support: >https://patchwork.freedesktop.org/patch/522827/?series=114023 >https://patchwork.freedesktop.org/patch/522822/?series=114023 >https://patchwork.freedesktop.org/patch/522826/?series=114023 >https://patchwork.freedesktop.org/patch/522828/?series=114023 >https://patchwork.freedesktop.org/patch/522816/?series=114023 >https://patchwork.freedesktop.org/patch/522825/?series=114023 GPUvis PR is here - https://github.com/mikesart/gpuvis/pull/81 Regards, Umesh > >Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >--- > drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++------------- > include/uapi/drm/i915_drm.h | 20 +++++++++++++ > 2 files changed, 49 insertions(+), 20 deletions(-) > >diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >index d3a1892c93be..f028df812067 100644 >--- a/drivers/gpu/drm/i915/i915_perf.c >+++ b/drivers/gpu/drm/i915/i915_perf.c >@@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf, > struct drm_i915_gem_context_param_sseu user_sseu; > u64 __user *uprop = uprops; > bool config_sseu = false; >+ u8 class, instance; > u32 i; > int ret; > > memset(props, 0, sizeof(struct perf_open_properties)); > props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; > >- if (!n_props) { >- drm_dbg(&perf->i915->drm, >- "No i915 perf properties given\n"); >- return -EINVAL; >- } >- >- /* At the moment we only support using i915-perf on the RCS. */ >- props->engine = intel_engine_lookup_user(perf->i915, >- I915_ENGINE_CLASS_RENDER, >- 0); >- if (!props->engine) { >- drm_dbg(&perf->i915->drm, >- "No RENDER-capable engines\n"); >- return -EINVAL; >- } >- > /* Considering that ID = 0 is reserved and assuming that we don't > * (currently) expect any configurations to ever specify duplicate > * values for a particular property ID then the last _PROP_MAX value is > * one greater than the maximum number of properties we expect to get > * from userspace. > */ >- if (n_props >= DRM_I915_PERF_PROP_MAX) { >+ if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { > drm_dbg(&perf->i915->drm, >- "More i915 perf properties specified than exist\n"); >+ "Invalid no. of i915 perf properties given\n"); > return -EINVAL; > } > >+ /* Defaults when class:instance is not passed */ >+ class = I915_ENGINE_CLASS_RENDER; >+ instance = 0; >+ > for (i = 0; i < n_props; i++) { > u64 oa_period, oa_freq_hz; > u64 id, value; >@@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf, > } > props->poll_oa_period = value; > break; >- case DRM_I915_PERF_PROP_MAX: >+ case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: >+ class = (u8)value; >+ break; >+ case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: >+ instance = (u8)value; >+ break; >+ default: > MISSING_CASE(id); > return -EINVAL; > } >@@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf, > uprop += 2; > } > >+ props->engine = intel_engine_lookup_user(perf->i915, class, instance); >+ if (!props->engine) { >+ drm_dbg(&perf->i915->drm, >+ "OA engine class and instance invalid %d:%d\n", >+ class, instance); >+ return -EINVAL; >+ } >+ >+ if (!engine_supports_oa(props->engine)) >+ return -EINVAL; >+ > if (config_sseu) { > ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); > if (ret) { >@@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void) > * > * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the > * interval for the hrtimer used to check for OA data. >+ * >+ * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and >+ * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE > */ >- return 5; >+ return 6; > } > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >index 8df261c5ab9b..b6922b52d85c 100644 >--- a/include/uapi/drm/i915_drm.h >+++ b/include/uapi/drm/i915_drm.h >@@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id { > */ > DRM_I915_PERF_PROP_POLL_OA_PERIOD, > >+ /** >+ * In platforms with multiple OA buffers, the engine class instance must >+ * be passed to open a stream to a OA unit corresponding to the engine. >+ * Multiple engines may be mapped to the same OA unit. >+ * >+ * In addition to the class:instance, if a gem context is also passed, then >+ * 1) the report headers of OA reports from other engines are squashed. >+ * 2) OAR is enabled for the class:instance >+ * >+ * This property is available in perf revision 6. >+ */ >+ DRM_I915_PERF_PROP_OA_ENGINE_CLASS, >+ >+ /** >+ * This parameter specifies the engine instance. >+ * >+ * This property is available in perf revision 6. >+ */ >+ DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, >+ > DRM_I915_PERF_PROP_MAX /* non-ABI */ > }; > >-- >2.36.1 >
On Thu, 16 Feb 2023 16:58:49 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, Patch is mostly ok but a few questions below: > Current implementation of perf defaults to render and configures the > default OAG unit. Since there are more OA units on newer hardware, allow > user to pass engine class and instance to program specific OA units. I think we should more clearly say here that the OA unit is identified by means of one of the engines (class/instance of that engine) associated with that OA unit. The engine -> OA unit mapping is a static mapping depending on the particular platform. Something like that. > > UMD specific changes for GPUvis support: > https://patchwork.freedesktop.org/patch/522827/?series=114023 > https://patchwork.freedesktop.org/patch/522822/?series=114023 > https://patchwork.freedesktop.org/patch/522826/?series=114023 > https://patchwork.freedesktop.org/patch/522828/?series=114023 > https://patchwork.freedesktop.org/patch/522816/?series=114023 > https://patchwork.freedesktop.org/patch/522825/?series=114023 > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++------------- > include/uapi/drm/i915_drm.h | 20 +++++++++++++ > 2 files changed, 49 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index d3a1892c93be..f028df812067 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf, > struct drm_i915_gem_context_param_sseu user_sseu; > u64 __user *uprop = uprops; > bool config_sseu = false; > + u8 class, instance; > u32 i; > int ret; > > memset(props, 0, sizeof(struct perf_open_properties)); > props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; > > - if (!n_props) { > - drm_dbg(&perf->i915->drm, > - "No i915 perf properties given\n"); > - return -EINVAL; > - } > - > - /* At the moment we only support using i915-perf on the RCS. */ > - props->engine = intel_engine_lookup_user(perf->i915, > - I915_ENGINE_CLASS_RENDER, > - 0); > - if (!props->engine) { > - drm_dbg(&perf->i915->drm, > - "No RENDER-capable engines\n"); > - return -EINVAL; > - } > - > /* Considering that ID = 0 is reserved and assuming that we don't > * (currently) expect any configurations to ever specify duplicate > * values for a particular property ID then the last _PROP_MAX value is > * one greater than the maximum number of properties we expect to get > * from userspace. > */ > - if (n_props >= DRM_I915_PERF_PROP_MAX) { > + if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { > drm_dbg(&perf->i915->drm, > - "More i915 perf properties specified than exist\n"); > + "Invalid no. of i915 perf properties given\n"); Invalid number > return -EINVAL; > } > > + /* Defaults when class:instance is not passed */ > + class = I915_ENGINE_CLASS_RENDER; > + instance = 0; > + > for (i = 0; i < n_props; i++) { > u64 oa_period, oa_freq_hz; > u64 id, value; > @@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf, > } > props->poll_oa_period = value; > break; > - case DRM_I915_PERF_PROP_MAX: > + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: > + class = (u8)value; > + break; > + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: > + instance = (u8)value; > + break; > + default: > MISSING_CASE(id); > return -EINVAL; > } > @@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf, > uprop += 2; > } > > + props->engine = intel_engine_lookup_user(perf->i915, class, instance); > + if (!props->engine) { > + drm_dbg(&perf->i915->drm, > + "OA engine class and instance invalid %d:%d\n", > + class, instance); > + return -EINVAL; > + } > + > + if (!engine_supports_oa(props->engine)) > + return -EINVAL; Need drm_dbg here too? > + > if (config_sseu) { > ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); > if (ret) { > @@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void) > * > * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the > * interval for the hrtimer used to check for OA data. > + * > + * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and > + * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE > */ > - return 5; > + return 6; Do we need a separate revision for this? Maybe club it with OAM support since that is where this is getting introduced? > } > > #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 8df261c5ab9b..b6922b52d85c 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id { > */ > DRM_I915_PERF_PROP_POLL_OA_PERIOD, > > + /** > + * In platforms with multiple OA buffers, the engine class instance must > + * be passed to open a stream to a OA unit corresponding to the engine. > + * Multiple engines may be mapped to the same OA unit. > + * > + * In addition to the class:instance, if a gem context is also passed, then > + * 1) the report headers of OA reports from other engines are squashed. Other engines or you mean other contexts? > + * 2) OAR is enabled for the class:instance For render engine? Maybe the above comments need to be more crisp since they are in i915_drm.h or is it only I who is confused :) > + * > + * This property is available in perf revision 6. > + */ > + DRM_I915_PERF_PROP_OA_ENGINE_CLASS, > + > + /** > + * This parameter specifies the engine instance. > + * > + * This property is available in perf revision 6. > + */ > + DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, > + > DRM_I915_PERF_PROP_MAX /* non-ABI */ > }; > > -- > 2.36.1 > Thanks. -- Ashutosh
On Tue, 21 Feb 2023 15:53:57 -0800, Dixit, Ashutosh wrote: > > > + /** > > + * In platforms with multiple OA buffers, the engine class instance must > > + * be passed to open a stream to a OA unit corresponding to the engine. > > + * Multiple engines may be mapped to the same OA unit. > > + * > > + * In addition to the class:instance, if a gem context is also passed, then > > + * 1) the report headers of OA reports from other engines are squashed. > > Other engines or you mean other contexts? > > > + * 2) OAR is enabled for the class:instance > > For render engine? I think this means the engine will support MI_REPORT_PERF_COUNT but not sure how this (or if anything) is done.
On Tue, Feb 21, 2023 at 03:53:57PM -0800, Dixit, Ashutosh wrote: >On Thu, 16 Feb 2023 16:58:49 -0800, Umesh Nerlige Ramappa wrote: >> > >Hi Umesh, > >Patch is mostly ok but a few questions below: > >> Current implementation of perf defaults to render and configures the >> default OAG unit. Since there are more OA units on newer hardware, allow >> user to pass engine class and instance to program specific OA units. > >I think we should more clearly say here that the OA unit is identified by >means of one of the engines (class/instance of that engine) associated with >that OA unit. The engine -> OA unit mapping is a static mapping depending >on the particular platform. Something like that. > >> >> UMD specific changes for GPUvis support: >> https://patchwork.freedesktop.org/patch/522827/?series=114023 >> https://patchwork.freedesktop.org/patch/522822/?series=114023 >> https://patchwork.freedesktop.org/patch/522826/?series=114023 >> https://patchwork.freedesktop.org/patch/522828/?series=114023 >> https://patchwork.freedesktop.org/patch/522816/?series=114023 >> https://patchwork.freedesktop.org/patch/522825/?series=114023 >> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> --- >> drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++------------- >> include/uapi/drm/i915_drm.h | 20 +++++++++++++ >> 2 files changed, 49 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> index d3a1892c93be..f028df812067 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf, >> struct drm_i915_gem_context_param_sseu user_sseu; >> u64 __user *uprop = uprops; >> bool config_sseu = false; >> + u8 class, instance; >> u32 i; >> int ret; >> >> memset(props, 0, sizeof(struct perf_open_properties)); >> props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; >> >> - if (!n_props) { >> - drm_dbg(&perf->i915->drm, >> - "No i915 perf properties given\n"); >> - return -EINVAL; >> - } >> - >> - /* At the moment we only support using i915-perf on the RCS. */ >> - props->engine = intel_engine_lookup_user(perf->i915, >> - I915_ENGINE_CLASS_RENDER, >> - 0); >> - if (!props->engine) { >> - drm_dbg(&perf->i915->drm, >> - "No RENDER-capable engines\n"); >> - return -EINVAL; >> - } >> - >> /* Considering that ID = 0 is reserved and assuming that we don't >> * (currently) expect any configurations to ever specify duplicate >> * values for a particular property ID then the last _PROP_MAX value is >> * one greater than the maximum number of properties we expect to get >> * from userspace. >> */ >> - if (n_props >= DRM_I915_PERF_PROP_MAX) { >> + if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { >> drm_dbg(&perf->i915->drm, >> - "More i915 perf properties specified than exist\n"); >> + "Invalid no. of i915 perf properties given\n"); > >Invalid number > >> return -EINVAL; >> } >> >> + /* Defaults when class:instance is not passed */ >> + class = I915_ENGINE_CLASS_RENDER; >> + instance = 0; >> + >> for (i = 0; i < n_props; i++) { >> u64 oa_period, oa_freq_hz; >> u64 id, value; >> @@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf, >> } >> props->poll_oa_period = value; >> break; >> - case DRM_I915_PERF_PROP_MAX: >> + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: >> + class = (u8)value; >> + break; >> + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: >> + instance = (u8)value; >> + break; >> + default: >> MISSING_CASE(id); >> return -EINVAL; >> } >> @@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf, >> uprop += 2; >> } >> >> + props->engine = intel_engine_lookup_user(perf->i915, class, instance); >> + if (!props->engine) { >> + drm_dbg(&perf->i915->drm, >> + "OA engine class and instance invalid %d:%d\n", >> + class, instance); >> + return -EINVAL; >> + } >> + >> + if (!engine_supports_oa(props->engine)) >> + return -EINVAL; > >Need drm_dbg here too? > >> + >> if (config_sseu) { >> ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); >> if (ret) { >> @@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void) >> * >> * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the >> * interval for the hrtimer used to check for OA data. >> + * >> + * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and >> + * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE >> */ >> - return 5; >> + return 6; > >Do we need a separate revision for this? Maybe club it with OAM support >since that is where this is getting introduced? It's a separate revision to identify support for multiple GTs, even without OAM. > >> } >> >> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index 8df261c5ab9b..b6922b52d85c 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id { >> */ >> DRM_I915_PERF_PROP_POLL_OA_PERIOD, >> >> + /** >> + * In platforms with multiple OA buffers, the engine class instance must >> + * be passed to open a stream to a OA unit corresponding to the engine. >> + * Multiple engines may be mapped to the same OA unit. >> + * >> + * In addition to the class:instance, if a gem context is also passed, then >> + * 1) the report headers of OA reports from other engines are squashed. > >Other engines or you mean other contexts? > >> + * 2) OAR is enabled for the class:instance > >For render engine? > >Maybe the above comments need to be more crisp since they are in i915_drm.h >or is it only I who is confused :) > >> + * >> + * This property is available in perf revision 6. >> + */ >> + DRM_I915_PERF_PROP_OA_ENGINE_CLASS, >> + >> + /** >> + * This parameter specifies the engine instance. >> + * >> + * This property is available in perf revision 6. >> + */ >> + DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, >> + >> DRM_I915_PERF_PROP_MAX /* non-ABI */ >> }; >> >> -- >> 2.36.1 >> > >Thanks. >-- >Ashutosh
On Fri, 24 Feb 2023 11:37:01 -0800, Umesh Nerlige Ramappa wrote: > > On Tue, Feb 21, 2023 at 03:53:57PM -0800, Dixit, Ashutosh wrote: > > On Thu, 16 Feb 2023 16:58:49 -0800, Umesh Nerlige Ramappa wrote: > >> > > > > Hi Umesh, > > > > Patch is mostly ok but a few questions below: > > > >> Current implementation of perf defaults to render and configures the > >> default OAG unit. Since there are more OA units on newer hardware, allow > >> user to pass engine class and instance to program specific OA units. > > > > I think we should more clearly say here that the OA unit is identified by > > means of one of the engines (class/instance of that engine) associated with > > that OA unit. The engine -> OA unit mapping is a static mapping depending > > on the particular platform. Something like that. > > > >> > >> UMD specific changes for GPUvis support: > >> https://patchwork.freedesktop.org/patch/522827/?series=114023 > >> https://patchwork.freedesktop.org/patch/522822/?series=114023 > >> https://patchwork.freedesktop.org/patch/522826/?series=114023 > >> https://patchwork.freedesktop.org/patch/522828/?series=114023 > >> https://patchwork.freedesktop.org/patch/522816/?series=114023 > >> https://patchwork.freedesktop.org/patch/522825/?series=114023 > >> > >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++------------- > >> include/uapi/drm/i915_drm.h | 20 +++++++++++++ > >> 2 files changed, 49 insertions(+), 20 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > >> index d3a1892c93be..f028df812067 100644 > >> --- a/drivers/gpu/drm/i915/i915_perf.c > >> +++ b/drivers/gpu/drm/i915/i915_perf.c > >> @@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf, > >> struct drm_i915_gem_context_param_sseu user_sseu; > >> u64 __user *uprop = uprops; > >> bool config_sseu = false; > >> + u8 class, instance; > >> u32 i; > >> int ret; > >> > >> memset(props, 0, sizeof(struct perf_open_properties)); > >> props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; > >> > >> - if (!n_props) { > >> - drm_dbg(&perf->i915->drm, > >> - "No i915 perf properties given\n"); > >> - return -EINVAL; > >> - } > >> - > >> - /* At the moment we only support using i915-perf on the RCS. */ > >> - props->engine = intel_engine_lookup_user(perf->i915, > >> - I915_ENGINE_CLASS_RENDER, > >> - 0); > >> - if (!props->engine) { > >> - drm_dbg(&perf->i915->drm, > >> - "No RENDER-capable engines\n"); > >> - return -EINVAL; > >> - } > >> - > >> /* Considering that ID = 0 is reserved and assuming that we don't > >> * (currently) expect any configurations to ever specify duplicate > >> * values for a particular property ID then the last _PROP_MAX value is > >> * one greater than the maximum number of properties we expect to get > >> * from userspace. > >> */ > >> - if (n_props >= DRM_I915_PERF_PROP_MAX) { > >> + if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { > >> drm_dbg(&perf->i915->drm, > >> - "More i915 perf properties specified than exist\n"); > >> + "Invalid no. of i915 perf properties given\n"); > > > > Invalid number > > > >> return -EINVAL; > >> } > >> > >> + /* Defaults when class:instance is not passed */ > >> + class = I915_ENGINE_CLASS_RENDER; > >> + instance = 0; > >> + > >> for (i = 0; i < n_props; i++) { > >> u64 oa_period, oa_freq_hz; > >> u64 id, value; > >> @@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf, > >> } > >> props->poll_oa_period = value; > >> break; > >> - case DRM_I915_PERF_PROP_MAX: > >> + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: > >> + class = (u8)value; > >> + break; > >> + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: > >> + instance = (u8)value; > >> + break; > >> + default: > >> MISSING_CASE(id); > >> return -EINVAL; > >> } > >> @@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf, > >> uprop += 2; > >> } > >> > >> + props->engine = intel_engine_lookup_user(perf->i915, class, instance); > >> + if (!props->engine) { > >> + drm_dbg(&perf->i915->drm, > >> + "OA engine class and instance invalid %d:%d\n", > >> + class, instance); > >> + return -EINVAL; > >> + } > >> + > >> + if (!engine_supports_oa(props->engine)) > >> + return -EINVAL; > > > > Need drm_dbg here too? > > > >> + > >> if (config_sseu) { > >> ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); > >> if (ret) { > >> @@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void) > >> * > >> * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the > >> * interval for the hrtimer used to check for OA data. > >> + * > >> + * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and > >> + * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE > >> */ > >> - return 5; > >> + return 6; > > > > Do we need a separate revision for this? Maybe club it with OAM support > > since that is where this is getting introduced? > > It's a separate revision to identify support for multiple GTs, even without > OAM. I was thinking that first there are no such products (xehpsdv was, but is now dead I believe) and even if it there were the series will be merged into a single kernel version so a single version would suffice. Maybe you mean that each patch which touches the uapi should up the OA version? In any case, since it is just a version number, no issues, either way is ok with me. Thanks. -- Ashutosh > > > >> } > >> > >> #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) > >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > >> index 8df261c5ab9b..b6922b52d85c 100644 > >> --- a/include/uapi/drm/i915_drm.h > >> +++ b/include/uapi/drm/i915_drm.h > >> @@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id { > >> */ > >> DRM_I915_PERF_PROP_POLL_OA_PERIOD, > >> > >> + /** > >> + * In platforms with multiple OA buffers, the engine class instance must > >> + * be passed to open a stream to a OA unit corresponding to the engine. > >> + * Multiple engines may be mapped to the same OA unit. > >> + * > >> + * In addition to the class:instance, if a gem context is also passed, then > >> + * 1) the report headers of OA reports from other engines are squashed. > > > > Other engines or you mean other contexts? > > > >> + * 2) OAR is enabled for the class:instance > > > > For render engine? > > > > Maybe the above comments need to be more crisp since they are in i915_drm.h > > or is it only I who is confused :) > > > >> + * > >> + * This property is available in perf revision 6. > >> + */ > >> + DRM_I915_PERF_PROP_OA_ENGINE_CLASS, > >> + > >> + /** > >> + * This parameter specifies the engine instance. > >> + * > >> + * This property is available in perf revision 6. > >> + */ > >> + DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, > >> + > >> DRM_I915_PERF_PROP_MAX /* non-ABI */ > >> }; > >> > >> -- > >> 2.36.1 > >> > > > > Thanks. > > -- > > Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index d3a1892c93be..f028df812067 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -4035,40 +4035,29 @@ static int read_properties_unlocked(struct i915_perf *perf, struct drm_i915_gem_context_param_sseu user_sseu; u64 __user *uprop = uprops; bool config_sseu = false; + u8 class, instance; u32 i; int ret; memset(props, 0, sizeof(struct perf_open_properties)); props->poll_oa_period = DEFAULT_POLL_PERIOD_NS; - if (!n_props) { - drm_dbg(&perf->i915->drm, - "No i915 perf properties given\n"); - return -EINVAL; - } - - /* At the moment we only support using i915-perf on the RCS. */ - props->engine = intel_engine_lookup_user(perf->i915, - I915_ENGINE_CLASS_RENDER, - 0); - if (!props->engine) { - drm_dbg(&perf->i915->drm, - "No RENDER-capable engines\n"); - return -EINVAL; - } - /* Considering that ID = 0 is reserved and assuming that we don't * (currently) expect any configurations to ever specify duplicate * values for a particular property ID then the last _PROP_MAX value is * one greater than the maximum number of properties we expect to get * from userspace. */ - if (n_props >= DRM_I915_PERF_PROP_MAX) { + if (!n_props || n_props >= DRM_I915_PERF_PROP_MAX) { drm_dbg(&perf->i915->drm, - "More i915 perf properties specified than exist\n"); + "Invalid no. of i915 perf properties given\n"); return -EINVAL; } + /* Defaults when class:instance is not passed */ + class = I915_ENGINE_CLASS_RENDER; + instance = 0; + for (i = 0; i < n_props; i++) { u64 oa_period, oa_freq_hz; u64 id, value; @@ -4189,7 +4178,13 @@ static int read_properties_unlocked(struct i915_perf *perf, } props->poll_oa_period = value; break; - case DRM_I915_PERF_PROP_MAX: + case DRM_I915_PERF_PROP_OA_ENGINE_CLASS: + class = (u8)value; + break; + case DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE: + instance = (u8)value; + break; + default: MISSING_CASE(id); return -EINVAL; } @@ -4197,6 +4192,17 @@ static int read_properties_unlocked(struct i915_perf *perf, uprop += 2; } + props->engine = intel_engine_lookup_user(perf->i915, class, instance); + if (!props->engine) { + drm_dbg(&perf->i915->drm, + "OA engine class and instance invalid %d:%d\n", + class, instance); + return -EINVAL; + } + + if (!engine_supports_oa(props->engine)) + return -EINVAL; + if (config_sseu) { ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); if (ret) { @@ -5208,8 +5214,11 @@ int i915_perf_ioctl_version(void) * * 5: Add DRM_I915_PERF_PROP_POLL_OA_PERIOD parameter that controls the * interval for the hrtimer used to check for OA data. + * + * 6: Add DRM_I915_PERF_PROP_OA_ENGINE_CLASS and + * DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE */ - return 5; + return 6; } #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h index 8df261c5ab9b..b6922b52d85c 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2758,6 +2758,26 @@ enum drm_i915_perf_property_id { */ DRM_I915_PERF_PROP_POLL_OA_PERIOD, + /** + * In platforms with multiple OA buffers, the engine class instance must + * be passed to open a stream to a OA unit corresponding to the engine. + * Multiple engines may be mapped to the same OA unit. + * + * In addition to the class:instance, if a gem context is also passed, then + * 1) the report headers of OA reports from other engines are squashed. + * 2) OAR is enabled for the class:instance + * + * This property is available in perf revision 6. + */ + DRM_I915_PERF_PROP_OA_ENGINE_CLASS, + + /** + * This parameter specifies the engine instance. + * + * This property is available in perf revision 6. + */ + DRM_I915_PERF_PROP_OA_ENGINE_INSTANCE, + DRM_I915_PERF_PROP_MAX /* non-ABI */ };
Current implementation of perf defaults to render and configures the default OAG unit. Since there are more OA units on newer hardware, allow user to pass engine class and instance to program specific OA units. UMD specific changes for GPUvis support: https://patchwork.freedesktop.org/patch/522827/?series=114023 https://patchwork.freedesktop.org/patch/522822/?series=114023 https://patchwork.freedesktop.org/patch/522826/?series=114023 https://patchwork.freedesktop.org/patch/522828/?series=114023 https://patchwork.freedesktop.org/patch/522816/?series=114023 https://patchwork.freedesktop.org/patch/522825/?series=114023 Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 49 +++++++++++++++++++------------- include/uapi/drm/i915_drm.h | 20 +++++++++++++ 2 files changed, 49 insertions(+), 20 deletions(-)