Re: [Intel-gfx] [PATCH v3 04/16] drm/i915/perf: Determine gen12 oa ctx offset at runtime

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

On Mon, Oct 10, 2022 at 05:17:44PM -0700, Dixit, Ashutosh wrote:
>On Mon, 10 Oct 2022 11:14:22 -0700, Umesh Nerlige Ramappa wrote:
>
>Hi Umesh,
>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index cd57b5836386..b292aa39633e 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -1358,6 +1358,68 @@ static int gen12_get_render_context_id(struct i915_perf_stream *stream)
>>	return 0;
>>  }
>>
>> +#define valid_oactxctrl_offset(x) ((x) && (x) != U32_MAX)
>> +static bool oa_find_reg_in_lri(u32 *state, u32 reg, u32 *offset, u32 end)
>> +{
>> +	u32 idx = *offset;
>> +	u32 len = min(MI_LRI_LEN(state[idx]) + idx, end);
>> +	bool found = false;
>
>My recommendation would be to put something like this here:
>
>	if (MI_LRI_LEN(state[idx]) & 0x1)
>		drm_warn("MI_LRI instruction with odd length\n");
>
>Because we expect the MI_LRI length to even and I am not sure what is in
>the context image. But maybe not needed?

will add.

>
>> @@ -2436,15 +2514,18 @@ static int gen12_configure_oar_context(struct i915_perf_stream *stream,
>>		},
>>	};
>>
>> -	/* Modify the context image of pinned context with regs_context*/
>> -	err = intel_context_lock_pinned(ce);
>> -	if (err)
>> -		return err;
>> +	/* Modify the context image of pinned context with regs_context */
>> +	if (valid_oactxctrl_offset(offset)) {
>
>As I said before, this check is not needed, if we didn't have valid a
>offset we should return error from oa_get_render_ctx_id. For if we have
>this check and offset is not valid, can we skip this code and will things
>still work? Or do we need to return an error from
>gen12_configure_oar_context?

I missed that from the previous comments. Will add.

>
>> +		err = intel_context_lock_pinned(ce);
>> +		if (err)
>> +			return err;
>>
>> -	err = gen8_modify_context(ce, regs_context, ARRAY_SIZE(regs_context));
>> -	intel_context_unlock_pinned(ce);
>> -	if (err)
>> -		return err;
>> +		err = gen8_modify_context(ce, regs_context,
>> +					  ARRAY_SIZE(regs_context));
>> +		intel_context_unlock_pinned(ce);
>> +		if (err)
>> +			return err;
>> +	}
>
>With the if statement removed or handled otherwise, this is:
>
>Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com>