> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> Syrjala
> Sent: Friday, December 16, 2022 6:08 AM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH 08/13] drm/i915/dsb: Handle the indexed vs. not
> inside the DSB code
>
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> The DSB indexed register write insturction is purely an internal DSB
> implementation detail, no reason why the caller should have to know about
> it. So let's just have the caller emit blind register writes let the DSB code
> convert things to an indexed write if/when multiple writes occur to the same
> register offset in a row.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
LGTM.
Reviewed-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/display/intel_color.c | 45 ++++------
> drivers/gpu/drm/i915/display/intel_dsb.c | 96 +++++++++-------------
> drivers/gpu/drm/i915/display/intel_dsb.h | 2 -
> 3 files changed, 54 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_color.c
> b/drivers/gpu/drm/i915/display/intel_color.c
> index d57631b0bb9a..76603357252d 100644
> --- a/drivers/gpu/drm/i915/display/intel_color.c
> +++ b/drivers/gpu/drm/i915/display/intel_color.c
> @@ -847,17 +847,6 @@ static void ilk_lut_write(const struct intel_crtc_state
> *crtc_state,
> intel_de_write_fw(i915, reg, val);
> }
>
> -static void ilk_lut_write_indexed(const struct intel_crtc_state *crtc_state,
> - i915_reg_t reg, u32 val)
> -{
> - struct drm_i915_private *i915 = to_i915(crtc_state->uapi.crtc->dev);
> -
> - if (crtc_state->dsb)
> - intel_dsb_indexed_reg_write(crtc_state->dsb, reg, val);
> - else
> - intel_de_write_fw(i915, reg, val);
> -}
> -
> static void ilk_load_lut_8(const struct intel_crtc_state *crtc_state,
> const struct drm_property_blob *blob) { @@ -
> 962,8 +951,8 @@ static void bdw_load_lut_10(const struct intel_crtc_state
> *crtc_state,
> prec_index);
>
> for (i = 0; i < lut_size; i++)
> - ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> - ilk_lut_10(&lut[i]));
> + ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> + ilk_lut_10(&lut[i]));
>
> /*
> * Reset the index, otherwise it prevents the legacy palette to be @@
> -1093,13 +1082,13 @@ static void glk_load_degamma_lut(const struct
> intel_crtc_state *crtc_state,
> * ToDo: Extend to max 7.0. Enable 32 bit input value
> * as compared to just 16 to achieve this.
> */
> - ilk_lut_write_indexed(crtc_state,
> PRE_CSC_GAMC_DATA(pipe),
> - lut[i].green);
> + ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe),
> + lut[i].green);
> }
>
> /* Clamp values > 1.0. */
> while (i++ < glk_degamma_lut_size(i915))
> - ilk_lut_write_indexed(crtc_state,
> PRE_CSC_GAMC_DATA(pipe), 1 << 16);
> + ilk_lut_write(crtc_state, PRE_CSC_GAMC_DATA(pipe), 1 <<
> 16);
>
> ilk_lut_write(crtc_state, PRE_CSC_GAMC_INDEX(pipe), 0); } @@ -
> 1165,10 +1154,10 @@ icl_program_gamma_superfine_segment(const struct
> intel_crtc_state *crtc_state)
> for (i = 0; i < 9; i++) {
> const struct drm_color_lut *entry = &lut[i];
>
> - ilk_lut_write_indexed(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> - ilk_lut_12p4_ldw(entry));
> - ilk_lut_write_indexed(crtc_state,
> PREC_PAL_MULTI_SEG_DATA(pipe),
> - ilk_lut_12p4_udw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> + ilk_lut_12p4_ldw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_DATA(pipe),
> + ilk_lut_12p4_udw(entry));
> }
>
> ilk_lut_write(crtc_state, PREC_PAL_MULTI_SEG_INDEX(pipe), @@ -
> 1204,10 +1193,10 @@ icl_program_gamma_multi_segment(const struct
> intel_crtc_state *crtc_state)
> for (i = 1; i < 257; i++) {
> entry = &lut[i * 8];
>
> - ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> - ilk_lut_12p4_ldw(entry));
> - ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> - ilk_lut_12p4_udw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> + ilk_lut_12p4_ldw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> + ilk_lut_12p4_udw(entry));
> }
>
> /*
> @@ -1225,10 +1214,10 @@ icl_program_gamma_multi_segment(const
> struct intel_crtc_state *crtc_state)
> for (i = 0; i < 256; i++) {
> entry = &lut[i * 8 * 128];
>
> - ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> - ilk_lut_12p4_ldw(entry));
> - ilk_lut_write_indexed(crtc_state, PREC_PAL_DATA(pipe),
> - ilk_lut_12p4_udw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> + ilk_lut_12p4_ldw(entry));
> + ilk_lut_write(crtc_state, PREC_PAL_DATA(pipe),
> + ilk_lut_12p4_udw(entry));
> }
>
> ilk_lut_write(crtc_state, PREC_PAL_INDEX(pipe), diff --git
> a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index fcc3f49c5445..fa4b808a8134 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -114,32 +114,28 @@ static bool intel_dsb_prev_ins_is_write(struct
> intel_dsb *dsb,
> return prev_opcode == opcode && prev_reg ==
> i915_mmio_reg_offset(reg); }
>
> +static bool intel_dsb_prev_ins_is_mmio_write(struct intel_dsb *dsb,
> +i915_reg_t reg) {
> + return intel_dsb_prev_ins_is_write(dsb,
> DSB_OPCODE_MMIO_WRITE, reg); }
> +
> static bool intel_dsb_prev_ins_is_indexed_write(struct intel_dsb *dsb,
> i915_reg_t reg) {
> return intel_dsb_prev_ins_is_write(dsb,
> DSB_OPCODE_INDEXED_WRITE, reg); }
>
> /**
> - * intel_dsb_indexed_reg_write() -Write to the DSB context for auto
> - * increment register.
> + * intel_dsb_reg_write() - Emit register wriite to the DSB context
> * @dsb: DSB context
> * @reg: register address.
> * @val: value.
> *
> * This function is used for writing register-value pair in command
> - * buffer of DSB for auto-increment register. During command buffer
> overflow,
> - * a warning is thrown and rest all erroneous condition register
> programming
> - * is done through mmio write.
> + * buffer of DSB.
> */
> -
> -void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
> - i915_reg_t reg, u32 val)
> +void intel_dsb_reg_write(struct intel_dsb *dsb,
> + i915_reg_t reg, u32 val)
> {
> - u32 *buf = dsb->cmd_buf;
> -
> - if (!assert_dsb_has_room(dsb))
> - return;
> -
> /*
> * For example the buffer will look like below for 3 dwords for auto
> * increment register:
> @@ -156,57 +152,39 @@ void intel_dsb_indexed_reg_write(struct intel_dsb
> *dsb,
> * we are writing odd no of dwords, Zeros will be added in the end
> for
> * padding.
> */
> - if (!intel_dsb_prev_ins_is_indexed_write(dsb, reg)) {
> - /* Every instruction should be 8 byte aligned. */
> - dsb->free_pos = ALIGN(dsb->free_pos, 2);
> -
> - dsb->ins_start_offset = dsb->free_pos;
> -
> - /* Update the size. */
> - buf[dsb->free_pos++] = 1;
> -
> - /* Update the opcode and reg. */
> - buf[dsb->free_pos++] = (DSB_OPCODE_INDEXED_WRITE <<
> - DSB_OPCODE_SHIFT) |
> - i915_mmio_reg_offset(reg);
> -
> - /* Update the value. */
> - buf[dsb->free_pos++] = val;
> + if (!intel_dsb_prev_ins_is_mmio_write(dsb, reg) &&
> + !intel_dsb_prev_ins_is_indexed_write(dsb, reg)) {
> + intel_dsb_emit(dsb, val,
> + (DSB_OPCODE_MMIO_WRITE <<
> DSB_OPCODE_SHIFT) |
> + (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
> + i915_mmio_reg_offset(reg));
> } else {
> - /* Update the new value. */
> + 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];
> +
> + buf[dsb->ins_start_offset + 0] = 1; /* size */
> + 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;
> +
> + dsb->free_pos++;
> + }
> +
> buf[dsb->free_pos++] = val;
> -
> /* Update the size. */
> buf[dsb->ins_start_offset]++;
> +
> + /* if number of data words is odd, then the last dword
> should be 0.*/
> + if (dsb->free_pos & 0x1)
> + buf[dsb->free_pos] = 0;
> }
> -
> - /* 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_reg_write() -Write to the DSB context for normal
> - * register.
> - * @crtc_state: intel_crtc_state structure
> - * @reg: register address.
> - * @val: value.
> - *
> - * This function is used for writing register-value pair in command
> - * buffer of DSB. During command buffer overflow, a warning is thrown
> - * and rest all erroneous condition register programming is done
> - * through mmio write.
> - */
> -void intel_dsb_reg_write(struct intel_dsb *dsb,
> - i915_reg_t reg, u32 val)
> -{
> - if (!assert_dsb_has_room(dsb))
> - return;
> -
> - intel_dsb_emit(dsb, val,
> - (DSB_OPCODE_MMIO_WRITE << DSB_OPCODE_SHIFT) |
> - (DSB_BYTE_EN << DSB_BYTE_EN_SHIFT) |
> - i915_mmio_reg_offset(reg));
> }
>
> /**
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h
> b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 25f13c4d5389..25d774049cc2 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -17,8 +17,6 @@ struct intel_dsb *intel_dsb_prepare(struct intel_crtc
> *crtc); void intel_dsb_cleanup(struct intel_dsb *dsb); void
> intel_dsb_reg_write(struct intel_dsb *dsb,
> i915_reg_t reg, u32 val);
> -void intel_dsb_indexed_reg_write(struct intel_dsb *dsb,
> - i915_reg_t reg, u32 val);
> void intel_dsb_commit(struct intel_dsb *dsb);
>
> #endif
> --
> 2.37.4
