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

[Sharma, Shashank]

On 9/19/2019 10:08 PM, Jani Nikula wrote:
> On Wed, 18 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)
>>
>> 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 | 66 ++++++++++++++++++++++++
>>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 +++
>>   2 files changed, 74 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
>> index f94cd6dc98b6..2d80e452e284 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)
>> @@ -86,6 +88,70 @@ void intel_dsb_put(struct intel_dsb *dsb)
>>   	}
>>   }
>>   
>> +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;
> dsb->ins_start_offset is never reset anywhere.
>
> Imagine you do:
>
> 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);
>
> The last one will screw up the previous indexed writes.
>
> BR,
> Jani.
>
>
>> +	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;
>> +

Not sure if I got your point correctly, but here is the ins_start_offset 
getting reset, with the new free_pos aligned with 2

- Shashank

>> +		/* 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