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:
pgpRZNuY8gyWh.pgp
Description: OpenPGP digital signature
