Re: [PATCH v5 18/22] media: rzg2l-cru: video: Implement .link_validate() callback

Laurent Pinchart
2024-10-15T10:33:51Z

Hi Prabhakar,

Thank you for the patch.

On Fri, Oct 11, 2024 at 06:30:48PM +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()`.

I would add here that the fmt.format.code and fmt.format.field checks
can be dropped as the subdev .set_fmt() handler ensures that those
fields always hold valid values.

>
> Suggested-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>

> ---
> .../platform/renesas/rzg2l-cru/rzg2l-video.c | 82 +++++++++----------
> 1 file changed, 38 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 0cc69a7440bf..df6114cbe5c7 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)
> {
> @@ -532,10 +492,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)
> @@ -986,6 +942,43 @@ 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;
> + struct v4l2_subdev *subdev;
> + struct rzg2l_cru_dev *cru;
> + int ret;
> +
> + subdev = media_entity_to_v4l2_subdev(link->source->entity);
> + fmt.pad = link->source->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 (fmt.format.width != cru->format.width ||
> + fmt.format.height != cru->format.height ||
> + fmt.format.field != cru->format.field ||
> + video_fmt->code != fmt.format.code)
> + 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;
> @@ -997,6 +990,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