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@xxxxxxxxx> > --- > 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
