[Intel-gfx] [PATCH v8 04/10] drm/i915/dsb: Indexed register write function for DSB.

[Jani Nikula]

On Fri, 20 Sep 2019, Animesh Manna <animesh.manna at intel.com> wrote:
> DSB can program large set of data through indexed register write
> (opcode 0x9) in one shot. DSB feature can be used for bulk register
> programming e.g. gamma lut programming, HDR meta data programming.
>
> v1: initial version.
> v2: simplified code by using ALIGN(). (Chris)
> v3: ascii table added as code comment. (Shashank)
> v4: cosmetic changes done. (Shashank)
> v5: reset ins_start_offset. (Jani)
>
> Cc: Shashank Sharma <shashank.sharma at intel.com>
> Cc: Imre Deak <imre.deak at intel.com>
> Cc: Jani Nikula <jani.nikula at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Reviewed-by: Shashank Sharma <shashank.sharma at intel.com>
> Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 67 ++++++++++++++++++++++++
>  drivers/gpu/drm/i915/display/intel_dsb.h |  8 +++
>  2 files changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index f94cd6dc98b6..0b5119135d4d 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -12,8 +12,10 @@
>  /* DSB opcodes. */
>  #define DSB_OPCODE_SHIFT		24
>  #define DSB_OPCODE_MMIO_WRITE		0x1
> +#define DSB_OPCODE_INDEXED_WRITE	0x9
>  #define DSB_BYTE_EN			0xF
>  #define DSB_BYTE_EN_SHIFT		20
> +#define DSB_REG_VALUE_MASK		0xfffff
>  
>  struct intel_dsb *
>  intel_dsb_get(struct intel_crtc *crtc)
> @@ -83,9 +85,74 @@ void intel_dsb_put(struct intel_dsb *dsb)
>  		mutex_unlock(&i915->drm.struct_mutex);
>  		dsb->cmd_buf = NULL;
>  		dsb->free_pos = 0;
> +		dsb->ins_start_offset = 0;

This is not enough to address the sequence I described. Sure, we're not
hitting the issue now, but it's not a benign thing.

So imagine you do:

intel_dsb_get()
intel_dsb_indexed_reg_write(dsb, FOO, 0);
intel_dsb_indexed_reg_write(dsb, FOO, 0);
intel_dsb_reg_write(dsb, BAR, 0);
intel_dsb_indexed_reg_write(dsb, FOO, 0);
intel_dsb_commit()
intel_dsb_put()

Do you see what happens in the last indexed write? It looks at the
*first* indexed writes for whether this is a continuation, updating the
size there, ignoring the fact that there's something else in between.

BR,
Jani.


>  	}
>  }
>  
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
> +				 u32 val)
> +{
> +	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +	u32 reg_val;
> +
> +	if (!buf) {
> +		I915_WRITE(reg, val);
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow\n");
> +		return;
> +	}
> +
> +	/*
> +	 * For example the buffer will look like below for 3 dwords for auto
> +	 * increment register:
> +	 * +--------------------------------------------------------+
> +	 * | size = 3 | offset &| value1 | value2 | value3 | zero   |
> +	 * |          | opcode  |        |        |        |        |
> +	 * +--------------------------------------------------------+
> +	 * +          +         +        +        +        +        +
> +	 * 0          4         8        12       16       20       24
> +	 * Byte
> +	 *
> +	 * As every instruction is 8 byte aligned the index of dsb instruction
> +	 * will start always from even number while dealing with u32 array. If
> +	 * we are writing odd no of dwords, Zeros will be added in the end for
> +	 * padding.
> +	 */
> +	reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;
> +	if (reg_val != i915_mmio_reg_offset(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;
> +	} else {
> +		/* Update the new value. */
> +		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;
> +}
> +
>  void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
>  {
>  	struct intel_crtc *crtc = container_of(dsb, typeof(*crtc), dsb);
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 0686d67b34d5..d6ced4422814 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -30,11 +30,19 @@ struct intel_dsb {
>  	 * and help in calculating tail of command buffer.
>  	 */
>  	int free_pos;
> +
> +	/*
> +	 * ins_start_offset will help to store start address
> +	 * of the dsb instuction of auto-increment register.
> +	 */
> +	u32 ins_start_offset;
>  };
>  
>  struct intel_dsb *
>  intel_dsb_get(struct intel_crtc *crtc);
>  void intel_dsb_put(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);
>  
>  #endif

-- 
Jani Nikula, Intel Open Source Graphics Center