Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units
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(>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, >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
Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units
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(>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, >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.
Thanks,
Umesh
Thanks.
--
Ashutosh
Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units
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(>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, >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.
>
> 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;
Thanks.
--
Ashutosh
Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units
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:
Hi Umesh,
-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(>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, >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.
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.
Thanks,
Umesh
Thanks.
--
Ashutosh
Re: [Intel-gfx] [PATCH v4 9/9] drm/i915/perf: Add support for OA media units
On Tue, 07 Mar 2023 12:16:11 -0800, Umesh Nerlige Ramappa wrote:
>
Hi Umesh,
> -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(>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, >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.
Thanks.
--
Ashutosh
