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

[Laurent Pinchart]

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 
> 
> ---
> 
> 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 

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

[Kieran Bingham]

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 

---

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;
+   u16 flags;
+   u32 pre_ext_cmd_plist;
+
+   u32 post_ext_cmd_qty;
+   u32 post_ext_cmd_plist;
+};
+
+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 {
+   u32 cmd;
+   u32 flags;
+   u32 data;
+   u32 reserved;
+};
+
 /**
  * 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
  * @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
  * @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