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

[Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>]

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@ideasonboard.com>
>>>>> ---
>>>>>     .../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