[Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring

[Manna, Animesh]

> -----Original Message-----
> From: Luca Coelho <luca at coelho.fi>
> Sent: Tuesday, October 31, 2023 3:35 PM
> To: Manna, Animesh <animesh.manna at intel.com>; intel-
> gfx at lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula at intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring
> 
> On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
> >
> > > -----Original Message-----
> > > From: Luca Coelho <luca at coelho.fi>
> > > Sent: Tuesday, October 31, 2023 1:14 PM
> > > To: Manna, Animesh <animesh.manna at intel.com>; intel-
> > > gfx at lists.freedesktop.org
> > > Cc: Nikula, Jani <jani.nikula at intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code
> > > refactoring
> > >
> > > On Fri, 2023-10-27 at 17:27 +0530, Animesh Manna wrote:
> > > > Refactor DSB implementation to be compatible with Xe driver.
> > > >
> > > > v1: RFC version.
> > > > v2: Make intel_dsb structure opaque from external usage. [Jani]
> > > > v3: Rebased on latest.
> > > > v4:
> > > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > > - Use size_t instead of u32. [Luca]
> > > >
> > > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > > > ---
> > >
> > > [...]
> > > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf,
> > > > +u32 idx, u32 val, size_t size) {
> > > > +	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size -
> > > > +idx
> > > > +* 4))
> > >
> > > You actually don't need the first expression.  This expression
> > > should
> > > enough:
> > >
> > > 	dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
> >
> > Here size is in bytes, but idx is index of 32 bytes array. So, the
> > above expression would be,
> >
> > dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)
> 
> Oh, you're right, of course.
> 
> 
> > The same is done with 2nd expression but agree to use sizeof() instead of
> magic number 4.
> >
> > The first expression is added if idx is big number and due to overflow the
> above check can pass which is not correct. Please let me know your thoughts,
> if you are not ok will drop maybe.
> 
> If you're worried about overflow when you're multiplying by 4, then you can
> just do it the opposite way, still with a single expression:
> 
> dsb_buf->buf_size / sizeof(*dsb_buf->cmd_buf) <= idx + size /
> sizeof(*dsb_buf->cmd_buf)
> 
> Or, taking advantage of the fact that both buf_size and size need to be
> divided by sizeof(), we could something like:
> 
> idx > (dsb_buf->buf_size - size) / sizeof(*dsb_buf->cmd_buf)
> 
> ...but we're bike-shedding.  I don't think the number of expressions or the
> complexity of the expressions matter much here, unless you're really in a
> hotpath, in which case you should add an unlikely() or so.
> 
> I'll leave it to you.
> 
> 
> > >
> > > > +		return;
> > >
> > > Blindly returning here doesn't solve the problem, it just hides it.
> > > I think the best would be to use WARN_ON() instead of if.
> > >
> > > So:
> > > 	WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> > > > cmd_buf));
> >
> > I will add the WARN_ON().
> 
> This is the part that I actually think is important. ;)

Thanks for review, have floated v5 after adding the changes.

Regards,
Animesh

> 
> --
> Cheers,
> Luca.