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.
> >>> +
> >>> + 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@xxxxxxxxxxxxxxxx>
--
Regards,
Laurent Pinchart
