Re: [Intel-gfx] [PATCH v2] drm/i915/perf: Do not parse context image for HSW

[Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>]

On Tue, Nov 22, 2022 at 07:25:36PM -0800, Dixit, Ashutosh wrote:
>On Tue, 22 Nov 2022 18:07:00 -0800, Umesh Nerlige Ramappa wrote:
>>
>
>Hi Umesh,
>
>> An earlier commit introduced a mechanism to parse the context image to
>> find the OA context control offset. This resulted in an NPD on haswell
>> when gem_context was passed into i915_perf_open_ioctl params. Haswell
>> does not support logical ring contexts, so ensure that the context image
>> is parsed only for platforms with logical ring contexts and also
>> validate lrc_reg_state.
>>
>> v2: Fix build failure
>>
>> Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7432
>> Fixes: a5c3a3cbf029 ("drm/i915/perf: Determine gen12 oa ctx offset at runtime")
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_perf.c | 6 +++++-
>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 00e09bb18b13..dbd785974f20 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1383,6 +1383,9 @@ static u32 oa_context_image_offset(struct intel_context *ce, u32 reg)
>>	u32 offset, len = (ce->engine->context_size - PAGE_SIZE) / 4;
>>	u32 *state = ce->lrc_reg_state;
>>
>> +	if (drm_WARN_ON(&ce->engine->i915->drm, state == NULL))
>> +		return U32_MAX;
>> +
>
>So if we didn't add the HAS_LOGICAL_RING_CONTEXTS check below state would
>be NULL correct? I couldn't figure out how it is NULL on HSW looking at the
>code (with the context image pin/unpin).

Ref: intel_engines_init()

Haswell is using ring submission. While it does have a context image or 
ce->state, there is no lrc state offset here. I think lrc is an execlist 
concept (used only for execlist and guc submission).


>>			/*
>> @@ -1447,7 +1450,8 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
>>	if (IS_ERR(ce))
>>		return PTR_ERR(ce);
>>
>> -	if (engine_supports_mi_query(stream->engine)) {
>> +	if (engine_supports_mi_query(stream->engine) &&
>> +	    HAS_LOGICAL_RING_CONTEXTS(stream->perf->i915)) {
>
>This check looks fine since we seem to be looking inside ce->lrc_reg_state
>for oactxctrl.

The earlier code also does not define perf->ctx_oactxctrl_offset for 
haswell. Ref: i915_perf_init().

>
>Overall looks fine so this is:
>
>Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Thanks,
Umesh
>
>
>>		/*
>>		 * We are enabling perf query here. If we don't find the context
>>		 * offset here, just return an error.
>> --
>> 2.36.1
>>