Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Fri, 10 Mar 2023 16:18:30 -0800, Umesh Nerlige Ramappa wrote:
>
> On Fri, Mar 10, 2023 at 09:36:52AM -0800, Dixit, Ashutosh wrote:
> > On Fri, 10 Mar 2023 08:39:27 -0800, Umesh Nerlige Ramappa wrote:
> >>
> >
> > Hi Umesh,
> >
> >> On Thu, Mar 09, 2023 at 03:57:48PM -0800, Dixit, Ashutosh wrote:
> >> > On Tue, 07 Mar 2023 12:16:11 -0800, Umesh Nerlige Ramappa wrote:
> >> >>
> >> >> -static int gen8_configure_context(struct i915_gem_context *ctx,
> >> >> +static int gen8_configure_context(struct i915_perf_stream *stream,
> >> >> +				  struct i915_gem_context *ctx,
> >> >>				  struct flex *flex, unsigned int count)
> >> >>  {
> >> >>	struct i915_gem_engines_iter it;
> >> >> @@ -2573,7 +2594,8 @@ static int gen8_configure_context(struct i915_gem_context *ctx,
> >> >>	for_each_gem_engine(ce, i915_gem_context_lock_engines(ctx), it) {
> >> >>		GEM_BUG_ON(ce == ce->engine->kernel_context);
> >> >>
> >> >> -		if (!engine_supports_oa(ce->engine))
> >> >> +		if (!engine_supports_oa(ce->engine) ||
> >> >> +		    ce->engine->class != stream->engine->class)
> >> >>			continue;
> >> >>
> >> >>		/* Otherwise OA settings will be set upon first use */
> >> >> @@ -2704,7 +2726,7 @@ oa_configure_all_contexts(struct i915_perf_stream *stream,
> >> >>
> >> >>		spin_unlock(&i915->gem.contexts.lock);
> >> >>
> >> >> -		err = gen8_configure_context(ctx, regs, num_regs);
> >> >> +		err = gen8_configure_context(stream, ctx, regs, num_regs);
> >> >>		if (err) {
> >> >>			i915_gem_context_put(ctx);
> >> >>			return err;
> >> >> @@ -2724,7 +2746,8 @@ oa_configure_all_contexts(struct i915_perf_stream *stream,
> >> >>	for_each_uabi_engine(engine, i915) {
> >> >>		struct intel_context *ce = engine->kernel_context;
> >> >>
> >> >> -		if (!engine_supports_oa(ce->engine))
> >> >> +		if (!engine_supports_oa(ce->engine) ||
> >> >> +		    ce->engine->class != stream->engine->class)
> >> >>			continue;
> >> >>
> >> >>		regs[0].value = intel_sseu_make_rpcs(engine->gt, &ce->sseu);
> >> >> @@ -2749,6 +2772,9 @@ gen12_configure_all_contexts(struct i915_perf_stream *stream,
> >> >>		},
> >> >>	};
> >> >>
> >> >> +	if (stream->engine->class != RENDER_CLASS)
> >> >> +		return 0;
> >> >> +
> >> >>	return oa_configure_all_contexts(stream,
> >> >>					 regs, ARRAY_SIZE(regs),
> >> >>					 active);
> >> >
> >> > Can you please explain the above changes? Why are we checking for
> >> > engine->class above? Should we be checking for both class and instance? Or
> >> > all engines connected to an OA unit (multiple classes can be connected to
> >> > an OA unit and be different from stream->engine->class, e.g. VDBOX and
> >> > VEBOX)? oa_configure_all_contexts is also called from
> >> > lrc_configure_all_contexts.
>
> This check primarily blocks media engine use cases from entering
> oa_configure_all_contexts().
>
> lrc_configure_all_contexts applies to pre-gen12 only. On pre-gen12,
> engine_supports_oa() should return true only for render.
> >>
> >> Only render (and compute when we support it) have OA specific configuration
> >> in the context image. Media engines do not have any context specific
> >> configurations.
> >
> > Yes I remember you answered this previously too. My question still is why
> > did we make the 2 instances of this change above:
> >
> > From the original code in drm-tip:
> >
> >		if (engine->class != RENDER_CLASS)
> >			continue;
> >
> > To the final code (changed in two patches):
> >
> >		if (!engine_supports_oa(ce->engine) ||
> >		    ce->engine->class != stream->engine->class)
> >			continue;
>
> I think some changes are a result of incrementally supporting compute and
> then media in OA.  Since we have not upstreamed the compute support, some
> lines of code remain.
>
> With compute support the "if (engine->class != RENDER_CLASS)" changed to
> "if (!engine_supports_oa(ce->engine)). Later, OAM support brought the other
> condition that checks classes because this code is under
> for_each_uabi_engine(engine, i915). When we run this for an OA use case
> where user has passed rcs0 for ex, it will still iterate over the media
> engines. Since we now support media engines, we should skip them in this
> loop.
> The other question on whether this should be class specific or span
> multiple engines, I have to check that specifically for OAG. Ideally, the
> PWR_CLK_STATE should be configured for all engines that support it (render
> and compute where available), so the above check should be
> if (!engine_supports_oa(ce->engine) ||
>     !engine_has_pwr_clk_state(ce->engine))
>
> A jira will help track this and I can address that in a separate
> patch/series if it turns out to be an issue.

OK, this is my last question on this patch, everything else is fine. Since
you will post another version of the series I will review your (hopefully
final) changes about the above segment there and let's go from there.

Thanks.
--
Ashutosh