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

From: Eugen.Hristev
Date: Tue Dec 07 2021 - 06:21:50 EST

Next message: Hans de Goede: "Re: [pdx86-platform-drivers-x86:review-hans 41/44] drivers/platform/x86/lenovo-yogabook-wmi.c:278:20: warning: no previous prototype for 'yogabook_wmi_suspend'"
Previous message: Amjad Ouled-Ameur: "Re: [PATCH v4 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use"
In reply to: Jacopo Mondi: "Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 12/6/21 6:38 PM, Jacopo Mondi wrote:
> 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

Hi Jacopo,

Yes, this also works, to have a different variable and compute the
bytesperline according to it.
I also considered this, but it was a bit more difficult to understand
later (what does subsampling 16 and subsampling 24 means ?)

I thought it would be easier to have a simple bpp which is the real bpp
of the format, and the bpp for v4l2 which requires the
bytesperpixel_in_first_plane_only .

So as you agree with this solution, I would keep it as-is

Thanks for reviewing,
>
> 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: Hans de Goede: "Re: [pdx86-platform-drivers-x86:review-hans 41/44] drivers/platform/x86/lenovo-yogabook-wmi.c:278:20: warning: no previous prototype for 'yogabook_wmi_suspend'"
Previous message: Amjad Ouled-Ameur: "Re: [PATCH v4 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use"
In reply to: Jacopo Mondi: "Re: [PATCH v2 12/25] media: atmel: atmel-isc-base: fix bytesperline value for planar formats"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]