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

["Sharma, Shashank" <shashank.sharma@intel.com>]

On 8/28/2019 12:40 AM, Animesh Manna wrote:
> DSB support single register write through opcode 0x1. Generic
> api created which accumulate all single register write in a batch
> buffer and once DSB is triggered, it will program all the registers
> at the same time.
>
> 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 | 36 ++++++++++++++++++++++++
>   drivers/gpu/drm/i915/display/intel_dsb.h |  9 ++++++
>   2 files changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> index a2845df90573..df288446caeb 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -9,6 +9,20 @@
>   
>   #define DSB_BUF_SIZE    (2 * PAGE_SIZE)
>   
> +/* DSB opcodes. */
> +#define DSB_OPCODE_SHIFT		24
> +#define DSB_OPCODE_NOOP			0x0
> +#define DSB_OPCODE_MMIO_WRITE		0x1
> +#define DSB_OPCODE_WAIT_FOR_US		0x2
> +#define DSB_OPCODE_WAIT_FOR_LINES	0x3
> +#define DSB_OPCODE_WAIT_FOR_VBLANK	0x4
> +#define DSB_OPCODE_WAIT_FOR_SL_IN	0x5
May be SL_IR (to indicate in range)
> +#define DSB_OPCODE_WAIT_FOR_SL_OUT	0x6
May be SL_OOR (out of range) or _OR
> +#define DSB_OPCODE_GENERATE_INT		0x7
> +#define DSB_OPCODE_INDEXED_WRITE	0x9
> +#define DSB_OPCODE_POLL			0xA
> +#define DSB_BYTE_EN			(0xf << 20)

But its a nitpick but as we are going by shifts for OPCODE, may be would look consistent to create 2 different macros for enable also.

#define DSB_BYTE_EN			0xF

#define DSB_BYTE_EN_SHIFT		20

   

> +

This patch is using only 3 of the defined commands: 
DSB_OPCODE_MMIO_WRITE, DSB_OPCODE_SHIFT, DSB_BYTE_EN

All other definitions are unused. Please add them only in the patch 
where they are being used.

>   struct intel_dsb *
>   intel_dsb_get(struct intel_crtc *crtc)
>   {
> @@ -81,3 +95,25 @@ void intel_dsb_put(struct intel_dsb *dsb)
>   		mutex_unlock(&i915->drm.struct_mutex);
>   	}
>   }
> +
> +void intel_dsb_reg_write(struct intel_dsb *dsb, i915_reg_t reg, u32 val)
> +{
> +	struct intel_crtc *crtc = dsb->crtc;
> +	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> +	u32 *buf = dsb->cmd_buf;
> +
Do we need a HAS_DSB() check here ? Or would it be taken care in the 
caller ?
> +	if (!buf) {
> +		I915_WRITE(reg, val);

We can debate on if this is a good idea to fallback to I915_write() if 
DSB is not available. May be we should think about why are we seeing 
this condition (!buf), and that can be checked only when I see the caller.

For now, lets assume we will fallback to I915_WRITE();

> +		return;
> +	}
> +
> +	if (WARN_ON(dsb->free_pos >= DSB_BUF_SIZE)) {
> +		DRM_DEBUG_KMS("DSB buffer overflow.\n");
Wouldn't it make sense to do a I915_WRITE(reg, val) here also, like above ?
> +		return;
> +	}
> +
> +	buf[dsb->free_pos++] = val;
> +	buf[dsb->free_pos++] = (DSB_OPCODE_MMIO_WRITE  <<
> +				DSB_OPCODE_SHIFT) | DSB_BYTE_EN |
> +				i915_mmio_reg_offset(reg);
Once the write is done, are we planning to reset this free_pos ? May be 
in some upcoming patch in the series ?
> +}
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> index 4a4091cadc1e..1b33ab118640 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> @@ -6,6 +6,8 @@
>   #ifndef _INTEL_DSB_H
>   #define _INTEL_DSB_H
>   
> +#include "i915_reg.h"
> +
>   struct intel_crtc;
>   struct i915_vma;
>   
> @@ -22,10 +24,17 @@ struct intel_dsb {
>   	enum dsb_id id;
>   	u32 *cmd_buf;
>   	struct i915_vma *vma;
> +
> +	/*
> +	 * free_pos will point the first free entry position
> +	 * and help in calculating cmd_buf_tail.
> +	 */
> +	int free_pos;
>   };
>   
>   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);
>   
>   #endif
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx