[v5,03/14] media: cadence: csi2rx: Add get_fmt and set_fmt pad ops
Commit Message
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@ti.com>
---
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(+)
Comments
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@ti.com>
>
> ---
>
> 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.
> +
> +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.
> +
> + 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);
Same comments for csi2rx_set_fmt().
> +
> + 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;
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> +
> + format->format.field = V4L2_FIELD_NONE;
> +
> + mutex_lock(&csi2rx->lock);
> + framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
> + format->which);
> + if (!framefmt) {
> + mutex_unlock(&csi2rx->lock);
> + return -EINVAL;
> + }
> +
> + *framefmt = format->format;
> + mutex_unlock(&csi2rx->lock);
> +
> + return 0;
> +}
> +
> +static int csi2rx_init_cfg(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_state *state)
> +{
> + struct v4l2_subdev_format format = {
> + .which = state ? V4L2_SUBDEV_FORMAT_TRY
> + : V4L2_SUBDEV_FORMAT_ACTIVE,
> + .pad = CSI2RX_PAD_SINK,
> + .format = {
> + .width = 640,
> + .height = 480,
> + .code = MEDIA_BUS_FMT_UYVY8_1X16,
> + .field = V4L2_FIELD_NONE,
> + .colorspace = V4L2_COLORSPACE_SRGB,
> + .ycbcr_enc = V4L2_YCBCR_ENC_601,
> + .quantization = V4L2_QUANTIZATION_LIM_RANGE,
> + .xfer_func = V4L2_XFER_FUNC_SRGB,
> + },
> + };
> +
> + return csi2rx_set_fmt(subdev, state, &format);
> +}
> +
> +static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> + .get_fmt = csi2rx_get_fmt,
> + .set_fmt = csi2rx_set_fmt,
> + .init_cfg = csi2rx_init_cfg,
> +};
> +
> static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> .s_stream = csi2rx_s_stream,
> };
>
> static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> .video = &csi2rx_video_ops,
> + .pad = &csi2rx_pad_ops,
> };
>
> static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
> @@ -457,6 +590,10 @@ static int csi2rx_probe(struct platform_device *pdev)
> if (ret)
> goto err_cleanup;
>
> + ret = csi2rx_init_cfg(&csi2rx->subdev, NULL);
> + if (ret)
> + goto err_cleanup;
> +
> ret = v4l2_async_register_subdev(&csi2rx->subdev);
> if (ret < 0)
> goto err_cleanup;
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@ti.com>
> >
> > ---
> >
> > 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@ideasonboard.com>
Thanks.
@@ -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,
+ },
+};
+
+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;
+
+ format->format = *framefmt;
+
+ 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;
+
+ format->format.field = V4L2_FIELD_NONE;
+
+ mutex_lock(&csi2rx->lock);
+ framefmt = csi2rx_get_pad_format(csi2rx, state, format->pad,
+ format->which);
+ if (!framefmt) {
+ mutex_unlock(&csi2rx->lock);
+ return -EINVAL;
+ }
+
+ *framefmt = format->format;
+ mutex_unlock(&csi2rx->lock);
+
+ return 0;
+}
+
+static int csi2rx_init_cfg(struct v4l2_subdev *subdev,
+ struct v4l2_subdev_state *state)
+{
+ struct v4l2_subdev_format format = {
+ .which = state ? V4L2_SUBDEV_FORMAT_TRY
+ : V4L2_SUBDEV_FORMAT_ACTIVE,
+ .pad = CSI2RX_PAD_SINK,
+ .format = {
+ .width = 640,
+ .height = 480,
+ .code = MEDIA_BUS_FMT_UYVY8_1X16,
+ .field = V4L2_FIELD_NONE,
+ .colorspace = V4L2_COLORSPACE_SRGB,
+ .ycbcr_enc = V4L2_YCBCR_ENC_601,
+ .quantization = V4L2_QUANTIZATION_LIM_RANGE,
+ .xfer_func = V4L2_XFER_FUNC_SRGB,
+ },
+ };
+
+ return csi2rx_set_fmt(subdev, state, &format);
+}
+
+static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
+ .get_fmt = csi2rx_get_fmt,
+ .set_fmt = csi2rx_set_fmt,
+ .init_cfg = csi2rx_init_cfg,
+};
+
static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
.s_stream = csi2rx_s_stream,
};
static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
.video = &csi2rx_video_ops,
+ .pad = &csi2rx_pad_ops,
};
static int csi2rx_async_bound(struct v4l2_async_notifier *notifier,
@@ -457,6 +590,10 @@ static int csi2rx_probe(struct platform_device *pdev)
if (ret)
goto err_cleanup;
+ ret = csi2rx_init_cfg(&csi2rx->subdev, NULL);
+ if (ret)
+ goto err_cleanup;
+
ret = v4l2_async_register_subdev(&csi2rx->subdev);
if (ret < 0)
goto err_cleanup;