diff mbox series

[v4] drm/i915/dsb: DSB code refactoring

Message ID 20231027115711.2713226-1-animesh.manna@intel.com (mailing list archive)
State New, archived
Headers show
Series [v4] drm/i915/dsb: DSB code refactoring | expand

Commit Message

Manna, Animesh Oct. 27, 2023, 11:57 a.m. UTC
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.
v4:
- Add boundary check in dsb_buffer_memset(). [Luca]
- Use size_t instead of u32. [Luca]

Cc: Jani Nikula <jani.nikula@intel.com>
Signed-off-by: Animesh Manna <animesh.manna@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |  1 +
 drivers/gpu/drm/i915/display/intel_dsb.c      | 98 +++++++------------
 .../gpu/drm/i915/display/intel_dsb_buffer.c   | 83 ++++++++++++++++
 .../gpu/drm/i915/display/intel_dsb_buffer.h   | 29 ++++++
 4 files changed, 149 insertions(+), 62 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb_buffer.c
 create mode 100644 drivers/gpu/drm/i915/display/intel_dsb_buffer.h

Comments

Luca Coelho Oct. 31, 2023, 7:44 a.m. UTC | #1
On Fri, 2023-10-27 at 17:27 +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.
> v4:
> - Add boundary check in dsb_buffer_memset(). [Luca]
> - Use size_t instead of u32. [Luca]
> 
> Cc: Jani Nikula <jani.nikula@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---

[...]
> +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32 idx, u32 val, size_t size)
> +{
> +	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx * 4))

You actually don't need the first expression.  This expression should
enough:

	dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)

> +		return;

Blindly returning here doesn't solve the problem, it just hides it.  I
think the best would be to use WARN_ON() instead of if.

So:
	WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf));

> +
> +	memset(&dsb_buf->cmd_buf[idx], val, size);
> +}
[...]

--
Cheers,
Luca.
Manna, Animesh Oct. 31, 2023, 9:15 a.m. UTC | #2
> -----Original Message-----
> From: Luca Coelho <luca@coelho.fi>
> Sent: Tuesday, October 31, 2023 1:14 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring
> 
> On Fri, 2023-10-27 at 17:27 +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.
> > v4:
> > - Add boundary check in dsb_buffer_memset(). [Luca]
> > - Use size_t instead of u32. [Luca]
> >
> > Cc: Jani Nikula <jani.nikula@intel.com>
> > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > ---
> 
> [...]
> > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32
> > +idx, u32 val, size_t size) {
> > +	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx
> > +* 4))
> 
> You actually don't need the first expression.  This expression should
> enough:
> 
> 	dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)

Here size is in bytes, but idx is index of 32 bytes array. So, the above expression would be,

dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)

The same is done with 2nd expression but agree to use sizeof() instead of magic number 4.

The first expression is added if idx is big number and due to overflow the above check can pass which is not correct. Please let me know your thoughts, if you are not ok will drop maybe.

> 
> > +		return;
> 
> Blindly returning here doesn't solve the problem, it just hides it.  I think the
> best would be to use WARN_ON() instead of if.
> 
> So:
> 	WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> >cmd_buf));

I will add the WARN_ON().

Regards,
Animesh

> 
> > +
> > +	memset(&dsb_buf->cmd_buf[idx], val, size); }
> [...]
> 
> --
> Cheers,
> Luca.
Luca Coelho Oct. 31, 2023, 10:05 a.m. UTC | #3
On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
> 
> > -----Original Message-----
> > From: Luca Coelho <luca@coelho.fi>
> > Sent: Tuesday, October 31, 2023 1:14 PM
> > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > gfx@lists.freedesktop.org
> > Cc: Nikula, Jani <jani.nikula@intel.com>
> > Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring
> > 
> > On Fri, 2023-10-27 at 17:27 +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.
> > > v4:
> > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > - Use size_t instead of u32. [Luca]
> > > 
> > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > ---
> > 
> > [...]
> > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32
> > > +idx, u32 val, size_t size) {
> > > +	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx
> > > +* 4))
> > 
> > You actually don't need the first expression.  This expression should
> > enough:
> > 
> > 	dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
> 
> Here size is in bytes, but idx is index of 32 bytes array. So, the above expression would be,
> 
> dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)

Oh, you're right, of course.


> The same is done with 2nd expression but agree to use sizeof() instead of magic number 4.
> 
> The first expression is added if idx is big number and due to overflow the above check can pass which is not correct. Please let me know your thoughts, if you are not ok will drop maybe.

If you're worried about overflow when you're multiplying by 4, then you
can just do it the opposite way, still with a single expression:

dsb_buf->buf_size / sizeof(*dsb_buf->cmd_buf) <= idx + size / sizeof(*dsb_buf->cmd_buf)

Or, taking advantage of the fact that both buf_size and size need to be
divided by sizeof(), we could something like:

idx > (dsb_buf->buf_size - size) / sizeof(*dsb_buf->cmd_buf)

...but we're bike-shedding.  I don't think the number of expressions or
the complexity of the expressions matter much here, unless you're
really in a hotpath, in which case you should add an unlikely() or so.

I'll leave it to you.


> > 
> > > +		return;
> > 
> > Blindly returning here doesn't solve the problem, it just hides it.  I think the
> > best would be to use WARN_ON() instead of if.
> > 
> > So:
> > 	WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> > > cmd_buf));
> 
> I will add the WARN_ON().

This is the part that I actually think is important. ;)

--
Cheers,
Luca.
Manna, Animesh Nov. 2, 2023, 5:40 p.m. UTC | #4
> -----Original Message-----
> From: Luca Coelho <luca@coelho.fi>
> Sent: Tuesday, October 31, 2023 3:35 PM
> To: Manna, Animesh <animesh.manna@intel.com>; intel-
> gfx@lists.freedesktop.org
> Cc: Nikula, Jani <jani.nikula@intel.com>
> Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring
> 
> On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
> >
> > > -----Original Message-----
> > > From: Luca Coelho <luca@coelho.fi>
> > > Sent: Tuesday, October 31, 2023 1:14 PM
> > > To: Manna, Animesh <animesh.manna@intel.com>; intel-
> > > gfx@lists.freedesktop.org
> > > Cc: Nikula, Jani <jani.nikula@intel.com>
> > > Subject: Re: [Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code
> > > refactoring
> > >
> > > On Fri, 2023-10-27 at 17:27 +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.
> > > > v4:
> > > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > > - Use size_t instead of u32. [Luca]
> > > >
> > > > Cc: Jani Nikula <jani.nikula@intel.com>
> > > > Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> > > > ---
> > >
> > > [...]
> > > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf,
> > > > +u32 idx, u32 val, size_t size) {
> > > > +	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size -
> > > > +idx
> > > > +* 4))
> > >
> > > You actually don't need the first expression.  This expression
> > > should
> > > enough:
> > >
> > > 	dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
> >
> > Here size is in bytes, but idx is index of 32 bytes array. So, the
> > above expression would be,
> >
> > dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)
> 
> Oh, you're right, of course.
> 
> 
> > The same is done with 2nd expression but agree to use sizeof() instead of
> magic number 4.
> >
> > The first expression is added if idx is big number and due to overflow the
> above check can pass which is not correct. Please let me know your thoughts,
> if you are not ok will drop maybe.
> 
> If you're worried about overflow when you're multiplying by 4, then you can
> just do it the opposite way, still with a single expression:
> 
> dsb_buf->buf_size / sizeof(*dsb_buf->cmd_buf) <= idx + size /
> sizeof(*dsb_buf->cmd_buf)
> 
> Or, taking advantage of the fact that both buf_size and size need to be
> divided by sizeof(), we could something like:
> 
> idx > (dsb_buf->buf_size - size) / sizeof(*dsb_buf->cmd_buf)
> 
> ...but we're bike-shedding.  I don't think the number of expressions or the
> complexity of the expressions matter much here, unless you're really in a
> hotpath, in which case you should add an unlikely() or so.
> 
> I'll leave it to you.
> 
> 
> > >
> > > > +		return;
> > >
> > > Blindly returning here doesn't solve the problem, it just hides it.
> > > I think the best would be to use WARN_ON() instead of if.
> > >
> > > So:
> > > 	WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> > > > cmd_buf));
> >
> > I will add the WARN_ON().
> 
> This is the part that I actually think is important. ;)

Thanks for review, have floated v5 after adding the changes.

Regards,
Animesh

> 
> --
> Cheers,
> Luca.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 3b9dcb606fc1..0687b00e1740 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -272,6 +272,7 @@  i915-y += \
 	display/intel_dpt.o \
 	display/intel_drrs.o \
 	display/intel_dsb.o \
+	display/intel_dsb_buffer.o \
 	display/intel_fb.o \
 	display/intel_fb_pin.o \
 	display/intel_fbc.o \
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
index 78b6fe24dcd8..3c7733a416a4 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -4,9 +4,6 @@ 
  *
  */
 
-#include "gem/i915_gem_internal.h"
-#include "gem/i915_gem_lmem.h"
-
 #include "i915_drv.h"
 #include "i915_irq.h"
 #include "i915_reg.h"
@@ -14,12 +11,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
 
 enum dsb_id {
 	INVALID_DSB = -1,
@@ -32,8 +30,7 @@  enum dsb_id {
 struct intel_dsb {
 	enum dsb_id id;
 
-	u32 *cmd_buf;
-	struct i915_vma *vma;
+	struct intel_dsb_buffer dsb_buf;
 	struct intel_crtc *crtc;
 
 	/*
@@ -109,15 +106,17 @@  static void intel_dsb_dump(struct intel_dsb *dsb)
 {
 	struct intel_crtc *crtc = dsb->crtc;
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
-	const u32 *buf = dsb->cmd_buf;
 	int i;
 
 	drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] DSB %d commands {\n",
 		    crtc->base.base.id, crtc->base.name, dsb->id);
 	for (i = 0; i < ALIGN(dsb->free_pos, 64 / 4); i += 4)
 		drm_dbg_kms(&i915->drm,
-			    " 0x%08x: 0x%08x 0x%08x 0x%08x 0x%08x\n",
-			    i * 4, buf[i], buf[i+1], buf[i+2], buf[i+3]);
+			    " 0x%08x: 0x%08x 0x%08x 0x%08x 0x%08x\n", i * 4,
+			    intel_dsb_buffer_read(&dsb->dsb_buf, i),
+			    intel_dsb_buffer_read(&dsb->dsb_buf, i + 1),
+			    intel_dsb_buffer_read(&dsb->dsb_buf, i + 2),
+			    intel_dsb_buffer_read(&dsb->dsb_buf, i + 3));
 	drm_dbg_kms(&i915->drm, "}\n");
 }
 
@@ -129,8 +128,6 @@  static bool is_dsb_busy(struct drm_i915_private *i915, enum pipe pipe,
 
 static void intel_dsb_emit(struct intel_dsb *dsb, u32 ldw, u32 udw)
 {
-	u32 *buf = dsb->cmd_buf;
-
 	if (!assert_dsb_has_room(dsb))
 		return;
 
@@ -139,14 +136,13 @@  static void intel_dsb_emit(struct intel_dsb *dsb, u32 ldw, u32 udw)
 
 	dsb->ins_start_offset = dsb->free_pos;
 
-	buf[dsb->free_pos++] = ldw;
-	buf[dsb->free_pos++] = udw;
+	intel_dsb_buffer_write(&dsb->dsb_buf, dsb->free_pos++, ldw);
+	intel_dsb_buffer_write(&dsb->dsb_buf, dsb->free_pos++, udw);
 }
 
 static bool intel_dsb_prev_ins_is_write(struct intel_dsb *dsb,
 					u32 opcode, i915_reg_t reg)
 {
-	const u32 *buf = dsb->cmd_buf;
 	u32 prev_opcode, prev_reg;
 
 	/*
@@ -157,8 +153,10 @@  static bool intel_dsb_prev_ins_is_write(struct intel_dsb *dsb,
 	if (dsb->free_pos == 0)
 		return false;
 
-	prev_opcode = buf[dsb->ins_start_offset + 1] & ~DSB_REG_VALUE_MASK;
-	prev_reg = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
+	prev_opcode = intel_dsb_buffer_read(&dsb->dsb_buf,
+					    dsb->ins_start_offset + 1) >> DSB_OPCODE_SHIFT;
+	prev_reg =  intel_dsb_buffer_read(&dsb->dsb_buf,
+					  dsb->ins_start_offset + 1) & DSB_REG_VALUE_MASK;
 
 	return prev_opcode == opcode && prev_reg == i915_mmio_reg_offset(reg);
 }
@@ -191,6 +189,8 @@  static bool intel_dsb_prev_ins_is_indexed_write(struct intel_dsb *dsb, i915_reg_
 void intel_dsb_reg_write(struct intel_dsb *dsb,
 			 i915_reg_t reg, u32 val)
 {
+	u32 old_val;
+
 	/*
 	 * For example the buffer will look like below for 3 dwords for auto
 	 * increment register:
@@ -214,31 +214,32 @@  void intel_dsb_reg_write(struct intel_dsb *dsb,
 			       (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
 			       i915_mmio_reg_offset(reg));
 	} else {
-		u32 *buf = dsb->cmd_buf;
-
 		if (!assert_dsb_has_room(dsb))
 			return;
 
 		/* convert to indexed write? */
 		if (intel_dsb_prev_ins_is_mmio_write(dsb, reg)) {
-			u32 prev_val = buf[dsb->ins_start_offset + 0];
+			u32 prev_val = intel_dsb_buffer_read(&dsb->dsb_buf,
+							     dsb->ins_start_offset + 0);
 
-			buf[dsb->ins_start_offset + 0] = 1; /* count */
-			buf[dsb->ins_start_offset + 1] =
-				(DSB_OPCODE_INDEXED_WRITE << DSB_OPCODE_SHIFT) |
-				i915_mmio_reg_offset(reg);
-			buf[dsb->ins_start_offset + 2] = prev_val;
+			intel_dsb_buffer_write(&dsb->dsb_buf,
+					       dsb->ins_start_offset + 0, 1); /* count */
+			intel_dsb_buffer_write(&dsb->dsb_buf, dsb->ins_start_offset + 1,
+					       (DSB_OPCODE_INDEXED_WRITE << DSB_OPCODE_SHIFT) |
+					       i915_mmio_reg_offset(reg));
+			intel_dsb_buffer_write(&dsb->dsb_buf, dsb->ins_start_offset + 2, prev_val);
 
 			dsb->free_pos++;
 		}
 
-		buf[dsb->free_pos++] = val;
+		intel_dsb_buffer_write(&dsb->dsb_buf, dsb->free_pos++, val);
 		/* Update the count */
-		buf[dsb->ins_start_offset]++;
+		old_val = intel_dsb_buffer_read(&dsb->dsb_buf, dsb->ins_start_offset);
+		intel_dsb_buffer_write(&dsb->dsb_buf, dsb->ins_start_offset, old_val + 1);
 
 		/* if number of data words is odd, then the last dword should be 0.*/
 		if (dsb->free_pos & 0x1)
-			buf[dsb->free_pos] = 0;
+			intel_dsb_buffer_write(&dsb->dsb_buf, dsb->free_pos, 0);
 	}
 }
 
@@ -297,8 +298,8 @@  static void intel_dsb_align_tail(struct intel_dsb *dsb)
 	aligned_tail = ALIGN(tail, CACHELINE_BYTES);
 
 	if (aligned_tail > tail)
-		memset(&dsb->cmd_buf[dsb->free_pos], 0,
-		       aligned_tail - tail);
+		intel_dsb_buffer_memset(&dsb->dsb_buf, dsb->free_pos, 0,
+					aligned_tail - tail);
 
 	dsb->free_pos = aligned_tail / 4;
 }
@@ -317,7 +318,7 @@  void intel_dsb_finish(struct intel_dsb *dsb)
 
 	intel_dsb_align_tail(dsb);
 
-	i915_gem_object_flush_map(dsb->vma->obj);
+	intel_dsb_buffer_flush_map(&dsb->dsb_buf);
 }
 
 static int intel_dsb_dewake_scanline(const struct intel_crtc_state *crtc_state)
@@ -361,7 +362,7 @@  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
 			  ctrl | DSB_ENABLE);
 
 	intel_de_write_fw(dev_priv, DSB_HEAD(pipe, dsb->id),
-			  i915_ggtt_offset(dsb->vma));
+			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf));
 
 	if (dewake_scanline >= 0) {
 		int diff, hw_dewake_scanline;
@@ -383,7 +384,7 @@  static void _intel_dsb_commit(struct intel_dsb *dsb, u32 ctrl,
 	}
 
 	intel_de_write_fw(dev_priv, DSB_TAIL(pipe, dsb->id),
-			  i915_ggtt_offset(dsb->vma) + tail);
+			  intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf) + tail);
 }
 
 /**
@@ -408,7 +409,7 @@  void intel_dsb_wait(struct intel_dsb *dsb)
 	enum pipe pipe = crtc->pipe;
 
 	if (wait_for(!is_dsb_busy(dev_priv, pipe, dsb->id), 1)) {
-		u32 offset = i915_ggtt_offset(dsb->vma);
+		u32 offset = intel_dsb_buffer_ggtt_offset(&dsb->dsb_buf);
 
 		intel_de_write_fw(dev_priv, DSB_CTRL(pipe, dsb->id),
 				  DSB_ENABLE | DSB_HALT);
@@ -445,12 +446,9 @@  struct intel_dsb *intel_dsb_prepare(const struct intel_crtc_state *crtc_state,
 {
 	struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
 	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
-	struct drm_i915_gem_object *obj;
 	intel_wakeref_t wakeref;
 	struct intel_dsb *dsb;
-	struct i915_vma *vma;
 	unsigned int size;
-	u32 *buf;
 
 	if (!HAS_DSB(i915))
 		return NULL;
@@ -464,37 +462,13 @@  struct intel_dsb *intel_dsb_prepare(const struct intel_crtc_state *crtc_state,
 	/* ~1 qword per instruction, full cachelines */
 	size = ALIGN(max_cmds * 8, CACHELINE_BYTES);
 
-	if (HAS_LMEM(i915)) {
-		obj = i915_gem_object_create_lmem(i915, PAGE_ALIGN(size),
-						  I915_BO_ALLOC_CONTIGUOUS);
-		if (IS_ERR(obj))
-			goto out_put_rpm;
-	} else {
-		obj = i915_gem_object_create_internal(i915, PAGE_ALIGN(size));
-		if (IS_ERR(obj))
-			goto out_put_rpm;
-
-		i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
-	}
-
-	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
-	if (IS_ERR(vma)) {
-		i915_gem_object_put(obj);
-		goto out_put_rpm;
-	}
-
-	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
-	if (IS_ERR(buf)) {
-		i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
+	if (!intel_dsb_buffer_create(crtc, &dsb->dsb_buf, size))
 		goto out_put_rpm;
-	}
 
 	intel_runtime_pm_put(&i915->runtime_pm, wakeref);
 
 	dsb->id = DSB1;
-	dsb->vma = vma;
 	dsb->crtc = crtc;
-	dsb->cmd_buf = buf;
 	dsb->size = size / 4; /* in dwords */
 	dsb->free_pos = 0;
 	dsb->ins_start_offset = 0;
@@ -522,6 +496,6 @@  struct intel_dsb *intel_dsb_prepare(const struct intel_crtc_state *crtc_state,
  */
 void intel_dsb_cleanup(struct intel_dsb *dsb)
 {
-	i915_vma_unpin_and_release(&dsb->vma, I915_VMA_RELEASE_MAP);
+	intel_dsb_buffer_cleanup(&dsb->dsb_buf);
 	kfree(dsb);
 }
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..ea835512fb08
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb_buffer.c
@@ -0,0 +1,83 @@ 
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright 2023, Intel Corporation.
+ */
+
+#include "gem/i915_gem_internal.h"
+#include "gem/i915_gem_lmem.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, size_t size)
+{
+	if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size - idx * 4))
+		return;
+
+	memset(&dsb_buf->cmd_buf[idx], val, size);
+}
+
+bool intel_dsb_buffer_create(struct intel_crtc *crtc, struct intel_dsb_buffer *dsb_buf, size_t size)
+{
+	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
+	struct drm_i915_gem_object *obj;
+	struct i915_vma *vma;
+	u32 *buf;
+
+	if (HAS_LMEM(i915)) {
+		obj = i915_gem_object_create_lmem(i915, PAGE_ALIGN(size),
+						  I915_BO_ALLOC_CONTIGUOUS);
+		if (IS_ERR(obj))
+			return false;
+	} else {
+		obj = i915_gem_object_create_internal(i915, PAGE_ALIGN(size));
+		if (IS_ERR(obj))
+			return false;
+
+		i915_gem_object_set_cache_coherency(obj, I915_CACHE_NONE);
+	}
+
+	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
+	if (IS_ERR(vma)) {
+		i915_gem_object_put(obj);
+		return false;
+	}
+
+	buf = i915_gem_object_pin_map_unlocked(vma->obj, I915_MAP_WC);
+	if (IS_ERR(buf)) {
+		i915_vma_unpin_and_release(&vma, I915_VMA_RELEASE_MAP);
+		return false;
+	}
+
+	dsb_buf->vma = vma;
+	dsb_buf->cmd_buf = buf;
+	dsb_buf->buf_size = size;
+
+	return true;
+}
+
+void intel_dsb_buffer_cleanup(struct intel_dsb_buffer *dsb_buf)
+{
+	i915_vma_unpin_and_release(&dsb_buf->vma, I915_VMA_RELEASE_MAP);
+}
+
+void intel_dsb_buffer_flush_map(struct intel_dsb_buffer *dsb_buf)
+{
+	i915_gem_object_flush_map(dsb_buf->vma->obj);
+}
diff --git a/drivers/gpu/drm/i915/display/intel_dsb_buffer.h b/drivers/gpu/drm/i915/display/intel_dsb_buffer.h
new file mode 100644
index 000000000000..425acd393905
--- /dev/null
+++ b/drivers/gpu/drm/i915/display/intel_dsb_buffer.h
@@ -0,0 +1,29 @@ 
+/* SPDX-License-Identifier: MIT
+ *
+ * Copyright © 2023 Intel Corporation
+ */
+
+#ifndef _INTEL_DSB_BUFFER_H
+#define _INTEL_DSB_BUFFER_H
+
+#include <linux/types.h>
+
+struct intel_crtc;
+struct i915_vma;
+
+struct intel_dsb_buffer {
+	u32 *cmd_buf;
+	struct i915_vma *vma;
+	size_t buf_size;
+};
+
+u32 intel_dsb_buffer_ggtt_offset(struct intel_dsb_buffer *dsb_buf);
+void intel_dsb_buffer_write(struct intel_dsb_buffer *dsb_buf, u32 idx, u32 val);
+u32 intel_dsb_buffer_read(struct intel_dsb_buffer *dsb_buf, u32 idx);
+void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf, u32 idx, u32 val, size_t size);
+bool intel_dsb_buffer_create(struct intel_crtc *crtc, struct intel_dsb_buffer *dsb_buf,
+			     size_t size);
+void intel_dsb_buffer_cleanup(struct intel_dsb_buffer *dsb_buf);
+void intel_dsb_buffer_flush_map(struct intel_dsb_buffer *dsb_buf);
+
+#endif