Hi Laurent,
On 17/07/18 11:53, Laurent Pinchart wrote:
Hi Kieran,
On Monday, 16 July 2018 20:14:55 EEST Kieran Bingham wrote:
On 24/05/18 12:44, Laurent Pinchart wrote:
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(-)
[snip]
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
[snip]
+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 ?
Yes, renamed.
- u16 flags;
Aren't the flags supposed to come before the pre_ext_dl_num_cmd field ?
These are out-of-order to account for the fact that they are 16bit values.
Ah OK. It makes sense, but is a bit confusing when reading the datasheet.
Yes, I agree. Realising the byte-ordering was off was a bit of a pain point when I was testing too :D
I felt that keeping them described in the struct was cleaner than shifts and masks - but clearly this stands out, due to the byte-ordering.
Would you prefer I re-write this as 32 bit accesses (or even 64bit), with shifts? Or is a comment sufficient here ?
If it doesn't make the code too ugly, using larger accesses would be better I think. Otherwise a comment would do I suppose.
If we rewrite to be 32 bit accesses, would you be happy with the following naming:
u32 reserved0; u32 pre_ext_dl_num_cmd; /* Also stores command flags. */ u32 pre_ext_dl_plist; u32 post_ext_dl_num_cmd; u32 post_ext_dl_plist;
(Technically the flags are for the whole header, not the just the pre_ext, which is why I wanted it separated)
Actually - now I write that - the 'number of commands' is sort of a flag or a parameter? so maybe the following is just as appropriate?:
u32 reserved0;
Maybe "u32 zero;" or "u32 padding;" ? The datasheet states this is padding for alignment purpose.
I've used "padding".
u32 pre_ext_dl_flags; u32 pre_ext_dl_plist; u32 post_ext_dl_flags; u32 post_ext_dl_plist;
Or they could be named 'options', or parameters?
Any comments before I hack that in?
The annoying part is that the 'flags' aren't part of the pre_ext cmds, they declare whether the pre or post cmd should be executed (or both I presume, we are yet to see post-cmd usage)
I agree with you, having a separate flag field would be nicer, as the flags are shared. I'll chose the easy option of letting you decide what you like best :-) All the above options are equally good to me, provided you add a comment explaining why the flag comes after the num_cmd field if you decide to keep it as a separate field.
I've added a comment to explain why the flags must be after num_cmd. I feel it's better to keep the flags separated as they are not specific to the pre_cmd.
- 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.
Renamed.
+};
+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) ?
I think I was getting too creative. The reality is this part is really a 'header' describing the data in the body, but yes - it should be named to match a "Pre Extended Display List Body"
s/vsp1_dl_ext_cmd_header/vsp1_pre_ext_dl_body/
Sounds good to me.
This will then leave "struct vsp1_dl_ext_cmd" which represents the object instance within the VSP1 driver only.
- u32 cmd;
This should really have been opcode then too :)
Good point.
Renamed
- u32 flags;
- u32 data;
- u32 reserved;
The datasheet documents this as two 64-bit fields, shouldn't we handle the structure the same way ?
I was trying to separate out the fields for clarity.
In this instance (unlike the 16bit handling above), the byte ordering of a 64 bit value works in our favour, and the ordering of the 4 u32s, follows the order of the datasheet.
If you'd prefer to handle them as 64bit with mask and shift, I'll update, and rename this to contain two fields : u64 ext_dl_cmd; u64 ext_dl_data;
But this is working well with the 32 bit definitions.
Up to you, I'm OK with both.
Great, in this instance - and because it works cleanly - I prefer the split, with named field accesses.
I'll add a documenting comment along side it that it is listed as a 64-bit access, but the storage order is the same.
+};
[snip]