| Message ID | 20241007184839.190519-14-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: platform: rzg2l-cru: CSI-2 and CRU enhancements | expand |
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; >
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
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;