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

[Manna, Animesh]

> -----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.

> 
> 
> [...]
> > 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.

Regards,
Animesh

> 
> --
> Cheers,
> Luca.