Hi Tomi, On Mon, Sep 20, 2021 at 01:19:54PM +0300, Tomi Valkeinen wrote: > On 30/08/2021 14:00, Tomi Valkeinen wrote: > > Hi, > > > > This is v8 of the multiplexed streams series. v7 can be found from: > > > > https://lore.kernel.org/linux-media/20210524104408.599645-1-tomi.valkeinen@xxxxxxxxxxxxxxxx/ > > > > The main change in this version is the implementation and use of > > centralized active state for subdevs. > > > > I have pushed my work branch to: > > > > git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git multistream/work-v8 > > > > which contains the patches in this series, along with subdev drivers > > using multiplexed streams. > > > > Both this series and the branch above are based on top of today's > > git://linuxtv.org/media_tree.git master. > > > > The documentation still needs improving, but I hope the docs in this > > series, and the drivers in the work branch, are enough to give the > > reviewers enough information to do a review. > > > > As can be guessed from the work branch, I have been testing this series > > with TI's FPDLink setup. I have also done a "backwards compatibility" > > test by dropping all multiplexed streams patches from the CAL driver > > (the CSI-2 RX on the TI SoC), and using the FPDLink drivers with > > single-stream configuration. > > We've had good discussions with Jacopo about this series. I hope my recent contribution was also useful to some extent :-) Up to patch 04/36, I like the direction this is taking and I'm quite confident that we'll reach an agreement. We need to get feedback from Sakari too though. > I chose the approaches in this series based on what I think the API > should be, even if the API has behaved differently before. And I think > I'm also leaning forward a bit, in the sense that the full benefit of > the API can only be had after more changes to the core and subdev > drivers (changes which may or may not happen). > > If I understood Jacopo correctly, his comments were essentially that my > approach is different than the current one, and as the current drivers > anyway do things the old way, this is very confusing. Basically I create > two different kinds of subdev drivers: the old and new ones, which > manage state differently. > > I want to summarize two particular topics: > > 1) Active state & subdev ops > > In upstream we have v4l2_subdev_state which contains only the pad_config > array. This state is "try" state, it's allocated per file-handle, and > passed to the subdev drivers when executing subdev ioctls in try-mode > (which == V4L2_SUBDEV_FORMAT_TRY). This try-state is sometimes also > passed to the subdev drivers when executing in active-mode > (V4L2_SUBDEV_FORMAT_ACTIVE), but the drivers are supposed to ignore it. > > There is also an active-state, but it's driver-specific and > driver-internal. The drivers check the 'which' value, and either use the > passed try-state, or the internal state. To be very clear here, let's note that the driver-internal state is stored in a driver-specific format, which does not reuse the state structure used for the TRY state. > What I did in this series aims to have both try- and active-states in > v4l2 core, and passing the correct state to subdevs so that they don't > (necessarily) need any internal state. There are some issues with it, > which have been discussed, but I believe those issues can be fixed. > > The subdev drivers need to be written to use this new active-state, so > it doesn't affect the current drivers. > > The question is, do we want to go that way? __ __ _______ ________ \ \ / / | _____| | ______| \ \ / / | | | | \ v / | |_____ | |______ | | | _____| |______ | | | | | | | | | | |_____ ______| | |_| |_______| |________| (please let me know if you require additional clarification) > We could as well keep the > current behavior of subdev drivers only getting the try-state as a > parameter, and the drivers digging out the active state manually. This > active state could either be internal to the driver, or it could be in > the base struct v4l2_subdev (see also topic 2). > > 2) Shared subdev active-state > > The try-state is specific to a file-handle, and afaics have no real > race-issues as it's not really shared. Although I guess in theory an > application could call subdev ioctls from multiple threads using the > same fd. That's right. We could possibly serialize ioctl calls in v4l2-subdev.c. > In upstream the subdev drivers' internal state is managed fully by the > subdev drivers. The drivers are expected to handle necessary locking in > their subdev ops and interrupt handlers. If, say, v4l2 core needs to get > a format from the subdev, it calls a subdev op to get it. "supposed to" is the correct term here. Most of them don't (including drivers I have written myself), which I believe shows quite clearly that the API is wrong and that this shouldn't be left to drivers to handle. > In my series I aimed to a shared active-state. The state is located in a > known place, struct v4l2_subdev, and can be accessed without the subdev > driver's help. This requires locking, which I have implemented. > > At the moment the only real benefit with this is reading the routing > table while doing pipeline validation: Instead of having to dynamically > allocate memory and call the subdev op to create a copy of the routing > table (for each subdev, possibly multiple times), the validator can just > lock the state, and use it. And, in fact, there is no get_routing subdev > op at all. > > But this means that the subdev drivers that support this new > active-state have to handle locking for the active state, and the > "mindset" is different than previously. That's the right mindset I believe, and forcing drivers to use helper functions that ensure proper locking is the right way to go in my opinion. > So the question is, do we want to go that way? We could as well mandate > that the active-state can only be accessed via subdev's ops (and add the > get-routing, of course), and the subdev manages the locking internally. Been there, failed, let's not repeat the same mistake. -- Regards, Laurent Pinchart
