Re: [PATCH v6 00/24] v4l: subdev internal routing

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

On 27/04/2021 15:44, Tomi Valkeinen wrote:
> Hi,
> 
> This is v6 of the series. Previous one can be found from:
> 
> https://lore.kernel.org/linux-media/20210415130450.421168-1-tomi.valkeinen@ideasonboard.com/
> 
> Changes to v5:
> - Dropped "v4l: mc: Add an S_ROUTING helper function for power state changes" as discussed
> - Added MEDIA_PAD_FL_MULTIPLEXED flag to indicate that a pad works in multiplexed mode
> - Documentation improvements
> - Renamed struct v4l2_mbus_frame_desc_entry_csi2 fields
> - Fixed setting routing->num_routes in G_ROUTING
> - Many smaller cosmetic changes as per comments
> 
> One bigger topic I didn't change is the in-kernel API for get/set routing.
> Currently the link validation needs to get the routing tables multiple times,
> each leading to memory allocations (and frees). A different API would allow the
> link validation to peek at the routing without any allocations, but I haven't
> solved that yet.
> 
> A simple solution would be to require a lock to be held when accessing the
> routing table, and making get_routing return a pointer to the driver's routing
> table. But this kind of API would be quite different than, say, get_fmt.
> 
> Another would be to keep the API, but have a state object during link
> validation, which would hold preallocated routing tables, so that link
> validation wouldn't need to allocate new ones for each get_routing call.
> 
> Anything we do here is a kernel internal change, so it's not super critical to
> solve it right away.

I decided to test the other approach: changing subdev format ioctls to 
work on a pad+stream tuple, instead of just on a pad. So essentially 
adding a 'stream' field (taking one u32 from the reserved area) to 
v4l2_subdev_format, v4l2_subdev_mbus_code_enum, and others. The current 
userspace sets the field to 0, to it makes sense even if with older 
userspace.

My branch is still quite messy, but this approach does give us some nice 
benefits:

- I was able to drop the whole "follow-the-stream-to-get-format" code. 
Link validation feels simpler.

- A sensor that provides pixel and metadata can be implemented in a 
single subdev, with a single source pad.

- It makes the configuration of subdevs self-contained.

- A subdev can do operations (like, say, scaling) with a stream, instead 
of having to split the subdev into multiple subdevs to get the 
non-multiplexed pads.

- I don't need the cal_streams_api parameter (introduced in another 
series for TI CAL driver), nor do I need the MEDIA_PAD_FL_MULTIPLEXED flag

- I can easily support a case where the subdev driver on one side of the 
link supports multiplexed streams and the other driver doesn't (as long 
as the multiplexed streams subdev's pad is configured for a single 
stream). Well, this would probably be possible with the previous 
approach also, but at last here it came kind of naturally.

Downsides:

- Need dynamic allocation for storing formats. It doesn't feel too bad, 
though, and it, of course, only affects the drivers that support routing 
and multiplexed streams. I do the reallocation in set_routing.

- TRY doesn't work, as there's no place to store the formats for 
streams. This needs the proposed change to pass subdev state, instead of 
just subdev pad state, to the various format ops.

- Possibly plenty of tooling changes to add support for streams

And I should perhaps add a subdev flag to indicate that a subdev 
supports multiple streams. This could at least be used in the ioctl 
handlers, rejecting stream field > 0 if the subdev does not support streams.

I'll continue working on this a bit to see how it goes, but I wanted to 
share this now to hear if there are any clear NACKs for the approach.

  Tomi