Re: [PATCH v4 08/11] media: vsp1: Add support for extended display list headers

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Thursday, 3 May 2018 16:36:19 EEST Kieran Bingham wrote:
> Extended display list headers allow pre and post command lists to be
> executed by the VSP pipeline. This provides the base support for
> features such as AUTO_FLD (for interlaced support) and AUTO_DISP (for
> supporting continuous camera preview pipelines.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
> 
> ---
> 
> v2:
>  - remove __packed attributes
> ---
>  drivers/media/platform/vsp1/vsp1.h      |  1 +-
>  drivers/media/platform/vsp1/vsp1_dl.c   | 83 +++++++++++++++++++++++++-
>  drivers/media/platform/vsp1/vsp1_dl.h   | 29 ++++++++-
>  drivers/media/platform/vsp1/vsp1_drv.c  |  7 +-
>  drivers/media/platform/vsp1/vsp1_regs.h |  5 +-
>  5 files changed, 116 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/platform/vsp1/vsp1.h
> b/drivers/media/platform/vsp1/vsp1.h index f0d21cc8e9ab..56c62122a81a
> 100644
> --- a/drivers/media/platform/vsp1/vsp1.h
> +++ b/drivers/media/platform/vsp1/vsp1.h
> @@ -53,6 +53,7 @@ struct vsp1_uif;
>  #define VSP1_HAS_HGO		(1 << 7)
>  #define VSP1_HAS_HGT		(1 << 8)
>  #define VSP1_HAS_BRS		(1 << 9)
> +#define VSP1_HAS_EXT_DL		(1 << 10)
> 
>  struct vsp1_device_info {
>  	u32 version;
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.c
> b/drivers/media/platform/vsp1/vsp1_dl.c index 56514cd51c51..b64d32535edc
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.c
> +++ b/drivers/media/platform/vsp1/vsp1_dl.c
> @@ -22,6 +22,9 @@
>  #define VSP1_DLH_INT_ENABLE		(1 << 1)
>  #define VSP1_DLH_AUTO_START		(1 << 0)
> 
> +#define VSP1_DLH_EXT_PRE_CMD_EXEC	(1 << 9)
> +#define VSP1_DLH_EXT_POST_CMD_EXEC	(1 << 8)
> +
>  struct vsp1_dl_header_list {
>  	u32 num_bytes;
>  	u32 addr;
> @@ -34,11 +37,34 @@ struct vsp1_dl_header {
>  	u32 flags;
>  };
> 
> +struct vsp1_dl_ext_header {
> +	u32 reserved0;		/* alignment padding */
> +
> +	u16 pre_ext_cmd_qty;

Should this be called pre_ext_dl_num_cmd to match the datasheet ?

> +	u16 flags;

Aren't the flags supposed to come before the pre_ext_dl_num_cmd field ?

> +	u32 pre_ext_cmd_plist;

And pre_ext_dl_plist ?

> +
> +	u32 post_ext_cmd_qty;
> +	u32 post_ext_cmd_plist;

Similar comments for these variables.

> +};
> +
> +struct vsp1_dl_header_extended {
> +	struct vsp1_dl_header header;
> +	struct vsp1_dl_ext_header ext;
> +};
> +
>  struct vsp1_dl_entry {
>  	u32 addr;
>  	u32 data;
>  };
> 
> +struct vsp1_dl_ext_cmd_header {

Isn't this referred to in the datasheet as a body entry, not a header ? How 
about naming it vsp1_dl_ext_cmd_entry ? Or just vsp1_dl_ext_cmd (in which case 
the other structure that goes by the same name would need to be renamed) ?

> +	u32 cmd;
> +	u32 flags;
> +	u32 data;
> +	u32 reserved;

The datasheet documents this as two 64-bit fields, shouldn't we handle the 
structure the same way ?

> +};
> +
>  /**
>   * struct vsp1_dl_body - Display list body
>   * @list: entry in the display list list of bodies
> @@ -95,9 +121,12 @@ struct vsp1_dl_body_pool {
>   * @list: entry in the display list manager lists
>   * @dlm: the display list manager
>   * @header: display list header
> + * @extended: extended display list header. NULL for normal lists

Should we name this extension instead of extended ?

>   * @dma: DMA address for the header
>   * @body0: first display list body
>   * @bodies: list of extra display list bodies
> + * @pre_cmd: pre cmd to be issued through extended dl header
> + * @post_cmd: post cmd to be issued through extended dl header

I think you can spell command in full.

>   * @has_chain: if true, indicates that there's a partition chain
>   * @chain: entry in the display list partition chain
>   * @internal: whether the display list is used for internal purpose
> @@ -107,11 +136,15 @@ struct vsp1_dl_list {
>  	struct vsp1_dl_manager *dlm;
> 
>  	struct vsp1_dl_header *header;
> +	struct vsp1_dl_ext_header *extended;
>  	dma_addr_t dma;
> 
>  	struct vsp1_dl_body *body0;
>  	struct list_head bodies;
> 
> +	struct vsp1_dl_ext_cmd *pre_cmd;
> +	struct vsp1_dl_ext_cmd *post_cmd;
> +
>  	bool has_chain;
>  	struct list_head chain;
> 
> @@ -496,6 +529,14 @@ int vsp1_dl_list_add_chain(struct vsp1_dl_list *head,
>  	return 0;
>  }
> 
> +static void vsp1_dl_ext_cmd_fill_header(struct vsp1_dl_ext_cmd *cmd)

> +{
> +	cmd->cmds[0].cmd = cmd->cmd_opcode;
> +	cmd->cmds[0].flags = cmd->flags;
> +	cmd->cmds[0].data = cmd->data_dma;
> +	cmd->cmds[0].reserved = 0;
> +}
> +
>  static void vsp1_dl_list_fill_header(struct vsp1_dl_list *dl, bool is_last)
> {
>  	struct vsp1_dl_manager *dlm = dl->dlm;
> @@ -548,6 +589,27 @@ static void vsp1_dl_list_fill_header(struct
> vsp1_dl_list *dl, bool is_last) */
>  		dl->header->flags = VSP1_DLH_INT_ENABLE;
>  	}
> +
> +	if (!dl->extended)
> +		return;
> +
> +	dl->extended->flags = 0;
> +
> +	if (dl->pre_cmd) {
> +		dl->extended->pre_ext_cmd_plist = dl->pre_cmd->cmd_dma;
> +		dl->extended->pre_ext_cmd_qty = dl->pre_cmd->num_cmds;
> +		dl->extended->flags |= VSP1_DLH_EXT_PRE_CMD_EXEC;
> +
> +		vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
> +	}
> +
> +	if (dl->post_cmd) {
> +		dl->extended->pre_ext_cmd_plist = dl->post_cmd->cmd_dma;
> +		dl->extended->pre_ext_cmd_qty = dl->post_cmd->num_cmds;
> +		dl->extended->flags |= VSP1_DLH_EXT_POST_CMD_EXEC;
> +
> +		vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);
> +	}
>  }
> 
>  static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm)
> @@ -735,14 +797,20 @@ unsigned int vsp1_dlm_irq_frame_end(struct
> vsp1_dl_manager *dlm) }
> 
>  /* Hardware Setup */
> -void vsp1_dlm_setup(struct vsp1_device *vsp1)
> +void vsp1_dlm_setup(struct vsp1_device *vsp1, unsigned int index)
>  {
>  	u32 ctrl = (256 << VI6_DL_CTRL_AR_WAIT_SHIFT)
> 
>  		 | VI6_DL_CTRL_DC2 | VI6_DL_CTRL_DC1 | VI6_DL_CTRL_DC0
>  		 | VI6_DL_CTRL_DLE;
> 
> +	if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL))
> +		vsp1_write(vsp1, VI6_DL_EXT_CTRL(index),
> +			   (0x02 << VI6_DL_EXT_CTRL_POLINT_SHIFT) |
> +			   VI6_DL_EXT_CTRL_DLPRI | VI6_DL_EXT_CTRL_EXT);
> +
>  	vsp1_write(vsp1, VI6_DL_CTRL, ctrl);
> -	vsp1_write(vsp1, VI6_DL_SWAP, VI6_DL_SWAP_LWS);
> +	vsp1_write(vsp1, VI6_DL_SWAP(index), VI6_DL_SWAP_LWS |
> +			 ((index == 1) ? VI6_DL_SWAP_IND : 0));

Is this change needed ? If VI6_DL_SWAP_IND is not set in VI6_DL_SWAP(1), 
display list swap for WPF1 is supposed to be controlled by VI6_DL_SWAP(0) 
according to the datasheet.

If that's not the case and this change is needed, I would split support for 
VI6_DL_SWAP(n) to a separate patch, and moved it before 07/11.

>  }
> 
>  void vsp1_dlm_reset(struct vsp1_dl_manager *dlm)
> @@ -787,7 +855,11 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct
> vsp1_device *vsp1,
>  	 * fragmentation, with the header located right after the body in
>  	 * memory.
>  	 */
> -	header_size = ALIGN(sizeof(struct vsp1_dl_header), 8);
> +	header_size = vsp1_feature(vsp1, VSP1_HAS_EXT_DL) ?
> +			sizeof(struct vsp1_dl_header_extended) :
> +			sizeof(struct vsp1_dl_header);
> +
> +	header_size = ALIGN(header_size, 8);

We will have to improve header handling at some point. Not all headers require 
extensions.

>  	dlm->pool = vsp1_dl_body_pool_create(vsp1, prealloc,
>  					     VSP1_DL_NUM_ENTRIES, header_size);
> @@ -803,6 +875,11 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct
> vsp1_device *vsp1, return NULL;
>  		}
> 
> +		/* The extended header immediately follows the header */

s/ \*/. */

> +		if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL))
> +			dl->extended = (void *)dl->header
> +				     + sizeof(*dl->header);
> +
>  		list_add_tail(&dl->list, &dlm->free);
>  	}
> 
> diff --git a/drivers/media/platform/vsp1/vsp1_dl.h
> b/drivers/media/platform/vsp1/vsp1_dl.h index 216bd23029dd..aa5f4adc6617
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_dl.h
> +++ b/drivers/media/platform/vsp1/vsp1_dl.h
> @@ -20,7 +20,34 @@ struct vsp1_dl_manager;
>  #define VSP1_DL_FRAME_END_COMPLETED		BIT(0)
>  #define VSP1_DL_FRAME_END_INTERNAL		BIT(1)
> 
> -void vsp1_dlm_setup(struct vsp1_device *vsp1);
> +/**
> + * struct vsp1_dl_ext_cmd - Extended Display command
> + * @free: entry in the pool of free commands list
> + * @cmd_opcode: command type opcode

Maybe just opcode ?

> + * @flags: flags used by the command
> + * @cmds: array of command bodies for this extended cmd
> + * @num_cmds: quantity of commands in @cmds array
> + * @cmd_dma: DMA address of the command bodies

s/command bodies/commands body/ ?

> + * @data: memory allocation for command specific data
> + * @data_dma: DMA address for command specific data

s/command specific/command-specific/

> + * @data_size: size of the @data_dma memory in bytes

data_size is set but otherwise never used. Should we drop the field, or make 
use of it ?

> + */
> +struct vsp1_dl_ext_cmd {
> +	struct list_head free;
> +
> +	u8 cmd_opcode;
> +	u32 flags;
> +
> +	struct vsp1_dl_ext_cmd_header *cmds;
> +	unsigned int num_cmds;
> +	dma_addr_t cmd_dma;
> +
> +	void *data;
> +	dma_addr_t data_dma;
> +	size_t data_size;
> +};
> +
> +void vsp1_dlm_setup(struct vsp1_device *vsp1, unsigned int index);
> 
>  struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1,
>  					unsigned int index,
> diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
> b/drivers/media/platform/vsp1/vsp1_drv.c index 0fc388bf5a33..26a7b4d32e6c
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_drv.c
> +++ b/drivers/media/platform/vsp1/vsp1_drv.c
> @@ -545,7 +545,8 @@ static int vsp1_device_init(struct vsp1_device *vsp1)
>  	vsp1_write(vsp1, VI6_DPR_HGT_SMPPT, (7 << VI6_DPR_SMPPT_TGW_SHIFT) |
>  		   (VI6_DPR_NODE_UNUSED << VI6_DPR_SMPPT_PT_SHIFT));
> 
> -	vsp1_dlm_setup(vsp1);
> +	for (i = 0; i < vsp1->info->wpf_count; ++i)
> +		vsp1_dlm_setup(vsp1, i);
> 
>  	return 0;
>  }
> @@ -754,7 +755,7 @@ static const struct vsp1_device_info vsp1_device_infos[]
> = { .version = VI6_IP_VERSION_MODEL_VSPD_GEN3,
>  		.model = "VSP2-D",
>  		.gen = 3,
> -		.features = VSP1_HAS_BRU | VSP1_HAS_WPF_VFLIP,
> +		.features = VSP1_HAS_BRU | VSP1_HAS_WPF_VFLIP | VSP1_HAS_EXT_DL,
>  		.lif_count = 1,
>  		.rpf_count = 5,
>  		.uif_count = 1,
> @@ -774,7 +775,7 @@ static const struct vsp1_device_info vsp1_device_infos[]
> = { .version = VI6_IP_VERSION_MODEL_VSPDL_GEN3,
>  		.model = "VSP2-DL",
>  		.gen = 3,
> -		.features = VSP1_HAS_BRS | VSP1_HAS_BRU,
> +		.features = VSP1_HAS_BRS | VSP1_HAS_BRU | VSP1_HAS_EXT_DL,
>  		.lif_count = 2,
>  		.rpf_count = 5,
>  		.uif_count = 2,
> diff --git a/drivers/media/platform/vsp1/vsp1_regs.h
> b/drivers/media/platform/vsp1/vsp1_regs.h index 0d249ff9f564..d054767570c1
> 100644
> --- a/drivers/media/platform/vsp1/vsp1_regs.h
> +++ b/drivers/media/platform/vsp1/vsp1_regs.h
> @@ -67,12 +67,13 @@
> 
>  #define VI6_DL_HDR_ADDR(n)		(0x0104 + (n) * 4)
> 
> -#define VI6_DL_SWAP			0x0114
> +#define VI6_DL_SWAP(n)			(0x0114 + (n) * 56)
> +#define VI6_DL_SWAP_IND			(1 << 31)
>  #define VI6_DL_SWAP_LWS			(1 << 2)
>  #define VI6_DL_SWAP_WDS			(1 << 1)
>  #define VI6_DL_SWAP_BTS			(1 << 0)
> 
> -#define VI6_DL_EXT_CTRL			0x011c
> +#define VI6_DL_EXT_CTRL(n)		(0x011c + (n) * 36)
>  #define VI6_DL_EXT_CTRL_NWE		(1 << 16)
>  #define VI6_DL_EXT_CTRL_POLINT_MASK	(0x3f << 8)
>  #define VI6_DL_EXT_CTRL_POLINT_SHIFT	8

-- 
Regards,

Laurent Pinchart