[Intel-gfx] [PATCH] drm/i915/perf: Clear out entire reports after reading if not power of 2 size

[Umesh Nerlige Ramappa]

On Mon, May 22, 2023 at 02:50:51PM -0700, Dixit, Ashutosh wrote:
>On Mon, 22 May 2023 14:34:18 -0700, Umesh Nerlige Ramappa wrote:
>>
>> On Mon, May 22, 2023 at 01:17:49PM -0700, Ashutosh Dixit wrote:
>> > Clearing out report id and timestamp as means to detect unlanded reports
>> > only works if report size is power of 2. That is, only when report size is
>> > a sub-multiple of the OA buffer size can we be certain that reports will
>> > land at the same place each time in the OA buffer (after rewind). If report
>> > size is not a power of 2, we need to zero out the entire report to be able
>> > to detect unlanded reports reliably.
>> >
>> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>> > ---
>> > drivers/gpu/drm/i915/i915_perf.c | 17 +++++++++++------
>> > 1 file changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> > index 19d5652300eeb..58284156428dc 100644
>> > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > @@ -877,12 +877,17 @@ static int gen8_append_oa_reports(struct i915_perf_stream *stream,
>> >			stream->oa_buffer.last_ctx_id = ctx_id;
>> >		}
>> >
>> > -		/*
>> > -		 * Clear out the report id and timestamp as a means to detect unlanded
>> > -		 * reports.
>> > -		 */
>> > -		oa_report_id_clear(stream, report32);
>> > -		oa_timestamp_clear(stream, report32);
>> > +		if (is_power_of_2(report_size)) {
>> > +			/*
>> > +			 * Clear out the report id and timestamp as a means
>> > +			 * to detect unlanded reports.
>> > +			 */
>> > +			oa_report_id_clear(stream, report32);
>> > +			oa_timestamp_clear(stream, report32);
>> > +		} else {
>> > +			/* Zero out the entire report */
>> > +			memset(report32, 0, report_size);
>>
>> Indeed, this was a bug. For a minute, I started wondering if this is the
>> issue I am running into with the other patch posted for DG2, but then I see
>> the issue within the first fill of the OA buffer where chunks of the
>> reports are zeroed out, so this is a new issue.
>
>Yes I saw this while reviewing your patch. And also I thought your issue
>was happening on DG2 with power of 2 report size, only on MTL OAM we
>introduce non power of 2 report size.
>
>> lgtm,
>>
>> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>

Maybe this should include Fixes: tag pointing to the patch that 
introduced the OAM non-power-of-2 format.

Umesh

>
>Thanks.
>--
>Ashutosh
>
>>
>> > +		}
>> >	}
>> >
>> >	if (start_offset != *offset) {
>> > --
>> > 2.38.0
>> >