Re: [PATCH v3 13/17] media: rzg2l-cru: video: Implement .link_validate() callback

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Thu, Oct 3, 2024 at 3:51 PM Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Tue, Oct 01, 2024 at 03:09:15PM +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>
> > ---
> > v2->v3
> > - New patch
> > ---
> >  .../platform/renesas/rzg2l-cru/rzg2l-video.c  | 99 ++++++++++---------
> >  1 file changed, 55 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..c6c82b9b130a 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,60 @@ 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,
> > +     };
> > +     struct v4l2_subdev *subdev;
> > +     struct media_entity *entity;
> > +     struct rzg2l_cru_dev *cru;
> > +     struct media_pad *remote;
> > +     int ret;
> > +
> > +     entity = link->sink->entity;
> > +     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;
> > +
> > +     if (!rzg2l_cru_ip_code_to_fmt(fmt.format.code))
> > +             return -EPIPE;
>
> Here you should check that the format on the subdev matches the format
> on the video device.
>
OK, I'll use the return value from rzg2l_cru_ip_code_to_fmt() and
match it with the video device node.

> > +
> > +     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;
> > +     }
>
> Instead of checking the field here, shouldn't it be forced to a valid
> value in the subdev .set_fmt() function ? The link validation handler is
> responsible for validating that the configuration of the two sides of
> the link (IP subdev and video device) match. The driver shouldn't allow
> setting formats that can't be supported.
>
Agreed, this is already taken care of in .set_fmt(), so I'll drop this
check here.

> What you should check here is that the field of the subdev and the
> field of the video device match.
>
OK, I'll add a check for this.

> > +
> > +     cru = container_of(media_entity_to_video_device(entity),
>
> You can drop the local entity variable and write
>
OK.

Cheers,
Prabhakar