| Message ID | 20211019114718.827400-1-dorota.czaplejewicz@puri.sm (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: Add 16-bit Bayer formats | expand |
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. I wonder what other formats are missing from that table too? > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.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 ?) > > 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@ideasonboard.com> > }; > unsigned int i; > > -- > 2.31.1 >
On Mon, 29 Nov 2021 15:46:11 +0000 Kieran Bingham <kieran.bingham@ideasonboard.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@puri.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@ideasonboard.com> > Thanks! --Dorota > > }; > > unsigned int i; > > > > -- > > 2.31.1 > >
Hi Dorota, On Mon, Nov 29, 2021 at 05:05:23PM +0100, Dorota Czaplejewicz wrote: > On Mon, 29 Nov 2021 15:46:11 +0000 Kieran Bingham wrote: > > 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. The format is defined by V4L2 but isn't present in that table. I'd state the this patch is fixing an oversight, and reference the commit that forgot to add these formats in a Fixes: tag. While at it, I'd also add at least the 14bpp Bayer formats, and possibly the packed formats too. > > I wonder what other formats are missing from that table too? > > > > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.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. Is the 10 bit data stored in the MSB or LSB of the 2 bytes ? > > > 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@ideasonboard.com> > > Thanks! > > > > }; > > > unsigned int i; > > >
Am Dienstag, dem 30.11.2021 um 04:33 +0200 schrieb Laurent Pinchart: > Hi Dorota, > > On Mon, Nov 29, 2021 at 05:05:23PM +0100, Dorota Czaplejewicz wrote: > > On Mon, 29 Nov 2021 15:46:11 +0000 Kieran Bingham wrote: > > > 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. > > The format is defined by V4L2 but isn't present in that table. I'd > state > the this patch is fixing an oversight, and reference the commit that > forgot to add these formats in a Fixes: tag. While at it, I'd also > add > at least the 14bpp Bayer formats, and possibly the packed formats > too. > > > > I wonder what other formats are missing from that table too? > > > > > > > Signed-off-by: Dorota Czaplejewicz < > > > > dorota.czaplejewicz@puri.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. > > Is the 10 bit data stored in the MSB or LSB of the 2 bytes ? Oh, I get a dejavu here. I assume this is on an i.MX8QM or i.MX8QXP, but not one of the other i.MX8 ones. They have a similar name, but are very (!) diffeent in some aspects. To answer your question, neither of those two alignments. The RM for i.MX8QM and i.MX8QXP states: > NOTE > The CSI data is right LSB aligned and zero padded depending > on data format. When interfacing ISI, CSI data is shifted 6-bits > due to ISI bits [5:0] always being zero > (0bxxCSIDATAxxxxxx). All RAW14, RAW12, RAW10, > RAW8, and RAW6 video data is filled from BIT[13] to LSB, > the remaining bits are zero padded. Only RAW16 input data > will be saved to memory from BIT[15] to LSB. See also [1]. This essentially means, unless you use RAW16, you will get RAW14 with a different amount of LSB bits set to 0. IIRC there was some patchset to introduce a RAW14 format ([2]) for exactly this use cas. There is also some kind of demo doing post-processing ([3]). Best regards, Alexander [1] https://community.nxp.com/t5/i-MX-Processors/i-MX8QM-MIPI-Raw-formats-not-working-correctly/m-p/1040832/highlight/true#M153336 [2] https://yhbt.net/lore/all/20200226151431.GY5379@paasikivi.fi.intel.com/T/ [3] https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX8QXP-capture-raw-bayer-data-and-debayer/ta-p/1098963
Hi Alexander, On Tue, Nov 30, 2021 at 08:43:25AM +0100, Alexander Stein wrote: > Am Dienstag, dem 30.11.2021 um 04:33 +0200 schrieb Laurent Pinchart: > > On Mon, Nov 29, 2021 at 05:05:23PM +0100, Dorota Czaplejewicz wrote: > > > On Mon, 29 Nov 2021 15:46:11 +0000 Kieran Bingham wrote: > > > > 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. > > > > The format is defined by V4L2 but isn't present in that table. I'd state > > the this patch is fixing an oversight, and reference the commit that > > forgot to add these formats in a Fixes: tag. While at it, I'd also add > > at least the 14bpp Bayer formats, and possibly the packed formats too. > > > > > > I wonder what other formats are missing from that table too? > > > > > > > > > Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.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. > > > > Is the 10 bit data stored in the MSB or LSB of the 2 bytes ? > > Oh, I get a dejavu here. I assume this is on an i.MX8QM or i.MX8QXP, > but not one of the other i.MX8 ones. They have a similar name, but are > very (!) diffeent in some aspects. > > To answer your question, neither of those two alignments. The RM for > i.MX8QM and i.MX8QXP states: > > > NOTE > > The CSI data is right LSB aligned and zero padded depending > > on data format. When interfacing ISI, CSI data is shifted 6-bits > > due to ISI bits [5:0] always being zero > > (0bxxCSIDATAxxxxxx). All RAW14, RAW12, RAW10, > > RAW8, and RAW6 video data is filled from BIT[13] to LSB, > > the remaining bits are zero padded. Only RAW16 input data > > will be saved to memory from BIT[15] to LSB. I think Dorota is working on the i.MX8MQ, which, unlike the QM and QXP, isn't based on the ISI IP core but the CSI bridge IP core. > See also [1]. > > This essentially means, unless you use RAW16, you will get RAW14 with a > different amount of LSB bits set to 0. > IIRC there was some patchset to introduce a RAW14 format ([2]) for > exactly this use cas. > There is also some kind of demo doing post-processing ([3]). > > Best regards, > Alexander > > [1] https://community.nxp.com/t5/i-MX-Processors/i-MX8QM-MIPI-Raw-formats-not-working-correctly/m-p/1040832/highlight/true#M153336 > [2] https://yhbt.net/lore/all/20200226151431.GY5379@paasikivi.fi.intel.com/T/ > [3] https://community.nxp.com/t5/i-MX-Processors-Knowledge-Base/i-MX8QXP-capture-raw-bayer-data-and-debayer/ta-p/1098963
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 }, }; unsigned int i;
16-bit bayer formats are used by the i.MX driver. Signed-off-by: Dorota Czaplejewicz <dorota.czaplejewicz@puri.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 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(+)