Hi Laurent,
On 02/08/18 15:39, Laurent Pinchart wrote:
Hi Kieran,
Thank you for the patch.
Thank you for (hopefully the last) review on this series :)
On Tuesday, 17 July 2018 23:35:50 EEST Kieran Bingham wrote:
From: Kieran Bingham kieran.bingham+renesas@ideasonboard.com
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
v5:
- Rename vsp1_dl_ext_header field names
- Rename @extended -> @extension
- Remove unnecessary VI6_DL_SWAP changes
- Rename @cmd_opcode -> @opcode
- Drop unused @data_size field
- Move iteration of WPF's inside vsp1_dlm_setup
- Rename vsp1_dl_ext_cmd_header -> vsp1_pre_ext_dl_body
- Rename vsp1_pre_ext_dl_body->cmd to vsp1_pre_ext_dl_body->opcode
- Rename vsp1_dl_ext_header->reserved0 to vsp1_dl_ext_header->padding
- vsp1_pre_ext_dl_body: Rename 'data' to 'address_set'
- vsp1_pre_ext_dl_body: Add struct documentation
- Document ordering of 16bit accesses for flags in vsp1_dl_ext_header
drivers/media/platform/vsp1/vsp1.h | 1 +- drivers/media/platform/vsp1/vsp1_dl.c | 104 ++++++++++++++++++++++++- drivers/media/platform/vsp1/vsp1_dl.h | 25 ++++++- drivers/media/platform/vsp1/vsp1_drv.c | 4 +- drivers/media/platform/vsp1/vsp1_regs.h | 2 +- 5 files changed, 132 insertions(+), 4 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 90e0a11c29b5..2fffe977aa35 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,12 +37,59 @@ struct vsp1_dl_header { u32 flags; };
+/**
- struct vsp1_dl_ext_header - Extended display list header
- @padding: padding zero bytes for alignment
- @pre_ext_dl_num_cmd: number of pre-extended command bodies to parse
- @flags: enables or disables execution of the pre and post command
- @pre_ext_dl_plist: start address of pre-extended display list bodies
- @post_ext_dl_num_cmd: number of post-extended command bodies to parse
- @post_ext_dl_plist: start address of post-extended display list bodies
- */
+struct vsp1_dl_ext_header {
- u32 padding;
- /*
* The datasheet represents flags as stored before pre_ext_dl_num_cmd,* expecting 32-bit accesses. The flags are appropriate to the whole* header, not just the pre_ext command, and thus warrant being* separated out. Due to byte ordering, and representing as 16 bit* values here, the flags must be positioned after the* pre_ext_dl_num_cmd.*/- u16 pre_ext_dl_num_cmd;
- u16 flags;
- u32 pre_ext_dl_plist;
- u32 post_ext_dl_num_cmd;
- u32 post_ext_dl_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_pre_ext_dl_body - Pre Extended Display List Body
- @opcode: Extended display list command operation code
- @flags: Pre-extended command flags. These are specific to each command
- @address_set: Source address set pointer. Must have 16 byte alignment
s/byte/bytes/
I've made this "16-byte alignment" - I don't think you can align to a plural, or at least it doesn't sound right in my head.
It's an interesting grammatical question that I couldn't find the answer to, so I've asked online @ ell.stackexchange.com [0]
[0] https://ell.stackexchange.com/questions/175063/what-is-the-correct-use-of-a-...
- @reserved: Zero bits for alignment.
- */
+struct vsp1_pre_ext_dl_body {
- u32 opcode;
- u32 flags;
- u32 address_set;
- u32 reserved;
+};
+/**
- struct vsp1_dl_body - Display list body
- @list: entry in the display list list of bodies
- @free: entry in the pool free body list
@@ -95,9 +145,12 @@ struct vsp1_dl_body_pool {
- @list: entry in the display list manager lists
- @dlm: the display list manager
- @header: display list header
- @extension: 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 command to be issued through extended dl header
- @post_cmd: post command 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 +160,15 @@ struct vsp1_dl_list { struct vsp1_dl_manager *dlm;
struct vsp1_dl_header *header;
struct vsp1_dl_ext_header *extension; 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;
@@ -495,6 +552,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].opcode = cmd->opcode;
- cmd->cmds[0].flags = cmd->flags;
- cmd->cmds[0].address_set = 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; @@ -547,6 +612,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->extension)
return;- dl->extension->flags = 0;
- if (dl->pre_cmd) {
dl->extension->pre_ext_dl_plist = dl->pre_cmd->cmd_dma;dl->extension->pre_ext_dl_num_cmd = dl->pre_cmd->num_cmds;dl->extension->flags |= VSP1_DLH_EXT_PRE_CMD_EXEC;vsp1_dl_ext_cmd_fill_header(dl->pre_cmd);- }
- if (dl->post_cmd) {
dl->extension->post_ext_dl_plist = dl->post_cmd->cmd_dma;dl->extension->post_ext_dl_num_cmd = dl->post_cmd->num_cmds;dl->extension->flags |= VSP1_DLH_EXT_POST_CMD_EXEC;vsp1_dl_ext_cmd_fill_header(dl->post_cmd);- }
}
static bool vsp1_dl_list_hw_update_pending(struct vsp1_dl_manager *dlm) @@ -736,9 +822,16 @@ unsigned int vsp1_dlm_irq_frame_end(struct vsp1_dl_manager *dlm) /* Hardware Setup */ void vsp1_dlm_setup(struct vsp1_device *vsp1) {
- unsigned int i; 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;
- u32 ext_dl = (0x02 << VI6_DL_EXT_CTRL_POLINT_SHIFT) |
VI6_DL_EXT_CTRL_DLPRI | VI6_DL_EXT_CTRL_EXT;To match the style of the file, can you move the trailing | under the = ?
Done.
- if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL))
for (i = 0; i < vsp1->info->wpf_count; ++i)vsp1_write(vsp1, VI6_DL_EXT_CTRL(i), ext_dl);And add {} to the if ?
Added.
With these small updates,
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Collected.
If there's no other issue with v5 that would require a v6, I'll fix this when applying.
vsp1_write(vsp1, VI6_DL_CTRL, ctrl); vsp1_write(vsp1, VI6_DL_SWAP, VI6_DL_SWAP_LWS); @@ -792,7 +885,11 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, * memory. An extra body is allocated on top of the prealloc to account * for the cached body used by the vsp1_pipeline object. */
- 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);
dlm->pool = vsp1_dl_body_pool_create(vsp1, prealloc + 1, VSP1_DL_NUM_ENTRIES, header_size);
@@ -808,6 +905,11 @@ struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, return NULL; }
/* The extended header immediately follows the header. */if (vsp1_feature(vsp1, VSP1_HAS_EXT_DL))dl->extension = (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 7dba0469c92e..afefd5bfa136 100644 --- a/drivers/media/platform/vsp1/vsp1_dl.h +++ b/drivers/media/platform/vsp1/vsp1_dl.h @@ -20,6 +20,31 @@ struct vsp1_dl_manager; #define VSP1_DL_FRAME_END_COMPLETED BIT(0) #define VSP1_DL_FRAME_END_INTERNAL BIT(1)
+/**
- struct vsp1_dl_ext_cmd - Extended Display command
- @free: entry in the pool of free commands list
- @opcode: command type 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 body
- @data: memory allocation for command-specific data
- @data_dma: DMA address for command-specific data
- */
+struct vsp1_dl_ext_cmd {
- struct list_head free;
- u8 opcode;
- u32 flags;
- struct vsp1_pre_ext_dl_body *cmds;
- unsigned int num_cmds;
- dma_addr_t cmd_dma;
- void *data;
- dma_addr_t data_dma;
+};
void vsp1_dlm_setup(struct vsp1_device *vsp1);
struct vsp1_dl_manager *vsp1_dlm_create(struct vsp1_device *vsp1, diff --git a/drivers/media/platform/vsp1/vsp1_drv.c b/drivers/media/platform/vsp1/vsp1_drv.c index 3367c2ba990d..b6619c9c18bb 100644 --- a/drivers/media/platform/vsp1/vsp1_drv.c +++ b/drivers/media/platform/vsp1/vsp1_drv.c @@ -754,7 +754,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,@@ -774,7 +774,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,diff --git a/drivers/media/platform/vsp1/vsp1_regs.h b/drivers/media/platform/vsp1/vsp1_regs.h index 0d249ff9f564..5ea9f9070cf3 100644 --- a/drivers/media/platform/vsp1/vsp1_regs.h +++ b/drivers/media/platform/vsp1/vsp1_regs.h @@ -72,7 +72,7 @@ #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