Re: [Intel-gfx] [PATCH 06/19] drm/i915/perf: Use helpers to process reports w.r.t. OA buffer size

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

On Wed, Sep 14, 2022 at 09:04:10AM -0700, Dixit, Ashutosh wrote:
>On Tue, 23 Aug 2022 13:41:42 -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 6fc4f0d8fc5a..bbf1c574f393 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -385,6 +385,21 @@ static struct ctl_table_header *sysctl_header;
>>
>>  static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer);
>>
>> +static u32 _oa_taken(struct i915_perf_stream * stream, u32 tail, u32 head)
>
>nit: no space between * and stream.
>
>> +{
>> +	u32 size = stream->oa_buffer.vma->size;
>> +
>> +	return tail >= head ? tail - head : size - (head - tail);
>> +}
>
>If we are doing this we should probably eliminate references to OA_TAKEN
>which serves an identical purpose (I think there is one remaining
>reference) and also delete OA_TAKEN #define.
>
>> +
>> +static u32 _rewind_tail(struct i915_perf_stream * stream, u32 relative_hw_tail,
>> +			u32 rewind_delta)
>> +{
>> +	return rewind_delta > relative_hw_tail ?
>> +	       stream->oa_buffer.vma->size - (rewind_delta - relative_hw_tail) :
>> +	       relative_hw_tail - rewind_delta;
>> +}
>
>Also are we really saying here that we are supporting non-power-of-2 OA
>buffer sizes? Because if we stayed with power-of-2 sizes the expression
>above are nice and elegant and actually closer to the previous code being
>changed in this patch. For example:
>
>#include <linux/circ_buf.h>
>
>static u32 _oa_taken(struct i915_perf_stream *stream, u32 tail, u32 head)
>{
>	return CIRC_CNT(tail, head, stream->oa_buffer.vma->size);
>}
>
>static u32 _rewind_tail(struct i915_perf_stream *stream, u32 relative_hw_tail,
>       			u32 rewind_delta)
>{
>	return CIRC_CNT(relative_hw_tail, rewind_delta, stream->oa_buffer.vma->size);
>}
>
>Note that for power-of-2 sizes the two functions above are identical but we
>should keep them separate for clarity (as is done in the patch) since they
>are serving two different functions in the OA code.
>
>Also another assumption in the code seems to be:
>
>	stream->oa_buffer.vma->size == OA_BUFFER_SIZE
>
>which I am pretty sure will not hold for arbitrary non-power-of-2 OA buffer
>sizes? So we might as well stick with power-of-2 sizes and change later in
>a separate patch only if needed?

Most changes here are related to the OA buffer size issue and that is 
specific to xehpsdv where the size is not a power of 2. I am thinking of 
dropping these changes in the next revision since DG2 is fixed and OA 
buffer sizes are power of 2.

Thanks,
Umesh

>
>Thanks.
>--
>Ashutosh