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@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.
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
