Hi Tomi,
Thank you for the patch.
On Wed, Aug 10, 2022 at 03:10:57PM +0300, Tomi Valkeinen wrote:
> With the upcoming stream related improvements to the pipelines, the
> pipelines are moved from media entities to media pads. As the drivers
> currently use the pipelines with the entity based model, moving the
> pipelines to pads will cause changes to the drivers.
>
> However, most of the uses of media pipelines are related to a video
> device (a DMA engine) with a single pad, and thus there's never a need
> to support multiple pads in these use cases. We can avoid pushing the
> complexities of the pad based model to the drivers by adding video
> device wrappers for the pipeline related functions.
>
> This patch adds a number of wrappers to media_pipeline functions, all of
> which take a video_device as a parameter (instead of a media_entity),
> and verify that there's just one pad.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/v4l2-dev.c | 61 ++++++++++++++++++++
> include/media/v4l2-dev.h | 90 ++++++++++++++++++++++++++++++
> 2 files changed, 151 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-dev.c b/drivers/media/v4l2-core/v4l2-dev.c
> index d00237ee4cae..7f933ff89fd4 100644
> --- a/drivers/media/v4l2-core/v4l2-dev.c
> +++ b/drivers/media/v4l2-core/v4l2-dev.c
> @@ -1095,6 +1095,67 @@ void video_unregister_device(struct video_device *vdev)
> }
> EXPORT_SYMBOL(video_unregister_device);
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +
> +__must_check int video_device_pipeline_start(struct video_device *vdev,
> + struct media_pipeline *pipe)
> +{
> + struct media_entity *entity = &vdev->entity;
> +
> + if (entity->num_pads != 1)
> + return -ENODEV;
> +
> + return media_pipeline_start(entity, pipe);
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_start);
> +
> +__must_check int __video_device_pipeline_start(struct video_device *vdev,
> + struct media_pipeline *pipe)
> +{
> + struct media_entity *entity = &vdev->entity;
> +
> + if (entity->num_pads != 1)
> + return -ENODEV;
> +
> + return __media_pipeline_start(entity, pipe);
> +}
> +EXPORT_SYMBOL_GPL(__video_device_pipeline_start);
> +
> +void video_device_pipeline_stop(struct video_device *vdev)
> +{
> + struct media_entity *entity = &vdev->entity;
> +
> + if (WARN_ON(entity->num_pads != 1))
> + return;
> +
> + return media_pipeline_stop(entity);
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline_stop);
> +
> +void __video_device_pipeline_stop(struct video_device *vdev)
> +{
> + struct media_entity *entity = &vdev->entity;
> +
> + if (WARN_ON(entity->num_pads != 1))
> + return;
> +
> + return __media_pipeline_stop(entity);
> +}
> +EXPORT_SYMBOL_GPL(__video_device_pipeline_stop);
> +
> +struct media_pipeline *video_device_pipeline(struct video_device *vdev)
> +{
> + struct media_entity *entity = &vdev->entity;
> +
> + if (WARN_ON(entity->num_pads != 1))
> + return NULL;
> +
> + return media_entity_pipeline(entity);
> +}
> +EXPORT_SYMBOL_GPL(video_device_pipeline);
> +
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +
> /*
> * Initialise video for linux
> */
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 5cf1edefb822..4ccc24f5918a 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -539,4 +539,94 @@ static inline int video_is_registered(struct video_device *vdev)
> return test_bit(V4L2_FL_REGISTERED, &vdev->flags);
> }
>
> +#if defined(CONFIG_MEDIA_CONTROLLER)
> +
> +/**
> + * video_device_pipeline_start - Mark a pipeline as streaming
> + * @vdev: Starting video device
> + * @pipe: Media pipeline to be assigned to all entities in the pipeline.
> + *
> + * Mark all entities connected to a given video device through enabled links,
> + * either directly or indirectly, as streaming. The given pipeline object is
> + * assigned to every entity in the pipeline and stored in the media_entity pipe
> + * field.
> + *
> + * Calls to this function can be nested, in which case the same number of
> + * video_device_pipeline_stop() calls will be required to stop streaming. The
> + * pipeline pointer must be identical for all nested calls to
> + * video_device_pipeline_start().
> + *
> + * The video device must contain a single pad.
> + *
> + * This is a convenience wrapper to media_pipeline_start().
s/wrapper to/wrapper around/ maybe. Same below.
> + */
> +__must_check int video_device_pipeline_start(struct video_device *vdev,
> + struct media_pipeline *pipe);
> +
> +/**
> + * __video_device_pipeline_start - Mark a pipeline as streaming
> + *
I'd drop the blank line as you don't have one for the previous function.
Same for stop.
Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> + * @vdev: Starting video device
> + * @pipe: Media pipeline to be assigned to all entities in the pipeline.
> + *
> + * ..note:: This is the non-locking version of video_device_pipeline_start()
> + *
> + * The video device must contain a single pad.
> + *
> + * This is a convenience wrapper to __media_pipeline_start().
> + */
> +__must_check int __video_device_pipeline_start(struct video_device *vdev,
> + struct media_pipeline *pipe);
> +
> +/**
> + * video_device_pipeline_stop - Mark a pipeline as not streaming
> + * @vdev: Starting video device
> + *
> + * Mark all entities connected to a given entity through enabled links, either
> + * directly or indirectly, as not streaming. The media_entity pipe field is
> + * reset to %NULL.
> + *
> + * If multiple calls to media_pipeline_start() have been made, the same
> + * number of calls to this function are required to mark the pipeline as not
> + * streaming.
> + *
> + * The video device must contain a single pad.
> + *
> + * This is a convenience wrapper to media_pipeline_stop().
> + */
> +void video_device_pipeline_stop(struct video_device *vdev);
> +
> +/**
> + * __video_device_pipeline_stop - Mark a pipeline as not streaming
> + *
> + * @vdev: Starting video device
> + *
> + * .. note:: This is the non-locking version of media_pipeline_stop()
> + *
> + * The video device must contain a single pad.
> + *
> + * This is a convenience wrapper to __media_pipeline_stop().
> + */
> +void __video_device_pipeline_stop(struct video_device *vdev);
> +
> +/**
> + * video_device_pipeline - Get the media pipeline a video device is part of
> + * @vdev: The video device
> + *
> + * This function returns the media pipeline that a video device has been
> + * associated with when constructing the pipeline with
> + * video_device_pipeline_start(). The pointer remains valid until
> + * video_device_pipeline_stop() is called.
> + *
> + * Return: The media_pipeline the video device is part of, or NULL if the video
> + * device is not part of any pipeline.
> + *
> + * The video device must contain a single pad.
> + *
> + * This is a convenience wrapper to media_entity_pipeline().
> + */
> +struct media_pipeline *video_device_pipeline(struct video_device *vdev);
> +
> +#endif /* CONFIG_MEDIA_CONTROLLER */
> +
> #endif /* _V4L2_DEV_H */
--
Regards,
Laurent Pinchart
