Re: [Intel-gfx] [PATCH 3/3] drm/i915/perf: Initialize gen12 OA buffer unconditionally

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

On Fri, Sep 08, 2023 at 06:24:16PM -0700, Dixit, Ashutosh wrote:
>On Fri, 08 Sep 2023 18:16:26 -0700, Ashutosh Dixit wrote:
>>
>
>Hi Umesh,
>
>> From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>
>> Correct values for OAR counters are still dependent on enabling the
>> GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
>> bit means OAG unit will write reports to the OAG buffer, so
>> initialize the OAG buffer unconditionally for all use cases.
>>
>> BSpec: 46822
>>
>> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_perf.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> index 1347e4ec9dd5a..30cf37d6e79be 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -3032,12 +3032,12 @@ static void gen12_oa_enable(struct i915_perf_stream *stream)
>>	u32 val;
>>
>>	/*
>> -	 * If we don't want OA reports from the OA buffer, then we don't even
>> -	 * need to program the OAG unit.
>> +	 * BSpec: 46822
>> +	 * Correct values for OAR counters are still dependent on enabling the
>> +	 * GEN12_OAG_OACONTROL_OA_COUNTER_ENABLE in OAG_OACONTROL. Enabling this
>> +	 * bit means OAG unit will write reports to the OAG buffer, so
>> +	 * initialize the OAG buffer correctly.
>>	 */
>> -	if (!(stream->sample_flags & SAMPLE_OA_REPORT))
>> -		return;
>> -
>>	gen12_init_oa_buffer(stream);
>>
>>	regs = __oa_regs(stream);
>
>Looks like this should be needed, I can R-b it.
>
>However, gen12_test_mi_rpc IGT says:
>
>	/* OA unit configuration:
>	 * DRM_I915_PERF_PROP_SAMPLE_OA is no longer required for Gen12
>	 * because the OAR unit increments counters only for the
>	 * relevant context. No other parameters are needed since we do
>	 * not rely on the OA buffer anymore to normalize the counter
>	 * values.
>	 */

That's wrong. When TGL support was added, this was misunderstood and I 
removed the OAR-OAG dependency. Ideally we should enforce user to pass 
SAMPLE_OA always, but now that will break uabi.

For for the OAR case, let's just enable OAG unconditionally so that the 
OAR counters tick correctly. While we do that, we should disable all 
events that trigger a report into the OA buffer. In addition, I would 
also allocate the smallest OA buffer size for this case, so that memory 
impact is low.

Needs a Fixes tag with the commit that enabled OA for TGL.

Regards,
Umesh

>
>So gen12_test_mi_rpc doesn't set DRM_I915_PERF_PROP_SAMPLE_OA and also
>seems to be passing in CI (don't see it but there seem to be no open
>bugs). Thoughts?
>
>Thanks.
>--
>Ashutosh