| Message ID | 20210830110116.488338-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | v4l: subdev internal routing and streams | expand |
Hi all, 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@ideasonboard.com/ > > 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 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. 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? 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. 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. 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. 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. Tomi
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@ideasonboard.com/ > > > > 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
Hi Tomi, Laurent On Mon, Sep 27, 2021 at 04:24:53AM +0300, Laurent Pinchart wrote: > 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@ideasonboard.com/ > > > > > > 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. > Thanks Tomi for summing it up here > 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. > > Most of my comments (I guess all except the addition of which to state and a few other things) are about the quantity of helpers that have been introduced. Let me explain this: I understand the benefits of relying on helpers to reduce the amount of work the drivers have to do and enforce correctness, so I won't argue about the fact they're helpful, but whenever I see something like v4l2_subdev_validate_state(state) { return state ? state : sd->state; } instead of having a driver doing void v4l2_subdev_op(sd, try_state, ...) { subdev_state *state; if (which == TRY) state = try_state; else state = sd->state; } My immediate concern is the levels of obfuscation we introduce which makes approaching the code a lot more difficult, at least for me, and everytime I happen to jump into an helper which does something trivial I'm left with the question "why an helper ? why a function name I have to remember or jump into from my editor when it's so simple to open code ?" So yes, I guess you can call them tastes, I understand the benfit and there seems to be a large consensus, so I'll just have to get to like these extra levels of indirection. (Talking with Tomi I said him "seems like reading DRM/KMS code, full of helpers which do a single thing I could have open coded". I guess it's a compliment). > > 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. > That's the current situation. I guess in the long term we should aim to have as much as possible of the subdev state moved to the 'state' in the subdev. > > 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. > > That's very nice, and I understand an helper here is useful, as locking could be very well overlooked if subdev drivers are encouraged to access their active state by just sd->state. > > 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. > Point taken. I'll learn how to love more those additional helpers. Thanks j > > 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
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@ideasonboard.com/ 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. Tomi Jacopo Mondi (2): media: entity: Add iterator helper for entity pads media: Documentation: Add GS_ROUTING documentation Laurent Pinchart (4): media: entity: Add has_route entity operation media: entity: Add media_entity_has_route() function media: entity: Use routing information during graph traversal media: subdev: Add [GS]_ROUTING subdev ioctls and operations Sakari Ailus (13): media: entity: Use pad as a starting point for graph walk media: entity: Use pads instead of entities in the media graph walk stack media: entity: Walk the graph based on pads media: mc: Start walk from a specific pad in use count calculation media: entity: Move the pipeline from entity to pads media: entity: Use pad as the starting point for a pipeline media: entity: Skip link validation for pads to which there is no route media: entity: Add an iterator helper for connected pads media: entity: Add only connected pads to the pipeline media: entity: Add debug information in graph walk route check media: Add bus type to frame descriptors media: Add CSI-2 bus configuration to frame descriptors media: Add stream to frame descriptor Tomi Valkeinen (17): media: subdev: rename subdev-state alloc & free media: subdev: add active state to struct v4l2_subdev media: subdev: add 'which' to subdev state media: subdev: pass also the active state to subdevs from ioctls media: subdev: add subdev state locking media: subdev: Add v4l2_subdev_validate(_and_lock)_state() media: Documentation: add documentation about subdev state media: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 media: add V4L2_SUBDEV_FL_MULTIPLEXED media: subdev: add v4l2_subdev_has_route() media: subdev: add v4l2_subdev_set_routing helper() media: subdev: add stream based configuration media: subdev: use streams in v4l2_subdev_link_validate() media: subdev: add "opposite" stream helper funcs media: subdev: add v4l2_subdev_get_fmt() helper function media: subdev: add v4l2_subdev_set_routing_with_fmt() helper media: subdev: add v4l2_routing_simple_verify() helper Documentation/driver-api/media/mc-core.rst | 18 +- .../driver-api/media/v4l2-subdev.rst | 25 + .../userspace-api/media/v4l/dev-subdev.rst | 2 + .../userspace-api/media/v4l/user-func.rst | 1 + .../v4l/vidioc-subdev-enum-frame-interval.rst | 5 +- .../v4l/vidioc-subdev-enum-frame-size.rst | 5 +- .../v4l/vidioc-subdev-enum-mbus-code.rst | 5 +- .../media/v4l/vidioc-subdev-g-crop.rst | 5 +- .../media/v4l/vidioc-subdev-g-fmt.rst | 5 +- .../v4l/vidioc-subdev-g-frame-interval.rst | 5 +- .../media/v4l/vidioc-subdev-g-routing.rst | 146 ++++ .../media/v4l/vidioc-subdev-g-selection.rst | 5 +- drivers/media/mc/mc-device.c | 13 +- drivers/media/mc/mc-entity.c | 257 +++--- drivers/media/pci/intel/ipu3/ipu3-cio2-main.c | 6 +- .../media/platform/exynos4-is/fimc-capture.c | 8 +- .../platform/exynos4-is/fimc-isp-video.c | 8 +- drivers/media/platform/exynos4-is/fimc-isp.c | 2 +- drivers/media/platform/exynos4-is/fimc-lite.c | 10 +- drivers/media/platform/exynos4-is/media-dev.c | 20 +- drivers/media/platform/omap3isp/isp.c | 2 +- drivers/media/platform/omap3isp/ispvideo.c | 25 +- drivers/media/platform/omap3isp/ispvideo.h | 2 +- .../media/platform/qcom/camss/camss-video.c | 6 +- drivers/media/platform/rcar-vin/rcar-core.c | 16 +- drivers/media/platform/rcar-vin/rcar-dma.c | 8 +- drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +- .../platform/rockchip/rkisp1/rkisp1-capture.c | 6 +- .../media/platform/s3c-camif/camif-capture.c | 6 +- drivers/media/platform/stm32/stm32-dcmi.c | 6 +- .../platform/sunxi/sun4i-csi/sun4i_dma.c | 6 +- .../platform/sunxi/sun6i-csi/sun6i_video.c | 6 +- drivers/media/platform/ti-vpe/cal-video.c | 6 +- drivers/media/platform/vsp1/vsp1_entity.c | 4 +- drivers/media/platform/vsp1/vsp1_video.c | 18 +- drivers/media/platform/xilinx/xilinx-dma.c | 20 +- drivers/media/platform/xilinx/xilinx-dma.h | 2 +- .../media/test-drivers/vimc/vimc-capture.c | 6 +- drivers/media/usb/au0828/au0828-core.c | 8 +- drivers/media/v4l2-core/v4l2-ioctl.c | 25 +- drivers/media/v4l2-core/v4l2-mc.c | 43 +- drivers/media/v4l2-core/v4l2-subdev.c | 735 +++++++++++++++++- drivers/staging/media/imx/imx-media-utils.c | 8 +- drivers/staging/media/ipu3/ipu3-v4l2.c | 6 +- drivers/staging/media/omap4iss/iss.c | 2 +- drivers/staging/media/omap4iss/iss_video.c | 40 +- drivers/staging/media/omap4iss/iss_video.h | 2 +- drivers/staging/media/tegra-video/tegra210.c | 6 +- drivers/staging/media/tegra-video/vi.c | 4 +- include/media/media-entity.h | 142 +++- include/media/v4l2-subdev.h | 371 ++++++++- include/uapi/linux/v4l2-subdev.h | 85 +- 52 files changed, 1808 insertions(+), 369 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst