From: Jacopo Mondi <jacopo@jmondi.org>
To: Eugen Hristev <eugen.hristev@microchip.com>
Cc: linux-media@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
laurent.pinchart@ideasonboard.com, sakari.ailus@iki.fi,
nicolas.ferre@microchip.com
Subject: Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats
Date: Mon, 6 Dec 2021 17:38:15 +0100 [thread overview]
Message-ID: <20211206163815.wq5tq3fcsqkj2etk@uno.localdomain> (raw)
In-Reply-To: <20211112142509.2230884-13-eugen.hristev@microchip.com>
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@microchip.com>
> ---
> 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@jmondi.org>
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 prev parent reply other threads:[~2021-12-06 16:37 UTC|newest]
Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-11-12 14:24 [PATCH v2 00/25] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 01/25] MAINTAINERS: add microchip csi2dc Eugen Hristev
2021-11-29 23:14 ` Laurent Pinchart
2021-12-13 13:40 ` Nicolas Ferre
2021-11-12 14:24 ` [PATCH v2 02/25] dt-bindings: media: atmel: csi2dc: add bindings for " Eugen Hristev
2021-11-29 21:10 ` Rob Herring
2021-11-29 21:37 ` Sakari Ailus
2021-12-06 8:57 ` Eugen.Hristev
2021-11-29 23:14 ` Laurent Pinchart
2021-11-12 14:24 ` [PATCH v2 03/25] media: atmel: introduce microchip csi2dc driver Eugen Hristev
2021-12-06 10:51 ` Jacopo Mondi
2021-12-06 11:42 ` Eugen.Hristev
2021-12-06 12:06 ` Jacopo Mondi
2021-12-10 11:07 ` Eugen.Hristev
2021-12-10 12:59 ` Jacopo Mondi
2021-11-12 14:24 ` [PATCH v2 04/25] MAINTAINERS: atmel-isc: add new file atmel-isc-clk.c Eugen Hristev
2021-11-29 23:18 ` Laurent Pinchart
2021-11-12 14:24 ` [PATCH v2 05/25] media: atmel: atmel-isc: split the clock code into separate source file Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 06/25] media: atmel: atmel-isc: replace video device name with module name Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 07/25] media: atmel: atmel-sama7g5-isc: fix ispck leftover Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 08/25] media: atmel: atmel-isc-base: use streaming status when queueing buffers Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 09/25] media: atmel: atmel-isc-base: remove frameintervals VIDIOC Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 10/25] media: atmel: atmel-isc-base: report frame sizes as full supported range Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 11/25] media: atmel: atmel-isc-base: implement mbus_code support in enumfmt Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats Eugen Hristev
2021-12-06 16:38 ` Jacopo Mondi [this message]
2021-12-07 11:21 ` Eugen.Hristev
2021-11-12 14:24 ` [PATCH v2 13/25] MAINTAINERS: atmel-isc: add new file atmel-isc-mc.c Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 14/25] media: atmel: atmel-isc: implement media controller Eugen Hristev
2021-11-12 14:24 ` [PATCH v2 15/25] ARM: dts: at91: sama7g5: add nodes for video capture Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 16/25] ARM: configs: at91: sama7: add xisc and csi2dc Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 17/25] ARM: multi_v7_defconfig: add atmel video pipeline modules Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 18/25] media: atmel: atmel-sama5d2-isc: fix wrong mask in YUYV format check Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 19/25] media: atmel: atmel-isc-base: use mutex to lock awb workqueue from streaming Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 20/25] media: atmel: atmel-isc-base: add wb debug messages Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 21/25] media: atmel: atmel-isc-base: clamp wb gain coefficients Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 22/25] media: atmel: atmel-sama7g5-isc: fix UYVY input format mbus_code typo Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 23/25] media: atmel: atmel-isc: add raw Bayer 8bit 10bit output formats Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 24/25] media: atmel: atmel-isc: compact the controller formats list Eugen Hristev
2021-11-12 14:25 ` [PATCH v2 25/25] media: atmel: atmel-isc: change format propagation to subdev into only verification Eugen Hristev
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211206163815.wq5tq3fcsqkj2etk@uno.localdomain \
--to=jacopo@jmondi.org \
--cc=devicetree@vger.kernel.org \
--cc=eugen.hristev@microchip.com \
--cc=laurent.pinchart@ideasonboard.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=nicolas.ferre@microchip.com \
--cc=sakari.ailus@iki.fi \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).