Re: [Intel-gfx] [PATCH 01/19] drm/i915/perf: Fix OA filtering logic for GuC mode

[Lionel Landwerlin <lionel.g.landwerlin@intel.com>]

On 15/09/2022 02:13, Umesh Nerlige Ramappa wrote:
> On Wed, Sep 14, 2022 at 03:26:15PM -0700, Umesh Nerlige Ramappa wrote:
>> On Tue, Sep 06, 2022 at 09:39:33PM +0300, Lionel Landwerlin wrote:
>>> On 06/09/2022 20:39, Umesh Nerlige Ramappa wrote:
>>>> On Tue, Sep 06, 2022 at 05:33:00PM +0300, Lionel Landwerlin wrote:
>>>>> On 23/08/2022 23:41, Umesh Nerlige Ramappa wrote:
>>>>>> With GuC mode of submission, GuC is in control of defining the 
>>>>>> context id field
>>>>>> that is part of the OA reports. To filter reports, UMD and KMD 
>>>>>> must know what sw
>>>>>> context id was chosen by GuC. There is not interface between KMD 
>>>>>> and GuC to
>>>>>> determine this, so read the upper-dword of EXECLIST_STATUS to 
>>>>>> filter/squash OA
>>>>>> reports for the specific context.
>>>>>>
>>>>>> Signed-off-by: Umesh Nerlige Ramappa 
>>>>>> <umesh.nerlige.ramappa@intel.com>
>>>>>
>>>>>
>>>>> I assume you checked with GuC that this doesn't change as the 
>>>>> context is running?
>>>>
>>>> Correct.
>>>>
>>>>>
>>>>> With i915/execlist submission mode, we had to ask i915 to pin the 
>>>>> sw_id/ctx_id.
>>>>>
>>>>
>>>> From GuC perspective, the context id can change once KMD 
>>>> de-registers the context and that will not happen while the context 
>>>> is in use.
>>>>
>>>> Thanks,
>>>> Umesh
>>>
>>>
>>> Thanks Umesh,
>>>
>>>
>>> Maybe I should have been more precise in my question :
>>>
>>>
>>> Can the ID change while the i915-perf stream is opened?
>>>
>>> Because the ID not changing while the context is running makes sense.
>>>
>>> But since the number of available IDs is limited to 2k or something 
>>> on Gfx12, it's possible the GuC has to reuse IDs if too many apps 
>>> want to run during the period of time while i915-perf is active and 
>>> filtering.
>>>
>>
>> available guc ids are 64k with 4k reserved for multi-lrc, so GuC may 
>> have to reuse ids once 60k ids are used up.
>
> Spoke to the GuC team again and if there are a lot of contexts (> 60K) 
> running, there is a possibility of the context id being recycled. In 
> that case, the capture would be broken. I would track this as a 
> separate JIRA and follow up on a solution.
>
> From OA use case perspective, are we interested in monitoring just one 
> hardware context? If we make sure this context is not stolen, are we 
> good?
> Thanks,
> Umesh


Yep, we only care about that one ID not changing.


Thanks,

-Lionel


>
>>
>> Thanks,
>> Umesh
>>
>>>
>>> -Lionel
>>>
>>>
>>>>
>>>>>
>>>>> If that's not the case then filtering is broken.
>>>>>
>>>>>
>>>>> -Lionel
>>>>>
>>>>>
>>>>>> ---
>>>>>>  drivers/gpu/drm/i915/gt/intel_lrc.h |   2 +
>>>>>>  drivers/gpu/drm/i915/i915_perf.c    | 141 
>>>>>> ++++++++++++++++++++++++----
>>>>>>  2 files changed, 124 insertions(+), 19 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/gt/intel_lrc.h 
>>>>>> b/drivers/gpu/drm/i915/gt/intel_lrc.h
>>>>>> index a390f0813c8b..7111bae759f3 100644
>>>>>> --- a/drivers/gpu/drm/i915/gt/intel_lrc.h
>>>>>> +++ b/drivers/gpu/drm/i915/gt/intel_lrc.h
>>>>>> @@ -110,6 +110,8 @@ enum {
>>>>>>  #define XEHP_SW_CTX_ID_WIDTH            16
>>>>>>  #define XEHP_SW_COUNTER_SHIFT            58
>>>>>>  #define XEHP_SW_COUNTER_WIDTH            6
>>>>>> +#define GEN12_GUC_SW_CTX_ID_SHIFT        39
>>>>>> +#define GEN12_GUC_SW_CTX_ID_WIDTH        16
>>>>>>  static inline void lrc_runtime_start(struct intel_context *ce)
>>>>>>  {
>>>>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c 
>>>>>> b/drivers/gpu/drm/i915/i915_perf.c
>>>>>> index f3c23fe9ad9c..735244a3aedd 100644
>>>>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>>>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>>>>> @@ -1233,6 +1233,125 @@ static struct intel_context 
>>>>>> *oa_pin_context(struct i915_perf_stream *stream)
>>>>>>      return stream->pinned_ctx;
>>>>>>  }
>>>>>> +static int
>>>>>> +__store_reg_to_mem(struct i915_request *rq, i915_reg_t reg, u32 
>>>>>> ggtt_offset)
>>>>>> +{
>>>>>> +    u32 *cs, cmd;
>>>>>> +
>>>>>> +    cmd = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
>>>>>> +    if (GRAPHICS_VER(rq->engine->i915) >= 8)
>>>>>> +        cmd++;
>>>>>> +
>>>>>> +    cs = intel_ring_begin(rq, 4);
>>>>>> +    if (IS_ERR(cs))
>>>>>> +        return PTR_ERR(cs);
>>>>>> +
>>>>>> +    *cs++ = cmd;
>>>>>> +    *cs++ = i915_mmio_reg_offset(reg);
>>>>>> +    *cs++ = ggtt_offset;
>>>>>> +    *cs++ = 0;
>>>>>> +
>>>>>> +    intel_ring_advance(rq, cs);
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>> +static int
>>>>>> +__read_reg(struct intel_context *ce, i915_reg_t reg, u32 
>>>>>> ggtt_offset)
>>>>>> +{
>>>>>> +    struct i915_request *rq;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    rq = i915_request_create(ce);
>>>>>> +    if (IS_ERR(rq))
>>>>>> +        return PTR_ERR(rq);
>>>>>> +
>>>>>> +    i915_request_get(rq);
>>>>>> +
>>>>>> +    err = __store_reg_to_mem(rq, reg, ggtt_offset);
>>>>>> +
>>>>>> +    i915_request_add(rq);
>>>>>> +    if (!err && i915_request_wait(rq, 0, HZ / 2) < 0)
>>>>>> +        err = -ETIME;
>>>>>> +
>>>>>> +    i915_request_put(rq);
>>>>>> +
>>>>>> +    return err;
>>>>>> +}
>>>>>> +
>>>>>> +static int
>>>>>> +gen12_guc_sw_ctx_id(struct intel_context *ce, u32 *ctx_id)
>>>>>> +{
>>>>>> +    struct i915_vma *scratch;
>>>>>> +    u32 *val;
>>>>>> +    int err;
>>>>>> +
>>>>>> +    scratch = 
>>>>>> __vm_create_scratch_for_read_pinned(&ce->engine->gt->ggtt->vm, 4);
>>>>>> +    if (IS_ERR(scratch))
>>>>>> +        return PTR_ERR(scratch);
>>>>>> +
>>>>>> +    err = i915_vma_sync(scratch);
>>>>>> +    if (err)
>>>>>> +        goto err_scratch;
>>>>>> +
>>>>>> +    err = __read_reg(ce, 
>>>>>> RING_EXECLIST_STATUS_HI(ce->engine->mmio_base),
>>>>>> +             i915_ggtt_offset(scratch));
>>>>>> +    if (err)
>>>>>> +        goto err_scratch;
>>>>>> +
>>>>>> +    val = i915_gem_object_pin_map_unlocked(scratch->obj, 
>>>>>> I915_MAP_WB);
>>>>>> +    if (IS_ERR(val)) {
>>>>>> +        err = PTR_ERR(val);
>>>>>> +        goto err_scratch;
>>>>>> +    }
>>>>>> +
>>>>>> +    *ctx_id = *val;
>>>>>> +    i915_gem_object_unpin_map(scratch->obj);
>>>>>> +
>>>>>> +err_scratch:
>>>>>> +    i915_vma_unpin_and_release(&scratch, 0);
>>>>>> +    return err;
>>>>>> +}
>>>>>> +
>>>>>> +/*
>>>>>> + * For execlist mode of submission, pick an unused context id
>>>>>> + * 0 - (NUM_CONTEXT_TAG -1) are used by other contexts
>>>>>> + * XXX_MAX_CONTEXT_HW_ID is used by idle context
>>>>>> + *
>>>>>> + * For GuC mode of submission read context id from the upper 
>>>>>> dword of the
>>>>>> + * EXECLIST_STATUS register.
>>>>>> + */
>>>>>> +static int gen12_get_render_context_id(struct i915_perf_stream 
>>>>>> *stream)
>>>>>> +{
>>>>>> +    u32 ctx_id, mask;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    if (intel_engine_uses_guc(stream->engine)) {
>>>>>> +        ret = gen12_guc_sw_ctx_id(stream->pinned_ctx, &ctx_id);
>>>>>> +        if (ret)
>>>>>> +            return ret;
>>>>>> +
>>>>>> +        mask = ((1U << GEN12_GUC_SW_CTX_ID_WIDTH) - 1) <<
>>>>>> +            (GEN12_GUC_SW_CTX_ID_SHIFT - 32);
>>>>>> +    } else if (GRAPHICS_VER_FULL(stream->engine->i915) >= 
>>>>>> IP_VER(12, 50)) {
>>>>>> +        ctx_id = (XEHP_MAX_CONTEXT_HW_ID - 1) <<
>>>>>> +            (XEHP_SW_CTX_ID_SHIFT - 32);
>>>>>> +
>>>>>> +        mask = ((1U << XEHP_SW_CTX_ID_WIDTH) - 1) <<
>>>>>> +            (XEHP_SW_CTX_ID_SHIFT - 32);
>>>>>> +    } else {
>>>>>> +        ctx_id = (GEN12_MAX_CONTEXT_HW_ID - 1) <<
>>>>>> +             (GEN11_SW_CTX_ID_SHIFT - 32);
>>>>>> +
>>>>>> +        mask = ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) <<
>>>>>> +            (GEN11_SW_CTX_ID_SHIFT - 32);
>>>>>> +    }
>>>>>> +    stream->specific_ctx_id = ctx_id & mask;
>>>>>> +    stream->specific_ctx_id_mask = mask;
>>>>>> +
>>>>>> +    return 0;
>>>>>> +}
>>>>>> +
>>>>>>  /**
>>>>>>   * oa_get_render_ctx_id - determine and hold ctx hw id
>>>>>>   * @stream: An i915-perf stream opened for OA metrics
>>>>>> @@ -1246,6 +1365,7 @@ static struct intel_context 
>>>>>> *oa_pin_context(struct i915_perf_stream *stream)
>>>>>>  static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>>>>>>  {
>>>>>>      struct intel_context *ce;
>>>>>> +    int ret = 0;
>>>>>>      ce = oa_pin_context(stream);
>>>>>>      if (IS_ERR(ce))
>>>>>> @@ -1292,24 +1412,7 @@ static int oa_get_render_ctx_id(struct 
>>>>>> i915_perf_stream *stream)
>>>>>>      case 11:
>>>>>>      case 12:
>>>>>> -        if (GRAPHICS_VER_FULL(ce->engine->i915) >= IP_VER(12, 
>>>>>> 50)) {
>>>>>> -            stream->specific_ctx_id_mask =
>>>>>> -                ((1U << XEHP_SW_CTX_ID_WIDTH) - 1) <<
>>>>>> -                (XEHP_SW_CTX_ID_SHIFT - 32);
>>>>>> -            stream->specific_ctx_id =
>>>>>> -                (XEHP_MAX_CONTEXT_HW_ID - 1) <<
>>>>>> -                (XEHP_SW_CTX_ID_SHIFT - 32);
>>>>>> -        } else {
>>>>>> -            stream->specific_ctx_id_mask =
>>>>>> -                ((1U << GEN11_SW_CTX_ID_WIDTH) - 1) << 
>>>>>> (GEN11_SW_CTX_ID_SHIFT - 32);
>>>>>> -            /*
>>>>>> -             * Pick an unused context id
>>>>>> -             * 0 - BITS_PER_LONG 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);
>>>>>> -        }
>>>>>> +        ret = gen12_get_render_context_id(stream);
>>>>>>          break;
>>>>>>      default:
>>>>>> @@ -1323,7 +1426,7 @@ static int oa_get_render_ctx_id(struct 
>>>>>> i915_perf_stream *stream)
>>>>>>          stream->specific_ctx_id,
>>>>>>          stream->specific_ctx_id_mask);
>>>>>> -    return 0;
>>>>>> +    return ret;
>>>>>>  }
>>>>>>  /**
>>>>>
>>>>>
>>>