| Message ID | 20230215005419.2100887-3-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add OAM support for MTL | expand |
On Tue, 14 Feb 2023 16:54:12 -0800, Umesh Nerlige Ramappa wrote: > > Add helper to check for supported OA engines. > > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/i915_perf.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 393a0da8b7c8..a879ae4bf8d7 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -1570,6 +1570,19 @@ free_noa_wait(struct i915_perf_stream *stream) > i915_vma_unpin_and_release(&stream->noa_wait, 0); > } > > +static bool engine_supports_oa(const struct intel_engine_cs *engine) > +{ > + enum intel_platform platform = INTEL_INFO(engine->i915)->platform; > + > + if (intel_engine_is_virtual(engine)) > + return false; Let's move this check to a different (or a separate) patch (with explanation about OA and virtual engines in case of separate patch). It is strange to see this check in this patch since previously we have only supported render (I think there is only a single render engine so no virtual render engines are possible, correct?). With the changes above this is: Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Is there anything intrinsically wrong with virtual engines that we cannot get OA data for them? Since we get OA data from an OA unit not engines so wondering why this restriction. So if I have a virtual engine consisting of two VDBOX engines attached to a single OAM unit we cannot get OA data for this virtual engine? Or is just that we haven't handled such cases? In that case it is fine to disallow virtual engines till we can support them. Sorry just trying to understand the restriction.
On Wed, Feb 15, 2023 at 07:58:02PM -0800, Dixit, Ashutosh wrote: >On Tue, 14 Feb 2023 16:54:12 -0800, Umesh Nerlige Ramappa wrote: >> >> Add helper to check for supported OA engines. >> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> --- >> drivers/gpu/drm/i915/i915_perf.c | 19 ++++++++++++++++--- >> 1 file changed, 16 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >> index 393a0da8b7c8..a879ae4bf8d7 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -1570,6 +1570,19 @@ free_noa_wait(struct i915_perf_stream *stream) >> i915_vma_unpin_and_release(&stream->noa_wait, 0); >> } >> >> +static bool engine_supports_oa(const struct intel_engine_cs *engine) >> +{ >> + enum intel_platform platform = INTEL_INFO(engine->i915)->platform; >> + >> + if (intel_engine_is_virtual(engine)) >> + return false; > >Let's move this check to a different (or a separate) patch (with >explanation about OA and virtual engines in case of separate patch). It is >strange to see this check in this patch since previously we have only >supported render (I think there is only a single render engine so no >virtual render engines are possible, correct?). will do. > >With the changes above this is: > >Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> > >Is there anything intrinsically wrong with virtual engines that we cannot >get OA data for them? Since we get OA data from an OA unit not engines so >wondering why this restriction. So if I have a virtual engine consisting of >two VDBOX engines attached to a single OAM unit we cannot get OA data for >this virtual engine? > >Or is just that we haven't handled such cases? In that case it is fine to >disallow virtual engines till we can support them. Sorry just trying to >understand the restriction. iirc, we cannot modify the context image for the virtual engine. Something to do with lrc state for such engines. Also OA supports only a concept of one engine instance passed to it, so a virtual engine does not fit the interface definition. Thanks, Umesh
On Thu, Feb 16, 2023 at 09:30:57AM -0800, Umesh Nerlige Ramappa wrote: >On Wed, Feb 15, 2023 at 07:58:02PM -0800, Dixit, Ashutosh wrote: >>On Tue, 14 Feb 2023 16:54:12 -0800, Umesh Nerlige Ramappa wrote: >>> >>>Add helper to check for supported OA engines. >>> >>>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >>>--- >>> drivers/gpu/drm/i915/i915_perf.c | 19 ++++++++++++++++--- >>> 1 file changed, 16 insertions(+), 3 deletions(-) >>> >>>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >>>index 393a0da8b7c8..a879ae4bf8d7 100644 >>>--- a/drivers/gpu/drm/i915/i915_perf.c >>>+++ b/drivers/gpu/drm/i915/i915_perf.c >>>@@ -1570,6 +1570,19 @@ free_noa_wait(struct i915_perf_stream *stream) >>> i915_vma_unpin_and_release(&stream->noa_wait, 0); >>> } >>> >>>+static bool engine_supports_oa(const struct intel_engine_cs *engine) >>>+{ >>>+ enum intel_platform platform = INTEL_INFO(engine->i915)->platform; >>>+ >>>+ if (intel_engine_is_virtual(engine)) >>>+ return false; >> >>Let's move this check to a different (or a separate) patch (with >>explanation about OA and virtual engines in case of separate patch). It is >>strange to see this check in this patch since previously we have only >>supported render (I think there is only a single render engine so no >>virtual render engines are possible, correct?). > >will do. > >> >>With the changes above this is: >> >>Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> >> >>Is there anything intrinsically wrong with virtual engines that we cannot >>get OA data for them? Since we get OA data from an OA unit not engines so >>wondering why this restriction. So if I have a virtual engine consisting of >>two VDBOX engines attached to a single OAM unit we cannot get OA data for >>this virtual engine? >> >>Or is just that we haven't handled such cases? In that case it is fine to >>disallow virtual engines till we can support them. Sorry just trying to >>understand the restriction. > >iirc, we cannot modify the context image for the virtual engine. >Something to do with lrc state for such engines. Also OA supports only >a concept of one engine instance passed to it, so a virtual engine >does not fit the interface definition. Actually, it's not the lrc state, but getting engine_pm wakeref for virtual engine was failing, so we had to use this check, but at this moment the check is not required in OA like you said - there's just one render. Thanks, Umesh > >Thanks, >Umesh
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 393a0da8b7c8..a879ae4bf8d7 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1570,6 +1570,19 @@ free_noa_wait(struct i915_perf_stream *stream) i915_vma_unpin_and_release(&stream->noa_wait, 0); } +static bool engine_supports_oa(const struct intel_engine_cs *engine) +{ + enum intel_platform platform = INTEL_INFO(engine->i915)->platform; + + if (intel_engine_is_virtual(engine)) + return false; + + switch (platform) { + default: + return engine->class == RENDER_CLASS; + } +} + static void i915_oa_stream_destroy(struct i915_perf_stream *stream) { struct i915_perf *perf = stream->perf; @@ -2505,7 +2518,7 @@ 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 (ce->engine->class != RENDER_CLASS) + if (!engine_supports_oa(ce->engine)) continue; /* Otherwise OA settings will be set upon first use */ @@ -2656,7 +2669,7 @@ oa_configure_all_contexts(struct i915_perf_stream *stream, for_each_uabi_engine(engine, i915) { struct intel_context *ce = engine->kernel_context; - if (engine->class != RENDER_CLASS) + if (!engine_supports_oa(ce->engine)) continue; regs[0].value = intel_sseu_make_rpcs(engine->gt, &ce->sseu); @@ -3369,7 +3382,7 @@ void i915_oa_init_reg_state(const struct intel_context *ce, { struct i915_perf_stream *stream; - if (engine->class != RENDER_CLASS) + if (!engine_supports_oa(engine)) return; /* perf.exclusive_stream serialised by lrc_configure_all_contexts() */
Add helper to check for supported OA engines. Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)