| Message ID | 20230228022329.3615793-9-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Mon, 27 Feb 2023 18:23:28 -0800, Umesh Nerlige Ramappa wrote: > Hi Umesh, > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h > index 8df261c5ab9b..8ce20004a9dd 100644 > --- a/include/uapi/drm/i915_drm.h > +++ b/include/uapi/drm/i915_drm.h > @@ -2758,6 +2758,28 @@ enum drm_i915_perf_property_id { > */ > DRM_I915_PERF_PROP_POLL_OA_PERIOD, > > + /** > + * In platforms with multiple OA buffers, the engine class instance must I'd say "one engine class instance". > + * be passed to open a stream to a OA unit corresponding to the engine. > + * Multiple engines may be mapped to the same OA unit. Maybe (at the risk of over-stating) something like "Multiple engines may be mapped to the same OA unit. The OA unit is identified by class:instance of any engine mapped to it". But it's ok to not change anything here. > + * > + * In addition to the class:instance, if a gem context is also passed, then > + * 1) the report headers of OA reports from any contexts that do not > + * match this specific engine context are squashed. This one I don't understand: we seem to be mixing gem contexts and engine context here. Also, afais there are no changes related to this in this series. This context squashing has always been happening, so why add this comment to DRM_I915_PERF_PROP_OA_ENGINE_CLASS (or to this patch)? If we want to clarify something maybe this comment should be added to DRM_I915_PERF_PROP_CTX_HANDLE? But otherwise I think we should just drop this comment, at least from this patch? > + * 2) if the engine supports MI_REPORT_PERF_COUNT, this specific engine > + * context is configured for this command. > + * > + * 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 */ > }; Thanks. -- Ashutosh
On Wed, Mar 01, 2023 at 01:59:10PM -0800, Dixit, Ashutosh wrote: >On Mon, 27 Feb 2023 18:23:28 -0800, Umesh Nerlige Ramappa wrote: >> > >Hi Umesh, > >> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h >> index 8df261c5ab9b..8ce20004a9dd 100644 >> --- a/include/uapi/drm/i915_drm.h >> +++ b/include/uapi/drm/i915_drm.h >> @@ -2758,6 +2758,28 @@ enum drm_i915_perf_property_id { >> */ >> DRM_I915_PERF_PROP_POLL_OA_PERIOD, >> >> + /** >> + * In platforms with multiple OA buffers, the engine class instance must > >I'd say "one engine class instance". > >> + * be passed to open a stream to a OA unit corresponding to the engine. >> + * Multiple engines may be mapped to the same OA unit. > >Maybe (at the risk of over-stating) something like "Multiple engines may be >mapped to the same OA unit. The OA unit is identified by class:instance of >any engine mapped to it". > >But it's ok to not change anything here. > >> + * >> + * In addition to the class:instance, if a gem context is also passed, then >> + * 1) the report headers of OA reports from any contexts that do not >> + * match this specific engine context are squashed. > >This one I don't understand: we seem to be mixing gem contexts and engine >context here. Also, afais there are no changes related to this in this >series. This context squashing has always been happening, so why add this >comment to DRM_I915_PERF_PROP_OA_ENGINE_CLASS (or to this patch)? If we >want to clarify something maybe this comment should be added to >DRM_I915_PERF_PROP_CTX_HANDLE? But otherwise I think we should just drop >this comment, at least from this patch? > >> + * 2) if the engine supports MI_REPORT_PERF_COUNT, this specific engine >> + * context is configured for this command. I think I will drop 1 and 2 since they are not specifically related to the class:instance parameters. Thanks, Umesh >> + * >> + * 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 */ >> }; > >Thanks. >-- >Ashutosh
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index a8eb37be2aa2..9d3db1edab64 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -4014,47 +4014,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; - } - - if (!engine_supports_oa(props->engine)) { - drm_dbg(&perf->i915->drm, - "Engine not supported by OA %d:%d\n", - I915_ENGINE_CLASS_RENDER, 0); - 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 number 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; @@ -4175,7 +4157,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; } @@ -4183,6 +4171,21 @@ 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)) { + drm_dbg(&perf->i915->drm, + "Engine not supported by OA %d:%d\n", + class, instance); + return -EINVAL; + } + if (config_sseu) { ret = get_sseu_config(&props->sseu, props->engine, &user_sseu); if (ret) { @@ -5159,8 +5162,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..8ce20004a9dd 100644 --- a/include/uapi/drm/i915_drm.h +++ b/include/uapi/drm/i915_drm.h @@ -2758,6 +2758,28 @@ 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 any contexts that do not + * match this specific engine context are squashed. + * 2) if the engine supports MI_REPORT_PERF_COUNT, this specific engine + * context is configured for this command. + * + * 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 */ };
One or more engines map to a specific OA unit. All reports from these engines are captured in the OA buffer managed by this OA unit. Current i915 OA implementation supports only the OAG unit. OAG primarily caters to render engine, so i915 OA uses render as the default engine in the OA implementation. Since there are more OA units on newer hardware that map to other engines, allow user to pass engine class and instance to select and 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 v2: (Ashutosh) - Clarify commit message - Add drm_dbg - Clarify uapi description Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 60 ++++++++++++++++++-------------- include/uapi/drm/i915_drm.h | 22 ++++++++++++ 2 files changed, 55 insertions(+), 27 deletions(-)