From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Implement the `.link_validate()` callback for the video node and move the
format checking into this function. This change allows the removal of
`rzg2l_cru_mc_validate_format()`.
Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
.../platform/renesas/rzg2l-cru/rzg2l-video.c | 91 ++++++++++---------
1 file changed, 47 insertions(+), 44 deletions(-)
diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
index ceb9012c9d70..385b4242db2f 100644
--- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
+++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c
@@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb)
spin_unlock_irqrestore(&cru->qlock, flags);
}
-static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru,
- struct v4l2_subdev *sd,
- struct media_pad *pad)
-{
- struct v4l2_subdev_format fmt = {
- .which = V4L2_SUBDEV_FORMAT_ACTIVE,
- };
-
- fmt.pad = pad->index;
- if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt))
- return -EPIPE;
-
- switch (fmt.format.code) {
- case MEDIA_BUS_FMT_UYVY8_1X16:
- break;
- default:
- return -EPIPE;
- }
-
- switch (fmt.format.field) {
- case V4L2_FIELD_TOP:
- case V4L2_FIELD_BOTTOM:
- case V4L2_FIELD_NONE:
- case V4L2_FIELD_INTERLACED_TB:
- case V4L2_FIELD_INTERLACED_BT:
- case V4L2_FIELD_INTERLACED:
- case V4L2_FIELD_SEQ_TB:
- case V4L2_FIELD_SEQ_BT:
- break;
- default:
- return -EPIPE;
- }
-
- if (fmt.format.width != cru->format.width ||
- fmt.format.height != cru->format.height)
- return -EPIPE;
-
- return 0;
-}
-
static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru,
int slot, dma_addr_t addr)
{
@@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on)
return stream_off_ret;
}
- ret = rzg2l_cru_mc_validate_format(cru, sd, pad);
- if (ret)
- return ret;
-
pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe;
ret = video_device_pipeline_start(&cru->vdev, pipe);
if (ret)
@@ -995,6 +951,52 @@ static const struct v4l2_file_operations rzg2l_cru_fops = {
.read = vb2_fop_read,
};
+/* -----------------------------------------------------------------------------
+ * Media entity operations
+ */
+
+static int rzg2l_cru_video_link_validate(struct media_link *link)
+{
+ struct v4l2_subdev_format fmt = {
+ .which = V4L2_SUBDEV_FORMAT_ACTIVE,
+ };
+ const struct rzg2l_cru_ip_format *video_fmt;
+ const struct rzg2l_cru_ip_format *ip_fmt;
+ struct v4l2_subdev *subdev;
+ struct rzg2l_cru_dev *cru;
+ struct media_pad *remote;
+ int ret;
+
+ remote = link->source;
+ subdev = media_entity_to_v4l2_subdev(remote->entity);
+ fmt.pad = remote->index;
+ ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt);
+ if (ret < 0)
+ return ret == -ENOIOCTLCMD ? -EINVAL : ret;
+
+ cru = container_of(media_entity_to_video_device(link->sink->entity),
+ struct rzg2l_cru_dev, vdev);
+ video_fmt = rzg2l_cru_ip_format_to_fmt(cru->format.pixelformat);
+ if (!video_fmt)
+ return -EPIPE;
+ ip_fmt = rzg2l_cru_ip_code_to_fmt(fmt.format.code);
+ if (!ip_fmt)
+ return -EPIPE;
+
+ if (fmt.format.width != cru->format.width ||
+ fmt.format.height != cru->format.height ||
+ fmt.format.field != cru->format.field ||
+ video_fmt->code != fmt.format.code ||
+ ip_fmt->format != cru->format.pixelformat)
+ return -EPIPE;
+
+ return 0;
+}
+
+static const struct media_entity_operations rzg2l_cru_video_media_ops = {
+ .link_validate = rzg2l_cru_video_link_validate,
+};
+
static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
{
struct video_device *vdev = &cru->vdev;
@@ -1006,6 +1008,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru)
vdev->lock = &cru->lock;
vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING;
vdev->device_caps |= V4L2_CAP_IO_MC;
+ vdev->entity.ops = &rzg2l_cru_video_media_ops;
vdev->fops = &rzg2l_cru_fops;
vdev->ioctl_ops = &rzg2l_cru_ioctl_ops;
--
2.43.0
Hi Prabhakar, Thank you for the patch. On Mon, Oct 07, 2024 at 07:48:35PM +0100, Prabhakar wrote: > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > Implement the `.link_validate()` callback for the video node and move the > format checking into this function. This change allows the removal of > `rzg2l_cru_mc_validate_format()`. > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > --- > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 91 ++++++++++--------- > 1 file changed, 47 insertions(+), 44 deletions(-) > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > index ceb9012c9d70..385b4242db2f 100644 > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb) > spin_unlock_irqrestore(&cru->qlock, flags); > } > > -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, > - struct v4l2_subdev *sd, > - struct media_pad *pad) > -{ > - struct v4l2_subdev_format fmt = { > - .which = V4L2_SUBDEV_FORMAT_ACTIVE, > - }; > - > - fmt.pad = pad->index; > - if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt)) > - return -EPIPE; > - > - switch (fmt.format.code) { > - case MEDIA_BUS_FMT_UYVY8_1X16: > - break; > - default: > - return -EPIPE; > - } > - > - switch (fmt.format.field) { > - case V4L2_FIELD_TOP: > - case V4L2_FIELD_BOTTOM: > - case V4L2_FIELD_NONE: > - case V4L2_FIELD_INTERLACED_TB: > - case V4L2_FIELD_INTERLACED_BT: > - case V4L2_FIELD_INTERLACED: > - case V4L2_FIELD_SEQ_TB: > - case V4L2_FIELD_SEQ_BT: > - break; > - default: > - return -EPIPE; > - } > - > - if (fmt.format.width != cru->format.width || > - fmt.format.height != cru->format.height) > - return -EPIPE; > - > - return 0; > -} > - > static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru, > int slot, dma_addr_t addr) > { > @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > return stream_off_ret; > } > > - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > - if (ret) > - return ret; > - > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > ret = video_device_pipeline_start(&cru->vdev, pipe); > if (ret) > @@ -995,6 +951,52 @@ static const struct v4l2_file_operations rzg2l_cru_fops = { > .read = vb2_fop_read, > }; > > +/* ----------------------------------------------------------------------------- > + * Media entity operations > + */ > + > +static int rzg2l_cru_video_link_validate(struct media_link *link) > +{ > + struct v4l2_subdev_format fmt = { > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > + }; > + const struct rzg2l_cru_ip_format *video_fmt; > + const struct rzg2l_cru_ip_format *ip_fmt; > + struct v4l2_subdev *subdev; > + struct rzg2l_cru_dev *cru; > + struct media_pad *remote; > + int ret; > + > + remote = link->source; > + subdev = media_entity_to_v4l2_subdev(remote->entity); > + fmt.pad = remote->index; subdev = media_entity_to_v4l2_subdev(link->source->entity); fmt.pad = link->source->index; and drop the remote variable. Or, if you prefer keeping it, rename it to source. > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > + if (ret < 0) > + return ret == -ENOIOCTLCMD ? -EINVAL : ret; > + > + cru = container_of(media_entity_to_video_device(link->sink->entity), > + struct rzg2l_cru_dev, vdev); > + video_fmt = rzg2l_cru_ip_format_to_fmt(cru->format.pixelformat); > + if (!video_fmt) > + return -EPIPE; Can this happen, doesn't the s_fmt handler on the video device ensure that pixelformat is always valid. > + ip_fmt = rzg2l_cru_ip_code_to_fmt(fmt.format.code); > + if (!ip_fmt) > + return -EPIPE; Same question here. > + > + if (fmt.format.width != cru->format.width || > + fmt.format.height != cru->format.height || > + fmt.format.field != cru->format.field || > + video_fmt->code != fmt.format.code || > + ip_fmt->format != cru->format.pixelformat) The last two line seem to implement the same check. > + return -EPIPE; > + > + return 0; > +} > + > +static const struct media_entity_operations rzg2l_cru_video_media_ops = { > + .link_validate = rzg2l_cru_video_link_validate, > +}; > + > static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) > { > struct video_device *vdev = &cru->vdev; > @@ -1006,6 +1008,7 @@ static void rzg2l_cru_v4l2_init(struct rzg2l_cru_dev *cru) > vdev->lock = &cru->lock; > vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING; > vdev->device_caps |= V4L2_CAP_IO_MC; > + vdev->entity.ops = &rzg2l_cru_video_media_ops; > vdev->fops = &rzg2l_cru_fops; > vdev->ioctl_ops = &rzg2l_cru_ioctl_ops; > -- Regards, Laurent Pinchart
Hi Laurent, Thank you for the quick review. On Mon, Oct 7, 2024 at 9:06 PM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Prabhakar, > > Thank you for the patch. > > On Mon, Oct 07, 2024 at 07:48:35PM +0100, Prabhakar wrote: > > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > > > Implement the `.link_validate()` callback for the video node and move the > > format checking into this function. This change allows the removal of > > `rzg2l_cru_mc_validate_format()`. > > > > Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> > > --- > > .../platform/renesas/rzg2l-cru/rzg2l-video.c | 91 ++++++++++--------- > > 1 file changed, 47 insertions(+), 44 deletions(-) > > > > diff --git a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > index ceb9012c9d70..385b4242db2f 100644 > > --- a/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > +++ b/drivers/media/platform/renesas/rzg2l-cru/rzg2l-video.c > > @@ -189,46 +189,6 @@ static void rzg2l_cru_buffer_queue(struct vb2_buffer *vb) > > spin_unlock_irqrestore(&cru->qlock, flags); > > } > > > > -static int rzg2l_cru_mc_validate_format(struct rzg2l_cru_dev *cru, > > - struct v4l2_subdev *sd, > > - struct media_pad *pad) > > -{ > > - struct v4l2_subdev_format fmt = { > > - .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > - }; > > - > > - fmt.pad = pad->index; > > - if (v4l2_subdev_call_state_active(sd, pad, get_fmt, &fmt)) > > - return -EPIPE; > > - > > - switch (fmt.format.code) { > > - case MEDIA_BUS_FMT_UYVY8_1X16: > > - break; > > - default: > > - return -EPIPE; > > - } > > - > > - switch (fmt.format.field) { > > - case V4L2_FIELD_TOP: > > - case V4L2_FIELD_BOTTOM: > > - case V4L2_FIELD_NONE: > > - case V4L2_FIELD_INTERLACED_TB: > > - case V4L2_FIELD_INTERLACED_BT: > > - case V4L2_FIELD_INTERLACED: > > - case V4L2_FIELD_SEQ_TB: > > - case V4L2_FIELD_SEQ_BT: > > - break; > > - default: > > - return -EPIPE; > > - } > > - > > - if (fmt.format.width != cru->format.width || > > - fmt.format.height != cru->format.height) > > - return -EPIPE; > > - > > - return 0; > > -} > > - > > static void rzg2l_cru_set_slot_addr(struct rzg2l_cru_dev *cru, > > int slot, dma_addr_t addr) > > { > > @@ -531,10 +491,6 @@ static int rzg2l_cru_set_stream(struct rzg2l_cru_dev *cru, int on) > > return stream_off_ret; > > } > > > > - ret = rzg2l_cru_mc_validate_format(cru, sd, pad); > > - if (ret) > > - return ret; > > - > > pipe = media_entity_pipeline(&sd->entity) ? : &cru->vdev.pipe; > > ret = video_device_pipeline_start(&cru->vdev, pipe); > > if (ret) > > @@ -995,6 +951,52 @@ static const struct v4l2_file_operations rzg2l_cru_fops = { > > .read = vb2_fop_read, > > }; > > > > +/* ----------------------------------------------------------------------------- > > + * Media entity operations > > + */ > > + > > +static int rzg2l_cru_video_link_validate(struct media_link *link) > > +{ > > + struct v4l2_subdev_format fmt = { > > + .which = V4L2_SUBDEV_FORMAT_ACTIVE, > > + }; > > + const struct rzg2l_cru_ip_format *video_fmt; > > + const struct rzg2l_cru_ip_format *ip_fmt; > > + struct v4l2_subdev *subdev; > > + struct rzg2l_cru_dev *cru; > > + struct media_pad *remote; > > + int ret; > > + > > + remote = link->source; > > + subdev = media_entity_to_v4l2_subdev(remote->entity); > > + fmt.pad = remote->index; > > subdev = media_entity_to_v4l2_subdev(link->source->entity); > fmt.pad = link->source->index; > > and drop the remote variable. Or, if you prefer keeping it, rename it to > source. > OK, I will drop the local variable. > > + ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt); > > + if (ret < 0) > > + return ret == -ENOIOCTLCMD ? -EINVAL : ret; > > + > > + cru = container_of(media_entity_to_video_device(link->sink->entity), > > + struct rzg2l_cru_dev, vdev); > > + video_fmt = rzg2l_cru_ip_format_to_fmt(cru->format.pixelformat); > > + if (!video_fmt) > > + return -EPIPE; > > Can this happen, doesn't the s_fmt handler on the video device ensure > that pixelformat is always valid. > Agreed, this won't happen. I'll drop this check. > > + ip_fmt = rzg2l_cru_ip_code_to_fmt(fmt.format.code); > > + if (!ip_fmt) > > + return -EPIPE; > > Same question here. > This won't happen, so I will drop the check. Actually as you mentioned below we can drop one check so I'll get rid of fetching ip_fmt. > > + > > + if (fmt.format.width != cru->format.width || > > + fmt.format.height != cru->format.height || > > + fmt.format.field != cru->format.field || > > + video_fmt->code != fmt.format.code || > > + ip_fmt->format != cru->format.pixelformat) > > The last two line seem to implement the same check. > OK, I will drop the pixelformat check and keep the code check. Cheers, Prabhakar
© 2016 - 2024 Red Hat, Inc.