[v13,13/34] media: drivers: use video_device_pipeline_alloc_start()
Commit Message
Use video_device_pipeline_alloc_start() instead of manually
allocating/managing the media pipeline storage.
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
---
drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +-------------
drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 2 +-
.../media/platform/sunxi/sun6i-csi/sun6i_video.c | 2 +-
drivers/media/platform/ti/cal/cal-video.c | 2 +-
drivers/media/platform/ti/cal/cal.h | 1 -
5 files changed, 4 insertions(+), 17 deletions(-)
Comments
Hi Tomi,
Thank you for the patch.
On Wed, Aug 10, 2022 at 03:11:01PM +0300, Tomi Valkeinen wrote:
> Use video_device_pipeline_alloc_start() instead of manually
> allocating/managing the media pipeline storage.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> ---
> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +-------------
> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 2 +-
> .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 2 +-
> drivers/media/platform/ti/cal/cal-video.c | 2 +-
> drivers/media/platform/ti/cal/cal.h | 1 -
> 5 files changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> index 548067f19576..884875600231 100644
> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> @@ -1244,8 +1244,6 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>
> static int rvin_set_stream(struct rvin_dev *vin, int on)
> {
> - struct media_pipeline *pipe;
> - struct media_device *mdev;
> struct v4l2_subdev *sd;
> struct media_pad *pad;
> int ret;
> @@ -1273,17 +1271,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> if (ret)
> return ret;
>
> - /*
> - * The graph lock needs to be taken to protect concurrent
> - * starts of multiple VIN instances as they might share
> - * a common subdevice down the line and then should use
> - * the same pipe.
> - */
> - mdev = vin->vdev.entity.graph_obj.mdev;
> - mutex_lock(&mdev->graph_mutex);
> - pipe = media_entity_pipeline(&sd->entity) ? : &vin->vdev.pipe;
> - ret = __video_device_pipeline_start(&vin->vdev, pipe);
> - mutex_unlock(&mdev->graph_mutex);
> + ret = video_device_pipeline_alloc_start(&vin->vdev);
This doesn't look right, it will use different pipeline instances for
difference video devices, that's a change in behaviour.
> if (ret)
> return ret;
>
> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> index 17ad9a3caaa5..a3e826a755fc 100644
> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> @@ -266,7 +266,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
> goto err_clear_dma_queue;
> }
>
> - ret = video_device_pipeline_start(&csi->vdev, &csi->vdev.pipe);
What ? There is a pipe embedded in video_device ? Oh my, I didn't
realize how messed up the V4L2 core had become :-(
> + ret = video_device_pipeline_alloc_start(&csi->vdev);
> if (ret < 0)
> goto err_free_scratch_buffer;
>
> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> index de4c0d47240f..436922503ece 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_video.c
> @@ -141,7 +141,7 @@ static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
>
> video->sequence = 0;
>
> - ret = video_device_pipeline_start(&video->vdev, &video->vdev.pipe);
> + ret = video_device_pipeline_alloc_start(&video->vdev);
> if (ret < 0)
> goto clear_dma_queue;
>
> diff --git a/drivers/media/platform/ti/cal/cal-video.c b/drivers/media/platform/ti/cal/cal-video.c
> index ae29130df819..e8122e7ec944 100644
> --- a/drivers/media/platform/ti/cal/cal-video.c
> +++ b/drivers/media/platform/ti/cal/cal-video.c
> @@ -707,7 +707,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
> dma_addr_t addr;
> int ret;
>
> - ret = video_device_pipeline_start(&ctx->vdev, &ctx->phy->pipe);
> + ret = video_device_pipeline_alloc_start(&ctx->vdev);
> if (ret < 0) {
> ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
> goto error_release_buffers;
> diff --git a/drivers/media/platform/ti/cal/cal.h b/drivers/media/platform/ti/cal/cal.h
> index 61409ddced98..648cec43c3fc 100644
> --- a/drivers/media/platform/ti/cal/cal.h
> +++ b/drivers/media/platform/ti/cal/cal.h
> @@ -174,7 +174,6 @@ struct cal_camerarx {
> struct device_node *source_ep_node;
> struct device_node *source_node;
> struct v4l2_subdev *source;
> - struct media_pipeline pipe;
>
> struct v4l2_subdev subdev;
> struct media_pad pads[CAL_CAMERARX_NUM_PADS];
On 29/08/2022 20:00, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Wed, Aug 10, 2022 at 03:11:01PM +0300, Tomi Valkeinen wrote:
>> Use video_device_pipeline_alloc_start() instead of manually
>> allocating/managing the media pipeline storage.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
>> ---
>> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +-------------
>> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 2 +-
>> .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 2 +-
>> drivers/media/platform/ti/cal/cal-video.c | 2 +-
>> drivers/media/platform/ti/cal/cal.h | 1 -
>> 5 files changed, 4 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
>> index 548067f19576..884875600231 100644
>> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
>> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
>> @@ -1244,8 +1244,6 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
>>
>> static int rvin_set_stream(struct rvin_dev *vin, int on)
>> {
>> - struct media_pipeline *pipe;
>> - struct media_device *mdev;
>> struct v4l2_subdev *sd;
>> struct media_pad *pad;
>> int ret;
>> @@ -1273,17 +1271,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>> if (ret)
>> return ret;
>>
>> - /*
>> - * The graph lock needs to be taken to protect concurrent
>> - * starts of multiple VIN instances as they might share
>> - * a common subdevice down the line and then should use
>> - * the same pipe.
>> - */
>> - mdev = vin->vdev.entity.graph_obj.mdev;
>> - mutex_lock(&mdev->graph_mutex);
>> - pipe = media_entity_pipeline(&sd->entity) ? : &vin->vdev.pipe;
>> - ret = __video_device_pipeline_start(&vin->vdev, pipe);
>> - mutex_unlock(&mdev->graph_mutex);
>> + ret = video_device_pipeline_alloc_start(&vin->vdev);
>
> This doesn't look right, it will use different pipeline instances for
> difference video devices, that's a change in behaviour.
I hope not, but it's a bit difficult to be sure without having the board
and testing.
Afaics, the previous code uses the existing pipeline if such exists in
the first subdev (i.e. if media_pipeline_start has been called earlier
for another vdev), or if not, uses pipeline specific to the vdev.
This behavior should match the new one, although the operation
underneath is slightly different.
It's perhaps the name, video_device_pipeline_alloc_start, that confuses.
It doesn't indicate that it possibly uses an existing pipeline. But I
don't have any good idea for a better name.
video_device_pipeline_find_or_alloc_start? =) Well, it's not really
"finding" anything either...
>> if (ret)
>> return ret;
>>
>> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
>> index 17ad9a3caaa5..a3e826a755fc 100644
>> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
>> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
>> @@ -266,7 +266,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
>> goto err_clear_dma_queue;
>> }
>>
>> - ret = video_device_pipeline_start(&csi->vdev, &csi->vdev.pipe);
>
> What ? There is a pipe embedded in video_device ? Oh my, I didn't
> realize how messed up the V4L2 core had become :-(
That's what the current Renesas driver earlier in this patch also uses.
Didn't you write that code? =)
Tomi
Hi Tomi,
On Tue, Aug 30, 2022 at 02:20:21PM +0300, Tomi Valkeinen wrote:
> On 29/08/2022 20:00, Laurent Pinchart wrote:
> > On Wed, Aug 10, 2022 at 03:11:01PM +0300, Tomi Valkeinen wrote:
> >> Use video_device_pipeline_alloc_start() instead of manually
> >> allocating/managing the media pipeline storage.
> >>
> >> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
> >> ---
> >> drivers/media/platform/renesas/rcar-vin/rcar-dma.c | 14 +-------------
> >> drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c | 2 +-
> >> .../media/platform/sunxi/sun6i-csi/sun6i_video.c | 2 +-
> >> drivers/media/platform/ti/cal/cal-video.c | 2 +-
> >> drivers/media/platform/ti/cal/cal.h | 1 -
> >> 5 files changed, 4 insertions(+), 17 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> >> index 548067f19576..884875600231 100644
> >> --- a/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> >> +++ b/drivers/media/platform/renesas/rcar-vin/rcar-dma.c
> >> @@ -1244,8 +1244,6 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
> >>
> >> static int rvin_set_stream(struct rvin_dev *vin, int on)
> >> {
> >> - struct media_pipeline *pipe;
> >> - struct media_device *mdev;
> >> struct v4l2_subdev *sd;
> >> struct media_pad *pad;
> >> int ret;
> >> @@ -1273,17 +1271,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
> >> if (ret)
> >> return ret;
> >>
> >> - /*
> >> - * The graph lock needs to be taken to protect concurrent
> >> - * starts of multiple VIN instances as they might share
> >> - * a common subdevice down the line and then should use
> >> - * the same pipe.
> >> - */
> >> - mdev = vin->vdev.entity.graph_obj.mdev;
> >> - mutex_lock(&mdev->graph_mutex);
> >> - pipe = media_entity_pipeline(&sd->entity) ? : &vin->vdev.pipe;
> >> - ret = __video_device_pipeline_start(&vin->vdev, pipe);
> >> - mutex_unlock(&mdev->graph_mutex);
> >> + ret = video_device_pipeline_alloc_start(&vin->vdev);
> >
> > This doesn't look right, it will use different pipeline instances for
> > difference video devices, that's a change in behaviour.
>
> I hope not, but it's a bit difficult to be sure without having the board
> and testing.
>
> Afaics, the previous code uses the existing pipeline if such exists in
> the first subdev (i.e. if media_pipeline_start has been called earlier
> for another vdev), or if not, uses pipeline specific to the vdev.
>
> This behavior should match the new one, although the operation
> underneath is slightly different.
I think you're right. I'd probably feel better if we could test it :-)
> It's perhaps the name, video_device_pipeline_alloc_start, that confuses.
> It doesn't indicate that it possibly uses an existing pipeline. But I
> don't have any good idea for a better name.
> video_device_pipeline_find_or_alloc_start? =) Well, it's not really
> "finding" anything either...
Yes, I think the new threw me off, but I can't really think of a better
one right now.
> >> if (ret)
> >> return ret;
> >>
> >> diff --git a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> >> index 17ad9a3caaa5..a3e826a755fc 100644
> >> --- a/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> >> +++ b/drivers/media/platform/sunxi/sun4i-csi/sun4i_dma.c
> >> @@ -266,7 +266,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
> >> goto err_clear_dma_queue;
> >> }
> >>
> >> - ret = video_device_pipeline_start(&csi->vdev, &csi->vdev.pipe);
> >
> > What ? There is a pipe embedded in video_device ? Oh my, I didn't
> > realize how messed up the V4L2 core had become :-(
>
> That's what the current Renesas driver earlier in this patch also uses.
> Didn't you write that code? =)
As a matter of fact, no, I didn't :-)
@@ -1244,8 +1244,6 @@ static int rvin_mc_validate_format(struct rvin_dev *vin, struct v4l2_subdev *sd,
static int rvin_set_stream(struct rvin_dev *vin, int on)
{
- struct media_pipeline *pipe;
- struct media_device *mdev;
struct v4l2_subdev *sd;
struct media_pad *pad;
int ret;
@@ -1273,17 +1271,7 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
if (ret)
return ret;
- /*
- * The graph lock needs to be taken to protect concurrent
- * starts of multiple VIN instances as they might share
- * a common subdevice down the line and then should use
- * the same pipe.
- */
- mdev = vin->vdev.entity.graph_obj.mdev;
- mutex_lock(&mdev->graph_mutex);
- pipe = media_entity_pipeline(&sd->entity) ? : &vin->vdev.pipe;
- ret = __video_device_pipeline_start(&vin->vdev, pipe);
- mutex_unlock(&mdev->graph_mutex);
+ ret = video_device_pipeline_alloc_start(&vin->vdev);
if (ret)
return ret;
@@ -266,7 +266,7 @@ static int sun4i_csi_start_streaming(struct vb2_queue *vq, unsigned int count)
goto err_clear_dma_queue;
}
- ret = video_device_pipeline_start(&csi->vdev, &csi->vdev.pipe);
+ ret = video_device_pipeline_alloc_start(&csi->vdev);
if (ret < 0)
goto err_free_scratch_buffer;
@@ -141,7 +141,7 @@ static int sun6i_video_start_streaming(struct vb2_queue *vq, unsigned int count)
video->sequence = 0;
- ret = video_device_pipeline_start(&video->vdev, &video->vdev.pipe);
+ ret = video_device_pipeline_alloc_start(&video->vdev);
if (ret < 0)
goto clear_dma_queue;
@@ -707,7 +707,7 @@ static int cal_start_streaming(struct vb2_queue *vq, unsigned int count)
dma_addr_t addr;
int ret;
- ret = video_device_pipeline_start(&ctx->vdev, &ctx->phy->pipe);
+ ret = video_device_pipeline_alloc_start(&ctx->vdev);
if (ret < 0) {
ctx_err(ctx, "Failed to start media pipeline: %d\n", ret);
goto error_release_buffers;
@@ -174,7 +174,6 @@ struct cal_camerarx {
struct device_node *source_ep_node;
struct device_node *source_node;
struct v4l2_subdev *source;
- struct media_pipeline pipe;
struct v4l2_subdev subdev;
struct media_pad pads[CAL_CAMERARX_NUM_PADS];