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

From: Jacopo Mondi <jacopo@jmondi.org>
To: Sakari Ailus <sakari.ailus@linux.intel.com>
Cc: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	"Laurent Pinchart" <laurent.pinchart@ideasonboard.com>,
	"Benoit Parrot" <bparrot@ti.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
	"Michal Simek" <michal.simek@xilinx.com>
Subject: Re: [PATCH v2 16/30] v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations
Date: Fri, 22 Feb 2019 12:17:47 +0100	[thread overview]
Message-ID: <20190222111747.tlj2xdjhnjwrlqxx@uno.localdomain> (raw)
In-Reply-To: <20190222110429.ybmqdwba5rszntb7@paasikivi.fi.intel.com>

[-- Attachment #1: Type: text/plain, Size: 2771 bytes --]

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

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

Initializing in-place would solve both issues :)

> There is a check that requires that one pad is a source and another is a
> sink; I think we could remove that check as well.

Correct, I'll handle that!

Thanks again
   j

>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]