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