| Message ID | 20200124013701.40609-1-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/perf: Fix OA context id overlap with idle context id | expand |
On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote: > Engine context pinned in perf OA was set to same context id as > the idle context. Set the context id to an unused value. > > Clear the sw context id field in lrc descriptor before ORing with > ce->tag (Chris) > > Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- > drivers/gpu/drm/i915/i915_perf.c | 9 +++++++-- > 2 files changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c > index a13a8c4b65ab..cf6c43bd540a 100644 > --- a/drivers/gpu/drm/i915/gt/intel_lrc.c > +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c > @@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request *rq) > if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) > execlists_check_context(ce, engine); > > + ce->lrc_desc &= ~GENMASK_ULL(47, 37); > if (ce->tag) { > /* Use a fixed tag for OA and friends */ > ce->lrc_desc |= (u64)ce->tag << 32; > } else { > /* We don't need a strict matching tag, just different values */ > - ce->lrc_desc &= ~GENMASK_ULL(47, 37); I guess you can remove the line just above. > ce->lrc_desc |= > (u64)(++engine->context_tag % NUM_CONTEXT_TAG) << > GEN11_SW_CTX_ID_SHIFT; > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index 3c4647054557..5bd878c64504 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) > case 12: { > stream->specific_ctx_id_mask = > ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); > - stream->specific_ctx_id = stream->specific_ctx_id_mask; > + /* Pick an unused context id > + * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts > + * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context > + */ > + stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); > + BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); Arg yeah, we can't use an id that has all bits to 1 because that matches the idle value in the OA reports :/ This also affects gen8-10 cases (afaik). Thanks for spotting this! -Lionel > break; > } > > @@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) > MISSING_CASE(INTEL_GEN(ce->engine->i915)); > } > > - ce->tag = stream->specific_ctx_id_mask; > + ce->tag = stream->specific_ctx_id; > > DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", > stream->specific_ctx_id,
On 25/01/2020 03:37, Lionel Landwerlin wrote: > On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote: >> Engine context pinned in perf OA was set to same context id as >> the idle context. Set the context id to an unused value. >> >> Clear the sw context id field in lrc descriptor before ORing with >> ce->tag (Chris) >> >> Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 I think the Fixes: tag is to point to another commit. And I think you want to point to the commit that you're fixing. Not sure what you should use gitlab MRs. -Lionel >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >> --- >> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- >> drivers/gpu/drm/i915/i915_perf.c | 9 +++++++-- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >> b/drivers/gpu/drm/i915/gt/intel_lrc.c >> index a13a8c4b65ab..cf6c43bd540a 100644 >> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >> @@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request *rq) >> if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >> execlists_check_context(ce, engine); >> + ce->lrc_desc &= ~GENMASK_ULL(47, 37); >> if (ce->tag) { >> /* Use a fixed tag for OA and friends */ >> ce->lrc_desc |= (u64)ce->tag << 32; >> } else { >> /* We don't need a strict matching tag, just different >> values */ >> - ce->lrc_desc &= ~GENMASK_ULL(47, 37); > I guess you can remove the line just above. >> ce->lrc_desc |= >> (u64)(++engine->context_tag % NUM_CONTEXT_TAG) << >> GEN11_SW_CTX_ID_SHIFT; >> diff --git a/drivers/gpu/drm/i915/i915_perf.c >> b/drivers/gpu/drm/i915/i915_perf.c >> index 3c4647054557..5bd878c64504 100644 >> --- a/drivers/gpu/drm/i915/i915_perf.c >> +++ b/drivers/gpu/drm/i915/i915_perf.c >> @@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct >> i915_perf_stream *stream) >> case 12: { >> stream->specific_ctx_id_mask = >> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << >> (GEN11_SW_CTX_ID_SHIFT - 32); >> - stream->specific_ctx_id = stream->specific_ctx_id_mask; >> + /* Pick an unused context id >> + * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >> + * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >> + */ >> + stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << >> (GEN11_SW_CTX_ID_SHIFT - 32); >> + BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); > > > Arg yeah, we can't use an id that has all bits to 1 because that > matches the idle value in the OA reports :/ > > This also affects gen8-10 cases (afaik). > > > Thanks for spotting this! > > > -Lionel > > >> break; >> } >> @@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct >> i915_perf_stream *stream) >> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >> } >> - ce->tag = stream->specific_ctx_id_mask; >> + ce->tag = stream->specific_ctx_id; >> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >> stream->specific_ctx_id, > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoting Lionel Landwerlin (2020-01-25 15:02:21) > On 25/01/2020 03:37, Lionel Landwerlin wrote: > > On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote: > >> Engine context pinned in perf OA was set to same context id as > >> the idle context. Set the context id to an unused value. > >> > >> Clear the sw context id field in lrc descriptor before ORing with > >> ce->tag (Chris) > >> > >> Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 > > I think the Fixes: tag is to point to another commit. And I think you > want to point to the commit that you're fixing. > > Not sure what you should use gitlab MRs. We're using Closes: https://gitlab.fd.o/issues/# as a replacement for Bugzilla:. -Chris
On Sat, Jan 25, 2020 at 03:37:38AM +0200, Lionel Landwerlin wrote: >On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote: >>Engine context pinned in perf OA was set to same context id as >>the idle context. Set the context id to an unused value. >> >>Clear the sw context id field in lrc descriptor before ORing with >>ce->tag (Chris) >> >>Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 >>Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >>--- >> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- >> drivers/gpu/drm/i915/i915_perf.c | 9 +++++++-- >> 2 files changed, 8 insertions(+), 3 deletions(-) >> >>diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c >>index a13a8c4b65ab..cf6c43bd540a 100644 >>--- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>+++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>@@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request *rq) >> if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >> execlists_check_context(ce, engine); >>+ ce->lrc_desc &= ~GENMASK_ULL(47, 37); >> if (ce->tag) { >> /* Use a fixed tag for OA and friends */ >> ce->lrc_desc |= (u64)ce->tag << 32; >> } else { >> /* We don't need a strict matching tag, just different values */ >>- ce->lrc_desc &= ~GENMASK_ULL(47, 37); >I guess you can remove the line just above. Not sure what you mean, the line is moved from here to above the if. >> ce->lrc_desc |= >> (u64)(++engine->context_tag % NUM_CONTEXT_TAG) << >> GEN11_SW_CTX_ID_SHIFT; >>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c >>index 3c4647054557..5bd878c64504 100644 >>--- a/drivers/gpu/drm/i915/i915_perf.c >>+++ b/drivers/gpu/drm/i915/i915_perf.c >>@@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) >> case 12: { >> stream->specific_ctx_id_mask = >> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); >>- stream->specific_ctx_id = stream->specific_ctx_id_mask; >>+ /* Pick an unused context id >>+ * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >>+ * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >>+ */ >>+ stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); >>+ BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); > > >Arg yeah, we can't use an id that has all bits to 1 because that >matches the idle value in the OA reports :/ > >This also affects gen8-10 cases (afaik). For gen8-10, I did not see a specific definition for an idle context id. The from/to idle context switches are indicated by dedicated bits in the CSB instead (from spec). Thanks, Umesh > > >Thanks for spotting this! > > >-Lionel > > >> break; >> } >>@@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) >> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >> } >>- ce->tag = stream->specific_ctx_id_mask; >>+ ce->tag = stream->specific_ctx_id; >> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >> stream->specific_ctx_id, > >
On 27/01/2020 07:30, Umesh Nerlige Ramappa wrote: > On Sat, Jan 25, 2020 at 03:37:38AM +0200, Lionel Landwerlin wrote: >> On 24/01/2020 03:37, Umesh Nerlige Ramappa wrote: >>> Engine context pinned in perf OA was set to same context id as >>> the idle context. Set the context id to an unused value. >>> >>> Clear the sw context id field in lrc descriptor before ORing with >>> ce->tag (Chris) >>> >>> Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 >>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> >>> --- >>> drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- >>> drivers/gpu/drm/i915/i915_perf.c | 9 +++++++-- >>> 2 files changed, 8 insertions(+), 3 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c >>> b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> index a13a8c4b65ab..cf6c43bd540a 100644 >>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.c >>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c >>> @@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request >>> *rq) >>> if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) >>> execlists_check_context(ce, engine); >>> + ce->lrc_desc &= ~GENMASK_ULL(47, 37); >>> if (ce->tag) { >>> /* Use a fixed tag for OA and friends */ >>> ce->lrc_desc |= (u64)ce->tag << 32; >>> } else { >>> /* We don't need a strict matching tag, just different >>> values */ >>> - ce->lrc_desc &= ~GENMASK_ULL(47, 37); >> I guess you can remove the line just above. > > Not sure what you mean, the line is moved from here to above the if. Oh sorry, Looking at the responding email, the '-' didn't appear :( > >>> ce->lrc_desc |= >>> (u64)(++engine->context_tag % NUM_CONTEXT_TAG) << >>> GEN11_SW_CTX_ID_SHIFT; >>> diff --git a/drivers/gpu/drm/i915/i915_perf.c >>> b/drivers/gpu/drm/i915/i915_perf.c >>> index 3c4647054557..5bd878c64504 100644 >>> --- a/drivers/gpu/drm/i915/i915_perf.c >>> +++ b/drivers/gpu/drm/i915/i915_perf.c >>> @@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct >>> i915_perf_stream *stream) >>> case 12: { >>> stream->specific_ctx_id_mask = >>> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << >>> (GEN11_SW_CTX_ID_SHIFT - 32); >>> - stream->specific_ctx_id = stream->specific_ctx_id_mask; >>> + /* Pick an unused context id >>> + * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >>> + * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >>> + */ >>> + stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << >>> (GEN11_SW_CTX_ID_SHIFT - 32); >>> + BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); >> >> >> Arg yeah, we can't use an id that has all bits to 1 because that >> matches the idle value in the OA reports :/ >> >> This also affects gen8-10 cases (afaik). > > For gen8-10, I did not see a specific definition for an idle context > id. The from/to idle context switches are indicated by dedicated bits > in the CSB instead (from spec). I meant that I remember the periodic OA reports when HW is idle to have the contex_id=0xffffffff. I could remember wrong :/ -Lionel > > Thanks, > Umesh > >> >> >> Thanks for spotting this! >> >> >> -Lionel >> >> >>> break; >>> } >>> @@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct >>> i915_perf_stream *stream) >>> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >>> } >>> - ce->tag = stream->specific_ctx_id_mask; >>> + ce->tag = stream->specific_ctx_id; >>> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >>> stream->specific_ctx_id, >> >>
On Mon, Jan 27, 2020 at 11:16:32AM +0200, Lionel Landwerlin wrote: [snip] >>>>--- a/drivers/gpu/drm/i915/i915_perf.c >>>>+++ b/drivers/gpu/drm/i915/i915_perf.c >>>>@@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct >>>>i915_perf_stream *stream) >>>> case 12: { >>>> stream->specific_ctx_id_mask = >>>> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << >>>>(GEN11_SW_CTX_ID_SHIFT - 32); >>>>- stream->specific_ctx_id = stream->specific_ctx_id_mask; >>>>+ /* Pick an unused context id >>>>+ * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >>>>+ * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >>>>+ */ >>>>+ stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) >>>><< (GEN11_SW_CTX_ID_SHIFT - 32); >>>>+ BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); >>> >>> >>>Arg yeah, we can't use an id that has all bits to 1 because that >>>matches the idle value in the OA reports :/ >>> >>>This also affects gen8-10 cases (afaik). >> >>For gen8-10, I did not see a specific definition for an idle context >>id. The from/to idle context switches are indicated by dedicated >>bits in the CSB instead (from spec). > > >I meant that I remember the periodic OA reports when HW is idle to >have the contex_id=0xffffffff. For these gens we use 0x1fffff as the context id. Before we return reports to the user, we are setting context id to 0xffffffff for invalid and irrelevant contexts. Thanks, Umesh > >I could remember wrong :/ > > >-Lionel > > >> >>Thanks, >>Umesh >> >>> >>> >>>Thanks for spotting this! >>> >>> >>>-Lionel >>> >>> >>>> break; >>>> } >>>>@@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct >>>>i915_perf_stream *stream) >>>> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >>>> } >>>>- ce->tag = stream->specific_ctx_id_mask; >>>>+ ce->tag = stream->specific_ctx_id; >>>> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >>>> stream->specific_ctx_id, >>> >>> >
On 31/01/2020 01:54, Umesh Nerlige Ramappa wrote: > On Mon, Jan 27, 2020 at 11:16:32AM +0200, Lionel Landwerlin wrote: > > [snip] > >>>>> --- a/drivers/gpu/drm/i915/i915_perf.c >>>>> +++ b/drivers/gpu/drm/i915/i915_perf.c >>>>> @@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct >>>>> i915_perf_stream *stream) >>>>> case 12: { >>>>> stream->specific_ctx_id_mask = >>>>> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << >>>>> (GEN11_SW_CTX_ID_SHIFT - 32); >>>>> - stream->specific_ctx_id = stream->specific_ctx_id_mask; >>>>> + /* Pick an unused context id >>>>> + * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >>>>> + * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >>>>> + */ >>>>> + stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) >>>>> << (GEN11_SW_CTX_ID_SHIFT - 32); >>>>> + BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < >>>>> NUM_CONTEXT_TAG); >>>> >>>> >>>> Arg yeah, we can't use an id that has all bits to 1 because that >>>> matches the idle value in the OA reports :/ >>>> >>>> This also affects gen8-10 cases (afaik). >>> >>> For gen8-10, I did not see a specific definition for an idle context >>> id. The from/to idle context switches are indicated by dedicated >>> bits in the CSB instead (from spec). >> >> >> I meant that I remember the periodic OA reports when HW is idle to >> have the contex_id=0xffffffff. > > For these gens we use 0x1fffff as the context id. Before we return > reports to the user, we are setting context id to 0xffffffff for > invalid and irrelevant contexts. > > Thanks, > Umesh Sorry, I've been a bit out of the loop on OA reports lately. I just noticed that the context valid bit is not checked on gen12 anymore. The documentation is really horrible, but BSpec 52198 seems to indicate the bit is still around. Could it be the source of the issue? Thanks for your help :) -Lionel >> >> I could remember wrong :/ >> >> >> -Lionel >> >> >>> >>> Thanks, >>> Umesh >>> >>>> >>>> >>>> Thanks for spotting this! >>>> >>>> >>>> -Lionel >>>> >>>> >>>>> break; >>>>> } >>>>> @@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct >>>>> i915_perf_stream *stream) >>>>> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >>>>> } >>>>> - ce->tag = stream->specific_ctx_id_mask; >>>>> + ce->tag = stream->specific_ctx_id; >>>>> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >>>>> stream->specific_ctx_id, >>>> >>>> >>
On Fri, Jan 31, 2020 at 01:00:54PM +0200, Lionel Landwerlin wrote: >On 31/01/2020 01:54, Umesh Nerlige Ramappa wrote: >>On Mon, Jan 27, 2020 at 11:16:32AM +0200, Lionel Landwerlin wrote: >> >>[snip] >> >>>>>>--- a/drivers/gpu/drm/i915/i915_perf.c >>>>>>+++ b/drivers/gpu/drm/i915/i915_perf.c >>>>>>@@ -1323,7 +1323,12 @@ static int >>>>>>oa_get_render_ctx_id(struct i915_perf_stream *stream) >>>>>> case 12: { >>>>>> stream->specific_ctx_id_mask = >>>>>> ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << >>>>>>(GEN11_SW_CTX_ID_SHIFT - 32); >>>>>>- stream->specific_ctx_id = stream->specific_ctx_id_mask; >>>>>>+ /* Pick an unused context id >>>>>>+ * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts >>>>>>+ * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context >>>>>>+ */ >>>>>>+ stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID >>>>>>- 1) << (GEN11_SW_CTX_ID_SHIFT - 32); >>>>>>+ BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < >>>>>>NUM_CONTEXT_TAG); >>>>> >>>>> >>>>>Arg yeah, we can't use an id that has all bits to 1 because >>>>>that matches the idle value in the OA reports :/ >>>>> >>>>>This also affects gen8-10 cases (afaik). >>>> >>>>For gen8-10, I did not see a specific definition for an idle >>>>context id. The from/to idle context switches are indicated by >>>>dedicated bits in the CSB instead (from spec). >>> >>> >>>I meant that I remember the periodic OA reports when HW is idle to >>>have the contex_id=0xffffffff. >> >>For these gens we use 0x1fffff as the context id. Before we return >>reports to the user, we are setting context id to 0xffffffff for >>invalid and irrelevant contexts. >> >>Thanks, >>Umesh > > >Sorry, I've been a bit out of the loop on OA reports lately. > >I just noticed that the context valid bit is not checked on gen12 anymore. > >The documentation is really horrible, but BSpec 52198 seems to >indicate the bit is still around. I am looking at the first table describing the report ID in 52198 which has TGL next to it. I don't see a definition for this bit. I will try to ask around to see if that's not the case. > > >Could it be the source of the issue? You mean - seeing 0xffffffff during idle in periodic reports? Thanks, Umesh > > >Thanks for your help :) > > >-Lionel > > >>> >>>I could remember wrong :/ >>> >>> >>>-Lionel >>> >>> >>>> >>>>Thanks, >>>>Umesh >>>> >>>>> >>>>> >>>>>Thanks for spotting this! >>>>> >>>>> >>>>>-Lionel >>>>> >>>>> >>>>>> break; >>>>>> } >>>>>>@@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct >>>>>>i915_perf_stream *stream) >>>>>> MISSING_CASE(INTEL_GEN(ce->engine->i915)); >>>>>> } >>>>>>- ce->tag = stream->specific_ctx_id_mask; >>>>>>+ ce->tag = stream->specific_ctx_id; >>>>>> DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", >>>>>> stream->specific_ctx_id, >>>>> >>>>> >>> >
diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.c b/drivers/gpu/drm/i915/gt/intel_lrc.c index a13a8c4b65ab..cf6c43bd540a 100644 --- a/drivers/gpu/drm/i915/gt/intel_lrc.c +++ b/drivers/gpu/drm/i915/gt/intel_lrc.c @@ -1211,12 +1211,12 @@ __execlists_schedule_in(struct i915_request *rq) if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) execlists_check_context(ce, engine); + ce->lrc_desc &= ~GENMASK_ULL(47, 37); if (ce->tag) { /* Use a fixed tag for OA and friends */ ce->lrc_desc |= (u64)ce->tag << 32; } else { /* We don't need a strict matching tag, just different values */ - ce->lrc_desc &= ~GENMASK_ULL(47, 37); ce->lrc_desc |= (u64)(++engine->context_tag % NUM_CONTEXT_TAG) << GEN11_SW_CTX_ID_SHIFT; diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c index 3c4647054557..5bd878c64504 100644 --- a/drivers/gpu/drm/i915/i915_perf.c +++ b/drivers/gpu/drm/i915/i915_perf.c @@ -1323,7 +1323,12 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) case 12: { stream->specific_ctx_id_mask = ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); - stream->specific_ctx_id = stream->specific_ctx_id_mask; + /* Pick an unused context id + * 0 - (NUM_CONTEXT_TAG - 1) are used by other contexts + * GEN12_MAX_CONTEXT_HW_ID (0x7ff) is used by idle context + */ + stream->specific_ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) << (GEN11_SW_CTX_ID_SHIFT - 32); + BUILD_BUG_ON((GEN12_MAX_CONTEXT_HW_ID - 1) < NUM_CONTEXT_TAG); break; } @@ -1331,7 +1336,7 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream) MISSING_CASE(INTEL_GEN(ce->engine->i915)); } - ce->tag = stream->specific_ctx_id_mask; + ce->tag = stream->specific_ctx_id; DRM_DEBUG_DRIVER("filtering on ctx_id=0x%x ctx_id_mask=0x%x\n", stream->specific_ctx_id,
Engine context pinned in perf OA was set to same context id as the idle context. Set the context id to an unused value. Clear the sw context id field in lrc descriptor before ORing with ce->tag (Chris) Fixes: https://gitlab.freedesktop.org/drm/intel/issues/756 Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/gt/intel_lrc.c | 2 +- drivers/gpu/drm/i915/i915_perf.c | 9 +++++++-- 2 files changed, 8 insertions(+), 3 deletions(-)