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