Message-ID: <20251208-powerful-penguin-of-reputation-ffdc5c@houat>
Date: Mon, 8 Dec 2025 16:32:33 +0100
From: Maxime Ripard <mripard@...nel.org>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Mats Randgaard <matrandg@...co.com>, Alain Volmat <alain.volmat@...s.st.com>, 
	Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans Verkuil <hverkuil@...nel.org>, linux-media@...r.kernel.org, 
	linux-kernel@...r.kernel.org, Hans Verkuil <hans.verkuil@...co.com>, 
	Dave Stevenson <dave.stevenson@...pberrypi.com>
Subject: Re: [PATCH v4 2/4] media: uapi: Introduce MEDIA_BUS_FMT_BGR565_1X16

Hi Laurent,

On Mon, Oct 27, 2025 at 01:33:08AM +0200, Laurent Pinchart wrote:
> On Mon, Oct 27, 2025 at 01:15:54AM +0200, Laurent Pinchart wrote:
> > On Mon, Oct 13, 2025 at 01:01:34PM +0200, Maxime Ripard wrote:
> > > MIPI-CSI2 sends its RGB format on the wire with the blue component
> > > first, then green, then red. MIPI calls that format "RGB", but by v4l2
> > > conventions it would be BGR.
> > > 
> > > MIPI-CSI2 supports three RGB variants: 444, 555, 565, 666 and 888.
> > > 
> > > We already have BGR666 and BGR888 media bus formats, we don't have any
> > > CSI transceivers using the 444 and 555 variants, but some transceivers
> > > use the CSI RGB565 format, while using the RGB565 media bus code.
> > > 
> > > That's a mistake, but since we don't have a BGR565 media bus code we
> > > need to introduce one before fixing it.
> > > 
> > > Signed-off-by: Maxime Ripard <mripard@...nel.org>
> > > ---
> > >  .../userspace-api/media/v4l/subdev-formats.rst     | 37 ++++++++++++++++++++++
> > >  include/uapi/linux/media-bus-format.h              |  3 +-
> > >  2 files changed, 39 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/Documentation/userspace-api/media/v4l/subdev-formats.rst b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > index 8e92f784abd8123f9ea950f954a60af56ee76dbe..def0d24ef6cdb1a2ec9395af1468f56adf31a8de 100644
> > > --- a/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > +++ b/Documentation/userspace-api/media/v4l/subdev-formats.rst
> > > @@ -625,10 +625,47 @@ The following tables list existing packed RGB formats.
> > >        - b\ :sub:`4`
> > >        - b\ :sub:`3`
> > >        - b\ :sub:`2`
> > >        - b\ :sub:`1`
> > >        - b\ :sub:`0`
> > > +    * .. _MEDIA-BUS-FMT-BGR565-1X16:
> > > +
> > > +      - MEDIA_BUS_FMT_BGR565_1X16
> > > +      - 0x1028
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      -
> > > +      - b\ :sub:`4`
> > > +      - b\ :sub:`3`
> > > +      - b\ :sub:`2`
> > > +      - b\ :sub:`1`
> > > +      - b\ :sub:`0`
> > > +      - g\ :sub:`5`
> > > +      - g\ :sub:`4`
> > > +      - g\ :sub:`3`
> > > +      - g\ :sub:`2`
> > > +      - g\ :sub:`1`
> > > +      - g\ :sub:`0`
> > > +      - r\ :sub:`4`
> > > +      - r\ :sub:`3`
> > > +      - r\ :sub:`2`
> > > +      - r\ :sub:`1`
> > > +      - r\ :sub:`0`
> > 
> > We're definitely in convention territory, because this is not how 16-bit
> > RGB data is transmitted over CSI-2. CSI-2 transmits blue first, but
> > starts with bit 0, not bit 4.
> > 
> > Have you explored the alternative of picking the parallel bus code that
> > matches the serial order when transmitted with the least significant bit
> > first ? That would be MEDIA_BUS_FMT_RGB565_1X16 here, and
> > MEDIA_BUS_FMT_RGB888_1X24 for 24-bit RGB.
> 
> To be clear, media bus codes are a matter of conventions. Some
> conventions would be easier to explain that others, and can also be more
> consistent with pixel format namings, but at the end of the day they're
> all conventions. While saying "pick the media bus code that transmits a
> pixel in one clock sample, with the bit order matching LSB-first
> transmission" could be the simplest to document, there will be a
> mismatch in component orders between the media bus code and the pixel
> format in some cases. There may also be more drivers implementing other
> conventions, making the transition more difficult.
> 
> I'll be very busy the upcoming week and will likely not be able to
> participate in this discussion in the near future.

For the record, we've discussed it on IRC recently.

The suggestion to have all CSI Data Formats as MEDIA_BUS_FMT_RGB*_1X*
variants make sense to me. And we can easily document it, because we
could match the first bit transmitted with the least significant bit
of a media bus code indeed.

Thus a sensor using RGB888 would register MEDIA_BUS_FMT_RGB888_1X24.
That's indeed the case right now with tc358743:
https://elixir.bootlin.com/linux/v6.18/source/drivers/media/i2c/tc358743.c#L1775

Unicam however hardcodes (and validates) that the v4l2 format codes
matches the media bus code of the other end:

https://elixir.bootlin.com/linux/v6.18/source/drivers/media/platform/broadcom/bcm2835-unicam.c#L1333

That alone makes total sense, but it has an association between
V4l2_PIX_FMT_RGB24 and MEDIA_BUS_FMT_RGB888_1X24, and between
V4L2_PIX_FMT_BGR24 and MEDIA_BUS_FMT_BGR888_1X24

https://elixir.bootlin.com/linux/v6.18/source/drivers/media/platform/broadcom/bcm2835-unicam.c#L343

Using the convention you suggested, this association is wrong, and
V4L2_PIX_FMT_BGR24 should be associated MEDIA_BUS_FMT_RGB888_1X24. Thus,
the red and blue color components are mixed up.

I initially tried to fix it in my v1 by removing the RGB24 support
https://lore.kernel.org/all/20250606-rpi-unicam-rgb-bgr-fix-v1-1-9930b963f3eb@kernel.org/

This was shot down (rightfully) because it would still be broken.

The second version changed the media bus tc358743 reported:
https://lore.kernel.org/all/20250612-csi-bgr-rgb-v1-0-dc8a309118f8@kernel.org/

Dave was against it because it would potentially break userspace, citing
Linus that we shouldn't break userspace ever. I understand and somewhat
agree with his point, but having two drivers reporting the same data
format but with a different meaning is also a way of breaking userspace.

Anyway. It was then suggested to support both in the tc358743. That's
what the second, third and fourth that you commented on worked towards.

https://lore.kernel.org/all/20250911-csi-bgr-rgb-v2-0-e6c6b10c1040@kernel.org/
https://lore.kernel.org/all/20250917-csi-bgr-rgb-v3-0-0145571b3aa4@kernel.org/
https://lore.kernel.org/all/20251013-csi-bgr-rgb-v4-0-55eab2caa69f@kernel.org/

In order to implement your suggestion, I wouldn't to modify tc358743,
but would need to modify the association between the v4l2 format and
media bus code that unicam has. In a way, it's very similar to my first
version that got shot down, and suffers from the same flaws: we could
have a userspace application out there hardcoding formats and codes that
will get an error.

So I'm not sure your suggestion really works, unless we reevaluate what
we mean by breaking userspace. Either way, I don't care, I just want to
get pixels in the expected (and documented!) order when using unicam.

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists