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

[Luca Coelho]

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.


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

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

--
Cheers,
Luca.