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@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.
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]
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
