| Message ID | 20200316070123.2434-1-dafna.hirschfeld@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: v4l2-common: Add BGR666 to v4l2_format_info | expand |
Hi Dafna, Thank you for the patch. On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: > Add V4L2_PIX_FMT_BGR666 to the format table. > > Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > --- > Hi, > BGR66 is needed for the rkisp1 driver. > Currently it crashes since the call to > v4l2_format_info returns NULL. > > drivers/media/v4l2-core/v4l2-common.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > index d0e5ebc736f9..d7f82b2aa22f 100644 > --- a/drivers/media/v4l2-core/v4l2-common.c > +++ b/drivers/media/v4l2-core/v4l2-common.c > @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) > { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, Isn't BGR666 stored in 3 bytes per pixel ? > > /* YUV packed formats */ > { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
Hi, On 16.03.20 08:22, Laurent Pinchart wrote: > Hi Dafna, > > Thank you for the patch. > > On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: >> Add V4L2_PIX_FMT_BGR666 to the format table. >> >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >> --- >> Hi, >> BGR66 is needed for the rkisp1 driver. >> Currently it crashes since the call to >> v4l2_format_info returns NULL. >> >> drivers/media/v4l2-core/v4l2-common.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >> index d0e5ebc736f9..d7f82b2aa22f 100644 >> --- a/drivers/media/v4l2-core/v4l2-common.c >> +++ b/drivers/media/v4l2-core/v4l2-common.c >> @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) >> { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >> { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >> { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >> + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > > Isn't BGR666 stored in 3 bytes per pixel ? Hi, I also discussed it with Helen. From the documentation we understood that it uses 4 bytes and the last one is empty. https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html Dafna > >> >> /* YUV packed formats */ >> { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 }, >
Hi Dafna, On Mon, Mar 16, 2020 at 09:07:16AM +0100, Dafna Hirschfeld wrote: > On 16.03.20 08:22, Laurent Pinchart wrote: > > On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: > >> Add V4L2_PIX_FMT_BGR666 to the format table. > >> > >> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >> --- > >> Hi, > >> BGR66 is needed for the rkisp1 driver. > >> Currently it crashes since the call to > >> v4l2_format_info returns NULL. > >> > >> drivers/media/v4l2-core/v4l2-common.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > >> index d0e5ebc736f9..d7f82b2aa22f 100644 > >> --- a/drivers/media/v4l2-core/v4l2-common.c > >> +++ b/drivers/media/v4l2-core/v4l2-common.c > >> @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) > >> { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >> { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >> { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >> + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > > > > Isn't BGR666 stored in 3 bytes per pixel ? > > Hi, I also discussed it with Helen. From the documentation we > understood that it uses 4 bytes and the last one is empty. > https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html Would you then also understand V4L2_PIX_FMT_RGB565 to use 4 bytes with the last 2 bytes empty ? :-) I agree that the documentation is somehow ambiguous and should be fixed. Patches are welcome ;-) > >> > >> /* YUV packed formats */ > >> { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
On 16.03.20 09:15, Laurent Pinchart wrote: > Hi Dafna, > > On Mon, Mar 16, 2020 at 09:07:16AM +0100, Dafna Hirschfeld wrote: >> On 16.03.20 08:22, Laurent Pinchart wrote: >>> On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: >>>> Add V4L2_PIX_FMT_BGR666 to the format table. >>>> >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>> --- >>>> Hi, >>>> BGR66 is needed for the rkisp1 driver. >>>> Currently it crashes since the call to >>>> v4l2_format_info returns NULL. >>>> >>>> drivers/media/v4l2-core/v4l2-common.c | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >>>> index d0e5ebc736f9..d7f82b2aa22f 100644 >>>> --- a/drivers/media/v4l2-core/v4l2-common.c >>>> +++ b/drivers/media/v4l2-core/v4l2-common.c >>>> @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) >>>> { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>> { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>> { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>> + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>> >>> Isn't BGR666 stored in 3 bytes per pixel ? >> >> Hi, I also discussed it with Helen. From the documentation we >> understood that it uses 4 bytes and the last one is empty. >> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html > > Would you then also understand V4L2_PIX_FMT_RGB565 to use 4 bytes with > the last 2 bytes empty ? :-) hmm, the formats between BGR24 and XRGB32 in the docs table have vertical lines crossing all 4 bytes so we understood that they are all 4 bytes. Isn't it? Format RGB565 doesn't have does vertical lines on the last two bytes which means it uses 2. At least that is what we understood. Dafna > > I agree that the documentation is somehow ambiguous and should be fixed. > Patches are welcome ;-) > >>>> >>>> /* YUV packed formats */ >>>> { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 }, >
Hi Dafna, On Mon, Mar 16, 2020 at 09:59:56AM +0100, Dafna Hirschfeld wrote: > On 16.03.20 09:15, Laurent Pinchart wrote: > > On Mon, Mar 16, 2020 at 09:07:16AM +0100, Dafna Hirschfeld wrote: > >> On 16.03.20 08:22, Laurent Pinchart wrote: > >>> On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: > >>>> Add V4L2_PIX_FMT_BGR666 to the format table. > >>>> > >>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> > >>>> --- > >>>> Hi, > >>>> BGR66 is needed for the rkisp1 driver. > >>>> Currently it crashes since the call to > >>>> v4l2_format_info returns NULL. > >>>> > >>>> drivers/media/v4l2-core/v4l2-common.c | 1 + > >>>> 1 file changed, 1 insertion(+) > >>>> > >>>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c > >>>> index d0e5ebc736f9..d7f82b2aa22f 100644 > >>>> --- a/drivers/media/v4l2-core/v4l2-common.c > >>>> +++ b/drivers/media/v4l2-core/v4l2-common.c > >>>> @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) > >>>> { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >>>> { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >>>> { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >>>> + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, > >>> > >>> Isn't BGR666 stored in 3 bytes per pixel ? > >> > >> Hi, I also discussed it with Helen. From the documentation we > >> understood that it uses 4 bytes and the last one is empty. > >> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html > > > > Would you then also understand V4L2_PIX_FMT_RGB565 to use 4 bytes with > > the last 2 bytes empty ? :-) > > hmm, the formats between BGR24 and XRGB32 in the docs table have vertical lines crossing all 4 bytes so we understood that they are all 4 bytes. Isn't it? > Format RGB565 doesn't have does vertical lines on the last two bytes which means it uses 2. At least that is what we understood. I stand corrected, looking at the drivers implementing V4L2_PIX_FMT_BGR666, they all handle it as a 32bpp format. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > I agree that the documentation is somehow ambiguous and should be fixed. > > Patches are welcome ;-) I think adding explicit '-' or 'x' in the cells that contain "don't care" bits would help avoiding confusion. > >>>> > >>>> /* YUV packed formats */ > >>>> { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
On 16.03.20 11:00, Laurent Pinchart wrote: > Hi Dafna, > > On Mon, Mar 16, 2020 at 09:59:56AM +0100, Dafna Hirschfeld wrote: >> On 16.03.20 09:15, Laurent Pinchart wrote: >>> On Mon, Mar 16, 2020 at 09:07:16AM +0100, Dafna Hirschfeld wrote: >>>> On 16.03.20 08:22, Laurent Pinchart wrote: >>>>> On Mon, Mar 16, 2020 at 08:01:23AM +0100, Dafna Hirschfeld wrote: >>>>>> Add V4L2_PIX_FMT_BGR666 to the format table. >>>>>> >>>>>> Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> >>>>>> --- >>>>>> Hi, >>>>>> BGR66 is needed for the rkisp1 driver. >>>>>> Currently it crashes since the call to >>>>>> v4l2_format_info returns NULL. >>>>>> >>>>>> drivers/media/v4l2-core/v4l2-common.c | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c >>>>>> index d0e5ebc736f9..d7f82b2aa22f 100644 >>>>>> --- a/drivers/media/v4l2-core/v4l2-common.c >>>>>> +++ b/drivers/media/v4l2-core/v4l2-common.c >>>>>> @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) >>>>>> { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>>>> { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>>>> { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>>>> + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, >>>>> >>>>> Isn't BGR666 stored in 3 bytes per pixel ? >>>> >>>> Hi, I also discussed it with Helen. From the documentation we >>>> understood that it uses 4 bytes and the last one is empty. >>>> https://linuxtv.org/downloads/v4l-dvb-apis/uapi/v4l/pixfmt-rgb.html >>> >>> Would you then also understand V4L2_PIX_FMT_RGB565 to use 4 bytes with >>> the last 2 bytes empty ? :-) >> >> hmm, the formats between BGR24 and XRGB32 in the docs table have vertical lines crossing all 4 bytes so we understood that they are all 4 bytes. Isn't it? >> Format RGB565 doesn't have does vertical lines on the last two bytes which means it uses 2. At least that is what we understood. > > I stand corrected, looking at the drivers implementing > V4L2_PIX_FMT_BGR666, they all handle it as a 32bpp format. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >>> I agree that the documentation is somehow ambiguous and should be fixed. >>> Patches are welcome ;-) > > I think adding explicit '-' or 'x' in the cells that contain "don't > care" bits would help avoiding confusion. sure, I'll do that > >>>>>> >>>>>> /* YUV packed formats */ >>>>>> { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 }, >
diff --git a/drivers/media/v4l2-core/v4l2-common.c b/drivers/media/v4l2-core/v4l2-common.c index d0e5ebc736f9..d7f82b2aa22f 100644 --- a/drivers/media/v4l2-core/v4l2-common.c +++ b/drivers/media/v4l2-core/v4l2-common.c @@ -253,6 +253,7 @@ const struct v4l2_format_info *v4l2_format_info(u32 format) { .format = V4L2_PIX_FMT_GREY, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 1, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, { .format = V4L2_PIX_FMT_RGB565, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, { .format = V4L2_PIX_FMT_RGB555, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, + { .format = V4L2_PIX_FMT_BGR666, .pixel_enc = V4L2_PIXEL_ENC_RGB, .mem_planes = 1, .comp_planes = 1, .bpp = { 4, 0, 0, 0 }, .hdiv = 1, .vdiv = 1 }, /* YUV packed formats */ { .format = V4L2_PIX_FMT_YUYV, .pixel_enc = V4L2_PIXEL_ENC_YUV, .mem_planes = 1, .comp_planes = 1, .bpp = { 2, 0, 0, 0 }, .hdiv = 2, .vdiv = 1 },
Add V4L2_PIX_FMT_BGR666 to the format table. Signed-off-by: Dafna Hirschfeld <dafna.hirschfeld@collabora.com> --- Hi, BGR66 is needed for the rkisp1 driver. Currently it crashes since the call to v4l2_format_info returns NULL. drivers/media/v4l2-core/v4l2-common.c | 1 + 1 file changed, 1 insertion(+)