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

[Manna, Animesh]

> -----Original Message-----
> From: Luca Coelho <luca at coelho.fi>
> Sent: Friday, October 27, 2023 12:19 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 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.x

Have floated v4 after addressing above feedback. Please let me know if you like it or not.

Regards,
Animesh

> 
> --
> Cheers,
> Luca.