linux-kernel - Re: [PATCH] media: Add 16-bit Bayer formats

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20211129170356.7258d6db.dorota.czaplejewicz@puri.sm>
Date:   Mon, 29 Nov 2021 17:05:23 +0100
From:   Dorota Czaplejewicz <dorota.czaplejewicz@...i.sm>
To:     Kieran Bingham <kieran.bingham@...asonboard.com>
Cc:     Andrey Konovalov <andrey.konovalov@...aro.org>,
        Jacopo Mondi <jacopo@...ndi.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Sakari Ailus <sakari.ailus@...ux.intel.com>, kernel@...i.sm,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] media: Add 16-bit Bayer formats

On Mon, 29 Nov 2021 15:46:11 +0000
Kieran Bingham <kieran.bingham@...asonboard.com> wrote:

> Hi Dorota,
> 
> Quoting Dorota Czaplejewicz (2021-10-19 12:59:29)
> > 16-bit bayer formats are used by the i.MX driver.  
> 
> Can we expand upon this at all?
> 
> The Subject says "Add 16-bit Bayer formats" but this isn't adding the
> format, it's purely extending the v4l2_format_info table with the
> information for that format which is otherwise missing.
> 
What do you suggest for a better commit message? My reasoning was that I'm adding entries to a table.

> I wonder what other formats are missing from that table too?
> 
> 
> > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@...i.sm>
> > ---
> > Hello,
> > 
> > While working on the i.MX8 video driver, I discovered that `v4l2_fill_pixfmt` will fail when using 10-bit sensor formats. (For background, see the conversation at https://lkml.org/lkml/2021/10/17/93 .)
> > 
> > It appears that the video hardware will fill a 16-bit-per-pixel buffer when fed 10-bit-per-pixel Bayer data, making `v4l2_fill_pixfmt` effectively broken for this case.  
> 
> This statement is confusing to me. Are you saying you're programming the
> hardware with 10 bit, and it's using 16 bits per pixel to store that
> data? (Which is simply 'unpacked' I think ?)
> 
I know the sensor I'm dealing with sends 10-bit data. I'm observing that the data arriving at this stage of the pipeline is encoded with 16 bits per pixel. As far as I understand, that's what i.MX8 does at some stage of the MIPI/CSI2 pipeline by design, but I can't elaborate at the moment, and I don't think it affects the validity of the addition.
> 
> > 
> > This change adds the relevant entries to the format info structure.
> > 
> > Difference in behaviour observed using the i846 driver on the Librem 5.
> > 
> > Regards,
> > Dorota Czaplejewicz
> > 
> >  drivers/media/v4l2-core/v4l2-common.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c
> > index 04af03285a20..d2e61538e979 100644
> > --- a/drivers/media/v4l2-core/v4l2-common.c
> > +++ b/drivers/media/v4l2-core/v4l2-common.c
> > @@ -309,6 +309,10 @@ const struct v4l2_format_info *v4l2_format_info(u32 format)
> >                 { .format = V4L2_PIX_FMT_SGBRG12,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> >                 { .format = V4L2_PIX_FMT_SGRBG12,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> >                 { .format = V4L2_PIX_FMT_SRGGB12,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +               { .format = V4L2_PIX_FMT_SBGGR16,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +               { .format = V4L2_PIX_FMT_SGBRG16,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +               { .format = V4L2_PIX_FMT_SGRBG16,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },
> > +               { .format = V4L2_PIX_FMT_SRGGB16,       .pixel_enc = V4L2_PIXEL_ENC_BAYER, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 },  
> 
> This looks right as far as I can see though, so for the change, and
> ideally with the commit message improved to be clearer about the
> content and reasoning for the change:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham+renesas@...asonboard.com>
> 

Thanks!

--Dorota
> >         };
> >         unsigned int i;
> >  
> > -- 
> > 2.31.1
> >  


Content of type "application/pgp-signature" skipped