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@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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 :-)
--
Regards,
Laurent Pinchart
