Re: [PATCH v3 04/11] drm/i915/dsb: Indexed register write function for DSB.

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: Animesh Manna <animesh.manna@intel.com>, intel-gfx@lists.freedesktop.org
Cc: Jani Nikula <jani.nikula@intel.com>
Subject: Re: [PATCH v3 04/11] drm/i915/dsb: Indexed register write function for DSB.
Date: Wed, 28 Aug 2019 22:16:45 +0530	[thread overview]
Message-ID: <e676c002-e4b7-dcb5-201b-58feb3d6337e@intel.com> (raw)
In-Reply-To: <20190827191026.26175-5-animesh.manna@intel.com>

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> DSB can program large set of data through indexed register write
> (opcode 0x9) in one shot. Will be using for bulk register programming
Reshuffle-> This feature can be used for bulk register programming e.g.
> e.g. gamma lut programming, HDR meta data programming.
>
> v1: Initial version.
>
> v2: simplified code by using ALIGN(). (Chris)
>
> Cc: Shashank Sharma <shashank.sharma@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Animesh Manna <animesh.manna@intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_dsb.c | 48 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  8 ++++
>   2 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index df288446caeb..520f2bbcc8ae 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -22,6 +22,7 @@
>   #define DSB_OPCODE_INDEXED_WRITE	0x9
>   #define DSB_OPCODE_POLL			0xA
>   #define DSB_BYTE_EN			(0xf << 20)
> +#define DSB_REG_VALUE_MASK		0xfffff
>   
>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
> @@ -96,6 +97,53 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   	}
>   }
>   
> +void intel_dsb_indexed_reg_write(struct intel_dsb *dsb, i915_reg_t reg,
> +				 u32 val)
We might need one space here, to get this aligned to start of the line 
above (or is this due to my mail client ?).
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +	u32 reg_val;
> +
- Do we need a HAS_DSB check at start or the caller will take care of it ?
> +	if (!buf) {
> +		I915_WRITE(reg, val);
I am under the assumption that indexed reg write is to write multiple 
registers, and 'reg' is the base value of the register set. I dont think 
it makes sense to write a single register here in case of DSB failure ?
> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow.\n");
> +		return;
> +	}
> +	reg_val = buf[dsb->ins_start_offset + 1] & DSB_REG_VALUE_MASK;

Why do we have this +1 here ? on the very first run (when 
ins_start_offset is 0), this will fetch reg_val = buf[1]; Is this what 
we want to do ?

> +	if (reg_val != i915_mmio_reg_offset(reg)) {
> +		/* Every instruction should be 8 byte aligned. */
> +		dsb->free_pos = ALIGN(dsb->free_pos, 2);
The comment says that you want the position to be 8 bytes align, but the 
factor sent to the macro is 2 ?
> +
> +		/* Update the size. */
> +		dsb->ins_start_offset = dsb->free_pos;
This comment is irrelevant, you are not updating the size, you are 
caching the base memory location.
> +		buf[dsb->free_pos++] = 1;

What does this indicate ? What is 1 ?

It would be great if you can add an aski graph/table and explain what 
does this DSB command buffer actually contains as per your design.

> +
> +		/* 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]++;
> +	}

The code is looking unnecessarily complicated as ins_start_offset and 
free_pos are being used for multiple reasons. I guess you can use some 
local variables like:

u32 base = count = ALIGN();

  {

     buf [count++] = size;

    buf [count++] = command;

   buf [count++] = value;

}

dsb->start_offset = base;

dsb->free_pos = count;

> +
> +	/* 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 = dsb->crtc;
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 1b33ab118640..c848747f52d9 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 cmd_buf_tail.
>   	 */
>   	int free_pos;
> +
> +	/*
> +	 * ins_start_offset will help to store start address
> +	 * of the dsb instuction of auto-increment register.
> +	 */
> +	u32 ins_start_offset;
How about the variable to be named as base_offset ? Also, do we need to 
keep this saved, even when the writing is done ?
>   };
>   
>   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);

Cross check the alignment of second line.

- Shashank

>   
>   #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx