> -----Original Message----- > From: Luca Coelho <l...@coelho.fi> > Sent: Friday, October 27, 2023 12:19 PM > To: Manna, Animesh <animesh.ma...@intel.com>; intel- > g...@lists.freedesktop.org > Cc: Nikula, Jani <jani.nik...@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 <l...@coelho.fi> > > > Sent: Thursday, October 26, 2023 1:08 PM > > > To: Manna, Animesh <animesh.ma...@intel.com>; intel- > > > g...@lists.freedesktop.org > > > Cc: Nikula, Jani <jani.nik...@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.nik...@intel.com> > > > > Signed-off-by: Animesh Manna <animesh.ma...@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.