* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-19 8:15 ` [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Sakari Ailus
@ 2025-06-19 11:42 ` kernel test robot
2025-06-20 3:58 ` Dan Carpenter
` (3 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2025-06-19 11:42 UTC (permalink / raw)
To: Sakari Ailus, linux-media
Cc: llvm, oe-kbuild-all, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen, laurent.pinchart
Hi Sakari,
kernel test robot noticed the following build warnings:
[auto build test WARNING on linuxtv-media-pending/master]
[also build test WARNING on linus/master media-tree/master v6.16-rc2 next-20250618]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/media-ipu6-Use-correct-pads-for-xlate_streams/20250619-161847
base: https://git.linuxtv.org/media-ci/media-pending.git master
patch link: https://lore.kernel.org/r/20250619081546.1582969-13-sakari.ailus%40linux.intel.com
patch subject: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
config: x86_64-buildonly-randconfig-003-20250619 (https://download.01.org/0day-ci/archive/20250619/202506191934.t9FEXlhl-lkp@intel.com/config)
compiler: clang version 20.1.2 (https://github.com/llvm/llvm-project 58df0ef89dd64126512e4ee27b4ac3fd8ddf6247)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250619/202506191934.t9FEXlhl-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506191934.t9FEXlhl-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/media/v4l2-core/v4l2-mc.c:622:6: warning: variable 'src_streams' set but not used [-Wunused-but-set-variable]
622 | u64 src_streams = 0, sink_streams = 0;
| ^
>> drivers/media/v4l2-core/v4l2-mc.c:732:44: warning: variable 'source_stream' is uninitialized when used here [-Wuninitialized]
732 | return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
| ^~~~~~~~~~~~~
drivers/media/v4l2-core/v4l2-mc.c:671:19: note: initialize the variable 'source_stream' to silence this warning
671 | u32 source_stream;
| ^
| = 0
2 warnings generated.
vim +/src_streams +622 drivers/media/v4l2-core/v4l2-mc.c
615
616 static int
617 __v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
618 struct media_pad *src_pad, u64 __src_streams,
619 struct media_pad **__sink_pad, u64 *__sink_streams)
620 {
621 struct v4l2_subdev_route *route;
> 622 u64 src_streams = 0, sink_streams = 0;
623 bool has_sink_pad = false;
624 unsigned int sink_pad;
625
626 dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
627 state->sd->entity.name, src_pad->index, __src_streams);
628 for_each_active_route(&state->routing, route) {
629 dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
630 state->sd->entity.name,
631 route->sink_pad, route->sink_stream, route->source_pad,
632 route->source_stream, route->flags);
633 if (route->source_pad != src_pad->index)
634 continue;
635
636 if (!(BIT_ULL(route->source_stream) & __src_streams))
637 continue;
638
639 if (!has_sink_pad) {
640 has_sink_pad = true;
641 sink_pad = route->sink_pad;
642 }
643
644 if (route->sink_pad != sink_pad) {
645 dev_dbg(state->sd->dev,
646 "sink pads (%u vs. %u) differ\n",
647 route->sink_pad, sink_pad);
648 return -EMLINK;
649 }
650
651 sink_streams |= BIT_ULL(route->sink_stream);
652 src_streams |= BIT_ULL(route->source_stream);
653 }
654
655 *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
656 *__sink_streams = sink_streams;
657
658 return 0;
659 }
660
661 static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
662 unsigned int sink_stream,
663 bool (*func)(struct video_device *vdev),
664 struct media_pad **__sink_pad,
665 u64 *__sink_streams)
666 {
667 struct v4l2_subdev_state *state;
668 struct v4l2_subdev_route *route;
669 struct v4l2_subdev *sd;
670 struct media_pad *source_pad, *tmp_pad;
671 u32 source_stream;
672
673 if (!is_media_entity_v4l2_subdev(sink_pad->entity))
674 return -ENXIO;
675
676 sd = media_entity_to_v4l2_subdev(sink_pad->entity);
677 dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
678 sd->entity.name);
679
680 state = v4l2_subdev_lock_and_get_active_state(sd);
681 route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
682 sink_stream, true, 0);
683 if (IS_ERR(route)) {
684 v4l2_subdev_unlock_state(state);
685 dev_dbg(sd->dev,
686 "path_enabled: can't find opposite route for %s:%u/%u",
687 sd->entity.name, sink_pad->index, sink_stream);
688 return 2;
689 }
690
691 source_pad = &sd->entity.pads[route->source_pad];
692 v4l2_subdev_unlock_state(state);
693
694 tmp_pad = sink_pad;
695 sink_pad = media_pad_remote_pad_unique(source_pad);
696 if (IS_ERR(sink_pad)) {
697 dev_dbg(sd->dev,
698 "path_enabled: can't find remote source for %s:%u\n",
699 source_pad->entity->name, source_pad->index);
700 return PTR_ERR(sink_pad);
701 }
702
703 if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
704 struct video_device *vdev;
705
706 vdev = media_entity_to_video_device(sink_pad->entity);
707 if (!vdev)
708 return -ENXIO;
709
710 dev_dbg(vdev->dev_parent,
711 "path_enabled: found video device %s\n",
712 vdev->name);
713
714 if (!*__sink_pad) {
715 *__sink_pad = tmp_pad;
716 dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
717 tmp_pad->index, sink_stream);
718 } else if (tmp_pad != *__sink_pad) {
719 dev_dbg(sd->dev,
720 "path_enabled: pads %s/%u and %s/%u differ\n",
721 tmp_pad->entity->name, tmp_pad->index,
722 (*__sink_pad)->entity->name,
723 (*__sink_pad)->index);
724 return -EXDEV;
725 }
726
727 *__sink_streams |= BIT_ULL(sink_stream);
728
729 return func(vdev);
730 }
731
> 732 return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
733 __sink_pad, __sink_streams);
734 }
735
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-19 8:15 ` [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Sakari Ailus
2025-06-19 11:42 ` kernel test robot
@ 2025-06-20 3:58 ` Dan Carpenter
2025-06-20 8:53 ` Jacopo Mondi
` (2 subsequent siblings)
4 siblings, 0 replies; 14+ messages in thread
From: Dan Carpenter @ 2025-06-20 3:58 UTC (permalink / raw)
To: oe-kbuild, Sakari Ailus, linux-media
Cc: lkp, oe-kbuild-all, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen, laurent.pinchart
Hi Sakari,
kernel test robot noticed the following build warnings:
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/media-ipu6-Use-correct-pads-for-xlate_streams/20250619-161847
base: https://git.linuxtv.org/media-ci/media-pending.git master
patch link: https://lore.kernel.org/r/20250619081546.1582969-13-sakari.ailus%40linux.intel.com
patch subject: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
config: i386-randconfig-141-20250620 (https://download.01.org/0day-ci/archive/20250620/202506201121.oifnpp7r-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202506201121.oifnpp7r-lkp@intel.com/
smatch warnings:
drivers/media/v4l2-core/v4l2-mc.c:732 v4l2_mc_downpath_enabled() error: uninitialized symbol 'source_stream'.
drivers/media/v4l2-core/v4l2-mc.c:834 v4l2_mc_pipeline_enabled() warn: variable dereferenced before IS_ERR check 'src_pad' (see line 802)
vim +/source_stream +732 drivers/media/v4l2-core/v4l2-mc.c
bc83f513f8ad94 Sakari Ailus 2025-06-19 771 int v4l2_mc_pipeline_enabled(struct video_device *vdev,
bc83f513f8ad94 Sakari Ailus 2025-06-19 772 bool (*func)(struct video_device *vdev),
bc83f513f8ad94 Sakari Ailus 2025-06-19 773 struct media_pad **__sink_pad, u64 *__sink_streams)
bc83f513f8ad94 Sakari Ailus 2025-06-19 774 {
bc83f513f8ad94 Sakari Ailus 2025-06-19 775 u64 sink_streams = 1U;
bc83f513f8ad94 Sakari Ailus 2025-06-19 776 struct media_pad *src_pad;
bc83f513f8ad94 Sakari Ailus 2025-06-19 777 u64 src_streams;
bc83f513f8ad94 Sakari Ailus 2025-06-19 778 struct v4l2_subdev_state *state;
bc83f513f8ad94 Sakari Ailus 2025-06-19 779 struct media_pad *sink_pad = vdev->entity.pads;
bc83f513f8ad94 Sakari Ailus 2025-06-19 780 struct v4l2_subdev *sd = NULL;
bc83f513f8ad94 Sakari Ailus 2025-06-19 781 bool streaming = true;
bc83f513f8ad94 Sakari Ailus 2025-06-19 782 struct media_pad *tmp_pad;
bc83f513f8ad94 Sakari Ailus 2025-06-19 783 u64 tmp_streams;
bc83f513f8ad94 Sakari Ailus 2025-06-19 784 int ret;
bc83f513f8ad94 Sakari Ailus 2025-06-19 785
bc83f513f8ad94 Sakari Ailus 2025-06-19 786 if (!__sink_pad)
bc83f513f8ad94 Sakari Ailus 2025-06-19 787 __sink_pad = &tmp_pad;
bc83f513f8ad94 Sakari Ailus 2025-06-19 788 if (!__sink_streams)
bc83f513f8ad94 Sakari Ailus 2025-06-19 789 __sink_streams = &tmp_streams;
bc83f513f8ad94 Sakari Ailus 2025-06-19 790 *__sink_pad = NULL;
bc83f513f8ad94 Sakari Ailus 2025-06-19 791 *__sink_streams = 0;
bc83f513f8ad94 Sakari Ailus 2025-06-19 792
bc83f513f8ad94 Sakari Ailus 2025-06-19 793 do {
bc83f513f8ad94 Sakari Ailus 2025-06-19 794 src_pad = media_pad_remote_pad_unique(sink_pad);
bc83f513f8ad94 Sakari Ailus 2025-06-19 795 if (IS_ERR(src_pad)) {
bc83f513f8ad94 Sakari Ailus 2025-06-19 796 dev_dbg(sd ? sd->dev : vdev->dev_parent,
bc83f513f8ad94 Sakari Ailus 2025-06-19 797 "no unique remote pad found from %s:%u\n",
bc83f513f8ad94 Sakari Ailus 2025-06-19 798 sink_pad->entity->name, sink_pad->index);
bc83f513f8ad94 Sakari Ailus 2025-06-19 799 return PTR_ERR(src_pad);
bc83f513f8ad94 Sakari Ailus 2025-06-19 800 }
bc83f513f8ad94 Sakari Ailus 2025-06-19 801
bc83f513f8ad94 Sakari Ailus 2025-06-19 @802 sd = media_entity_to_v4l2_subdev(src_pad->entity);
bc83f513f8ad94 Sakari Ailus 2025-06-19 803 if (!sd) {
bc83f513f8ad94 Sakari Ailus 2025-06-19 804 dev_dbg(sd->dev,
bc83f513f8ad94 Sakari Ailus 2025-06-19 805 "media entity %s is not a V4L2 sub-device\n",
bc83f513f8ad94 Sakari Ailus 2025-06-19 806 src_pad->entity->name);
bc83f513f8ad94 Sakari Ailus 2025-06-19 807 return -ENXIO;
bc83f513f8ad94 Sakari Ailus 2025-06-19 808 }
bc83f513f8ad94 Sakari Ailus 2025-06-19 809
bc83f513f8ad94 Sakari Ailus 2025-06-19 810 /* Source streams match sink. */
bc83f513f8ad94 Sakari Ailus 2025-06-19 811 src_streams = sink_streams;
bc83f513f8ad94 Sakari Ailus 2025-06-19 812
bc83f513f8ad94 Sakari Ailus 2025-06-19 813 state = v4l2_subdev_lock_and_get_active_state(sd);
bc83f513f8ad94 Sakari Ailus 2025-06-19 814 ret = __v4l2_mc_pipeline_enabled(state, src_pad,
bc83f513f8ad94 Sakari Ailus 2025-06-19 815 src_streams, &sink_pad,
bc83f513f8ad94 Sakari Ailus 2025-06-19 816 &sink_streams);
bc83f513f8ad94 Sakari Ailus 2025-06-19 817 v4l2_subdev_unlock_state(state);
bc83f513f8ad94 Sakari Ailus 2025-06-19 818 if (ret)
bc83f513f8ad94 Sakari Ailus 2025-06-19 819 return ret;
bc83f513f8ad94 Sakari Ailus 2025-06-19 820 } while (sink_pad);
bc83f513f8ad94 Sakari Ailus 2025-06-19 821
bc83f513f8ad94 Sakari Ailus 2025-06-19 822 ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
bc83f513f8ad94 Sakari Ailus 2025-06-19 823 if (ret)
bc83f513f8ad94 Sakari Ailus 2025-06-19 824 return ret;
bc83f513f8ad94 Sakari Ailus 2025-06-19 825
bc83f513f8ad94 Sakari Ailus 2025-06-19 826 sd = media_entity_to_v4l2_subdev(src_pad->entity);
bc83f513f8ad94 Sakari Ailus 2025-06-19 827
bc83f513f8ad94 Sakari Ailus 2025-06-19 828 dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
bc83f513f8ad94 Sakari Ailus 2025-06-19 829 src_pad->index, src_streams);
bc83f513f8ad94 Sakari Ailus 2025-06-19 830
bc83f513f8ad94 Sakari Ailus 2025-06-19 831 for (unsigned int i = __ffs(src_streams); src_streams;
bc83f513f8ad94 Sakari Ailus 2025-06-19 832 src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
bc83f513f8ad94 Sakari Ailus 2025-06-19 833 sink_pad = media_pad_remote_pad_unique(src_pad);
bc83f513f8ad94 Sakari Ailus 2025-06-19 @834 if (IS_ERR(src_pad)) {
^^^^^^^
Copy and paste. s/src_pad/sink_pad/.
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-19 8:15 ` [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Sakari Ailus
2025-06-19 11:42 ` kernel test robot
2025-06-20 3:58 ` Dan Carpenter
@ 2025-06-20 8:53 ` Jacopo Mondi
2025-06-21 8:10 ` Sakari Ailus
2025-07-15 10:49 ` Sakari Ailus
2025-06-23 9:48 ` kernel test robot
2025-06-26 23:07 ` Laurent Pinchart
4 siblings, 2 replies; 14+ messages in thread
From: Jacopo Mondi @ 2025-06-20 8:53 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen, laurent.pinchart
Hi Sakari
On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> lacked any sort of general solution: with multiple streams, when streaming
> is started on video nodes one by one, when should streaming be started in
> the source?
I tried quite some time to understand this, but if I'm not mistaken,
a stream-aware subdev, which links to vdev, will always "demux"
streams to different pads and will connect to the vdev from there
Source
subdev
+-----------------+
| (1/0) ------> vdev0
| |
(0)[1,2,3] (2/0 ------> vdev1
| |
| (3/0) ------> vdev2
+-----------------+
With
(0) multiplexed sink pad with 3 streams
(1) (2) and (3) source pad with a single stream
Can't we relay on the media-link state between the source pads and the
video devices with something like what Dan has proposed here ?
https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
What am I missing ?
>
> v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> queries the streams generated by the source and traces them back to the
> video nodes.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> include/media/v4l2-mc.h | 44 ++++++
> 2 files changed, 287 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 937d358697e1..1731088ad436 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> +
> +static int
> +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> + struct media_pad *src_pad, u64 __src_streams,
> + struct media_pad **__sink_pad, u64 *__sink_streams)
> +{
> + struct v4l2_subdev_route *route;
> + u64 src_streams = 0, sink_streams = 0;
> + bool has_sink_pad = false;
> + unsigned int sink_pad;
> +
> + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> + state->sd->entity.name, src_pad->index, __src_streams);
> + for_each_active_route(&state->routing, route) {
> + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> + state->sd->entity.name,
> + route->sink_pad, route->sink_stream, route->source_pad,
> + route->source_stream, route->flags);
> + if (route->source_pad != src_pad->index)
> + continue;
> +
> + if (!(BIT_ULL(route->source_stream) & __src_streams))
> + continue;
> +
> + if (!has_sink_pad) {
> + has_sink_pad = true;
> + sink_pad = route->sink_pad;
> + }
> +
> + if (route->sink_pad != sink_pad) {
> + dev_dbg(state->sd->dev,
> + "sink pads (%u vs. %u) differ\n",
> + route->sink_pad, sink_pad);
> + return -EMLINK;
> + }
> +
> + sink_streams |= BIT_ULL(route->sink_stream);
> + src_streams |= BIT_ULL(route->source_stream);
> + }
> +
> + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> + *__sink_streams = sink_streams;
> +
> + return 0;
> +}
> +
> +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> + unsigned int sink_stream,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad,
> + u64 *__sink_streams)
> +{
> + struct v4l2_subdev_state *state;
> + struct v4l2_subdev_route *route;
> + struct v4l2_subdev *sd;
> + struct media_pad *source_pad, *tmp_pad;
> + u32 source_stream;
> +
> + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> + return -ENXIO;
> +
> + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> + sd->entity.name);
> +
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> + sink_stream, true, 0);
> + if (IS_ERR(route)) {
> + v4l2_subdev_unlock_state(state);
> + dev_dbg(sd->dev,
> + "path_enabled: can't find opposite route for %s:%u/%u",
> + sd->entity.name, sink_pad->index, sink_stream);
> + return 2;
> + }
> +
> + source_pad = &sd->entity.pads[route->source_pad];
> + v4l2_subdev_unlock_state(state);
> +
> + tmp_pad = sink_pad;
> + sink_pad = media_pad_remote_pad_unique(source_pad);
> + if (IS_ERR(sink_pad)) {
> + dev_dbg(sd->dev,
> + "path_enabled: can't find remote source for %s:%u\n",
> + source_pad->entity->name, source_pad->index);
> + return PTR_ERR(sink_pad);
> + }
> +
> + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> + struct video_device *vdev;
> +
> + vdev = media_entity_to_video_device(sink_pad->entity);
> + if (!vdev)
> + return -ENXIO;
> +
> + dev_dbg(vdev->dev_parent,
> + "path_enabled: found video device %s\n",
> + vdev->name);
> +
> + if (!*__sink_pad) {
> + *__sink_pad = tmp_pad;
> + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> + tmp_pad->index, sink_stream);
> + } else if (tmp_pad != *__sink_pad) {
> + dev_dbg(sd->dev,
> + "path_enabled: pads %s/%u and %s/%u differ\n",
> + tmp_pad->entity->name, tmp_pad->index,
> + (*__sink_pad)->entity->name,
> + (*__sink_pad)->index);
> + return -EXDEV;
> + }
> +
> + *__sink_streams |= BIT_ULL(sink_stream);
> +
> + return func(vdev);
> + }
> +
> + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> + __sink_pad, __sink_streams);
> +}
> +
> +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> + u64 *__streams)
> +{
> + struct v4l2_mbus_frame_desc desc;
> + u64 streams = 0;
> + int ret;
> +
> + if (!__streams)
> + return -EINVAL;
> +
> + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> + if (ret == -ENOIOCTLCMD) {
> + *__streams = 1ULL;
> + return 0;
> + }
> + if (ret)
> + return ret;
> +
> + for (unsigned int i = 0; i < desc.num_entries; i++) {
> + if (streams & BIT_ULL(desc.entry[i].stream))
> + return -EINVAL;
> +
> + streams |= BIT_ULL(desc.entry[i].stream);
> + }
> +
> + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> + *__streams, streams);
> + if (*__streams & ~streams)
> + return -EINVAL;
> +
> + *__streams = streams;
> +
> + return 0;
> +}
> +
> +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad, u64 *__sink_streams)
> +{
> + u64 sink_streams = 1U;
> + struct media_pad *src_pad;
> + u64 src_streams;
> + struct v4l2_subdev_state *state;
> + struct media_pad *sink_pad = vdev->entity.pads;
> + struct v4l2_subdev *sd = NULL;
> + bool streaming = true;
> + struct media_pad *tmp_pad;
> + u64 tmp_streams;
> + int ret;
> +
> + if (!__sink_pad)
> + __sink_pad = &tmp_pad;
> + if (!__sink_streams)
> + __sink_streams = &tmp_streams;
> + *__sink_pad = NULL;
> + *__sink_streams = 0;
> +
> + do {
> + src_pad = media_pad_remote_pad_unique(sink_pad);
> + if (IS_ERR(src_pad)) {
> + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> + "no unique remote pad found from %s:%u\n",
> + sink_pad->entity->name, sink_pad->index);
> + return PTR_ERR(src_pad);
> + }
> +
> + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> + if (!sd) {
> + dev_dbg(sd->dev,
> + "media entity %s is not a V4L2 sub-device\n",
> + src_pad->entity->name);
> + return -ENXIO;
> + }
> +
> + /* Source streams match sink. */
> + src_streams = sink_streams;
> +
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> + src_streams, &sink_pad,
> + &sink_streams);
> + v4l2_subdev_unlock_state(state);
> + if (ret)
> + return ret;
> + } while (sink_pad);
> +
> + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> + if (ret)
> + return ret;
> +
> + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> +
> + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> + src_pad->index, src_streams);
> +
> + for (unsigned int i = __ffs(src_streams); src_streams;
> + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> + sink_pad = media_pad_remote_pad_unique(src_pad);
> + if (IS_ERR(src_pad)) {
> + dev_dbg(sd->dev,
> + "no unique remote pad found from %s:%u\n",
> + sink_pad->entity->name, sink_pad->index);
> + return PTR_ERR(src_pad);
> + }
> +
> + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> + __sink_streams);
> + if (ret == 2)
> + continue;
> + if (ret < 0)
> + return ret;
> + if (!ret)
> + streaming = false;
> + }
> +
> + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> + (*__sink_pad)->index, *__sink_streams);
> +
> + return streaming;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 1837c9fd78cf..e72c0f62fa34 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> unsigned int notification);
>
> +/**
> + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> + * @vdev: The video device
> + * @func: Caller-provided function to tell a video device's streaming state
> + * @__sink_pad: sink pad at the root of the local pipeline
> + * @__sink_streams: streams to start
> + *
> + * Use to tell whether streaming should start on a video node. @func returns
> + * true if streaming has been started on a given video node. @__sink_pad and
> + * @__sink_streams are filled with pad and streams on the sub-device closest to
> + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> + * v4l2_subdev_disable_streams().
> + *
> + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> + * a to-do list):
> + * * only unbranched streams can be supported albeit adding support for
> + * downstream branches would be fairly trivial,
> + * * streams within a single source sub-device are considered to start at the
> + * same time, more control could be added in two ways: 1) for sources to
> + * determine stream starting, a control could be added to UAPI and 2) sources
> + * could tell which streams start at the same time using a sub-device
> + * operation,
> + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> + * be implemented by letting the caller to provide a function to determine
> + * which streams are of interest and
> + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> + * but this can also be rather trivially addressed.
> + *
> + * Return:
> + * * 0: Success, but don't start streaming yet
> + * * 1: Success, now it's time to start streaming
> + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> + * * -ENOTUNIQ: No unique remote pad
> + * * -ENOLINK: No remote pad found
> + * * -ENOENT: Enabled upstream route not found
> + * * -EMLINK: No unique downstream route found
> + * * -EINVAL: Stream could not be followed to source or was not produced by
> + * the source
> + */
> +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad,
> + u64 *__sink_streams);
> +
> #else /* CONFIG_MEDIA_CONTROLLER */
>
> static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
> --
> 2.39.5
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-20 8:53 ` Jacopo Mondi
@ 2025-06-21 8:10 ` Sakari Ailus
2025-07-15 10:49 ` Sakari Ailus
1 sibling, 0 replies; 14+ messages in thread
From: Sakari Ailus @ 2025-06-21 8:10 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Sakari Ailus, linux-media, bingbu.cao, stanislaw.gruszka,
tian.shu.qiu, tomi.valkeinen, laurent.pinchart
Hi Jacopo,
Thank you for the comments.
On Fri, Jun 20, 2025 at 10:53:13AM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > lacked any sort of general solution: with multiple streams, when streaming
> > is started on video nodes one by one, when should streaming be started in
> > the source?
>
> I tried quite some time to understand this, but if I'm not mistaken,
> a stream-aware subdev, which links to vdev, will always "demux"
> streams to different pads and will connect to the vdev from there
>
>
> Source
> subdev
> +-----------------+
> | (1/0) ------> vdev0
> | |
> (0)[1,2,3] (2/0 ------> vdev1
> | |
> | (3/0) ------> vdev2
> +-----------------+
>
> With
>
> (0) multiplexed sink pad with 3 streams
> (1) (2) and (3) source pad with a single stream
>
> Can't we relay on the media-link state between the source pads and the
> video devices with something like what Dan has proposed here ?
> https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
>
> What am I missing ?
There are two different concepts here that are probably underdocumented at
the moment: Media entity pipeline state and then when and in which order
streaming control is applied on hardware. But... maybe the latter can be
derived from the former when it comes to video nodes. The pipeline is set
for entities that are upstream from the video device, it's not set on
downstream entities. I'll see if I could use that instead. Otherwise, I'll
check if Dan's approach could be used for the purpose.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-20 8:53 ` Jacopo Mondi
2025-06-21 8:10 ` Sakari Ailus
@ 2025-07-15 10:49 ` Sakari Ailus
2025-07-15 11:25 ` Laurent Pinchart
1 sibling, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2025-07-15 10:49 UTC (permalink / raw)
To: Jacopo Mondi
Cc: linux-media, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen, laurent.pinchart, Daniel Scally
Hi Jacopo,
On Fri, Jun 20, 2025 at 10:53:13AM +0200, Jacopo Mondi wrote:
> Hi Sakari
>
> On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > lacked any sort of general solution: with multiple streams, when streaming
> > is started on video nodes one by one, when should streaming be started in
> > the source?
>
> I tried quite some time to understand this, but if I'm not mistaken,
> a stream-aware subdev, which links to vdev, will always "demux"
> streams to different pads and will connect to the vdev from there
>
>
> Source
> subdev
> +-----------------+
> | (1/0) ------> vdev0
> | |
> (0)[1,2,3] (2/0 ------> vdev1
> | |
> | (3/0) ------> vdev2
> +-----------------+
>
> With
>
> (0) multiplexed sink pad with 3 streams
> (1) (2) and (3) source pad with a single stream
>
> Can't we relay on the media-link state between the source pads and the
> video devices with something like what Dan has proposed here ?
> https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
This isn't entirely the same thing: pipeline is specific to the pad but not
streams, ad here we're interested in streams. Two streams may start at
different points of time even if both are part of the same pipeline.
We definitely should have just one way to figure this out.
I'll send v2 with comments and before that also see if (or how) we could
get rid of the callback.
Cc Dan.
>
> What am I missing ?
>
> >
> > v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> > queries the streams generated by the source and traces them back to the
> > video nodes.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> > include/media/v4l2-mc.h | 44 ++++++
> > 2 files changed, 287 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > index 937d358697e1..1731088ad436 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> > +
> > +static int
> > +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> > + struct media_pad *src_pad, u64 __src_streams,
> > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > +{
> > + struct v4l2_subdev_route *route;
> > + u64 src_streams = 0, sink_streams = 0;
> > + bool has_sink_pad = false;
> > + unsigned int sink_pad;
> > +
> > + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> > + state->sd->entity.name, src_pad->index, __src_streams);
> > + for_each_active_route(&state->routing, route) {
> > + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> > + state->sd->entity.name,
> > + route->sink_pad, route->sink_stream, route->source_pad,
> > + route->source_stream, route->flags);
> > + if (route->source_pad != src_pad->index)
> > + continue;
> > +
> > + if (!(BIT_ULL(route->source_stream) & __src_streams))
> > + continue;
> > +
> > + if (!has_sink_pad) {
> > + has_sink_pad = true;
> > + sink_pad = route->sink_pad;
> > + }
> > +
> > + if (route->sink_pad != sink_pad) {
> > + dev_dbg(state->sd->dev,
> > + "sink pads (%u vs. %u) differ\n",
> > + route->sink_pad, sink_pad);
> > + return -EMLINK;
> > + }
> > +
> > + sink_streams |= BIT_ULL(route->sink_stream);
> > + src_streams |= BIT_ULL(route->source_stream);
> > + }
> > +
> > + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> > + *__sink_streams = sink_streams;
> > +
> > + return 0;
> > +}
> > +
> > +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> > + unsigned int sink_stream,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad,
> > + u64 *__sink_streams)
> > +{
> > + struct v4l2_subdev_state *state;
> > + struct v4l2_subdev_route *route;
> > + struct v4l2_subdev *sd;
> > + struct media_pad *source_pad, *tmp_pad;
> > + u32 source_stream;
> > +
> > + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> > + return -ENXIO;
> > +
> > + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> > + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> > + sd->entity.name);
> > +
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> > + sink_stream, true, 0);
> > + if (IS_ERR(route)) {
> > + v4l2_subdev_unlock_state(state);
> > + dev_dbg(sd->dev,
> > + "path_enabled: can't find opposite route for %s:%u/%u",
> > + sd->entity.name, sink_pad->index, sink_stream);
> > + return 2;
> > + }
> > +
> > + source_pad = &sd->entity.pads[route->source_pad];
> > + v4l2_subdev_unlock_state(state);
> > +
> > + tmp_pad = sink_pad;
> > + sink_pad = media_pad_remote_pad_unique(source_pad);
> > + if (IS_ERR(sink_pad)) {
> > + dev_dbg(sd->dev,
> > + "path_enabled: can't find remote source for %s:%u\n",
> > + source_pad->entity->name, source_pad->index);
> > + return PTR_ERR(sink_pad);
> > + }
> > +
> > + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> > + struct video_device *vdev;
> > +
> > + vdev = media_entity_to_video_device(sink_pad->entity);
> > + if (!vdev)
> > + return -ENXIO;
> > +
> > + dev_dbg(vdev->dev_parent,
> > + "path_enabled: found video device %s\n",
> > + vdev->name);
> > +
> > + if (!*__sink_pad) {
> > + *__sink_pad = tmp_pad;
> > + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> > + tmp_pad->index, sink_stream);
> > + } else if (tmp_pad != *__sink_pad) {
> > + dev_dbg(sd->dev,
> > + "path_enabled: pads %s/%u and %s/%u differ\n",
> > + tmp_pad->entity->name, tmp_pad->index,
> > + (*__sink_pad)->entity->name,
> > + (*__sink_pad)->index);
> > + return -EXDEV;
> > + }
> > +
> > + *__sink_streams |= BIT_ULL(sink_stream);
> > +
> > + return func(vdev);
> > + }
> > +
> > + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> > + __sink_pad, __sink_streams);
> > +}
> > +
> > +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> > + u64 *__streams)
> > +{
> > + struct v4l2_mbus_frame_desc desc;
> > + u64 streams = 0;
> > + int ret;
> > +
> > + if (!__streams)
> > + return -EINVAL;
> > +
> > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> > + if (ret == -ENOIOCTLCMD) {
> > + *__streams = 1ULL;
> > + return 0;
> > + }
> > + if (ret)
> > + return ret;
> > +
> > + for (unsigned int i = 0; i < desc.num_entries; i++) {
> > + if (streams & BIT_ULL(desc.entry[i].stream))
> > + return -EINVAL;
> > +
> > + streams |= BIT_ULL(desc.entry[i].stream);
> > + }
> > +
> > + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> > + *__streams, streams);
> > + if (*__streams & ~streams)
> > + return -EINVAL;
> > +
> > + *__streams = streams;
> > +
> > + return 0;
> > +}
> > +
> > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > +{
> > + u64 sink_streams = 1U;
> > + struct media_pad *src_pad;
> > + u64 src_streams;
> > + struct v4l2_subdev_state *state;
> > + struct media_pad *sink_pad = vdev->entity.pads;
> > + struct v4l2_subdev *sd = NULL;
> > + bool streaming = true;
> > + struct media_pad *tmp_pad;
> > + u64 tmp_streams;
> > + int ret;
> > +
> > + if (!__sink_pad)
> > + __sink_pad = &tmp_pad;
> > + if (!__sink_streams)
> > + __sink_streams = &tmp_streams;
> > + *__sink_pad = NULL;
> > + *__sink_streams = 0;
> > +
> > + do {
> > + src_pad = media_pad_remote_pad_unique(sink_pad);
> > + if (IS_ERR(src_pad)) {
> > + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> > + "no unique remote pad found from %s:%u\n",
> > + sink_pad->entity->name, sink_pad->index);
> > + return PTR_ERR(src_pad);
> > + }
> > +
> > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > + if (!sd) {
> > + dev_dbg(sd->dev,
> > + "media entity %s is not a V4L2 sub-device\n",
> > + src_pad->entity->name);
> > + return -ENXIO;
> > + }
> > +
> > + /* Source streams match sink. */
> > + src_streams = sink_streams;
> > +
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> > + src_streams, &sink_pad,
> > + &sink_streams);
> > + v4l2_subdev_unlock_state(state);
> > + if (ret)
> > + return ret;
> > + } while (sink_pad);
> > +
> > + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> > + if (ret)
> > + return ret;
> > +
> > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > +
> > + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> > + src_pad->index, src_streams);
> > +
> > + for (unsigned int i = __ffs(src_streams); src_streams;
> > + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> > + sink_pad = media_pad_remote_pad_unique(src_pad);
> > + if (IS_ERR(src_pad)) {
> > + dev_dbg(sd->dev,
> > + "no unique remote pad found from %s:%u\n",
> > + sink_pad->entity->name, sink_pad->index);
> > + return PTR_ERR(src_pad);
> > + }
> > +
> > + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> > + __sink_streams);
> > + if (ret == 2)
> > + continue;
> > + if (ret < 0)
> > + return ret;
> > + if (!ret)
> > + streaming = false;
> > + }
> > +
> > + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> > + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> > + (*__sink_pad)->index, *__sink_streams);
> > +
> > + return streaming;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> > diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> > index 1837c9fd78cf..e72c0f62fa34 100644
> > --- a/include/media/v4l2-mc.h
> > +++ b/include/media/v4l2-mc.h
> > @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> > int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > unsigned int notification);
> >
> > +/**
> > + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> > + * @vdev: The video device
> > + * @func: Caller-provided function to tell a video device's streaming state
> > + * @__sink_pad: sink pad at the root of the local pipeline
> > + * @__sink_streams: streams to start
> > + *
> > + * Use to tell whether streaming should start on a video node. @func returns
> > + * true if streaming has been started on a given video node. @__sink_pad and
> > + * @__sink_streams are filled with pad and streams on the sub-device closest to
> > + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> > + * v4l2_subdev_disable_streams().
> > + *
> > + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> > + * a to-do list):
> > + * * only unbranched streams can be supported albeit adding support for
> > + * downstream branches would be fairly trivial,
> > + * * streams within a single source sub-device are considered to start at the
> > + * same time, more control could be added in two ways: 1) for sources to
> > + * determine stream starting, a control could be added to UAPI and 2) sources
> > + * could tell which streams start at the same time using a sub-device
> > + * operation,
> > + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> > + * be implemented by letting the caller to provide a function to determine
> > + * which streams are of interest and
> > + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> > + * but this can also be rather trivially addressed.
> > + *
> > + * Return:
> > + * * 0: Success, but don't start streaming yet
> > + * * 1: Success, now it's time to start streaming
> > + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> > + * * -ENOTUNIQ: No unique remote pad
> > + * * -ENOLINK: No remote pad found
> > + * * -ENOENT: Enabled upstream route not found
> > + * * -EMLINK: No unique downstream route found
> > + * * -EINVAL: Stream could not be followed to source or was not produced by
> > + * the source
> > + */
> > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad,
> > + u64 *__sink_streams);
> > +
> > #else /* CONFIG_MEDIA_CONTROLLER */
> >
> > static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-07-15 10:49 ` Sakari Ailus
@ 2025-07-15 11:25 ` Laurent Pinchart
2025-07-15 11:32 ` Sakari Ailus
0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2025-07-15 11:25 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, linux-media, bingbu.cao, stanislaw.gruszka,
tian.shu.qiu, tomi.valkeinen, Daniel Scally
On Tue, Jul 15, 2025 at 10:49:32AM +0000, Sakari Ailus wrote:
> On Fri, Jun 20, 2025 at 10:53:13AM +0200, Jacopo Mondi wrote:
> > On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > > lacked any sort of general solution: with multiple streams, when streaming
> > > is started on video nodes one by one, when should streaming be started in
> > > the source?
> >
> > I tried quite some time to understand this, but if I'm not mistaken,
> > a stream-aware subdev, which links to vdev, will always "demux"
> > streams to different pads and will connect to the vdev from there
> >
> >
> > Source
> > subdev
> > +-----------------+
> > | (1/0) ------> vdev0
> > | |
> > (0)[1,2,3] (2/0 ------> vdev1
> > | |
> > | (3/0) ------> vdev2
> > +-----------------+
> >
> > With
> >
> > (0) multiplexed sink pad with 3 streams
> > (1) (2) and (3) source pad with a single stream
> >
> > Can't we relay on the media-link state between the source pads and the
> > video devices with something like what Dan has proposed here ?
> > https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
>
> This isn't entirely the same thing: pipeline is specific to the pad but not
> streams, ad here we're interested in streams. Two streams may start at
> different points of time even if both are part of the same pipeline.
>
> We definitely should have just one way to figure this out.
But can we ? Isn't it dependent on use cases ? I can imagine an
application wanting to capture two image streams where one of them is
enabled all the time and the other one is regularly turned on and off,
in which case you would need to start the pipeline when the first stream
starts. A different application may instead want to capture the same two
streams and make sure it gets all frames on both, which possibly
requires delaying the start of capture until both video capture devices
are started.
> I'll send v2 with comments and before that also see if (or how) we could
> get rid of the callback.
>
> Cc Dan.
>
> > What am I missing ?
> >
> > > v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> > > queries the streams generated by the source and traces them back to the
> > > video nodes.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> > > include/media/v4l2-mc.h | 44 ++++++
> > > 2 files changed, 287 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > > index 937d358697e1..1731088ad436 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > > @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > > return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> > > +
> > > +static int
> > > +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> > > + struct media_pad *src_pad, u64 __src_streams,
> > > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > > +{
> > > + struct v4l2_subdev_route *route;
> > > + u64 src_streams = 0, sink_streams = 0;
> > > + bool has_sink_pad = false;
> > > + unsigned int sink_pad;
> > > +
> > > + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> > > + state->sd->entity.name, src_pad->index, __src_streams);
> > > + for_each_active_route(&state->routing, route) {
> > > + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> > > + state->sd->entity.name,
> > > + route->sink_pad, route->sink_stream, route->source_pad,
> > > + route->source_stream, route->flags);
> > > + if (route->source_pad != src_pad->index)
> > > + continue;
> > > +
> > > + if (!(BIT_ULL(route->source_stream) & __src_streams))
> > > + continue;
> > > +
> > > + if (!has_sink_pad) {
> > > + has_sink_pad = true;
> > > + sink_pad = route->sink_pad;
> > > + }
> > > +
> > > + if (route->sink_pad != sink_pad) {
> > > + dev_dbg(state->sd->dev,
> > > + "sink pads (%u vs. %u) differ\n",
> > > + route->sink_pad, sink_pad);
> > > + return -EMLINK;
> > > + }
> > > +
> > > + sink_streams |= BIT_ULL(route->sink_stream);
> > > + src_streams |= BIT_ULL(route->source_stream);
> > > + }
> > > +
> > > + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> > > + *__sink_streams = sink_streams;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> > > + unsigned int sink_stream,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad,
> > > + u64 *__sink_streams)
> > > +{
> > > + struct v4l2_subdev_state *state;
> > > + struct v4l2_subdev_route *route;
> > > + struct v4l2_subdev *sd;
> > > + struct media_pad *source_pad, *tmp_pad;
> > > + u32 source_stream;
> > > +
> > > + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> > > + return -ENXIO;
> > > +
> > > + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> > > + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> > > + sd->entity.name);
> > > +
> > > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > > + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> > > + sink_stream, true, 0);
> > > + if (IS_ERR(route)) {
> > > + v4l2_subdev_unlock_state(state);
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: can't find opposite route for %s:%u/%u",
> > > + sd->entity.name, sink_pad->index, sink_stream);
> > > + return 2;
> > > + }
> > > +
> > > + source_pad = &sd->entity.pads[route->source_pad];
> > > + v4l2_subdev_unlock_state(state);
> > > +
> > > + tmp_pad = sink_pad;
> > > + sink_pad = media_pad_remote_pad_unique(source_pad);
> > > + if (IS_ERR(sink_pad)) {
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: can't find remote source for %s:%u\n",
> > > + source_pad->entity->name, source_pad->index);
> > > + return PTR_ERR(sink_pad);
> > > + }
> > > +
> > > + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> > > + struct video_device *vdev;
> > > +
> > > + vdev = media_entity_to_video_device(sink_pad->entity);
> > > + if (!vdev)
> > > + return -ENXIO;
> > > +
> > > + dev_dbg(vdev->dev_parent,
> > > + "path_enabled: found video device %s\n",
> > > + vdev->name);
> > > +
> > > + if (!*__sink_pad) {
> > > + *__sink_pad = tmp_pad;
> > > + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> > > + tmp_pad->index, sink_stream);
> > > + } else if (tmp_pad != *__sink_pad) {
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: pads %s/%u and %s/%u differ\n",
> > > + tmp_pad->entity->name, tmp_pad->index,
> > > + (*__sink_pad)->entity->name,
> > > + (*__sink_pad)->index);
> > > + return -EXDEV;
> > > + }
> > > +
> > > + *__sink_streams |= BIT_ULL(sink_stream);
> > > +
> > > + return func(vdev);
> > > + }
> > > +
> > > + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> > > + __sink_pad, __sink_streams);
> > > +}
> > > +
> > > +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> > > + u64 *__streams)
> > > +{
> > > + struct v4l2_mbus_frame_desc desc;
> > > + u64 streams = 0;
> > > + int ret;
> > > +
> > > + if (!__streams)
> > > + return -EINVAL;
> > > +
> > > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> > > + if (ret == -ENOIOCTLCMD) {
> > > + *__streams = 1ULL;
> > > + return 0;
> > > + }
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for (unsigned int i = 0; i < desc.num_entries; i++) {
> > > + if (streams & BIT_ULL(desc.entry[i].stream))
> > > + return -EINVAL;
> > > +
> > > + streams |= BIT_ULL(desc.entry[i].stream);
> > > + }
> > > +
> > > + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> > > + *__streams, streams);
> > > + if (*__streams & ~streams)
> > > + return -EINVAL;
> > > +
> > > + *__streams = streams;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > > +{
> > > + u64 sink_streams = 1U;
> > > + struct media_pad *src_pad;
> > > + u64 src_streams;
> > > + struct v4l2_subdev_state *state;
> > > + struct media_pad *sink_pad = vdev->entity.pads;
> > > + struct v4l2_subdev *sd = NULL;
> > > + bool streaming = true;
> > > + struct media_pad *tmp_pad;
> > > + u64 tmp_streams;
> > > + int ret;
> > > +
> > > + if (!__sink_pad)
> > > + __sink_pad = &tmp_pad;
> > > + if (!__sink_streams)
> > > + __sink_streams = &tmp_streams;
> > > + *__sink_pad = NULL;
> > > + *__sink_streams = 0;
> > > +
> > > + do {
> > > + src_pad = media_pad_remote_pad_unique(sink_pad);
> > > + if (IS_ERR(src_pad)) {
> > > + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> > > + "no unique remote pad found from %s:%u\n",
> > > + sink_pad->entity->name, sink_pad->index);
> > > + return PTR_ERR(src_pad);
> > > + }
> > > +
> > > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > > + if (!sd) {
> > > + dev_dbg(sd->dev,
> > > + "media entity %s is not a V4L2 sub-device\n",
> > > + src_pad->entity->name);
> > > + return -ENXIO;
> > > + }
> > > +
> > > + /* Source streams match sink. */
> > > + src_streams = sink_streams;
> > > +
> > > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > > + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> > > + src_streams, &sink_pad,
> > > + &sink_streams);
> > > + v4l2_subdev_unlock_state(state);
> > > + if (ret)
> > > + return ret;
> > > + } while (sink_pad);
> > > +
> > > + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > > +
> > > + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> > > + src_pad->index, src_streams);
> > > +
> > > + for (unsigned int i = __ffs(src_streams); src_streams;
> > > + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> > > + sink_pad = media_pad_remote_pad_unique(src_pad);
> > > + if (IS_ERR(src_pad)) {
> > > + dev_dbg(sd->dev,
> > > + "no unique remote pad found from %s:%u\n",
> > > + sink_pad->entity->name, sink_pad->index);
> > > + return PTR_ERR(src_pad);
> > > + }
> > > +
> > > + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> > > + __sink_streams);
> > > + if (ret == 2)
> > > + continue;
> > > + if (ret < 0)
> > > + return ret;
> > > + if (!ret)
> > > + streaming = false;
> > > + }
> > > +
> > > + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> > > + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> > > + (*__sink_pad)->index, *__sink_streams);
> > > +
> > > + return streaming;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> > > diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> > > index 1837c9fd78cf..e72c0f62fa34 100644
> > > --- a/include/media/v4l2-mc.h
> > > +++ b/include/media/v4l2-mc.h
> > > @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> > > int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > > unsigned int notification);
> > >
> > > +/**
> > > + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> > > + * @vdev: The video device
> > > + * @func: Caller-provided function to tell a video device's streaming state
> > > + * @__sink_pad: sink pad at the root of the local pipeline
> > > + * @__sink_streams: streams to start
> > > + *
> > > + * Use to tell whether streaming should start on a video node. @func returns
> > > + * true if streaming has been started on a given video node. @__sink_pad and
> > > + * @__sink_streams are filled with pad and streams on the sub-device closest to
> > > + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> > > + * v4l2_subdev_disable_streams().
> > > + *
> > > + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> > > + * a to-do list):
> > > + * * only unbranched streams can be supported albeit adding support for
> > > + * downstream branches would be fairly trivial,
> > > + * * streams within a single source sub-device are considered to start at the
> > > + * same time, more control could be added in two ways: 1) for sources to
> > > + * determine stream starting, a control could be added to UAPI and 2) sources
> > > + * could tell which streams start at the same time using a sub-device
> > > + * operation,
> > > + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> > > + * be implemented by letting the caller to provide a function to determine
> > > + * which streams are of interest and
> > > + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> > > + * but this can also be rather trivially addressed.
> > > + *
> > > + * Return:
> > > + * * 0: Success, but don't start streaming yet
> > > + * * 1: Success, now it's time to start streaming
> > > + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> > > + * * -ENOTUNIQ: No unique remote pad
> > > + * * -ENOLINK: No remote pad found
> > > + * * -ENOENT: Enabled upstream route not found
> > > + * * -EMLINK: No unique downstream route found
> > > + * * -EINVAL: Stream could not be followed to source or was not produced by
> > > + * the source
> > > + */
> > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad,
> > > + u64 *__sink_streams);
> > > +
> > > #else /* CONFIG_MEDIA_CONTROLLER */
> > >
> > > static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
>
> --
> Kind regards,
>
> Sakari Ailus
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-07-15 11:25 ` Laurent Pinchart
@ 2025-07-15 11:32 ` Sakari Ailus
2025-07-15 18:18 ` Laurent Pinchart
0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2025-07-15 11:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Jacopo Mondi, linux-media, bingbu.cao, stanislaw.gruszka,
tian.shu.qiu, tomi.valkeinen, Daniel Scally
Hi Laurent,
On Tue, Jul 15, 2025 at 02:25:20PM +0300, Laurent Pinchart wrote:
> On Tue, Jul 15, 2025 at 10:49:32AM +0000, Sakari Ailus wrote:
> > On Fri, Jun 20, 2025 at 10:53:13AM +0200, Jacopo Mondi wrote:
> > > On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > > > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > > > lacked any sort of general solution: with multiple streams, when streaming
> > > > is started on video nodes one by one, when should streaming be started in
> > > > the source?
> > >
> > > I tried quite some time to understand this, but if I'm not mistaken,
> > > a stream-aware subdev, which links to vdev, will always "demux"
> > > streams to different pads and will connect to the vdev from there
> > >
> > >
> > > Source
> > > subdev
> > > +-----------------+
> > > | (1/0) ------> vdev0
> > > | |
> > > (0)[1,2,3] (2/0 ------> vdev1
> > > | |
> > > | (3/0) ------> vdev2
> > > +-----------------+
> > >
> > > With
> > >
> > > (0) multiplexed sink pad with 3 streams
> > > (1) (2) and (3) source pad with a single stream
> > >
> > > Can't we relay on the media-link state between the source pads and the
> > > video devices with something like what Dan has proposed here ?
> > > https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
> >
> > This isn't entirely the same thing: pipeline is specific to the pad but not
> > streams, ad here we're interested in streams. Two streams may start at
> > different points of time even if both are part of the same pipeline.
> >
> > We definitely should have just one way to figure this out.
>
> But can we ? Isn't it dependent on use cases ? I can imagine an
> application wanting to capture two image streams where one of them is
> enabled all the time and the other one is regularly turned on and off,
> in which case you would need to start the pipeline when the first stream
> starts. A different application may instead want to capture the same two
> streams and make sure it gets all frames on both, which possibly
> requires delaying the start of capture until both video capture devices
> are started.
Exactly. It's indeed dependent on the use case but the driver is the same
in both cases. This is why the generic solution needs more information from
the user space but I think we can work out how to support this after
merging the metadata series.
--
Regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-07-15 11:32 ` Sakari Ailus
@ 2025-07-15 18:18 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2025-07-15 18:18 UTC (permalink / raw)
To: Sakari Ailus
Cc: Jacopo Mondi, linux-media, bingbu.cao, stanislaw.gruszka,
tian.shu.qiu, tomi.valkeinen, Daniel Scally
On Tue, Jul 15, 2025 at 11:32:14AM +0000, Sakari Ailus wrote:
> On Tue, Jul 15, 2025 at 02:25:20PM +0300, Laurent Pinchart wrote:
> > On Tue, Jul 15, 2025 at 10:49:32AM +0000, Sakari Ailus wrote:
> > > On Fri, Jun 20, 2025 at 10:53:13AM +0200, Jacopo Mondi wrote:
> > > > On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > > > > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > > > > lacked any sort of general solution: with multiple streams, when streaming
> > > > > is started on video nodes one by one, when should streaming be started in
> > > > > the source?
> > > >
> > > > I tried quite some time to understand this, but if I'm not mistaken,
> > > > a stream-aware subdev, which links to vdev, will always "demux"
> > > > streams to different pads and will connect to the vdev from there
> > > >
> > > >
> > > > Source
> > > > subdev
> > > > +-----------------+
> > > > | (1/0) ------> vdev0
> > > > | |
> > > > (0)[1,2,3] (2/0 ------> vdev1
> > > > | |
> > > > | (3/0) ------> vdev2
> > > > +-----------------+
> > > >
> > > > With
> > > >
> > > > (0) multiplexed sink pad with 3 streams
> > > > (1) (2) and (3) source pad with a single stream
> > > >
> > > > Can't we relay on the media-link state between the source pads and the
> > > > video devices with something like what Dan has proposed here ?
> > > > https://patchwork.linuxtv.org/project/linux-media/patch/20250519140403.443915-2-dan.scally@ideasonboard.com/
> > >
> > > This isn't entirely the same thing: pipeline is specific to the pad but not
> > > streams, ad here we're interested in streams. Two streams may start at
> > > different points of time even if both are part of the same pipeline.
> > >
> > > We definitely should have just one way to figure this out.
> >
> > But can we ? Isn't it dependent on use cases ? I can imagine an
> > application wanting to capture two image streams where one of them is
> > enabled all the time and the other one is regularly turned on and off,
> > in which case you would need to start the pipeline when the first stream
> > starts. A different application may instead want to capture the same two
> > streams and make sure it gets all frames on both, which possibly
> > requires delaying the start of capture until both video capture devices
> > are started.
>
> Exactly. It's indeed dependent on the use case but the driver is the same
> in both cases. This is why the generic solution needs more information from
> the user space but I think we can work out how to support this after
> merging the metadata series.
Yes, we need more information from userspace.
It's also a driver-dependent behaviour. If the streams come from
multiple cameras that all go through the same deserializer (e.g. a quad
FPD-Link or GMSL deserializer), then the decision on when to start
streaming on some of the components in the pipeline could depend on
which deserializer is used. Some will require all cameras to be started
together, while other could allow starting and stopping sources
independently. The nature of upstream elements could influence when
downstream elements will need to be started, for the same
userspace-facing behaviour.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-19 8:15 ` [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Sakari Ailus
` (2 preceding siblings ...)
2025-06-20 8:53 ` Jacopo Mondi
@ 2025-06-23 9:48 ` kernel test robot
2025-06-26 23:07 ` Laurent Pinchart
4 siblings, 0 replies; 14+ messages in thread
From: kernel test robot @ 2025-06-23 9:48 UTC (permalink / raw)
To: Sakari Ailus; +Cc: llvm, oe-kbuild-all
Hi Sakari,
kernel test robot noticed the following build errors:
[auto build test ERROR on linuxtv-media-pending/master]
[also build test ERROR on linus/master media-tree/master v6.16-rc3 next-20250623]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Sakari-Ailus/media-ipu6-Use-correct-pads-for-xlate_streams/20250619-161847
base: https://git.linuxtv.org/media-ci/media-pending.git master
patch link: https://lore.kernel.org/r/20250619081546.1582969-13-sakari.ailus%40linux.intel.com
patch subject: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
config: arm-imx_v4_v5_defconfig (https://download.01.org/0day-ci/archive/20250623/202506231722.jPtC2RyB-lkp@intel.com/config)
compiler: clang version 21.0.0git (https://github.com/llvm/llvm-project 875b36a8742437b95f623bab1e0332562c7b4b3f)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250623/202506231722.jPtC2RyB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506231722.jPtC2RyB-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/media/v4l2-core/v4l2-mc.c:628:2: error: call to undeclared function 'for_each_active_route'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
628 | for_each_active_route(&state->routing, route) {
| ^
drivers/media/v4l2-core/v4l2-mc.c:628:47: error: expected ';' after expression
628 | for_each_active_route(&state->routing, route) {
| ^
| ;
drivers/media/v4l2-core/v4l2-mc.c:634:4: error: 'continue' statement not in loop statement
634 | continue;
| ^
drivers/media/v4l2-core/v4l2-mc.c:637:4: error: 'continue' statement not in loop statement
637 | continue;
| ^
>> drivers/media/v4l2-core/v4l2-mc.c:681:10: error: call to undeclared function 'v4l2_subdev_find_route'; ISO C99 and later do not support implicit function declarations [-Wimplicit-function-declaration]
681 | route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
| ^
>> drivers/media/v4l2-core/v4l2-mc.c:681:8: error: incompatible integer to pointer conversion assigning to 'struct v4l2_subdev_route *' from 'int' [-Wint-conversion]
681 | route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
| ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
682 | sink_stream, true, 0);
| ~~~~~~~~~~~~~~~~~~~~~
6 errors generated.
vim +/for_each_active_route +628 drivers/media/v4l2-core/v4l2-mc.c
615
616 static int
617 __v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
618 struct media_pad *src_pad, u64 __src_streams,
619 struct media_pad **__sink_pad, u64 *__sink_streams)
620 {
621 struct v4l2_subdev_route *route;
622 u64 src_streams = 0, sink_streams = 0;
623 bool has_sink_pad = false;
624 unsigned int sink_pad;
625
626 dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
627 state->sd->entity.name, src_pad->index, __src_streams);
> 628 for_each_active_route(&state->routing, route) {
629 dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
630 state->sd->entity.name,
631 route->sink_pad, route->sink_stream, route->source_pad,
632 route->source_stream, route->flags);
633 if (route->source_pad != src_pad->index)
634 continue;
635
636 if (!(BIT_ULL(route->source_stream) & __src_streams))
637 continue;
638
639 if (!has_sink_pad) {
640 has_sink_pad = true;
641 sink_pad = route->sink_pad;
642 }
643
644 if (route->sink_pad != sink_pad) {
645 dev_dbg(state->sd->dev,
646 "sink pads (%u vs. %u) differ\n",
647 route->sink_pad, sink_pad);
648 return -EMLINK;
649 }
650
651 sink_streams |= BIT_ULL(route->sink_stream);
652 src_streams |= BIT_ULL(route->source_stream);
653 }
654
655 *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
656 *__sink_streams = sink_streams;
657
658 return 0;
659 }
660
661 static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
662 unsigned int sink_stream,
663 bool (*func)(struct video_device *vdev),
664 struct media_pad **__sink_pad,
665 u64 *__sink_streams)
666 {
667 struct v4l2_subdev_state *state;
668 struct v4l2_subdev_route *route;
669 struct v4l2_subdev *sd;
670 struct media_pad *source_pad, *tmp_pad;
671 u32 source_stream;
672
673 if (!is_media_entity_v4l2_subdev(sink_pad->entity))
674 return -ENXIO;
675
676 sd = media_entity_to_v4l2_subdev(sink_pad->entity);
677 dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
678 sd->entity.name);
679
680 state = v4l2_subdev_lock_and_get_active_state(sd);
> 681 route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
682 sink_stream, true, 0);
683 if (IS_ERR(route)) {
684 v4l2_subdev_unlock_state(state);
685 dev_dbg(sd->dev,
686 "path_enabled: can't find opposite route for %s:%u/%u",
687 sd->entity.name, sink_pad->index, sink_stream);
688 return 2;
689 }
690
691 source_pad = &sd->entity.pads[route->source_pad];
692 v4l2_subdev_unlock_state(state);
693
694 tmp_pad = sink_pad;
695 sink_pad = media_pad_remote_pad_unique(source_pad);
696 if (IS_ERR(sink_pad)) {
697 dev_dbg(sd->dev,
698 "path_enabled: can't find remote source for %s:%u\n",
699 source_pad->entity->name, source_pad->index);
700 return PTR_ERR(sink_pad);
701 }
702
703 if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
704 struct video_device *vdev;
705
706 vdev = media_entity_to_video_device(sink_pad->entity);
707 if (!vdev)
708 return -ENXIO;
709
710 dev_dbg(vdev->dev_parent,
711 "path_enabled: found video device %s\n",
712 vdev->name);
713
714 if (!*__sink_pad) {
715 *__sink_pad = tmp_pad;
716 dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
717 tmp_pad->index, sink_stream);
718 } else if (tmp_pad != *__sink_pad) {
719 dev_dbg(sd->dev,
720 "path_enabled: pads %s/%u and %s/%u differ\n",
721 tmp_pad->entity->name, tmp_pad->index,
722 (*__sink_pad)->entity->name,
723 (*__sink_pad)->index);
724 return -EXDEV;
725 }
726
727 *__sink_streams |= BIT_ULL(sink_stream);
728
729 return func(vdev);
730 }
731
732 return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
733 __sink_pad, __sink_streams);
734 }
735
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-19 8:15 ` [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled() Sakari Ailus
` (3 preceding siblings ...)
2025-06-23 9:48 ` kernel test robot
@ 2025-06-26 23:07 ` Laurent Pinchart
2025-08-04 11:32 ` Sakari Ailus
4 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2025-06-26 23:07 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen
Hi Sakari,
Thank you for the patch.
On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> lacked any sort of general solution: with multiple streams, when streaming
> is started on video nodes one by one, when should streaming be started in
> the source?
>
> v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> queries the streams generated by the source and traces them back to the
> video nodes.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
> drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> include/media/v4l2-mc.h | 44 ++++++
> 2 files changed, 287 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> index 937d358697e1..1731088ad436 100644
> --- a/drivers/media/v4l2-core/v4l2-mc.c
> +++ b/drivers/media/v4l2-core/v4l2-mc.c
> @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> return ret;
> }
> EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> +
> +static int
> +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> + struct media_pad *src_pad, u64 __src_streams,
> + struct media_pad **__sink_pad, u64 *__sink_streams)
> +{
> + struct v4l2_subdev_route *route;
> + u64 src_streams = 0, sink_streams = 0;
> + bool has_sink_pad = false;
> + unsigned int sink_pad;
> +
> + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> + state->sd->entity.name, src_pad->index, __src_streams);
> + for_each_active_route(&state->routing, route) {
> + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> + state->sd->entity.name,
> + route->sink_pad, route->sink_stream, route->source_pad,
> + route->source_stream, route->flags);
> + if (route->source_pad != src_pad->index)
> + continue;
> +
> + if (!(BIT_ULL(route->source_stream) & __src_streams))
> + continue;
> +
> + if (!has_sink_pad) {
> + has_sink_pad = true;
> + sink_pad = route->sink_pad;
> + }
> +
> + if (route->sink_pad != sink_pad) {
> + dev_dbg(state->sd->dev,
> + "sink pads (%u vs. %u) differ\n",
> + route->sink_pad, sink_pad);
> + return -EMLINK;
> + }
> +
> + sink_streams |= BIT_ULL(route->sink_stream);
> + src_streams |= BIT_ULL(route->source_stream);
> + }
> +
> + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> + *__sink_streams = sink_streams;
> +
> + return 0;
> +}
> +
> +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> + unsigned int sink_stream,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad,
> + u64 *__sink_streams)
> +{
> + struct v4l2_subdev_state *state;
> + struct v4l2_subdev_route *route;
> + struct v4l2_subdev *sd;
> + struct media_pad *source_pad, *tmp_pad;
> + u32 source_stream;
> +
> + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> + return -ENXIO;
> +
> + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> + sd->entity.name);
> +
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> + sink_stream, true, 0);
> + if (IS_ERR(route)) {
> + v4l2_subdev_unlock_state(state);
> + dev_dbg(sd->dev,
> + "path_enabled: can't find opposite route for %s:%u/%u",
> + sd->entity.name, sink_pad->index, sink_stream);
> + return 2;
> + }
> +
> + source_pad = &sd->entity.pads[route->source_pad];
> + v4l2_subdev_unlock_state(state);
> +
> + tmp_pad = sink_pad;
> + sink_pad = media_pad_remote_pad_unique(source_pad);
> + if (IS_ERR(sink_pad)) {
> + dev_dbg(sd->dev,
> + "path_enabled: can't find remote source for %s:%u\n",
> + source_pad->entity->name, source_pad->index);
> + return PTR_ERR(sink_pad);
> + }
> +
> + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> + struct video_device *vdev;
> +
> + vdev = media_entity_to_video_device(sink_pad->entity);
> + if (!vdev)
> + return -ENXIO;
> +
> + dev_dbg(vdev->dev_parent,
> + "path_enabled: found video device %s\n",
> + vdev->name);
> +
> + if (!*__sink_pad) {
> + *__sink_pad = tmp_pad;
> + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> + tmp_pad->index, sink_stream);
> + } else if (tmp_pad != *__sink_pad) {
> + dev_dbg(sd->dev,
> + "path_enabled: pads %s/%u and %s/%u differ\n",
> + tmp_pad->entity->name, tmp_pad->index,
> + (*__sink_pad)->entity->name,
> + (*__sink_pad)->index);
> + return -EXDEV;
> + }
> +
> + *__sink_streams |= BIT_ULL(sink_stream);
> +
> + return func(vdev);
> + }
> +
> + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> + __sink_pad, __sink_streams);
> +}
> +
> +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> + u64 *__streams)
> +{
> + struct v4l2_mbus_frame_desc desc;
> + u64 streams = 0;
> + int ret;
> +
> + if (!__streams)
> + return -EINVAL;
> +
> + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> + if (ret == -ENOIOCTLCMD) {
> + *__streams = 1ULL;
> + return 0;
> + }
> + if (ret)
> + return ret;
> +
> + for (unsigned int i = 0; i < desc.num_entries; i++) {
> + if (streams & BIT_ULL(desc.entry[i].stream))
> + return -EINVAL;
> +
> + streams |= BIT_ULL(desc.entry[i].stream);
> + }
> +
> + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> + *__streams, streams);
> + if (*__streams & ~streams)
> + return -EINVAL;
> +
> + *__streams = streams;
> +
> + return 0;
> +}
> +
> +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad, u64 *__sink_streams)
> +{
> + u64 sink_streams = 1U;
> + struct media_pad *src_pad;
> + u64 src_streams;
> + struct v4l2_subdev_state *state;
> + struct media_pad *sink_pad = vdev->entity.pads;
> + struct v4l2_subdev *sd = NULL;
> + bool streaming = true;
> + struct media_pad *tmp_pad;
> + u64 tmp_streams;
> + int ret;
> +
> + if (!__sink_pad)
> + __sink_pad = &tmp_pad;
> + if (!__sink_streams)
> + __sink_streams = &tmp_streams;
> + *__sink_pad = NULL;
> + *__sink_streams = 0;
> +
> + do {
> + src_pad = media_pad_remote_pad_unique(sink_pad);
> + if (IS_ERR(src_pad)) {
> + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> + "no unique remote pad found from %s:%u\n",
> + sink_pad->entity->name, sink_pad->index);
> + return PTR_ERR(src_pad);
> + }
> +
> + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> + if (!sd) {
> + dev_dbg(sd->dev,
> + "media entity %s is not a V4L2 sub-device\n",
> + src_pad->entity->name);
> + return -ENXIO;
> + }
> +
> + /* Source streams match sink. */
> + src_streams = sink_streams;
> +
> + state = v4l2_subdev_lock_and_get_active_state(sd);
> + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> + src_streams, &sink_pad,
> + &sink_streams);
> + v4l2_subdev_unlock_state(state);
> + if (ret)
> + return ret;
> + } while (sink_pad);
> +
> + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> + if (ret)
> + return ret;
> +
> + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> +
> + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> + src_pad->index, src_streams);
> +
> + for (unsigned int i = __ffs(src_streams); src_streams;
> + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> + sink_pad = media_pad_remote_pad_unique(src_pad);
> + if (IS_ERR(src_pad)) {
> + dev_dbg(sd->dev,
> + "no unique remote pad found from %s:%u\n",
> + sink_pad->entity->name, sink_pad->index);
> + return PTR_ERR(src_pad);
> + }
> +
> + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> + __sink_streams);
> + if (ret == 2)
> + continue;
> + if (ret < 0)
> + return ret;
> + if (!ret)
> + streaming = false;
> + }
> +
> + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> + (*__sink_pad)->index, *__sink_streams);
> +
> + return streaming;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> index 1837c9fd78cf..e72c0f62fa34 100644
> --- a/include/media/v4l2-mc.h
> +++ b/include/media/v4l2-mc.h
> @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> unsigned int notification);
>
> +/**
> + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> + * @vdev: The video device
> + * @func: Caller-provided function to tell a video device's streaming state
> + * @__sink_pad: sink pad at the root of the local pipeline
> + * @__sink_streams: streams to start
Any reason for the double underscore ?
> + *
> + * Use to tell whether streaming should start on a video node. @func returns
> + * true if streaming has been started on a given video node. @__sink_pad and
> + * @__sink_streams are filled with pad and streams on the sub-device closest to
> + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> + * v4l2_subdev_disable_streams().
> + *
> + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> + * a to-do list):
> + * * only unbranched streams can be supported albeit adding support for
> + * downstream branches would be fairly trivial,
I can't tell from the documentation here what you mean exactly by
"unbranched streams".
> + * * streams within a single source sub-device are considered to start at the
> + * same time, more control could be added in two ways: 1) for sources to
> + * determine stream starting, a control could be added to UAPI and 2) sources
> + * could tell which streams start at the same time using a sub-device
> + * operation,
> + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> + * be implemented by letting the caller to provide a function to determine
> + * which streams are of interest and
> + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> + * but this can also be rather trivially addressed.
I'm afraid this function looks like a hack, to solve a problem that is
not even explicitly described. You don't explain the issue in the cover
letter or in Documentation/, the cover letter merely states that this is
a "partial solution". The documentation of the function doesn't explain
what criteria the decision is based on. We need a proper explanation of
the problem in Documentation/, with a description of the behaviour (or
behaviours) drivers are expected to implement.
Furthermore, on the implementation side, things are fairly inefficient.
We already traverse the whole pipeline in media_pipeline_start(), based
on links and routes, and populate the media_pipeline structure. We
shouldn't do the same here, but instead inspect media_pipeline to
extract the information we need. If you're missing information there,
let's add it.
> + *
> + * Return:
> + * * 0: Success, but don't start streaming yet
> + * * 1: Success, now it's time to start streaming
> + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> + * * -ENOTUNIQ: No unique remote pad
> + * * -ENOLINK: No remote pad found
> + * * -ENOENT: Enabled upstream route not found
> + * * -EMLINK: No unique downstream route found
> + * * -EINVAL: Stream could not be followed to source or was not produced by
> + * the source
> + */
> +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> + bool (*func)(struct video_device *vdev),
> + struct media_pad **__sink_pad,
> + u64 *__sink_streams);
> +
> #else /* CONFIG_MEDIA_CONTROLLER */
>
> static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-06-26 23:07 ` Laurent Pinchart
@ 2025-08-04 11:32 ` Sakari Ailus
2025-08-04 11:46 ` Laurent Pinchart
0 siblings, 1 reply; 14+ messages in thread
From: Sakari Ailus @ 2025-08-04 11:32 UTC (permalink / raw)
To: Laurent Pinchart
Cc: linux-media, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen
Hi Laurent,
Thanks for the review.
On Fri, Jun 27, 2025 at 02:07:10AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
>
> Thank you for the patch.
>
> On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > lacked any sort of general solution: with multiple streams, when streaming
> > is started on video nodes one by one, when should streaming be started in
> > the source?
> >
> > v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> > queries the streams generated by the source and traces them back to the
> > video nodes.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> > drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> > include/media/v4l2-mc.h | 44 ++++++
> > 2 files changed, 287 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > index 937d358697e1..1731088ad436 100644
> > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > return ret;
> > }
> > EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> > +
> > +static int
> > +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> > + struct media_pad *src_pad, u64 __src_streams,
> > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > +{
> > + struct v4l2_subdev_route *route;
> > + u64 src_streams = 0, sink_streams = 0;
> > + bool has_sink_pad = false;
> > + unsigned int sink_pad;
> > +
> > + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> > + state->sd->entity.name, src_pad->index, __src_streams);
> > + for_each_active_route(&state->routing, route) {
> > + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> > + state->sd->entity.name,
> > + route->sink_pad, route->sink_stream, route->source_pad,
> > + route->source_stream, route->flags);
> > + if (route->source_pad != src_pad->index)
> > + continue;
> > +
> > + if (!(BIT_ULL(route->source_stream) & __src_streams))
> > + continue;
> > +
> > + if (!has_sink_pad) {
> > + has_sink_pad = true;
> > + sink_pad = route->sink_pad;
> > + }
> > +
> > + if (route->sink_pad != sink_pad) {
> > + dev_dbg(state->sd->dev,
> > + "sink pads (%u vs. %u) differ\n",
> > + route->sink_pad, sink_pad);
> > + return -EMLINK;
> > + }
> > +
> > + sink_streams |= BIT_ULL(route->sink_stream);
> > + src_streams |= BIT_ULL(route->source_stream);
> > + }
> > +
> > + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> > + *__sink_streams = sink_streams;
> > +
> > + return 0;
> > +}
> > +
> > +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> > + unsigned int sink_stream,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad,
> > + u64 *__sink_streams)
> > +{
> > + struct v4l2_subdev_state *state;
> > + struct v4l2_subdev_route *route;
> > + struct v4l2_subdev *sd;
> > + struct media_pad *source_pad, *tmp_pad;
> > + u32 source_stream;
> > +
> > + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> > + return -ENXIO;
> > +
> > + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> > + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> > + sd->entity.name);
> > +
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> > + sink_stream, true, 0);
> > + if (IS_ERR(route)) {
> > + v4l2_subdev_unlock_state(state);
> > + dev_dbg(sd->dev,
> > + "path_enabled: can't find opposite route for %s:%u/%u",
> > + sd->entity.name, sink_pad->index, sink_stream);
> > + return 2;
> > + }
> > +
> > + source_pad = &sd->entity.pads[route->source_pad];
> > + v4l2_subdev_unlock_state(state);
> > +
> > + tmp_pad = sink_pad;
> > + sink_pad = media_pad_remote_pad_unique(source_pad);
> > + if (IS_ERR(sink_pad)) {
> > + dev_dbg(sd->dev,
> > + "path_enabled: can't find remote source for %s:%u\n",
> > + source_pad->entity->name, source_pad->index);
> > + return PTR_ERR(sink_pad);
> > + }
> > +
> > + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> > + struct video_device *vdev;
> > +
> > + vdev = media_entity_to_video_device(sink_pad->entity);
> > + if (!vdev)
> > + return -ENXIO;
> > +
> > + dev_dbg(vdev->dev_parent,
> > + "path_enabled: found video device %s\n",
> > + vdev->name);
> > +
> > + if (!*__sink_pad) {
> > + *__sink_pad = tmp_pad;
> > + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> > + tmp_pad->index, sink_stream);
> > + } else if (tmp_pad != *__sink_pad) {
> > + dev_dbg(sd->dev,
> > + "path_enabled: pads %s/%u and %s/%u differ\n",
> > + tmp_pad->entity->name, tmp_pad->index,
> > + (*__sink_pad)->entity->name,
> > + (*__sink_pad)->index);
> > + return -EXDEV;
> > + }
> > +
> > + *__sink_streams |= BIT_ULL(sink_stream);
> > +
> > + return func(vdev);
> > + }
> > +
> > + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> > + __sink_pad, __sink_streams);
> > +}
> > +
> > +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> > + u64 *__streams)
> > +{
> > + struct v4l2_mbus_frame_desc desc;
> > + u64 streams = 0;
> > + int ret;
> > +
> > + if (!__streams)
> > + return -EINVAL;
> > +
> > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> > + if (ret == -ENOIOCTLCMD) {
> > + *__streams = 1ULL;
> > + return 0;
> > + }
> > + if (ret)
> > + return ret;
> > +
> > + for (unsigned int i = 0; i < desc.num_entries; i++) {
> > + if (streams & BIT_ULL(desc.entry[i].stream))
> > + return -EINVAL;
> > +
> > + streams |= BIT_ULL(desc.entry[i].stream);
> > + }
> > +
> > + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> > + *__streams, streams);
> > + if (*__streams & ~streams)
> > + return -EINVAL;
> > +
> > + *__streams = streams;
> > +
> > + return 0;
> > +}
> > +
> > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > +{
> > + u64 sink_streams = 1U;
> > + struct media_pad *src_pad;
> > + u64 src_streams;
> > + struct v4l2_subdev_state *state;
> > + struct media_pad *sink_pad = vdev->entity.pads;
> > + struct v4l2_subdev *sd = NULL;
> > + bool streaming = true;
> > + struct media_pad *tmp_pad;
> > + u64 tmp_streams;
> > + int ret;
> > +
> > + if (!__sink_pad)
> > + __sink_pad = &tmp_pad;
> > + if (!__sink_streams)
> > + __sink_streams = &tmp_streams;
> > + *__sink_pad = NULL;
> > + *__sink_streams = 0;
> > +
> > + do {
> > + src_pad = media_pad_remote_pad_unique(sink_pad);
> > + if (IS_ERR(src_pad)) {
> > + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> > + "no unique remote pad found from %s:%u\n",
> > + sink_pad->entity->name, sink_pad->index);
> > + return PTR_ERR(src_pad);
> > + }
> > +
> > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > + if (!sd) {
> > + dev_dbg(sd->dev,
> > + "media entity %s is not a V4L2 sub-device\n",
> > + src_pad->entity->name);
> > + return -ENXIO;
> > + }
> > +
> > + /* Source streams match sink. */
> > + src_streams = sink_streams;
> > +
> > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> > + src_streams, &sink_pad,
> > + &sink_streams);
> > + v4l2_subdev_unlock_state(state);
> > + if (ret)
> > + return ret;
> > + } while (sink_pad);
> > +
> > + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> > + if (ret)
> > + return ret;
> > +
> > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > +
> > + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> > + src_pad->index, src_streams);
> > +
> > + for (unsigned int i = __ffs(src_streams); src_streams;
> > + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> > + sink_pad = media_pad_remote_pad_unique(src_pad);
> > + if (IS_ERR(src_pad)) {
> > + dev_dbg(sd->dev,
> > + "no unique remote pad found from %s:%u\n",
> > + sink_pad->entity->name, sink_pad->index);
> > + return PTR_ERR(src_pad);
> > + }
> > +
> > + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> > + __sink_streams);
> > + if (ret == 2)
> > + continue;
> > + if (ret < 0)
> > + return ret;
> > + if (!ret)
> > + streaming = false;
> > + }
> > +
> > + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> > + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> > + (*__sink_pad)->index, *__sink_streams);
> > +
> > + return streaming;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> > diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> > index 1837c9fd78cf..e72c0f62fa34 100644
> > --- a/include/media/v4l2-mc.h
> > +++ b/include/media/v4l2-mc.h
> > @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> > int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > unsigned int notification);
> >
> > +/**
> > + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> > + * @vdev: The video device
> > + * @func: Caller-provided function to tell a video device's streaming state
> > + * @__sink_pad: sink pad at the root of the local pipeline
> > + * @__sink_streams: streams to start
>
> Any reason for the double underscore ?
The function used internally a variable without the underscores for a
different purpose.
>
> > + *
> > + * Use to tell whether streaming should start on a video node. @func returns
> > + * true if streaming has been started on a given video node. @__sink_pad and
> > + * @__sink_streams are filled with pad and streams on the sub-device closest to
> > + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> > + * v4l2_subdev_disable_streams().
> > + *
> > + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> > + * a to-do list):
> > + * * only unbranched streams can be supported albeit adding support for
> > + * downstream branches would be fairly trivial,
>
> I can't tell from the documentation here what you mean exactly by
> "unbranched streams".
These may have been referred to as "linear" streams elsewhere. I'll address
this in the next version.
>
> > + * * streams within a single source sub-device are considered to start at the
> > + * same time, more control could be added in two ways: 1) for sources to
> > + * determine stream starting, a control could be added to UAPI and 2) sources
> > + * could tell which streams start at the same time using a sub-device
> > + * operation,
> > + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> > + * be implemented by letting the caller to provide a function to determine
> > + * which streams are of interest and
> > + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> > + * but this can also be rather trivially addressed.
>
> I'm afraid this function looks like a hack, to solve a problem that is
> not even explicitly described. You don't explain the issue in the cover
> letter or in Documentation/, the cover letter merely states that this is
> a "partial solution". The documentation of the function doesn't explain
> what criteria the decision is based on. We need a proper explanation of
> the problem in Documentation/, with a description of the behaviour (or
> behaviours) drivers are expected to implement.
I believe this is discussed in the cover letter, not in detail though. I'm
fine with adding more documentation, there isn't much as I also wanted to
get feedback on the approach itself. No alternatives have been proposed so
far either.
>
> Furthermore, on the implementation side, things are fairly inefficient.
> We already traverse the whole pipeline in media_pipeline_start(), based
> on links and routes, and populate the media_pipeline structure. We
> shouldn't do the same here, but instead inspect media_pipeline to
> extract the information we need. If you're missing information there,
> let's add it.
The media pipeline is created and traversed, yes, but the media pipeline
does not include streams which are strictly a V4L2 concept. I agree there
is some overlap between the two but as long as MC remains separate from
V4L2, we can't reasonably access streams from the pipeline traversal.
>
> > + *
> > + * Return:
> > + * * 0: Success, but don't start streaming yet
> > + * * 1: Success, now it's time to start streaming
> > + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> > + * * -ENOTUNIQ: No unique remote pad
> > + * * -ENOLINK: No remote pad found
> > + * * -ENOENT: Enabled upstream route not found
> > + * * -EMLINK: No unique downstream route found
> > + * * -EINVAL: Stream could not be followed to source or was not produced by
> > + * the source
> > + */
> > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > + bool (*func)(struct video_device *vdev),
> > + struct media_pad **__sink_pad,
> > + u64 *__sink_streams);
> > +
> > #else /* CONFIG_MEDIA_CONTROLLER */
> >
> > static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
--
Kind regards,
Sakari Ailus
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH 12/13] media: v4l2-mc: Introduce v4l2_mc_pipeline_enabled()
2025-08-04 11:32 ` Sakari Ailus
@ 2025-08-04 11:46 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2025-08-04 11:46 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, bingbu.cao, stanislaw.gruszka, tian.shu.qiu,
tomi.valkeinen
On Mon, Aug 04, 2025 at 11:32:17AM +0000, Sakari Ailus wrote:
> On Fri, Jun 27, 2025 at 02:07:10AM +0300, Laurent Pinchart wrote:
> > On Thu, Jun 19, 2025 at 11:15:45AM +0300, Sakari Ailus wrote:
> > > v4l2_mc_pipeline_enabled() helps solving a problem known for long but
> > > lacked any sort of general solution: with multiple streams, when streaming
> > > is started on video nodes one by one, when should streaming be started in
> > > the source?
> > >
> > > v4l2_mc_pipeline_enabled() traverses the pipeline towards the source,
> > > queries the streams generated by the source and traces them back to the
> > > video nodes.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > > drivers/media/v4l2-core/v4l2-mc.c | 243 ++++++++++++++++++++++++++++++
> > > include/media/v4l2-mc.h | 44 ++++++
> > > 2 files changed, 287 insertions(+)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-mc.c b/drivers/media/v4l2-core/v4l2-mc.c
> > > index 937d358697e1..1731088ad436 100644
> > > --- a/drivers/media/v4l2-core/v4l2-mc.c
> > > +++ b/drivers/media/v4l2-core/v4l2-mc.c
> > > @@ -612,3 +612,246 @@ int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > > return ret;
> > > }
> > > EXPORT_SYMBOL_GPL(v4l2_pipeline_link_notify);
> > > +
> > > +static int
> > > +__v4l2_mc_pipeline_enabled(struct v4l2_subdev_state *state,
> > > + struct media_pad *src_pad, u64 __src_streams,
> > > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > > +{
> > > + struct v4l2_subdev_route *route;
> > > + u64 src_streams = 0, sink_streams = 0;
> > > + bool has_sink_pad = false;
> > > + unsigned int sink_pad;
> > > +
> > > + dev_dbg(state->sd->dev, "%s: source enabled, pad/streams %u/%#llx\n",
> > > + state->sd->entity.name, src_pad->index, __src_streams);
> > > + for_each_active_route(&state->routing, route) {
> > > + dev_dbg(state->sd->dev, "%s: %u/%u -> %u/%u, flags %x\n",
> > > + state->sd->entity.name,
> > > + route->sink_pad, route->sink_stream, route->source_pad,
> > > + route->source_stream, route->flags);
> > > + if (route->source_pad != src_pad->index)
> > > + continue;
> > > +
> > > + if (!(BIT_ULL(route->source_stream) & __src_streams))
> > > + continue;
> > > +
> > > + if (!has_sink_pad) {
> > > + has_sink_pad = true;
> > > + sink_pad = route->sink_pad;
> > > + }
> > > +
> > > + if (route->sink_pad != sink_pad) {
> > > + dev_dbg(state->sd->dev,
> > > + "sink pads (%u vs. %u) differ\n",
> > > + route->sink_pad, sink_pad);
> > > + return -EMLINK;
> > > + }
> > > +
> > > + sink_streams |= BIT_ULL(route->sink_stream);
> > > + src_streams |= BIT_ULL(route->source_stream);
> > > + }
> > > +
> > > + *__sink_pad = has_sink_pad ? &state->sd->entity.pads[sink_pad] : NULL;
> > > + *__sink_streams = sink_streams;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int v4l2_mc_downpath_enabled(struct media_pad *sink_pad,
> > > + unsigned int sink_stream,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad,
> > > + u64 *__sink_streams)
> > > +{
> > > + struct v4l2_subdev_state *state;
> > > + struct v4l2_subdev_route *route;
> > > + struct v4l2_subdev *sd;
> > > + struct media_pad *source_pad, *tmp_pad;
> > > + u32 source_stream;
> > > +
> > > + if (!is_media_entity_v4l2_subdev(sink_pad->entity))
> > > + return -ENXIO;
> > > +
> > > + sd = media_entity_to_v4l2_subdev(sink_pad->entity);
> > > + dev_dbg(sd->dev, "path_enabled: found sub-device %s\n",
> > > + sd->entity.name);
> > > +
> > > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > > + route = v4l2_subdev_find_route(&state->routing, sink_pad->index,
> > > + sink_stream, true, 0);
> > > + if (IS_ERR(route)) {
> > > + v4l2_subdev_unlock_state(state);
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: can't find opposite route for %s:%u/%u",
> > > + sd->entity.name, sink_pad->index, sink_stream);
> > > + return 2;
> > > + }
> > > +
> > > + source_pad = &sd->entity.pads[route->source_pad];
> > > + v4l2_subdev_unlock_state(state);
> > > +
> > > + tmp_pad = sink_pad;
> > > + sink_pad = media_pad_remote_pad_unique(source_pad);
> > > + if (IS_ERR(sink_pad)) {
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: can't find remote source for %s:%u\n",
> > > + source_pad->entity->name, source_pad->index);
> > > + return PTR_ERR(sink_pad);
> > > + }
> > > +
> > > + if (is_media_entity_v4l2_video_device(sink_pad->entity)) {
> > > + struct video_device *vdev;
> > > +
> > > + vdev = media_entity_to_video_device(sink_pad->entity);
> > > + if (!vdev)
> > > + return -ENXIO;
> > > +
> > > + dev_dbg(vdev->dev_parent,
> > > + "path_enabled: found video device %s\n",
> > > + vdev->name);
> > > +
> > > + if (!*__sink_pad) {
> > > + *__sink_pad = tmp_pad;
> > > + dev_dbg(sd->dev, "path_enabled: sink %u/%u\n",
> > > + tmp_pad->index, sink_stream);
> > > + } else if (tmp_pad != *__sink_pad) {
> > > + dev_dbg(sd->dev,
> > > + "path_enabled: pads %s/%u and %s/%u differ\n",
> > > + tmp_pad->entity->name, tmp_pad->index,
> > > + (*__sink_pad)->entity->name,
> > > + (*__sink_pad)->index);
> > > + return -EXDEV;
> > > + }
> > > +
> > > + *__sink_streams |= BIT_ULL(sink_stream);
> > > +
> > > + return func(vdev);
> > > + }
> > > +
> > > + return v4l2_mc_downpath_enabled(sink_pad, source_stream, func,
> > > + __sink_pad, __sink_streams);
> > > +}
> > > +
> > > +static int v4l2_mc_source_get_streams(struct v4l2_subdev *sd, unsigned int pad,
> > > + u64 *__streams)
> > > +{
> > > + struct v4l2_mbus_frame_desc desc;
> > > + u64 streams = 0;
> > > + int ret;
> > > +
> > > + if (!__streams)
> > > + return -EINVAL;
> > > +
> > > + ret = v4l2_subdev_call(sd, pad, get_frame_desc, pad, &desc);
> > > + if (ret == -ENOIOCTLCMD) {
> > > + *__streams = 1ULL;
> > > + return 0;
> > > + }
> > > + if (ret)
> > > + return ret;
> > > +
> > > + for (unsigned int i = 0; i < desc.num_entries; i++) {
> > > + if (streams & BIT_ULL(desc.entry[i].stream))
> > > + return -EINVAL;
> > > +
> > > + streams |= BIT_ULL(desc.entry[i].stream);
> > > + }
> > > +
> > > + dev_dbg(sd->dev, "found streams %#llx based on streams %#llx\n",
> > > + *__streams, streams);
> > > + if (*__streams & ~streams)
> > > + return -EINVAL;
> > > +
> > > + *__streams = streams;
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad, u64 *__sink_streams)
> > > +{
> > > + u64 sink_streams = 1U;
> > > + struct media_pad *src_pad;
> > > + u64 src_streams;
> > > + struct v4l2_subdev_state *state;
> > > + struct media_pad *sink_pad = vdev->entity.pads;
> > > + struct v4l2_subdev *sd = NULL;
> > > + bool streaming = true;
> > > + struct media_pad *tmp_pad;
> > > + u64 tmp_streams;
> > > + int ret;
> > > +
> > > + if (!__sink_pad)
> > > + __sink_pad = &tmp_pad;
> > > + if (!__sink_streams)
> > > + __sink_streams = &tmp_streams;
> > > + *__sink_pad = NULL;
> > > + *__sink_streams = 0;
> > > +
> > > + do {
> > > + src_pad = media_pad_remote_pad_unique(sink_pad);
> > > + if (IS_ERR(src_pad)) {
> > > + dev_dbg(sd ? sd->dev : vdev->dev_parent,
> > > + "no unique remote pad found from %s:%u\n",
> > > + sink_pad->entity->name, sink_pad->index);
> > > + return PTR_ERR(src_pad);
> > > + }
> > > +
> > > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > > + if (!sd) {
> > > + dev_dbg(sd->dev,
> > > + "media entity %s is not a V4L2 sub-device\n",
> > > + src_pad->entity->name);
> > > + return -ENXIO;
> > > + }
> > > +
> > > + /* Source streams match sink. */
> > > + src_streams = sink_streams;
> > > +
> > > + state = v4l2_subdev_lock_and_get_active_state(sd);
> > > + ret = __v4l2_mc_pipeline_enabled(state, src_pad,
> > > + src_streams, &sink_pad,
> > > + &sink_streams);
> > > + v4l2_subdev_unlock_state(state);
> > > + if (ret)
> > > + return ret;
> > > + } while (sink_pad);
> > > +
> > > + ret = v4l2_mc_source_get_streams(sd, src_pad->index, &src_streams);
> > > + if (ret)
> > > + return ret;
> > > +
> > > + sd = media_entity_to_v4l2_subdev(src_pad->entity);
> > > +
> > > + dev_dbg(sd->dev, "following %s:%u/%#llx\n", sd->entity.name,
> > > + src_pad->index, src_streams);
> > > +
> > > + for (unsigned int i = __ffs(src_streams); src_streams;
> > > + src_streams &= ~BIT_ULL(i), i = __ffs(src_streams)) {
> > > + sink_pad = media_pad_remote_pad_unique(src_pad);
> > > + if (IS_ERR(src_pad)) {
> > > + dev_dbg(sd->dev,
> > > + "no unique remote pad found from %s:%u\n",
> > > + sink_pad->entity->name, sink_pad->index);
> > > + return PTR_ERR(src_pad);
> > > + }
> > > +
> > > + ret = v4l2_mc_downpath_enabled(sink_pad, i, func, __sink_pad,
> > > + __sink_streams);
> > > + if (ret == 2)
> > > + continue;
> > > + if (ret < 0)
> > > + return ret;
> > > + if (!ret)
> > > + streaming = false;
> > > + }
> > > +
> > > + dev_dbg(media_entity_to_v4l2_subdev((*__sink_pad)->entity)->dev,
> > > + "sink pad %s:%u/%#llx\n", (*__sink_pad)->entity->name,
> > > + (*__sink_pad)->index, *__sink_streams);
> > > +
> > > + return streaming;
> > > +}
> > > +EXPORT_SYMBOL_GPL(v4l2_mc_pipeline_enabled);
> > > diff --git a/include/media/v4l2-mc.h b/include/media/v4l2-mc.h
> > > index 1837c9fd78cf..e72c0f62fa34 100644
> > > --- a/include/media/v4l2-mc.h
> > > +++ b/include/media/v4l2-mc.h
> > > @@ -193,6 +193,50 @@ void v4l2_pipeline_pm_put(struct media_entity *entity);
> > > int v4l2_pipeline_link_notify(struct media_link *link, u32 flags,
> > > unsigned int notification);
> > >
> > > +/**
> > > + * v4l2_mc_pipeline_enabled - Tell when to start streaming
> > > + * @vdev: The video device
> > > + * @func: Caller-provided function to tell a video device's streaming state
> > > + * @__sink_pad: sink pad at the root of the local pipeline
> > > + * @__sink_streams: streams to start
> >
> > Any reason for the double underscore ?
>
> The function used internally a variable without the underscores for a
> different purpose.
Then it's better to use the __ for internal variables instead of the
publicly documented API. Or rename internal variables so they don't
clash.
> > > + *
> > > + * Use to tell whether streaming should start on a video node. @func returns
> > > + * true if streaming has been started on a given video node. @__sink_pad and
> > > + * @__sink_streams are filled with pad and streams on the sub-device closest to
> > > + * the video nodes, to be used for calling v4l2_subdev_enable_streams() and
> > > + * v4l2_subdev_disable_streams().
> > > + *
> > > + * Using v4l2_mc_pipeline_enabled() has a few limitations currently (consider it
> > > + * a to-do list):
> > > + * * only unbranched streams can be supported albeit adding support for
> > > + * downstream branches would be fairly trivial,
> >
> > I can't tell from the documentation here what you mean exactly by
> > "unbranched streams".
>
> These may have been referred to as "linear" streams elsewhere. I'll address
> this in the next version.
>
> > > + * * streams within a single source sub-device are considered to start at the
> > > + * same time, more control could be added in two ways: 1) for sources to
> > > + * determine stream starting, a control could be added to UAPI and 2) sources
> > > + * could tell which streams start at the same time using a sub-device
> > > + * operation,
> > > + * * CSI-2 VC framing is ignored currently, but VC-based stream starting could
> > > + * be implemented by letting the caller to provide a function to determine
> > > + * which streams are of interest and
> > > + * * routes leading to nowhere are ignored, on some hardware this is a problem,
> > > + * but this can also be rather trivially addressed.
> >
> > I'm afraid this function looks like a hack, to solve a problem that is
> > not even explicitly described. You don't explain the issue in the cover
> > letter or in Documentation/, the cover letter merely states that this is
> > a "partial solution". The documentation of the function doesn't explain
> > what criteria the decision is based on. We need a proper explanation of
> > the problem in Documentation/, with a description of the behaviour (or
> > behaviours) drivers are expected to implement.
>
> I believe this is discussed in the cover letter, not in detail though. I'm
> fine with adding more documentation, there isn't much as I also wanted to
> get feedback on the approach itself.
But that's my point, I have trouble giving feedback on the approach if
you don't explain what problem you're trying to solve, and why you
picked this particular solution. It may be all clear in your head, but
it isn't for me.
> No alternatives have been proposed so
> far either.
>
> > Furthermore, on the implementation side, things are fairly inefficient.
> > We already traverse the whole pipeline in media_pipeline_start(), based
> > on links and routes, and populate the media_pipeline structure. We
> > shouldn't do the same here, but instead inspect media_pipeline to
> > extract the information we need. If you're missing information there,
> > let's add it.
>
> The media pipeline is created and traversed, yes, but the media pipeline
> does not include streams which are strictly a V4L2 concept. I agree there
> is some overlap between the two but as long as MC remains separate from
> V4L2, we can't reasonably access streams from the pipeline traversal.
>
> > > + *
> > > + * Return:
> > > + * * 0: Success, but don't start streaming yet
> > > + * * 1: Success, now it's time to start streaming
> > > + * * -ENXIO: Route traversal encountered a non-video device/sub-device entity
> > > + * * -ENOTUNIQ: No unique remote pad
> > > + * * -ENOLINK: No remote pad found
> > > + * * -ENOENT: Enabled upstream route not found
> > > + * * -EMLINK: No unique downstream route found
> > > + * * -EINVAL: Stream could not be followed to source or was not produced by
> > > + * the source
> > > + */
> > > +int v4l2_mc_pipeline_enabled(struct video_device *vdev,
> > > + bool (*func)(struct video_device *vdev),
> > > + struct media_pad **__sink_pad,
> > > + u64 *__sink_streams);
> > > +
> > > #else /* CONFIG_MEDIA_CONTROLLER */
> > >
> > > static inline int v4l2_mc_create_media_graph(struct media_device *mdev)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 14+ messages in thread