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

[Dorota Czaplejewicz]

On Mon, 29 Nov 2021 15:46:11 +0000
Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx> 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@xxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxx>
> 

Thanks!

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

Attachment: pgpU0Teso4tDS.pgp
Description: OpenPGP digital signature