[PATCH] media: bcm2835-unicam: Remove RGB24 support

[PATCH] media: bcm2835-unicam: Remove RGB24 support

[Maxime Ripard]

The RGB24 V4L2 format is defined as a pixel format with 8 bits per
components, with the components being in the red, green, and blue order
from left to right.

The RGB MIPI-CSI2 is defined in the specification (Section 11.3.1,
RGB888) with blue coming first, then green, then red. So the opposite of
what V4L2 means by RGB.

Since the hardware cannot reorder the components, this means that when
selecting the RGB24 format, you get inverted red and blue components
compared to what you'd expect.

The driver already supports BGR24, so we can simply remove the RGB24
format from the driver.

Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
Signed-off-by: Maxime Ripard <mripard@kernel.org>
---
 drivers/media/platform/broadcom/bcm2835-unicam.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
index f10064107d543caf867249d0566a0f42d6d8c4c6..1f549019efd53c9aae83193e74f1a3601ebf274d 100644
--- a/drivers/media/platform/broadcom/bcm2835-unicam.c
+++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
@@ -338,15 +338,10 @@ static const struct unicam_format_info unicam_image_formats[] = {
 	/* RGB Formats */
 		.fourcc		= V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
 		.code		= MEDIA_BUS_FMT_RGB565_1X16,
 		.depth		= 16,
 		.csi_dt		= MIPI_CSI2_DT_RGB565,
-	}, {
-		.fourcc		= V4L2_PIX_FMT_RGB24, /* rgb */
-		.code		= MEDIA_BUS_FMT_RGB888_1X24,
-		.depth		= 24,
-		.csi_dt		= MIPI_CSI2_DT_RGB888,
 	}, {
 		.fourcc		= V4L2_PIX_FMT_BGR24, /* bgr */
 		.code		= MEDIA_BUS_FMT_BGR888_1X24,
 		.depth		= 24,
 		.csi_dt		= MIPI_CSI2_DT_RGB888,

---
base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
change-id: 20250606-rpi-unicam-rgb-bgr-fix-d1b6f46a75ad

Best regards,
-- 
Maxime Ripard <mripard@kernel.org>

Re: [PATCH] media: bcm2835-unicam: Remove RGB24 support

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Fri, Jun 06, 2025 at 04:53:03PM +0200, Maxime Ripard wrote:
> The RGB24 V4L2 format is defined as a pixel format with 8 bits per
> components, with the components being in the red, green, and blue order
> from left to right.
> 
> The RGB MIPI-CSI2 is defined in the specification (Section 11.3.1,
> RGB888) with blue coming first, then green, then red. So the opposite of
> what V4L2 means by RGB.
> 
> Since the hardware cannot reorder the components, this means that when
> selecting the RGB24 format, you get inverted red and blue components
> compared to what you'd expect.
> 
> The driver already supports BGR24, so we can simply remove the RGB24
> format from the driver.

The only reason I could think of to explain why the driver exposes
V4L2_PIX_FMT_RGB24 is to support CSI-2 sources that transfer RGB888 data
with a non-standard order. I don't know what hardware would do that.
Dave, Naush, do you recall why this pixel format is supported by the
unicam driver ?

> Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> Signed-off-by: Maxime Ripard <mripard@kernel.org>
> ---
>  drivers/media/platform/broadcom/bcm2835-unicam.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> index f10064107d543caf867249d0566a0f42d6d8c4c6..1f549019efd53c9aae83193e74f1a3601ebf274d 100644
> --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> @@ -338,15 +338,10 @@ static const struct unicam_format_info unicam_image_formats[] = {
>  	/* RGB Formats */
>  		.fourcc		= V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
>  		.code		= MEDIA_BUS_FMT_RGB565_1X16,
>  		.depth		= 16,
>  		.csi_dt		= MIPI_CSI2_DT_RGB565,
> -	}, {
> -		.fourcc		= V4L2_PIX_FMT_RGB24, /* rgb */
> -		.code		= MEDIA_BUS_FMT_RGB888_1X24,
> -		.depth		= 24,
> -		.csi_dt		= MIPI_CSI2_DT_RGB888,
>  	}, {
>  		.fourcc		= V4L2_PIX_FMT_BGR24, /* bgr */
>  		.code		= MEDIA_BUS_FMT_BGR888_1X24,
>  		.depth		= 24,
>  		.csi_dt		= MIPI_CSI2_DT_RGB888,
> 
> ---
> base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
> change-id: 20250606-rpi-unicam-rgb-bgr-fix-d1b6f46a75ad

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: bcm2835-unicam: Remove RGB24 support

[Dave Stevenson]

Hi Laurent & Maxime

On Mon, 9 Jun 2025 at 01:38, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Maxime,
>
> Thank you for the patch.
>
> On Fri, Jun 06, 2025 at 04:53:03PM +0200, Maxime Ripard wrote:
> > The RGB24 V4L2 format is defined as a pixel format with 8 bits per
> > components, with the components being in the red, green, and blue order
> > from left to right.
> >
> > The RGB MIPI-CSI2 is defined in the specification (Section 11.3.1,
> > RGB888) with blue coming first, then green, then red. So the opposite of
> > what V4L2 means by RGB.
> >
> > Since the hardware cannot reorder the components, this means that when
> > selecting the RGB24 format, you get inverted red and blue components
> > compared to what you'd expect.
> >
> > The driver already supports BGR24, so we can simply remove the RGB24
> > format from the driver.
>
> The only reason I could think of to explain why the driver exposes
> V4L2_PIX_FMT_RGB24 is to support CSI-2 sources that transfer RGB888 data
> with a non-standard order. I don't know what hardware would do that.
> Dave, Naush, do you recall why this pixel format is supported by the
> unicam driver ?

I've lost track of exactly what gets validated along the pipeline.
unicam_video_link_validate [1] looks to ensure that the V4L2 pixel
format and media bus codes match as listed in the table.

tc358743, adv7604, adv7511, and adv748x are all saying they produce
MEDIA_BUS_FMT_RGB888_1X24
ov5640 says it produces MEDIA_BUS_FMT_BGR888_1X24.

Is that an error in the ov5640 driver? If not, then both entries have
to be in the table to support all those drivers.

Looking at alvium-csi2.c and st-mipid02.c, both mappings are included
there (and RBG888 in the case of alvium).
Unicam's hardware doesn't care about the ordering as it just writes
the incoming data to memory, so having all the sensible mappings
between MEDIA_BUS_FMT_* and V4L2_PIX_FMT_* values makes sense.


My initial reaction though is that simply removing the entry won't
solve the problem anyway. You won't get a match between the
MEDIA_BUS_FMT_RGB888_1X24 requested by tc358743 and a supported V4L2
pixel format, so the link_validate will fail.
Swapping either fourcc or code between the two entries would be the
fix I was expecting.

  Dave

[1] https://github.com/torvalds/linux/blob/master/drivers/media/platform/broadcom/bcm2835-unicam.c#L2151-L2169

> > Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > ---
> >  drivers/media/platform/broadcom/bcm2835-unicam.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > index f10064107d543caf867249d0566a0f42d6d8c4c6..1f549019efd53c9aae83193e74f1a3601ebf274d 100644
> > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > @@ -338,15 +338,10 @@ static const struct unicam_format_info unicam_image_formats[] = {
> >       /* RGB Formats */
> >               .fourcc         = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
> >               .code           = MEDIA_BUS_FMT_RGB565_1X16,
> >               .depth          = 16,
> >               .csi_dt         = MIPI_CSI2_DT_RGB565,
> > -     }, {
> > -             .fourcc         = V4L2_PIX_FMT_RGB24, /* rgb */
> > -             .code           = MEDIA_BUS_FMT_RGB888_1X24,
> > -             .depth          = 24,
> > -             .csi_dt         = MIPI_CSI2_DT_RGB888,
> >       }, {
> >               .fourcc         = V4L2_PIX_FMT_BGR24, /* bgr */
> >               .code           = MEDIA_BUS_FMT_BGR888_1X24,
> >               .depth          = 24,
> >               .csi_dt         = MIPI_CSI2_DT_RGB888,
> >
> > ---
> > base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
> > change-id: 20250606-rpi-unicam-rgb-bgr-fix-d1b6f46a75ad
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH] media: bcm2835-unicam: Remove RGB24 support

[Laurent Pinchart]

On Mon, Jun 09, 2025 at 03:33:29PM +0100, Dave Stevenson wrote:
> On Mon, 9 Jun 2025 at 01:38, Laurent Pinchart wrote:
> > On Fri, Jun 06, 2025 at 04:53:03PM +0200, Maxime Ripard wrote:
> > > The RGB24 V4L2 format is defined as a pixel format with 8 bits per
> > > components, with the components being in the red, green, and blue order
> > > from left to right.
> > >
> > > The RGB MIPI-CSI2 is defined in the specification (Section 11.3.1,
> > > RGB888) with blue coming first, then green, then red. So the opposite of
> > > what V4L2 means by RGB.
> > >
> > > Since the hardware cannot reorder the components, this means that when
> > > selecting the RGB24 format, you get inverted red and blue components
> > > compared to what you'd expect.
> > >
> > > The driver already supports BGR24, so we can simply remove the RGB24
> > > format from the driver.
> >
> > The only reason I could think of to explain why the driver exposes
> > V4L2_PIX_FMT_RGB24 is to support CSI-2 sources that transfer RGB888 data
> > with a non-standard order. I don't know what hardware would do that.
> > Dave, Naush, do you recall why this pixel format is supported by the
> > unicam driver ?
> 
> I've lost track of exactly what gets validated along the pipeline.
> unicam_video_link_validate [1] looks to ensure that the V4L2 pixel
> format and media bus codes match as listed in the table.
> 
> tc358743, adv7604, adv7511, and adv748x are all saying they produce
> MEDIA_BUS_FMT_RGB888_1X24
> ov5640 says it produces MEDIA_BUS_FMT_BGR888_1X24.
> 
> Is that an error in the ov5640 driver? If not, then both entries have
> to be in the table to support all those drivers.

We haven't standardized which media bus code to use for 24-bit RGB on
CSI-2, so I'm not surprised drivers didn't get it right :-( The adv7604
and adv7511 shouldn't be relevant here though, as they're not CSI-2
devices. tc358743 and adv748x are relevant.

Sakari, which media bus code would you expect ?

> Looking at alvium-csi2.c and st-mipid02.c, both mappings are included
> there (and RBG888 in the case of alvium).
> Unicam's hardware doesn't care about the ordering as it just writes
> the incoming data to memory, so having all the sensible mappings
> between MEDIA_BUS_FMT_* and V4L2_PIX_FMT_* values makes sense.
> 
> My initial reaction though is that simply removing the entry won't
> solve the problem anyway. You won't get a match between the
> MEDIA_BUS_FMT_RGB888_1X24 requested by tc358743 and a supported V4L2
> pixel format, so the link_validate will fail.
> Swapping either fourcc or code between the two entries would be the
> fix I was expecting.
> 
> [1] https://github.com/torvalds/linux/blob/master/drivers/media/platform/broadcom/bcm2835-unicam.c#L2151-L2169
> 
> > > Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > ---
> > >  drivers/media/platform/broadcom/bcm2835-unicam.c | 5 -----
> > >  1 file changed, 5 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > index f10064107d543caf867249d0566a0f42d6d8c4c6..1f549019efd53c9aae83193e74f1a3601ebf274d 100644
> > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > @@ -338,15 +338,10 @@ static const struct unicam_format_info unicam_image_formats[] = {
> > >       /* RGB Formats */
> > >               .fourcc         = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
> > >               .code           = MEDIA_BUS_FMT_RGB565_1X16,
> > >               .depth          = 16,
> > >               .csi_dt         = MIPI_CSI2_DT_RGB565,
> > > -     }, {
> > > -             .fourcc         = V4L2_PIX_FMT_RGB24, /* rgb */
> > > -             .code           = MEDIA_BUS_FMT_RGB888_1X24,
> > > -             .depth          = 24,
> > > -             .csi_dt         = MIPI_CSI2_DT_RGB888,
> > >       }, {
> > >               .fourcc         = V4L2_PIX_FMT_BGR24, /* bgr */
> > >               .code           = MEDIA_BUS_FMT_BGR888_1X24,
> > >               .depth          = 24,
> > >               .csi_dt         = MIPI_CSI2_DT_RGB888,
> > >
> > > ---
> > > base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
> > > change-id: 20250606-rpi-unicam-rgb-bgr-fix-d1b6f46a75ad

-- 
Regards,

Laurent Pinchart

Re: [PATCH] media: bcm2835-unicam: Remove RGB24 support

[Sakari Ailus]

Hi Laurent, others,

On Tue, Jun 10, 2025 at 12:52:38PM +0300, Laurent Pinchart wrote:
> On Mon, Jun 09, 2025 at 03:33:29PM +0100, Dave Stevenson wrote:
> > On Mon, 9 Jun 2025 at 01:38, Laurent Pinchart wrote:
> > > On Fri, Jun 06, 2025 at 04:53:03PM +0200, Maxime Ripard wrote:
> > > > The RGB24 V4L2 format is defined as a pixel format with 8 bits per
> > > > components, with the components being in the red, green, and blue order
> > > > from left to right.
> > > >
> > > > The RGB MIPI-CSI2 is defined in the specification (Section 11.3.1,
> > > > RGB888) with blue coming first, then green, then red. So the opposite of
> > > > what V4L2 means by RGB.
> > > >
> > > > Since the hardware cannot reorder the components, this means that when
> > > > selecting the RGB24 format, you get inverted red and blue components
> > > > compared to what you'd expect.
> > > >
> > > > The driver already supports BGR24, so we can simply remove the RGB24
> > > > format from the driver.
> > >
> > > The only reason I could think of to explain why the driver exposes
> > > V4L2_PIX_FMT_RGB24 is to support CSI-2 sources that transfer RGB888 data
> > > with a non-standard order. I don't know what hardware would do that.
> > > Dave, Naush, do you recall why this pixel format is supported by the
> > > unicam driver ?
> > 
> > I've lost track of exactly what gets validated along the pipeline.
> > unicam_video_link_validate [1] looks to ensure that the V4L2 pixel
> > format and media bus codes match as listed in the table.
> > 
> > tc358743, adv7604, adv7511, and adv748x are all saying they produce
> > MEDIA_BUS_FMT_RGB888_1X24
> > ov5640 says it produces MEDIA_BUS_FMT_BGR888_1X24.
> > 
> > Is that an error in the ov5640 driver? If not, then both entries have
> > to be in the table to support all those drivers.
> 
> We haven't standardized which media bus code to use for 24-bit RGB on
> CSI-2, so I'm not surprised drivers didn't get it right :-( The adv7604
> and adv7511 shouldn't be relevant here though, as they're not CSI-2
> devices. tc358743 and adv748x are relevant.

My understanding is the order indeed is BGR. But let's say that if there's
a device that generates RGB (as in pixel order) pixel data, then it should
be reflected in V4L2 pixelformat as well.

> 
> Sakari, which media bus code would you expect ?

MEDIA_BUS_FMT_BGR888_1X24. That's actually used in the example as well:
<URL:https://hverkuil.home.xs4all.nl/spec/userspace-api/v4l/subdev-formats.html#v4l2-mbus-pixelcode>.

> 
> > Looking at alvium-csi2.c and st-mipid02.c, both mappings are included
> > there (and RBG888 in the case of alvium).
> > Unicam's hardware doesn't care about the ordering as it just writes
> > the incoming data to memory, so having all the sensible mappings
> > between MEDIA_BUS_FMT_* and V4L2_PIX_FMT_* values makes sense.
> > 
> > My initial reaction though is that simply removing the entry won't
> > solve the problem anyway. You won't get a match between the
> > MEDIA_BUS_FMT_RGB888_1X24 requested by tc358743 and a supported V4L2
> > pixel format, so the link_validate will fail.
> > Swapping either fourcc or code between the two entries would be the
> > fix I was expecting.
> > 
> > [1] https://github.com/torvalds/linux/blob/master/drivers/media/platform/broadcom/bcm2835-unicam.c#L2151-L2169
> > 
> > > > Fixes: 392cd78d495f ("media: bcm2835-unicam: Add support for CCP2/CSI2 camera interface")
> > > > Signed-off-by: Maxime Ripard <mripard@kernel.org>
> > > > ---
> > > >  drivers/media/platform/broadcom/bcm2835-unicam.c | 5 -----
> > > >  1 file changed, 5 deletions(-)
> > > >
> > > > diff --git a/drivers/media/platform/broadcom/bcm2835-unicam.c b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > index f10064107d543caf867249d0566a0f42d6d8c4c6..1f549019efd53c9aae83193e74f1a3601ebf274d 100644
> > > > --- a/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > +++ b/drivers/media/platform/broadcom/bcm2835-unicam.c
> > > > @@ -338,15 +338,10 @@ static const struct unicam_format_info unicam_image_formats[] = {
> > > >       /* RGB Formats */
> > > >               .fourcc         = V4L2_PIX_FMT_RGB565, /* gggbbbbb rrrrrggg */
> > > >               .code           = MEDIA_BUS_FMT_RGB565_1X16,
> > > >               .depth          = 16,
> > > >               .csi_dt         = MIPI_CSI2_DT_RGB565,
> > > > -     }, {
> > > > -             .fourcc         = V4L2_PIX_FMT_RGB24, /* rgb */
> > > > -             .code           = MEDIA_BUS_FMT_RGB888_1X24,
> > > > -             .depth          = 24,
> > > > -             .csi_dt         = MIPI_CSI2_DT_RGB888,
> > > >       }, {
> > > >               .fourcc         = V4L2_PIX_FMT_BGR24, /* bgr */
> > > >               .code           = MEDIA_BUS_FMT_BGR888_1X24,
> > > >               .depth          = 24,
> > > >               .csi_dt         = MIPI_CSI2_DT_RGB888,
> > > >
> > > > ---
> > > > base-commit: 0ff41df1cb268fc69e703a08a57ee14ae967d0ca
> > > > change-id: 20250606-rpi-unicam-rgb-bgr-fix-d1b6f46a75ad
> 

-- 
Regards,

Sakari Ailus