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

[Lionel Landwerlin]

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