Re: [PATCH v3 2/4] dt-bindings: display: add new bus-format property for panel-dpi

[Dave Stevenson]

Hi Francesco

On Thu, 13 Oct 2022 at 13:58, Francesco Dolcini <francesco@xxxxxxxxxx> wrote:
>
> Hello Max, Marek, Dave et al.
>
> On Tue, Jun 28, 2022 at 08:18:36PM +0200, Max Krummenacher wrote:
> > From: Max Krummenacher <max.krummenacher@xxxxxxxxxxx>
> >
> > The property is used to set the enum bus_format and infer the bpc
> > for a panel defined by 'panel-dpi'.
> > This specifies how the panel is connected to the display interface.
> >
> > Signed-off-by: Max Krummenacher <max.krummenacher@xxxxxxxxxxx>
> >
>
> <snip>
>
> > diff --git a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > index dae0676b5c6e..52f5db03b6a8 100644
> > --- a/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > +++ b/Documentation/devicetree/bindings/display/panel/panel-dpi.yaml
> > @@ -26,7 +26,28 @@ properties:
> >    height-mm: true
> >    label: true
> >    panel-timing: true
> > -  port: true
> > +
> > +  port:
> > +    $ref: /schemas/graph.yaml#/$defs/port-base
> > +    description:
> > +      Input port node, receives the panel data.
> > +
> > +    properties:
> > +      endpoint:
> > +        $ref: /schemas/graph.yaml#/$defs/endpoint-base
> > +
> > +        properties:
> > +          bus-format:
> > +            $ref: /schemas/types.yaml#/definitions/uint32
> > +            minimum: 0x1001
> > +            maximum: 0x1fff
> > +            description: |
> > +              Describes how the display panel is connected to the display interface.
> > +              Valid values are defined in <dt-bindings/display/dt-media-bus-format.h>.
> > +              The mapping between the color/significance of the panel lines to the
> > +              parallel data lines are defined in:
> > +              https://www.kernel.org/doc/html/v5.17/userspace-api/media/v4l/subdev-formats.html#packed-rgb-formats
> > +
>
> Last month I had the chance to talk in person about this topic with
> Dave, Marek and Max in Dublin.
>
> My understanding is that this change is addressing a general need, Dave
> confirmed me they have a downstream patch for raspberrypi [1].
>
> From what I could tell the only concern is about the actual encoding of
> this `bus-format` property.
>
> I am personally convinced that a simple enum is the way to go, I think
> that Marek proposal is adding complexity and not flexibility (from my
> understanding Dave is on the same page, just correct me if I
> misunderstood you).

Yes I agree with you here.

This binding is for the panel, and currently the only path to pass the
panel mode to the DPI transmitter is one or more MEDIA_BUS_FMT_* enums
in struct drm_display_info *bus_formats.

Looking at Marek's comment over DSI and data-lanes, yes both source
and sink could advertise a data-lanes property to cover the condition
where they aren't wired up in a 1:1 fashion. Reality is that most
drivers don't support reordering the lanes - looking at the bindings,
only one (msm) documents the use of data-lanes on the host side.
rcar_mipi_dsi looks at the number of lanes specified only, and then
checks that the number requested by the device is <= the number
configured.

As I see it, the comparison here is that this "bus-format" property is
the equivalent of the data-lanes on the sink, and is the desired
number of lanes value passed from sink to source (one integer, not a
mapping).
If the source can reorder the lanes, then that is a property of the
source. This binding is for the sink, and so isn't a reasonable
comparison. It also doesn't have to be called "bus-format" on the
source, and can take a totally different form.
I'll admit that I know data-lane configuration more from CSI2, but
within V4L2 it is the node that can support reordering that should
have the lanes in a non-incrementing order, and that is normally the
SoC rather than the sensor. The same would seem to apply here - it's
the SoC that can remap the signals, not the panel.

It could be argued that for DPI the panel should only advertise the
panel's bit depth for each channel, not the padding. The panel is
generic and could handle any wiring/padding options, and it isn't
necessarily a simple 16/18/24/32 bit bus representation, just a
collection of N wires.
Padding and wiring is a function of the DPI transmitter / SoC, or
potentially an interconnect node between the two.

> The current proposal is already encoding the exact bit placing as
> described in Documentation/userspace-api/media/v4l/subdev-formats.rst [2],
> this enumeration can be extended to address any future needs
> and I would not invent a new one to define the exact same
> things (and using the same enum was also suggested by Rob).
>
> Marek: you told me that you had some concern about some valid use case
> not covered by this solution, would you mind explaining why that would
> not be covered with an addition on this enumeration?

All the MEDIA_BUS_FMT_* enums are explicitly defined with regard to
the colour component bit positions. Should someone be in the position
of needing to implement a YUV or similar DPI display, converting these
enums into the relevant new structure will be straightforward, so
backwards compatibility can be achieved easily.

  Dave

> Any other opinion on this topic? How can we move this forward?
>
> Francesco
>
> [1] https://github.com/raspberrypi/linux/commit/8e43f1898191b43aa7ed6e6ca3a4cd28709af86d
> [2] https://www.kernel.org/doc/html/latest/userspace-api/media/v4l/subdev-formats.html
>