Re: [Intel-gfx] [PATCH 10/13] drm/i915/dsb: Allow the caller to pass in the DSB buffer size
> -Original Message-
> From: Intel-gfx On Behalf Of Ville
> Syrjala
> Sent: Friday, December 16, 2022 6:08 AM
> To: intel-gfx@lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 10/13] drm/i915/dsb: Allow the caller to pass in
> the DSB buffer size
>
> From: Ville Syrjälä
>
> The caller should more or less know how many DSB commands it wants to
> emit into the command buffer, so allow it to specify the size of the command
> buffer rather than having the low level DSB code guess it.
>
> Technically we can emit as many as 134+1033 (for adl+ degamma + 10bit
> gamma) register writes but thanks to the DSB indexed register write
> command we get significant space savings so the current size estimate of
> 8KiB (~1024 DSB commands) is sufficient for now.
>
> Signed-off-by: Ville Syrjälä
LGTM.
Reviewed-by: Animesh Manna
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 2 +-
> drivers/gpu/drm/i915/display/intel_dsb.c | 42 +-
> drivers/gpu/drm/i915/display/intel_dsb.h | 3 +-
> 3 files changed, 29 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index 76603357252d..8d97c299e657 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -1380,7 +1380,7 @@ void intel_color_prepare_commit(struct
> intel_crtc_state *crtc_state)
> /* FIXME DSB has issues loading LUTs, disable it for now */
> return;
>
> - crtc_state->dsb = intel_dsb_prepare(crtc);
> + crtc_state->dsb = intel_dsb_prepare(crtc, 1024);
> }
>
> void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state) diff
> --git
> a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 636c57767f97..7c593ec84d41 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -30,21 +30,24 @@ struct intel_dsb {
> struct intel_crtc *crtc;
>
> /*
> - * free_pos will point the first free entry position
> - * and help in calculating tail of command buffer.
> + * maximum number of dwords the buffer will hold.
>*/
> - int free_pos;
> + unsigned int size;
>
> /*
> - * ins_start_offset will help to store start address of the dsb
> + * free_pos will point the first free dword and
> + * help in calculating tail of command buffer.
> + */
> + unsigned int free_pos;
> +
> + /*
> + * ins_start_offset will help to store start dword of the dsb
>* instuction and help in identifying the batch of auto-increment
>* register.
>*/
> - u32 ins_start_offset;
> + unsigned int ins_start_offset;
> };
>
> -#define DSB_BUF_SIZE(2 * PAGE_SIZE)
> -
> /**
> * DOC: DSB
> *
> @@ -76,7 +79,7 @@ static bool assert_dsb_has_room(struct intel_dsb *dsb)
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
>
> /* each instruction is 2 dwords */
> - return !drm_WARN(>drm, ALIGN(dsb->free_pos, 2) >
> DSB_BUF_SIZE / 4 - 2,
> + return !drm_WARN(>drm, dsb->free_pos > dsb->size - 2,
>"DSB buffer overflow\n");
> }
>
> @@ -168,7 +171,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
> if (intel_dsb_prev_ins_is_mmio_write(dsb, reg)) {
> u32 prev_val = buf[dsb->ins_start_offset + 0];
>
> - buf[dsb->ins_start_offset + 0] = 1; /* size */
> + 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);
> @@ -178,7 +181,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
> }
>
> buf[dsb->free_pos++] = val;
> - /* Update the size. */
> + /* Update the count */
> buf[dsb->ins_start_offset]++;
>
> /* if number of data words is odd, then the last dword
> should be 0.*/ @@ -250,6 +253,7 @@ void intel_dsb_commit(struct
> intel_dsb *dsb)
> /**
> * intel_dsb_prepare() - Allocate, pin and map the DSB command buffer.
> * @crtc: the CRTC
> + * @max_cmds: number of commands we need to fit into command buffer
> *
> * This function prepare the command buffer which is used to store dsb
> * instructions with data.
> @@ -257,25 +261,30 @@ void intel_dsb_commit(struct intel_dsb *dsb)
> * Returns:
> * DSB context, NULL on failure
> */
> -struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc)
> +struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
> + unsigned int max_cmds)
> {
> struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> - struct intel_dsb *dsb;
> struct drm_i915_gem_object *obj;
> - struct i915_vma *vma;
> -
[Intel-gfx] [PATCH 10/13] drm/i915/dsb: Allow the caller to pass in the DSB buffer size
From: Ville Syrjälä
The caller should more or less know how many DSB commands it
wants to emit into the command buffer, so allow it to specify
the size of the command buffer rather than having the low level
DSB code guess it.
Technically we can emit as many as 134+1033 (for adl+ degamma +
10bit gamma) register writes but thanks to the DSB indexed register
write command we get significant space savings so the current size
estimate of 8KiB (~1024 DSB commands) is sufficient for now.
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/i915/display/intel_color.c | 2 +-
drivers/gpu/drm/i915/display/intel_dsb.c | 42 +-
drivers/gpu/drm/i915/display/intel_dsb.h | 3 +-
3 files changed, 29 insertions(+), 18 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_color.c
b/drivers/gpu/drm/i915/display/intel_color.c
index 76603357252d..8d97c299e657 100644
--- a/drivers/gpu/drm/i915/display/intel_color.c
+++ b/drivers/gpu/drm/i915/display/intel_color.c
@@ -1380,7 +1380,7 @@ void intel_color_prepare_commit(struct intel_crtc_state
*crtc_state)
/* FIXME DSB has issues loading LUTs, disable it for now */
return;
- crtc_state->dsb = intel_dsb_prepare(crtc);
+ crtc_state->dsb = intel_dsb_prepare(crtc, 1024);
}
void intel_color_cleanup_commit(struct intel_crtc_state *crtc_state)
diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
b/drivers/gpu/drm/i915/display/intel_dsb.c
index 636c57767f97..7c593ec84d41 100644
--- a/drivers/gpu/drm/i915/display/intel_dsb.c
+++ b/drivers/gpu/drm/i915/display/intel_dsb.c
@@ -30,21 +30,24 @@ struct intel_dsb {
struct intel_crtc *crtc;
/*
-* free_pos will point the first free entry position
-* and help in calculating tail of command buffer.
+* maximum number of dwords the buffer will hold.
*/
- int free_pos;
+ unsigned int size;
/*
-* ins_start_offset will help to store start address of the dsb
+* free_pos will point the first free dword and
+* help in calculating tail of command buffer.
+*/
+ unsigned int free_pos;
+
+ /*
+* ins_start_offset will help to store start dword of the dsb
* instuction and help in identifying the batch of auto-increment
* register.
*/
- u32 ins_start_offset;
+ unsigned int ins_start_offset;
};
-#define DSB_BUF_SIZE(2 * PAGE_SIZE)
-
/**
* DOC: DSB
*
@@ -76,7 +79,7 @@ static bool assert_dsb_has_room(struct intel_dsb *dsb)
struct drm_i915_private *i915 = to_i915(crtc->base.dev);
/* each instruction is 2 dwords */
- return !drm_WARN(>drm, ALIGN(dsb->free_pos, 2) > DSB_BUF_SIZE / 4
- 2,
+ return !drm_WARN(>drm, dsb->free_pos > dsb->size - 2,
"DSB buffer overflow\n");
}
@@ -168,7 +171,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
if (intel_dsb_prev_ins_is_mmio_write(dsb, reg)) {
u32 prev_val = buf[dsb->ins_start_offset + 0];
- buf[dsb->ins_start_offset + 0] = 1; /* size */
+ 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);
@@ -178,7 +181,7 @@ void intel_dsb_reg_write(struct intel_dsb *dsb,
}
buf[dsb->free_pos++] = val;
- /* Update the size. */
+ /* Update the count */
buf[dsb->ins_start_offset]++;
/* if number of data words is odd, then the last dword should
be 0.*/
@@ -250,6 +253,7 @@ void intel_dsb_commit(struct intel_dsb *dsb)
/**
* intel_dsb_prepare() - Allocate, pin and map the DSB command buffer.
* @crtc: the CRTC
+ * @max_cmds: number of commands we need to fit into command buffer
*
* This function prepare the command buffer which is used to store dsb
* instructions with data.
@@ -257,25 +261,30 @@ void intel_dsb_commit(struct intel_dsb *dsb)
* Returns:
* DSB context, NULL on failure
*/
-struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc)
+struct intel_dsb *intel_dsb_prepare(struct intel_crtc *crtc,
+ unsigned int max_cmds)
{
struct drm_i915_private *i915 = to_i915(crtc->base.dev);
- struct intel_dsb *dsb;
struct drm_i915_gem_object *obj;
- struct i915_vma *vma;
- u32 *buf;
intel_wakeref_t wakeref;
+ struct intel_dsb *dsb;
+ struct i915_vma *vma;
+ unsigned int size;
+ u32 *buf;
if (!HAS_DSB(i915))
return NULL;
- dsb = kmalloc(sizeof(*dsb), GFP_KERNEL);
+ dsb = kzalloc(sizeof(*dsb), GFP_KERNEL);
if (!dsb)
goto out;
wakeref = intel_runtime_pm_get(>runtime_pm);
- obj =
