[RFC PATCH] drm/panel: simple: panel-dpi: use bus-format to set bpc and bus_format

[Maxime Ripard]

On Fri, Mar 18, 2022 at 05:05:11PM +0000, Dave Stevenson wrote:
> Hi Maxime
> 
> On Fri, 18 Mar 2022 at 16:35, Maxime Ripard <maxime at cerno.tech> wrote:
> >
> > On Mon, Mar 07, 2022 at 04:26:56PM +0100, Max Krummenacher wrote:
> > > On Wed, Mar 2, 2022 at 5:22 PM Marek Vasut <marex at denx.de> wrote:
> > > >
> > > > On 3/2/22 15:21, Maxime Ripard wrote:
> > > > > Hi,
> > > >
> > > > Hi,
> > > >
> > > > > Please try to avoid top posting
> > > Sorry.
> > >
> > > > >
> > > > > On Wed, Feb 23, 2022 at 04:25:19PM +0100, Max Krummenacher wrote:
> > > > >> The goal here is to set the element bus_format in the struct
> > > > >> panel_desc. This is an enum with the possible values defined in
> > > > >> include/uapi/linux/media-bus-format.h.
> > > > >>
> > > > >> The enum values are not constructed in a way that you could calculate
> > > > >> the value from color channel width/shift/mapping/whatever. You rather
> > > > >> would have to check if the combination of color channel
> > > > >> width/shift/mapping/whatever maps to an existing value and otherwise
> > > > >> EINVAL out.
> > > > >>
> > > > >> I don't see the value in having yet another way of how this
> > > > >> information can be specified and then having to write a more
> > > > >> complicated parser which maps the dt data to bus_format.
> > > > >
> > > > > Generally speaking, sending an RFC without explicitly stating what you
> > > > > want a comment on isn't very efficient.
> > > >
> > > > Isn't that what RFC stands for -- Request For Comment ?
> > >
> > > I hoped that the link to the original discussion was enough.
> > >
> > > panel-simple used to have a finite number of hardcoded panels selected
> > > by their compatible.
> > > The following patchsets added a compatible 'panel-dpi' which should
> > > allow to specify the panel in the device tree with timing etc.
> > >   https://patchwork.kernel.org/project/dri-devel/patch/20200216181513.28109-6-sam@ravnborg.org/
> > > In the same release cycle part of it got reverted:
> > >   https://patchwork.kernel.org/project/dri-devel/patch/20200314153047.2486-3-sam@ravnborg.org/
> > > With this it is no longer possible to set bus_format.
> > >
> > > The explanation what makes the use of a property "data-mapping" not a
> > > suitable way in that revert
> > > is a bit vague.
> >
> > Indeed, but I can only guess. BGR666 in itself doesn't mean much for
> > example. Chances are the DPI interface will use a 24 bit bus, so where
> > is the padding?
> >
> > I think that's what Sam and Laurent were talking about: there wasn't
> > enough information encoded in that property to properly describe the
> > format, hence the revert.
> 
> MEDIA_BUS_FMT_RGB666_1X18 defines an 18bit bus, therefore there is no
> padding. "bgr666" was selecting that media bus code (I won't ask about
> the rgb/bgr swap).
> 
> If there is padding on a 24 bit bus, then you'd use (for example)
> MEDIA_BUS_FMT_RGB666_1X24_CPADHI to denote that the top 2 bits of each
> colour are the padding. Define and use a PADLO variant if the padding
> is the low bits.

Yeah, that's kind of my point actually :)

Just having a rgb666 string won't allow to differentiate between
MEDIA_BUS_FMT_RGB666_1X18 and MEDIA_BUS_FMT_RGB666_1X24_CPADHI: both are
RGB666 formats. Or we could say that it's MEDIA_BUS_FMT_RGB666_1X18 and
then when we'll need MEDIA_BUS_FMT_RGB666_1X24_CPADHI we'll add a new
string but that usually leads to inconsistent or weird names, so this
isn't ideal.

> The string matching would need to be extended to have some string to
> select those codes ("lvds666" is a weird choice from the original
> patch).
> 
> Taking those media bus codes and handling them appropriately is
> already done in vc4_dpi [1], and the vendor tree has gained
> BGR666_1X18 and BGR666_1X24_CPADHI [2] as they aren't defined in
> mainline.
> 
> Now this does potentially balloon out the number of MEDIA_BUS_FMT_xxx
> defines needed, but that's the downside of having defines for all
> formats.
> 
> (I will admit to having a similar change in the Pi vendor tree that
> allows the media bus code to be selected explicitly by hex value).

I think having an integer value is indeed better: it doesn't change much
in the device tree if we're using a header, it makes the driver simpler
since we don't have to parse a string, and we can easily extend it or
rename the define, it won't change the ABI.

I'm not sure using the raw media bus format value is ideal though, since
that value could then be used by any OS, and it would effectively force
the mbus stuff down their throat.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220318/167f4daf/attachment-0001.sig>