Hi Laurent,
On 30/12/21 06:32AM, Laurent Pinchart wrote:
> Hi Pratyush,
>
> Thank you for the patch.
>
> On Fri, Dec 24, 2021 at 12:46:04AM +0530, Pratyush Yadav wrote:
> > The format is needed to calculate the link speed for the external DPHY
> > configuration. It is not right to query the format from the source
> > subdev. Add get_fmt and set_fmt pad operations so that the format can be
> > configured and correct bpp be selected.
> >
> > Signed-off-by: Pratyush Yadav <p.yadav@xxxxxx>
> >
> > ---
> >
> > Changes in v5:
> > - Use YUV 1X16 formats instead of 2X8.
> > - New in v5.
> >
> > drivers/media/platform/cadence/cdns-csi2rx.c | 137 +++++++++++++++++++
> > 1 file changed, 137 insertions(+)
> >
> > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > index 2547903f2e8e..4a2a5a9d019b 100644
> > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > @@ -54,6 +54,11 @@ enum csi2rx_pads {
> > CSI2RX_PAD_MAX,
> > };
> >
> > +struct csi2rx_fmt {
> > + u32 code;
> > + u8 bpp;
> > +};
> > +
> > struct csi2rx_priv {
> > struct device *dev;
> > unsigned int count;
> > @@ -79,12 +84,43 @@ struct csi2rx_priv {
> > struct v4l2_subdev subdev;
> > struct v4l2_async_notifier notifier;
> > struct media_pad pads[CSI2RX_PAD_MAX];
> > + struct v4l2_mbus_framefmt fmt;
> >
> > /* Remote source */
> > struct v4l2_subdev *source_subdev;
> > int source_pad;
> > };
> >
> > +static const struct csi2rx_fmt formats[] = {
> > + {
> > + .code = MEDIA_BUS_FMT_YUYV8_1X16,
> > + .bpp = 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> > + .bpp = 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_YVYU8_1X16,
> > + .bpp = 16,
> > + },
> > + {
> > + .code = MEDIA_BUS_FMT_VYUY8_1X16,
> > + .bpp = 16,
> > + },
> > +};
>
> bpp isn't used. Unless you need it in a subsequent patch in the series,
> you can turn the formats array into a u32 array.
It is used in the next patch for calculating DPHY parameters.
>
> > +
> > +static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> > +{
> > + unsigned int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(formats); i++)
> > + if (formats[i].code == code)
> > + return &formats[i];
> > +
> > + return NULL;
> > +}
> > +
> > static inline
> > struct csi2rx_priv *v4l2_subdev_to_csi2rx(struct v4l2_subdev *subdev)
> > {
> > @@ -236,12 +272,109 @@ static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > return ret;
> > }
> >
> > +static struct v4l2_mbus_framefmt *
> > +csi2rx_get_pad_format(struct csi2rx_priv *csi2rx,
> > + struct v4l2_subdev_state *state,
> > + unsigned int pad, u32 which)
> > +{
> > + switch (which) {
> > + case V4L2_SUBDEV_FORMAT_TRY:
> > + return v4l2_subdev_get_try_format(&csi2rx->subdev, state, pad);
> > + case V4L2_SUBDEV_FORMAT_ACTIVE:
> > + return &csi2rx->fmt;
> > + default:
> > + return NULL;
> > + }
> > +}
> > +
> > +static int csi2rx_get_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > + struct v4l2_mbus_framefmt *framefmt;
> > +
> > + mutex_lock(&csi2rx->lock);
> > +
> > + framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
> > + format->which);
> > + mutex_unlock(&csi2rx->lock);
> > +
> > + if (!framefmt)
> > + return -EINVAL;
>
> This can't happen, you can drop the check.
The function returns NULL when format->which is neither
V4L2_SUBDEV_FORMAT_TRY nor V4L2_SUBDEV_FORMAT_ACTIVE. Does V4L2 core
verify that which is always one of TRY or ACTIVE, and nothing else. Does
it also guarantee that this would *never* change (like adding a new
value for example)? If so, I am fine with dropping this check. Otherwise
I would like to keep it.
>
> > +
> > + format->format = *framefmt;
>
> This is the assignment that needs to be protected by the lock.
> csi2rx_get_pad_format() returns a pointer to the storage, it doesn't
> modify it.
>
> framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
> format->which);
>
> mutex_lock(&csi2rx->lock);
> format->format = *framefmt;
> mutex_unlock(&csi2rx->lock);
Ah, you're right. I feel very stupid now ;-)
>
> Same comments for csi2rx_set_fmt().
The assignment is csi2rx_set_fmt() is done under the lock. But I should
move the lock to after csi2rx_get_pad_format() is called so we only hold
the lock for as little as possible:
framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
format->which);
if (!framefmt) {
mutex_unlock(&csi2rx->lock);
return -EINVAL;
}
mutex_lock(&csi2rx->lock);
*framefmt = format->format;
mutex_unlock(&csi2rx->lock);
>
> > +
> > + return 0;
> > +}
> > +
> > +static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > + struct v4l2_subdev_state *state,
> > + struct v4l2_subdev_format *format)
> > +{
> > + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > + struct v4l2_mbus_framefmt *framefmt;
> > + const struct csi2rx_fmt *fmt;
> > +
> > + /* No transcoding, source and sink formats must match. */
> > + if (format->pad != CSI2RX_PAD_SINK)
> > + return csi2rx_get_fmt(subdev, state, format);
> > +
> > + fmt = csi2rx_get_fmt_by_code(format->format.code);
> > + if (!fmt)
> > + return -EOPNOTSUPP;
>
> This should not return an error, but instead adjust the code:
>
> if (!csi2rx_get_fmt_by_code(format->format.code))
> format->format.code = formats[0].code;
Ok. I figured it would be better to fail loudly if an unsupported format
is requested. But if this behavior is preferred that is also fine.
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
Thanks.
--
Regards,
Pratyush Yadav
Texas Instruments Inc.
