diff mbox series

[v4,13/17] media: rzg2l-cru: video: Implement .link_validate() callback

Message ID 20241007184839.190519-14-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State New
Headers show
Series media: platform: rzg2l-cru: CSI-2 and CRU enhancements | expand

Commit Message

Lad, Prabhakar Oct. 7, 2024, 6:48 p.m. UTC 
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(-)

Comments

Laurent Pinchart Oct. 7, 2024, 8:06 p.m. UTC | #1 
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;
>
Lad, Prabhakar Oct. 8, 2024, 11:05 a.m. UTC | #2 
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 mbox series

Patch

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;