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

  Tomi

>>> +
>>> +	fmt.pad = remote->index;
>>> +
>>> +	subdev = media_entity_to_v4l2_subdev(remote->entity);
>>> +	ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
>>> +	if (ret < 0)
>>> +		return ret == -ENOIOCTLCMD ? -EINVAL : ret;
>>> +
>>> +	video = to_vsp1_video(media_entity_to_video_device(entity));
>>> +
>>> +	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;
>>> +	}
>>
>> Why don't we have a common videodev state which could be used to do
>> these validations in a common function? =)
> 
> Because you haven't sent patches yet ;-)
> 
> But jokes aside, because there's no 1:1 mapping between media bus codes
> and pixel formats, so drivers have to validate at least that part.
> 
>> Fwiw:
>> Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com>
>