Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats

From: Jacopo Mondi
Date: Mon Dec 06 2021 - 11:37:40 EST

Next message: Adam Thomson: "RE: [RFC PATCH] watchdog: da9062: Correct the timeout values"
Previous message: Mihail Chindris: "[RESEND, PATCH v6 1/2] dt-bindings: iio: dac: Add adi,ad3552r.yaml"
Next in thread: Eugen.Hristev: "Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Eugen

On Fri, Nov 12, 2021 at 04:24:56PM +0200, Eugen Hristev wrote:
> The bytesperline field of the pixfmt should be only for the first plane
> in case of planar formats like YUV420 or YUV422.
> The bytesperline is used by the driver to compute the framesize.
> We have to report a different bpp (bytes per pixel) to v4l2 in bytesperline
> than the actual bpp.
> For example for YUV420, the real bpp is 12, but the first plane has only
> 8 bpp. Thus we report a bytesperline 8*width instead of 12*width.
> However, for real framezise we have to compute 12*width*height.
> Hence added a new variable to hold this information and to correctly
> compute the frame size.

Using single planar API for multiplanar format is really painful :(

>
> Signed-off-by: Eugen Hristev <eugen.hristev@xxxxxxxxxxxxx>
> ---
> drivers/media/platform/atmel/atmel-isc-base.c | 19 +++++++++++++++++--
> drivers/media/platform/atmel/atmel-isc.h | 4 ++++
> 2 files changed, 21 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index 2cb8446ff90c..d0542b97a391 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -654,6 +654,7 @@ static int isc_try_configure_rlp_dma(struct isc_device *isc, bool direct_dump)
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED8;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 8;
> + isc->try_config.bpp_v4l2 = 8;
> break;
> case V4L2_PIX_FMT_SBGGR10:
> case V4L2_PIX_FMT_SGBRG10:
> @@ -663,6 +664,7 @@ static int isc_try_configure_rlp_dma(struct isc_device *isc, bool direct_dump)
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_SBGGR12:
> case V4L2_PIX_FMT_SGBRG12:
> @@ -672,24 +674,28 @@ static int isc_try_configure_rlp_dma(struct isc_device *isc, bool direct_dump)
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_RGB565:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_RGB565;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_ARGB444:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_ARGB444;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_ARGB555:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_ARGB555;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_ABGR32:
> case V4L2_PIX_FMT_XBGR32:
> @@ -697,42 +703,49 @@ static int isc_try_configure_rlp_dma(struct isc_device *isc, bool direct_dump)
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED32;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 32;
> + isc->try_config.bpp_v4l2 = 32;
> break;
> case V4L2_PIX_FMT_YUV420:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_YYCC;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_YC420P;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PLANAR;
> isc->try_config.bpp = 12;
> + isc->try_config.bpp_v4l2 = 8; /* only first plane */
> break;
> case V4L2_PIX_FMT_YUV422P:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_YYCC;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_YC422P;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PLANAR;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 8; /* only first plane */

This could have also been described with by adding to the format a
subsampling factor for the CbCr plane and fixing the bpp to 8 as it
describes the first plane. In this way you could avoid setting
bpp_v4l2 for all formats that do not need it.

Something like a simple subsampling multiplier would do for planar YUV
formats, more complicated schemes would probably needed for other
formats.

420:
bpp = 8
subsampling = 12
bytesperline = w * bpp >> 3
sizeimage = w * h * subsampling >> 3

422:
bpp = 8
subsampling = 16
bytesperline = w * bpp >> 3
sizeimage = w * h * subsampling >> 3

444:
bpp = 8
subsampling = 24
bytesperline = w * bpp >> 3
sizeimage = w * h * subsampling >> 3

You would anyway need to set subsampling = 8 in other formats or
either
sizeimage = w * h * (subsampling ? subsampling : 8) >> 3

which is not super nice ;)

As you wish though, this is driver code, so anything goes

Reviewed-by: Jacopo Mondi <jacopo@xxxxxxxxxx>

Thanks
j

> break;
> case V4L2_PIX_FMT_YUYV:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_YCYC | ISC_RLP_CFG_YMODE_YUYV;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED32;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_UYVY:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_YCYC | ISC_RLP_CFG_YMODE_UYVY;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED32;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_VYUY:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_YCYC | ISC_RLP_CFG_YMODE_VYUY;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED32;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> case V4L2_PIX_FMT_GREY:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_DATY8;
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED8;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 8;
> + isc->try_config.bpp_v4l2 = 8;
> break;
> case V4L2_PIX_FMT_Y16:
> isc->try_config.rlp_cfg_mode = ISC_RLP_CFG_MODE_DATY10 | ISC_RLP_CFG_LSH;
> @@ -742,6 +755,7 @@ static int isc_try_configure_rlp_dma(struct isc_device *isc, bool direct_dump)
> isc->try_config.dcfg_imode = ISC_DCFG_IMODE_PACKED16;
> isc->try_config.dctrl_dview = ISC_DCTRL_DVIEW_PACKED;
> isc->try_config.bpp = 16;
> + isc->try_config.bpp_v4l2 = 16;
> break;
> default:
> return -EINVAL;
> @@ -990,8 +1004,9 @@ static int isc_try_fmt(struct isc_device *isc, struct v4l2_format *f,
> pixfmt->height = isc->max_height;
>
> pixfmt->field = V4L2_FIELD_NONE;
> - pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp) >> 3;
> - pixfmt->sizeimage = pixfmt->bytesperline * pixfmt->height;
> + pixfmt->bytesperline = (pixfmt->width * isc->try_config.bpp_v4l2) >> 3;
> + pixfmt->sizeimage = ((pixfmt->width * isc->try_config.bpp) >> 3) *
> + pixfmt->height;
>
> if (code)
> *code = mbus_code;
> diff --git a/drivers/media/platform/atmel/atmel-isc.h b/drivers/media/platform/atmel/atmel-isc.h
> index 32448ccfc636..07fa6dbf8460 100644
> --- a/drivers/media/platform/atmel/atmel-isc.h
> +++ b/drivers/media/platform/atmel/atmel-isc.h
> @@ -102,6 +102,9 @@ struct isc_format {
> configuration.
> * @fourcc: Fourcc code for this format.
> * @bpp: Bytes per pixel in the current format.
> + * @bpp_v4l2: Bytes per pixel in the current format, for v4l2.
> + This differs from 'bpp' in the sense that in planar
> + formats, it refers only to the first plane.
> * @rlp_cfg_mode: Configuration of the RLP (rounding, limiting packaging)
> * @dcfg_imode: Configuration of the input of the DMA module
> * @dctrl_dview: Configuration of the output of the DMA module
> @@ -112,6 +115,7 @@ struct fmt_config {
>
> u32 fourcc;
> u8 bpp;
> + u8 bpp_v4l2;
>
> u32 rlp_cfg_mode;
> u32 dcfg_imode;
> --
> 2.25.1
>

Next message: Adam Thomson: "RE: [RFC PATCH] watchdog: da9062: Correct the timeout values"
Previous message: Mihail Chindris: "[RESEND, PATCH v6 1/2] dt-bindings: iio: dac: Add adi,ad3552r.yaml"
Next in thread: Eugen.Hristev: "Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]