Re: [PATCH v8 4 3/7] drm/xe/eustall: Implement EU stall sampling APIs for Xe_HPC

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Tue, 28 Jan 2025 20:12:25 -0800, Dixit, Ashutosh wrote:
>
> On Wed, 15 Jan 2025 12:02:09 -0800, Harish Chegondi wrote:
> >
>
> Hi Harish,
>
> Reveiw #4 on the same patch. Final review on this version of the patch. I
> have suggested some low level code changes which should work but would need
> to be verified (the code, as well as tested).
>
> Also, I don't understand really what's going on in these circ buffer
> functions with the overflow bits. So I need some explanation as to what the
> code is doing and whether what's going on here is really correct.
>
> So basically what I don't understand here is whether:
>
>	if (write_offset > read_offset)
>
> should really be
>
>	if (write_offset >= read_offset)
>
> Basically what happens when 'write_offset == read_offset'. We should be
> using the overflow bits "somehow" according to me in this case but we don't
> seem to be doing that.
>
> This is repeated several times in my comments below. But if you have an
> explanation just explain it here, don't have to repeat it each time.

Let's have one or two centralized functions to do this everywhere rather
than compute read/write offsets everywhere. For example a correct
buf_data_size() function should return how many bytes are in the buffer,
taking into account whether the buffer is empty or full. And then this
function will be called everywhere where we need to do this. And we can
have more than one such centralized function. Similar to xe_oa_circ_diff
and xe_oa_circ_incr for OA.

> > +static u32
> > +buf_data_size(size_t buf_size, u32 read_ptr, u32 write_ptr)
> > +{
> > +	u32 read_offset, write_offset, size = 0;
> > +
> > +	read_offset = read_ptr & (buf_size - 1);
> > +	write_offset = write_ptr & (buf_size - 1);
> > +
> > +	if (write_offset > read_offset)