* [PATCH] media: Add 16-bit Bayer formats
@ 2021-10-19 11:59 Dorota Czaplejewicz
2021-11-29 15:46 ` Kieran Bingham
0 siblings, 1 reply; 6+ messages in thread
From: Dorota Czaplejewicz @ 2021-10-19 11:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab, Sakari Ailus, Laurent Pinchart,
Jacopo Mondi, Andrey Konovalov, linux-media, linux-kernel,
kernel
[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]
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(+)
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;
--
2.31.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] media: Add 16-bit Bayer formats
2021-10-19 11:59 [PATCH] media: Add 16-bit Bayer formats Dorota Czaplejewicz
@ 2021-11-29 15:46 ` Kieran Bingham
2021-11-29 16:05 ` Dorota Czaplejewicz
0 siblings, 1 reply; 6+ messages in thread
From: Kieran Bingham @ 2021-11-29 15:46 UTC (permalink / raw)
To: Andrey Konovalov, Dorota Czaplejewicz, Jacopo Mondi,
Laurent Pinchart, Mauro Carvalho Chehab, Sakari Ailus, kernel,
linux-kernel, linux-media
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: Add 16-bit Bayer formats
2021-11-29 15:46 ` Kieran Bingham
@ 2021-11-29 16:05 ` Dorota Czaplejewicz
2021-11-30 2:33 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Dorota Czaplejewicz @ 2021-11-29 16:05 UTC (permalink / raw)
To: Kieran Bingham
Cc: Andrey Konovalov, Jacopo Mondi, Laurent Pinchart,
Mauro Carvalho Chehab, Sakari Ailus, kernel, linux-kernel,
linux-media
[-- Attachment #1: Type: text/plain, Size: 3946 bytes --]
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
> >
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] media: Add 16-bit Bayer formats
2021-11-29 16:05 ` Dorota Czaplejewicz
@ 2021-11-30 2:33 ` Laurent Pinchart
2021-11-30 7:43 ` (EXT) " Alexander Stein
0 siblings, 1 reply; 6+ messages in thread
From: Laurent Pinchart @ 2021-11-30 2:33 UTC (permalink / raw)
To: Dorota Czaplejewicz
Cc: Kieran Bingham, Andrey Konovalov, Jacopo Mondi,
Mauro Carvalho Chehab, Sakari Ailus, kernel, linux-kernel,
linux-media
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;
> > >
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (EXT) Re: [PATCH] media: Add 16-bit Bayer formats
2021-11-30 2:33 ` Laurent Pinchart
@ 2021-11-30 7:43 ` Alexander Stein
2021-12-01 6:52 ` Laurent Pinchart
0 siblings, 1 reply; 6+ messages in thread
From: Alexander Stein @ 2021-11-30 7:43 UTC (permalink / raw)
To: Laurent Pinchart, Dorota Czaplejewicz
Cc: Kieran Bingham, Andrey Konovalov, Jacopo Mondi,
Mauro Carvalho Chehab, Sakari Ailus, kernel, linux-kernel,
linux-media
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: (EXT) Re: [PATCH] media: Add 16-bit Bayer formats
2021-11-30 7:43 ` (EXT) " Alexander Stein
@ 2021-12-01 6:52 ` Laurent Pinchart
0 siblings, 0 replies; 6+ messages in thread
From: Laurent Pinchart @ 2021-12-01 6:52 UTC (permalink / raw)
To: Alexander Stein
Cc: Dorota Czaplejewicz, Kieran Bingham, Andrey Konovalov,
Jacopo Mondi, Mauro Carvalho Chehab, Sakari Ailus, kernel,
linux-kernel, linux-media
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
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-12-01 6:53 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 11:59 [PATCH] media: Add 16-bit Bayer formats Dorota Czaplejewicz
2021-11-29 15:46 ` Kieran Bingham
2021-11-29 16:05 ` Dorota Czaplejewicz
2021-11-30 2:33 ` Laurent Pinchart
2021-11-30 7:43 ` (EXT) " Alexander Stein
2021-12-01 6:52 ` Laurent Pinchart
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).