| Message ID | 20211223191615.17803-4-p.yadav@ti.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | CSI2RX support on J721E | expand |
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.
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, + }, +}; + +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;
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(+)