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

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

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.

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

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

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

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

> >> +};

[snip]

-- 
Regards,

Laurent Pinchart