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

[Luca Coelho]

On Thu, 2023-10-26 at 14:23 +0000, Manna, Animesh wrote:
> 
> > -----Original Message-----
> > From: Luca Coelho <luca at coelho.fi>
> > Sent: Thursday, October 26, 2023 1:08 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 v3] drm/i915/dsb: DSB code refactoring
> > 
> > On Sun, 2023-10-08 at 15:42 +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.
> > > 
> > > Cc: Jani Nikula <jani.nikula at intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > > ---
> > 
> > Looks great overall! Just a couple of small comments below.
> 
> Thanks for review.
> 
> > 
> > 
> > [...]
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > index 3e32aa49b8eb..ec89d968a873 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > @@ -13,12 +13,13 @@
> > >  #include "intel_de.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_dsb.h"
> > > +#include "intel_dsb_buffer.h"
> > >  #include "intel_dsb_regs.h"
> > >  #include "intel_vblank.h"
> > >  #include "intel_vrr.h"
> > >  #include "skl_watermark.h"
> > > 
> > > -struct i915_vma;
> > > +#define CACHELINE_BYTES 64
> > 
> > I see that this macro is defined in GT and you want to avoid depending on
> > the definition from GT, but you don't make any other changes related to the
> > cacheline size here, so maybe this change should be a separate patch? Also,
> > it looks a bit magic without an explanation on where the number is coming
> > from.
> 
> For Xe driver macro definition in GT may not accessible, so have redefined in Intel_dsb.c itself. It's related to dsb so kept in the same patch.
> DSB command buffer is cacheline aligned. DSB support added from gen12 and size of cacheline size will be 64 bytes. As per bspec each cacheline can have 8 dsb-instructions and 64 bits per instruction.

Okay, even though this is clearly related to DSB only, I still don't
think it should be in the same patch.  In any case, I'm not going to
block on this.


> > [...]
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb_buffer.c
> > > b/drivers/gpu/drm/i915/display/intel_dsb_buffer.c
> > > new file mode 100644
> > > index 000000000000..723937591831
> > > --- /dev/null
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb_buffer.c
> > > @@ -0,0 +1,64 @@
> > > +// SPDX-License-Identifier: MIT
> > > +/*
> > > + * Copyright 2023, Intel Corporation.
> > > + */
> > > +
> > > +#include "gem/i915_gem_internal.h"
> > > +#include "i915_drv.h"
> > > +#include "i915_vma.h"
> > > +#include "intel_display_types.h"
> > > +#include "intel_dsb_buffer.h"
> > > +
> > > +u32 intel_dsb_buffer_ggtt_offset(struct intel_dsb_buffer *dsb_buf) {
> > > +	return i915_ggtt_offset(dsb_buf->vma); }
> > > +
> > > +void intel_dsb_buffer_write(struct intel_dsb_buffer *dsb_buf, u32
> > > +idx, u32 val) {
> > > +	dsb_buf->cmd_buf[idx] = val;
> > > +}
> > > +
> > > +u32 intel_dsb_buffer_read(struct intel_dsb_buffer *dsb_buf, u32 idx)
> > > +{
> > > +	return dsb_buf->cmd_buf[idx];
> > > +}
> > > +
> > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32
> > > +idx, u32 val, u32 sz) {
> > > +	memset(&dsb_buf->cmd_buf[idx], val, sz);
> > 
> > I think you should check the array boundaries here, to be sure.
> > Probably a good idea to do with the other functions as well, but I think this is
> > the most critical and easiest to make mistakes with.
> 
> assert_dsb_has_room() function is taking care for not crossing the boundaries. Here will check from the allocated buffer-size versus used/unused buffer.
> Specifically intel_dsb_buffer_memset() is called from intel_dsb_align_tail() where zero get set for unused cacheline space. No chance to cross the boundaries in this case.
> Please let me know for any further info.

I mean, if someone accidentally calls intel_dsb_buffer_memset() with a
wrong index or too large size, the memset here will write out-of-
bounds, no matter what you do in assert_dsb_has_room().  This shouldn't
happen, but if it does, it will be hard to find and can lead to
security issues.

I don't know how time critical the calls to intel_dsb_buffer_memset()
will be, but I think it's worth adding a splat if someone does
something wrong.

As an additional comment, instead of "u32 sz" you should use size_t for
the size.  And I would use the full word "size", as you do in
intel_dsb_buffer_create() (where it should also be size_t), for
consistency.

--
Cheers,
Luca.