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

Umesh Nerlige Ramappa
2022-09-14T23:37:11Z

On Fri, Sep 09, 2022 at 04:47:36PM -0700, Dixit, Ashutosh wrote:
>On Tue, 23 Aug 2022 13:41:37 -0700, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> 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.
>
>Do you think it is worth defining an interface for GuC to return the sw
>ctx_id it will be using for a ctx, say at ctx registration time?
>
>The scheme implemented in this patch to read the ctx_id is certainly very
>clever, at least to me. But as Lionel was saying is it a agreed upon
>immutable interface? If it is, we can go with this patch.
>
>(Though even then we will need to maintain this code even if in the future
>GuC FW is changed to return the ctx_id in order to preserve backwards
>comptability with previous GuC versions. So maybe better to have a real
>interface between GuC and KMD earlier rather than later?).

Agree, ideally this should be obtained from GuC and properly
synchronized with kmd. OR GuC should provide a way to pin the context id
for such cases so that the id is not stolen/unpinned. Anyways, we need
to follow this up as a JIRA.

I may drop this patch and add a message that OA buffer filtering may be
broken if a gem context is passed.

>
>Also a couple of general comments below.
>
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>> 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));
>
>Actually the RING_EXECLIST_STATUS_HI is MMIO so can be read using say
>ENGINE_READ/intel_uncore_read. The only issue is how to read it when this
>ctx is scheduled which is cleverly solved by the scheme above. But I am not
>sure if there is any other simpler way to do it.
>
>> + 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);
>
>Previously I missed that these ctx_id's for non-GuC cases are just
>constants. How does it work in these cases?

In those cases we use a fixed id for the OA use case:

in gen12_get_render_context_id()
stream->specific_ctx_id = ctx_id & mask

in oa_get_render_ctx_id()
ce->tag = stream->specific_ctx_id;

in __execlists_schedule_in()
ce->lrc.ccid = ce->tag;

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh