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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
>>





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux