[Intel-gfx] [PATCH] drm/i915/perf: Fix OA context id overlap with idle context id

[Lionel Landwerlin]

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 at 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,
>>
>>