Re: [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations

[Ian Arkver <ian.arkver.dev@gmail.com>]

Hi,

On 22/02/2019 11:29, Sakari Ailus wrote:
> Hi Jacopo,
> 
> On Fri, Feb 22, 2019 at 12:17:47PM +0100, Jacopo Mondi wrote:
>> Hi Sakari,
>>      thanks for your suggestions.
>>
>> On Fri, Feb 22, 2019 at 01:04:29PM +0200, Sakari Ailus wrote:
>>> Hi Jacopo,
>>
>> [snip]
>>
>>>> On the previous example, I thought about GMSL-like devices, that can
>>>> output the video streams received from different remotes in a
>>>> separate virtual channel, at the same time.
>>>>
>>>> A possible routing table in that case would be like:
>>>>
>>>> Pads 0, 1, 2, 3 = SINKS
>>>> Pad 4 = SOURCE with 4 streams (1 for each VC)
>>>>
>>>> 0/0 -> 4/0
>>>> 0/0 -> 4/1
>>>> 0/0 -> 4/2
>>>> 0/0 -> 4/3
>>>> 1/0 -> 4/0
>>>> 1/0 -> 4/1
>>>> 1/0 -> 4/2
>>>> 1/0 -> 4/3
>>>> 2/0 -> 4/0
>>>> 2/0 -> 4/1
>>>> 2/0 -> 4/2
>>>> 2/0 -> 4/3
>>>> 3/0 -> 4/0
>>>> 3/0 -> 4/1
>>>> 3/0 -> 4/2
>>>> 3/0 -> 4/3
>>>
>>> If more than one pad can handle multiplexed streams, then you may end up in
>>> a situation like that. Indeed.
>>>
>>
>> Please note that in this case there is only one pad that can handle
>> multiplexed stream. The size of the routing table is the
>> multiplication of the total number of pads by the product of all
>> streams per pad. In this case (4 * (1 * 1 * 1 * 4))
> 
> Oh, good point, that's the case for G_ROUTING. I thought of S_ROUTING only.
> :-)
> 
>>
>>>>
>>>> With only one route per virtual channel active at a time.
>>
>> [snip]
>>
>>>>
>>>> Thanks, I had a look at the MEDIA_ ioctls yesterday, G_TOPOLOGY in
>>>> particular, which uses several pointers to arrays.
>>>>
>>>> Unfortunately, I didn't come up with anything better than using a
>>>> translation structure, from the IOCTL layer to the subdevice
>>>> operations layer:
>>>> https://paste.debian.net/hidden/b192969d/
>>>> (sharing a link for early comments, I can send v3 and you can comment
>>>> there directly if you prefer to :)
>>>
>>> Hmm. That is a downside indeed. It's still a lesser problem than the compat
>>> code in general --- which has been a source for bugs as well as nasty
>>> security problems over time.
>>>
>>
>> Good!
>>
>>> I think we need a naming scheme for such structs. How about just
>>> calling that struct e.g. v4l2_subdev_krouting instead? It's simple, easy to
>>> understand and it includes a suggestion which one is the kernel-only
>>> variant.
>>>
>>
>> I kind of like that! thanks!
>>
>>> You can btw. zero the struct memory by assigning { 0 } to it in
>>> declaration. memset() in general is much more trouble. In this case you
>>> could even do the assignments in delaration as well.
>>>
>>
>> Thanks, noted. I have been lazy and copied memset from other places in
>> the ioctl handling code. I should check on your suggestions because I
>> remember one of the many 0-initialization statement was a GCC specific one,
>> don't remember which...
> 
> {} is GCC specific whereas { 0 } is not. But there have been long-standing
> GCC bugs related to the use of { 0 } which is quite unfortunate --- they've
> produced warnings or errors from code that is valid C...
> 

Using = {} to intialise structs is all over the kernel and quite 
accepted. Eg. recent discussion with Mauro at [1].

[1] https://lore.kernel.org/linux-media/20181207105816.4c53aeaa@coco.lan/

Regards,
Ian