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 <[email protected]>
> >> ---
> >> 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
