| Message ID | 20240822154531.25912-8-laurent.pinchart+renesas@ideasonboard.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | media: v4l2: Improve media link validation | expand |
Hi, 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? 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? > + > + 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? =) Fwiw: Reviewed-by: Tomi Valkeinen <tomi.valkeinen+renesas@ideasonboard.com> Tomi
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 ? > > + > > + 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>
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> >
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. > >>> + > >>> + 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>
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
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; + } + + 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; + } + return 0; }
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(-)