| Message ID | 20210427124523.990938-1-tomi.valkeinen@ideasonboard.com |
|---|---|
| Headers | show |
| Series | v4l: subdev internal routing | expand |
Hi Tomi, Thank you for the patch. On Tue, Apr 27, 2021 at 03:45:11PM +0300, Tomi Valkeinen wrote: > Add a helper macro for iterating over pads that are connected through > enabled routes. This can be used to find all the connected pads within an > entity, for instance starting from the pad which has been obtained during > the graph walk. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > - Make __media_entity_next_routed_pad() return NULL and adjust the > iterator to handle that > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/media/media-entity.h | 28 ++++++++++++++++++++++++++++ > 1 file changed, 28 insertions(+) > > diff --git a/include/media/media-entity.h b/include/media/media-entity.h > index f8fa952fa38e..42193d6c58e9 100644 > --- a/include/media/media-entity.h > +++ b/include/media/media-entity.h > @@ -916,6 +916,34 @@ __must_check int media_graph_walk_init( > bool media_entity_has_route(struct media_entity *entity, unsigned int pad0, > unsigned int pad1); > > +static inline struct media_pad *__media_entity_next_routed_pad( > + struct media_pad *start, struct media_pad *iter) > +{ > + struct media_entity *entity = start->entity; > + > + for (; iter < &entity->pads[entity->num_pads]; iter++) { > + if (media_entity_has_route(entity, start->index, iter->index)) > + return iter; > + } > + > + return NULL; > +} > + > +/** > + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes > + * > + * @start: The starting pad > + * @iter: The iterator pad > + * > + * Iterate over all pads connected through routes from the @start pad > + * within an entity. The iteration will include the @start pad itself. > + */ I still think a better name would be, well, better :-) Let's continue the discussion in the v5 thread to avoid splitting it. > +#define media_entity_for_each_routed_pad(start, iter) \ > + for (iter = __media_entity_next_routed_pad( \ > + start, (start)->entity->pads); \ > + iter != NULL; \ > + iter = __media_entity_next_routed_pad(start, iter + 1)) > + > /** > * media_graph_walk_cleanup - Release resources used by graph walk. > * -- Regards, Laurent Pinchart
Hi Tomi, Thank you for the patch. Authorship lost here too it seems. On Tue, Apr 27, 2021 at 03:45:14PM +0300, Tomi Valkeinen wrote: > Add the media bus type to the frame descriptor. CSI-2 specific > information will be added in next patch to the frame descriptor. > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> > > - Make the bus type a named enum > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> > --- > include/media/v4l2-subdev.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h > index d0e9a5bdb08b..ac531b752028 100644 > --- a/include/media/v4l2-subdev.h > +++ b/include/media/v4l2-subdev.h > @@ -340,12 +340,31 @@ struct v4l2_mbus_frame_desc_entry { > > #define V4L2_FRAME_DESC_ENTRY_MAX 4 > > +/** > + * enum v4l2_mbus_frame_desc_type - media bus frame description type > + * > + * @V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM: > + * Platform specific frame desc type for backwards compatibility. I'd have named it UNKNOWN, or possibly LEGACY or something similar, to convey the fact that it shouldn't be used, but is only there so that drivers that don't set the field can be told apart. Maybe the documentation could capture this more clearly. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + * @V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL: > + * Parallel media bus. > + * @V4L2_MBUS_FRAME_DESC_TYPE_CSI2: > + * CSI-2 media bus. Frame desc parameters must be set in > + * &struct v4l2_mbus_frame_desc_entry->csi2. > + */ > +enum v4l2_mbus_frame_desc_type { > + V4L2_MBUS_FRAME_DESC_TYPE_PLATFORM, > + V4L2_MBUS_FRAME_DESC_TYPE_PARALLEL, > + V4L2_MBUS_FRAME_DESC_TYPE_CSI2, > +}; > + > /** > * struct v4l2_mbus_frame_desc - media bus data frame description > + * @type: type of the bus (enum v4l2_mbus_frame_desc_type) > * @entry: frame descriptors array > * @num_entries: number of entries in @entry array > */ > struct v4l2_mbus_frame_desc { > + enum v4l2_mbus_frame_desc_type type; > struct v4l2_mbus_frame_desc_entry entry[V4L2_FRAME_DESC_ENTRY_MAX]; > unsigned short num_entries; > }; -- Regards, Laurent Pinchart
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
On 29/04/2021 05:18, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Tue, Apr 27, 2021 at 03:45:11PM +0300, Tomi Valkeinen wrote: >> Add a helper macro for iterating over pads that are connected through >> enabled routes. This can be used to find all the connected pads within an >> entity, for instance starting from the pad which has been obtained during >> the graph walk. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> >> Reviewed-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> >> >> - Make __media_entity_next_routed_pad() return NULL and adjust the >> iterator to handle that >> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> >> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> >> --- >> include/media/media-entity.h | 28 ++++++++++++++++++++++++++++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/include/media/media-entity.h b/include/media/media-entity.h >> index f8fa952fa38e..42193d6c58e9 100644 >> --- a/include/media/media-entity.h >> +++ b/include/media/media-entity.h >> @@ -916,6 +916,34 @@ __must_check int media_graph_walk_init( >> bool media_entity_has_route(struct media_entity *entity, unsigned int pad0, >> unsigned int pad1); >> >> +static inline struct media_pad *__media_entity_next_routed_pad( >> + struct media_pad *start, struct media_pad *iter) >> +{ >> + struct media_entity *entity = start->entity; >> + >> + for (; iter < &entity->pads[entity->num_pads]; iter++) { >> + if (media_entity_has_route(entity, start->index, iter->index)) >> + return iter; >> + } >> + >> + return NULL; >> +} >> + >> +/** >> + * media_entity_for_each_routed_pad - Iterate over entity pads connected by routes >> + * >> + * @start: The starting pad >> + * @iter: The iterator pad >> + * >> + * Iterate over all pads connected through routes from the @start pad >> + * within an entity. The iteration will include the @start pad itself. >> + */ > > I still think a better name would be, well, better :-) Let's continue > the discussion in the v5 thread to avoid splitting it. I removed the inlining of __media_entity_next_routed_pad and renamed 'start' to 'root'. If anyone has a better name idea, let's hear it =). But I think this at least removes the confusion of the parameter somehow defining the start pad. Tomi >> +#define media_entity_for_each_routed_pad(start, iter) \ >> + for (iter = __media_entity_next_routed_pad( \ >> + start, (start)->entity->pads); \ >> + iter != NULL; \ >> + iter = __media_entity_next_routed_pad(start, iter + 1)) >> + >> /** >> * media_graph_walk_cleanup - Release resources used by graph walk. >> * >
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. Tomi Jacopo Mondi (2): media: entity: Add iterator helper for entity pads media: Documentation: Add GS_ROUTING documentation Laurent Pinchart (3): media: entity: Add has_route entity operation media: entity: Add media_entity_has_route() function media: entity: Use routing information during graph traversal Sakari Ailus (9): media: entity: Use pad as a starting point for graph walk media: entity: Use pads instead of entities in the media graph walk stack v4l: 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 to media: entity: Add only connected pads to the pipeline media: entity: Add debug information in graph walk route check v4l: Add stream to frame descriptor Tomi Valkeinen (10): media: entity: Walk the graph based on pads media: entity: Add an iterator helper for connected pads v4l: Add bus type to frame descriptors v4l: Add CSI-2 bus configuration to frame descriptors v4l: subdev: Add [GS]_ROUTING subdev ioctls and operations v4l: subdev: routing kernel helper functions v4l: subdev: add v4l2_subdev_get_format_dir() media: uapi: add MEDIA_PAD_FL_MULTIPLEXED flag v4l: subdev: Take routing information into account in link validation v4l: subdev: increase V4L2_FRAME_DESC_ENTRY_MAX to 8 Documentation/driver-api/media/mc-core.rst | 15 +- .../media/mediactl/media-types.rst | 6 + .../userspace-api/media/v4l/dev-subdev.rst | 125 ++++++ .../userspace-api/media/v4l/user-func.rst | 1 + .../media/v4l/vidioc-subdev-g-routing.rst | 138 +++++++ drivers/media/mc/mc-device.c | 13 +- drivers/media/mc/mc-entity.c | 241 ++++++----- 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 +- .../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_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 | 376 +++++++++++++++++- 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 | 38 +- drivers/staging/media/omap4iss/iss_video.h | 2 +- drivers/staging/media/tegra-video/tegra210.c | 6 +- include/media/media-entity.h | 144 +++++-- include/media/v4l2-subdev.h | 107 ++++- include/uapi/linux/media.h | 1 + include/uapi/linux/v4l2-subdev.h | 48 +++ 43 files changed, 1246 insertions(+), 302 deletions(-) create mode 100644 Documentation/userspace-api/media/v4l/vidioc-subdev-g-routing.rst