Hi Laurent,
Thank you for the quick review.
On Mon, Oct 7, 2024 at 9:06 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
> >
> > 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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@xxxxxxxxxxxxxx>
> > ---
> > .../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
