Hi Jacopo
On 11/07/2025 14:39, Jacopo Mondi wrote:
Hi Dan
On Fri, Jul 11, 2025 at 01:43:16PM +0100, Dan Scally wrote:
On 11/07/2025 12:51, Dan Scally wrote:
Hi Jacopo - thanks for the comments
On 08/07/2025 14:10, Jacopo Mondi wrote:
Hi Dan
On Fri, Jul 04, 2025 at 12:20:19PM +0100, Daniel Scally wrote:
Add helpers to run the new media_pipeline_started() and
media_pipeline_stopped() functions. The helpers iterate over the
entities in the pipeline and count the number of video devices and
compare that to the pipeline's start_count() before acting. This
allows us to only run the media pipeline callbacks in the event that
the pipeline has had video_pipeline_start() called for each video
device.
Signed-off-by: Daniel Scally <dan.scally@xxxxxxxxxxxxxxxx>
---
We could take this further perhaps and include the equivalent routine
in video_device_pipeline[_alloc]_start()...if none of the entities
involved have .pipeline_started() or .pipeline_stopped() operations it
should be harmless, but I'm a bit reluctant to force the choice to run
those operations on users.
I know I've kind of suggested that, but after all I don't think it's a
very good idea, having this in two steps is probably better. And I
like the fact the v4l2-dev layer operates on the video device counting
and only relies on the mc layer for the callbacks notification.
Yeah me too. Let's stick to this
Changes in v2:
- Adapted now media_pipeline_for_each_entity() takes an iter
variable
- Fixed the Return: section of the kerneldoc comments
---
drivers/media/v4l2-core/v4l2-dev.c | 57 ++++++++++++++++++++++++++++++++++++++
include/media/v4l2-dev.h | 36 ++++++++++++++++++++++++
2 files changed, 93 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
index c369235113d98ae26c30a1aa386e7d60d541a66e..f3309f8349664f7296a95216a40dd9d9baae8d9e 100644
--- a/drivers/media/v4l2-core/v4l2-dev.c
+++ b/drivers/media/v4l2-core/v4l2-dev.c
@@ -1200,6 +1200,63 @@ struct media_pipeline *video_device_pipeline(struct video_device *vdev)
}
EXPORT_SYMBOL_GPL(video_device_pipeline);
+static int __video_device_pipeline_started(struct media_pipeline *pipe)
__function_name() is usually reserved for the non-locking version of
function_name().
This seems to be an helper only used internally by
video_device_pipeline_started() so I would use a different name
something like video_device_has_pipeline_started() ?
What it does is count the number of _unstarted_ video
devices..."video_device_pipeline_unstarted_vdevs()"?
+{
+ struct media_pipeline_entity_iter iter;
+ unsigned int n_video_devices = 0;
+ struct media_entity *entity;
+ int ret;
+
+ ret = media_pipeline_entity_iter_init(pipe, &iter);
+ if (ret)
+ return ret;
+
+ media_pipeline_for_each_entity(pipe, &iter, entity) {
+ if (entity->obj_type == MEDIA_ENTITY_TYPE_VIDEO_DEVICE)
+ n_video_devices++;
+ }
+
+ media_pipeline_entity_iter_cleanup(&iter);
+
+ return n_video_devices - pipe->start_count;
+}
+
+int video_device_pipeline_started(struct video_device *vdev)
+{
+ struct media_pipeline *pipe;
+ int ret;
+
+ pipe = video_device_pipeline(vdev);
+ if (!pipe)
+ return -ENODEV;
+
+ ret = __video_device_pipeline_started(pipe);
+ if (ret)
+ return ret;
I would not return ret, as it might take random values betwen
n_video_devices and 1. See below on the return value documentation
But we need to be able to signal to the driver three states:
1. No errors, but there are still unstarted video devices
2. No errors and there are no unstarted video devices
3. An error
So I expect a driver to do a two stage check:
ret = video_device_pipeline_started(vdev);
if (ret < 0)
goto err_out;
if (ret == 0)
// something appropriate here like run the media jobs scheduler
Sorry: I had a reading comprehension failure. You were suggesting to use
-ENODEV to signal that there are unstarted video devices remaining. I
understand now, but I'm still not sure about it, because then instead of the
"if (ret == 0) check here we'd have "if (ret == -ENODEV)", which I don't
especially like...or am I missing some way to avoid having that check here?
Yes, that would require drivers to check for -ENODEV to identify a
non-failure case when not all devices have started..
You're right it might not be optimal. With you implementation we
should then have
0: success
< 0: error
> 0: not all devices started
?
Yes
I might be actually ok with this, but could you please document it as
I've below suggested for clarity ?
Yep, will do
Thanks
j
Thanks
Dan
+
+ return media_pipeline_started(pipe);
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_started);
+
+int video_device_pipeline_stopped(struct video_device *vdev)
+{
+ struct media_pipeline *pipe;
+ int ret;
+
+ pipe = video_device_pipeline(vdev);
+ if (!pipe)
+ return -ENODEV;
+
+ ret = __video_device_pipeline_started(pipe);
+ if (ret)
+ return ret;
ditto
+
+ media_pipeline_stopped(pipe);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(video_device_pipeline_stopped);
+
#endif /* CONFIG_MEDIA_CONTROLLER */
/*
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 1b6222fab24eda96cbe459b435431c01f7259366..26b4a491024701ef47320aec6a1a680149ba4fc3 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -654,6 +654,42 @@ __must_check int video_device_pipeline_alloc_start(struct video_device *vdev);
*/
struct media_pipeline *video_device_pipeline(struct video_device *vdev);
+/**
+ * video_device_pipeline_started - Run the pipeline_started() entity operation
+ * for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
I would use the same text as the one from video_device_pipeline_start()
" connected to a given video device through enabled links, either
directly or indirectly,"
Ack
+ * streaming through the use of video_device_pipeline_start() or one of its
+ * equivalent functions. If so, media_pipeline_started() is called to inform
+ * entities in the pipeline of that fact. The intention is to provide drivers
+ * with a shortcut for checking whether their pipeline is fully ready to start
+ * processing data.
Not really a shortcut, I would use "mechanism" instead.
I would also specify that:
* entities in the pipeline of that fact. The intention is to provide drivers
* with a mechanism for checking whether their pipeline is fully ready to start
* processing data and call the .pipeline_started() media entity operation
* on all the entities in the pipeline.
Ack!
+ *
+ * Return: The number of video devices in the pipeline remaining to be started,
+ * or a negative error number on failure.
0 for success as well
I would anyway return 0 for success and a specific error code for the
three failure cases:
-ENOMEM if allocating the iterator fails
-ENODEV if not all video devices have started
-EINVAL if media_pipeline_started() fails
You can document them as (copying from iommu.h)
* Return:
* * 0 - success
* * EINVAL - call to pipeline_started() failed
* * ENOMEM - failed to allocate pipe iterator
* * ENODEV - pipeline not yet fully started
+ */
+int video_device_pipeline_started(struct video_device *vdev);
+
+/**
+ * video_device_pipeline_stopped - Run the pipeline_stopped() entity operation
+ * for a fully-started media pipeline
+ * @vdev: A video device that's part of the pipeline
+ *
+ * This function checks whether all MEDIA_ENTITY_TYPE_VIDEO_DEVICE entities
+ * connected to a given video device through enabled links have been marked as
+ * streaming through the use of video_device_pipeline_start() or one of its
What is the intended semantic here ? The first video device to receive
a streamoff() will trigger media_pipeline_stopped() or should the last
one do that ?
The first one should do it, so the first device caling stop should
trigger actual stop in all involved hardware.
+ * equivalent functions. If so, media_pipeline_stopped() is called for each
+ * entity in the pipeline. The intention is to provide drivers with a shortcut
+ * for checking whether this video device is the first device in the pipeline
+ * to be stopped.
+ *
+ * Return: The number of video devices in the pipeline remaining to be started, or a
+ * negative error number on failure.
+ */
+int video_device_pipeline_stopped(struct video_device *vdev);
+
#endif /* CONFIG_MEDIA_CONTROLLER */
#endif /* _V4L2_DEV_H */
--
2.34.1
