Re: [PATCH v2 7/7] [DNI] media: renesas: vsp1: Validate all links through .link_validate()

Tomi Valkeinen <tomi.valkeinen@xxxxxxxxxxxxxxxx> · Mon, 26 Aug 2024 15:36:10 +0300

On 26/08/2024 15:25, Laurent Pinchart wrote:
On Mon, Aug 26, 2024 at 03:22:02PM +0300, Tomi Valkeinen wrote:
On 26/08/2024 15:18, Laurent Pinchart wrote:
Hi Tomi,

On Mon, Aug 26, 2024 at 02:43:47PM +0300, Tomi Valkeinen wrote:
On 22/08/2024 18:45, Laurent Pinchart wrote:
Move validation of the links between video devices and subdevs,
performed manually in vsp1_video_streamon(), to the video device
.link_validate() handler.

This is how drivers should be implemented, but sadly, doing so for the
vsp1 driver could break userspace, introducing a regression. This patch
serves as an example to showcase usage of the .link_validate()
operation, but should not be merged.

Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
---
    .../media/platform/renesas/vsp1/vsp1_video.c  | 98 +++++++------------
    1 file changed, 37 insertions(+), 61 deletions(-)

diff --git a/drivers/media/platform/renesas/vsp1/vsp1_video.c b/drivers/media/platform/renesas/vsp1/vsp1_video.c
index e728f9f5160e..14575698bbe7 100644
--- a/drivers/media/platform/renesas/vsp1/vsp1_video.c
+++ b/drivers/media/platform/renesas/vsp1/vsp1_video.c
@@ -45,51 +45,6 @@
     * Helper functions
     */
    
-static struct v4l2_subdev *
-vsp1_video_remote_subdev(struct media_pad *local, u32 *pad)
-{
-	struct media_pad *remote;
-
-	remote = media_pad_remote_pad_first(local);
-	if (!remote || !is_media_entity_v4l2_subdev(remote->entity))
-		return NULL;
-
-	if (pad)
-		*pad = remote->index;
-
-	return media_entity_to_v4l2_subdev(remote->entity);
-}
-
-static int vsp1_video_verify_format(struct vsp1_video *video)
-{
-	struct v4l2_subdev_format fmt = {
-		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
-	};
-	struct v4l2_subdev *subdev;
-	int ret;
-
-	subdev = vsp1_video_remote_subdev(&video->pad, &fmt.pad);
-	if (subdev == NULL)
-		return -EINVAL;
-
-	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
-	if (ret < 0)
-		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
-
-	if (video->rwpf->fmtinfo->mbus != fmt.format.code ||
-	    video->rwpf->format.height != fmt.format.height ||
-	    video->rwpf->format.width != fmt.format.width) {
-		dev_dbg(video->vsp1->dev,
-			"Format mismatch: 0x%04x/%ux%u != 0x%04x/%ux%u\n",
-			video->rwpf->fmtinfo->mbus, video->rwpf->format.width,
-			video->rwpf->format.height, fmt.format.code,
-			fmt.format.width, fmt.format.height);
-		return -EPIPE;
-	}
-
-	return 0;
-}
-
    static int __vsp1_video_try_format(struct vsp1_video *video,
    				   struct v4l2_pix_format_mplane *pix,
    				   const struct vsp1_format_info **fmtinfo)
@@ -991,14 +946,6 @@ vsp1_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
    
    	mutex_unlock(&mdev->graph_mutex);
    
-	/*
-	 * Verify that the configured format matches the output of the connected
-	 * subdev.
-	 */
-	ret = vsp1_video_verify_format(video);
-	if (ret < 0)
-		goto err_stop;
-
    	/* Start the queue. */
    	ret = vb2_streamon(&video->queue, type);
    	if (ret < 0)
@@ -1087,14 +1034,43 @@ static const struct v4l2_file_operations vsp1_video_fops = {
    
    static int vsp1_video_link_validate(struct media_link *link)
    {
-	/*
-	 * Ideally, link validation should be implemented here instead of
-	 * calling vsp1_video_verify_format() in vsp1_video_streamon()
-	 * manually. That would however break userspace that start one video
-	 * device before configures formats on other video devices in the
-	 * pipeline. This operation is just a no-op to silence the warnings
-	 * from v4l2_subdev_link_validate().
-	 */
+	struct v4l2_subdev_format fmt = {
+		.which = V4L2_SUBDEV_FORMAT_ACTIVE,
+	};
+	struct v4l2_subdev *subdev;
+	struct media_entity *entity;
+	struct media_pad *remote;
+	struct vsp1_video *video;
+	int ret;
+
+	if (is_media_entity_v4l2_video_device(link->source->entity)) {
+		entity = link->source->entity;
+		remote = link->sink;
+	} else {
+		entity = link->sink->entity;
+		remote = link->source;
+	}

This looks a bit odd. So this device can be either a source and a sink?

Correct. The VSP has both capture and output video devices, and this
helper function is used for both.

This made me also wonder about the .link_validate(). It's the only
media_entity_operations op that does not get the media_entity as a
parameter. Which here means the driver has to go and "guess" whether it
is the source or the sink of the given link.

I wonder if there's a reason why .link_validate() doesn't have the
media_entity parameter?

Because it validates a link. Which of the sink or source entity would
you pass to the function ?

The one where the op is defined.

With the exception of links from video output devices to subdevs, it's
always the sink. I don't expect most drivers to use a single validation
function for the capture and output video devices, so I don't think
passing the entity pointer is that crucial. We could however improve
this on top in case it ends up being a common use case.

It's an issue even if the driver only acts as a sink or source, but I 
agree, not crucial at all.

Looking at the patch 1 of this series:

+static int isc_link_validate(struct media_link *link)
 {
+	struct video_device *vdev =
+		media_entity_to_video_device(link->sink->entity);

that should be:

static int isc_link_validate(struct media_entity *ent, struct media_link 
*link)
{
	struct video_device *vdev = media_entity_to_video_device(ent);

In any case, that's a topic for another series =).

 Tomi








