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

[Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>]

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@xxxxxxxxx>
> > > > >
> > > > >
> > > > > 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;
> > > > > >  }
> > > > > >  /**