Re: [PATCH] drm: panels: Add MAINTAINERS entry for LVS panel driver

[Rob Herring <robh@kernel.org>]

On Wed, Apr 12, 2017 at 10:26 AM, Thierry Reding <treding@nvidia.com> wrote:
> On Tue, Apr 11, 2017 at 12:10:47PM -0500, Rob Herring wrote:
>> On Fri, Apr 7, 2017 at 12:33 PM, Thierry Reding <treding@nvidia.com> wrote:
>> > On Fri, Apr 07, 2017 at 05:44:15AM +1000, Dave Airlie wrote:
>> >> On 5 April 2017 at 16:51, Laurent Pinchart
>> >> <laurent.pinchart+renesas@ideasonboard.com> wrote:
>> >> > As the DRM LVDS panel driver uses a different approach to DT bindings
>> >> > compared to what Thierry Reding advocates, add a specific MAINTAINERS
>> >> > entry to avoid bothering Thierry with requests related to that driver.
>>
>> This is not a good split. Panels should not be diverging based on
>> interface. That's not to say we can't have sub-classed bindings based
>> on interfaces.
>>
>> >> Could you document a bit more in the patch summary the finer points of
>> >> panel/dt doctrine, as I haven't got as much knowledge as I'd like.
>> >>
>> >> Just I believe, Thierry believes.
>> >
>> > I'm somewhat surprised how we arrived at the current situation. A very
>> > long time ago when we first discussed device tree bindings for panels, a
>> > number of attempts were made to generically describe everything in
>> > device tree. All of those attempts failed because you simply couldn't
>> > describe all of the required properties in DT in a sane way.
>> >
>> > Eventually everyone involved agreed that we would have to stick with the
>> > device-specific compatible, and in the best case we would be able to
>> > support many panels with a fairly generic driver. I think we did pretty
>> > well with the panel-simple driver. It started out very simple and then
>> > got improved over time as necessary to deal with more panels. And for
>> > cases where it wasn't suitable we simply added a custom driver. That's a
>> > completely natural way to write drivers. We do the same thing in other
>> > areas, nothing special here.
>>
>> That is still the case here.
>>
>> > Ever since the simple-panel binding was introduced, which is now about
>> > 3 1/2 years ago, people have kept asking why we couldn't simply put all
>> > data in DT and why kernel drivers had to be modified in order to add
>> > support for a new panel. I kept repeating myself a number of times until
>> > I finally wrote it all up[0], after which it was enough to point people
>> > to it. Still not everyone was convinced, but the people that were there
>> > when we made the decision all agreed that this was still the right thing
>> > to do. So, despite the many complaints I stuck to what we had agreed on
>> > because I am convinced that it is the right thing to do.
>>
>> The big difference was folks wanted "simple-panel" compatible strings
>> and nothing else. That remains wrong and is a constant discussion. I'd
>> say at least 30% of my reviews contain "needs a more specific
>> compatible string". Panels are not the only "simple" or "generic"
>> hardware. :)
>
> But that's really what panel-lvds is all about. While the binding
> documentation says that it needs "panel-lvds" and a panel-specific
> compatible string, the reality is that if we only use "panel-lvds"
> there's no way to fully describe the panel, and I suspect that we
> will be painting ourselves into a corner.

Using only "panel-lvds" is not valid. Now I generally don't think the
kernel should validate DTs, but if abusing it is a concern let's add a
big fat WARN if that's the only compatible.

> For example, the panel-lvds binding defines a couple of required and
> optional properties. None of that includes any power supplies. These
> power supplies are defined in panel-specific bindings, because they
> really are panel-specific. Matching on panel-lvds doesn't give you
> information about the regulators. The fact that the driver doesn't
> handle power supplies at all and has a large TODO comment about it
> doesn't give me much confidence that this is well thought out.

Right. The only way you should ever match on panel-lvds is if the
panel needs no power control. That can evolve with the OS over time as
needed, but the bindings shouldn't change.

simple-panel suffers from the same problem here. power supplies are
often not being described correctly when there is more than one. It's
only later or on another board that both the binding and driver get
fixed. I personally think having "power-supply" in simple-panel may
have been a mistake because folks just omit describing the supplies
correctly. I've been getting stricter on this as you've seen.

> So if matching on "panel-lvds" doesn't give you enough information to do
> anything useful with the panel, then I think it really shouldn't be
> there at all in the first place. If you look back at the history of some
> early simple-panel users, we used to include "simple-panel" in the list
> of compatible strings as well. Then we realized that there's really no
> use for it to be there because by itself it doesn't provide concrete
> information about the device. The panel-specific property already
> implies it. It's equivalent to having:
>
>         compatible = "nvidia,tegra20-gpio", "gpio-controller";
>
> "gpio-controller" is much too abstract to be useful in any way. And so
> is "simple-panel" or "panel-lvds".

Plus nodes are already tagged with "gpio-controller" as a boolean, but
I get your point. This is useful from a validation standpoint though.
If there is a property or compatible or standard node name to key off
of, we can validate the rest of the properties. Of course if we had
per compatible schema then we wouldn't need any of that.

For panel-lvds, minimally it tells me a) what the interface is which
could be useful for the upstream (parent) device and b) that various
LVDS related properties may be present. "simple-panel" never provided
any of that. Another compatible string is just additional data.

OTOH, I would not really object to removing the compatible either.

> It's also totally confusing because, although I haven't checked in
> detail, I'm fairly sure that a number of panels already supported by the
> panel-simple driver are actually LVDS panels. But adding "panel-lvds" to
> their compatible string list would be wrong and the panel-lvds driver
> wouldn't know what to do about it.

Yes, unfortunately the kernel can't work out multiple matches and pick
the driver with the most specific match. Generally, we're relying on
linking order here. That's a bigger issue in general.

IMO for this case, it is a problem because the drivers are split.
That's really a separate issue from the bindings. Dropping driver
matching on "panel-lvds" would solve it.

> Then there's the additional fact that since we can define so many
> properties in DT, we can also make up a completely fake device. With the
> current bindings its possible to add a panel with compatible
> "mitsubishi,aa121td01" but use property values from a panel that's
> actually compatible with "mitsubishi,aa104xd12".

If people lie and do stupid shit, then they get to keep it... That
only works until if the power control is the same or absent.

> As a side-note, there's a typo in the binding for the latter, it
> requires the compatible string list to contain the compatible for the
> former. The example has the correct one, though.
>
>> Parameterizing everything is indeed a losing battle. That doesn't mean
>> we can't parameterize some things in DT if they are completely
>> standard. IMO, roughly anything that can be in EDID could be in DT. So
>> I don't have a big problem with timings or physical size of the
>> display in DT. After all, we can always just ignore it.
>
> That's effectively saying that display timings, if we decide to add them
> to device tree, should be optional. Because if we can just ignore them,
> it means they have to be implied by the compatible string. So we would
> require the display timing database within drivers anyway, and the only
> real use-case for DT display timings would be to override the ones
> provided in the driver. That's something I had already agreed to, only
> we never actually implemented it because it has never been needed.

DT overriding the driver is completely backwards. The DT is part of
firmware. When firmware is wrong we override it. Just like if EDID is
crap, we'd ignore it (or perhaps adjust it).

> That's not, however, what the panel-lvds binding proposal is about. It
> makes the display timings mandatory such that you can fully define the
> panels in DT. It means that we have to trust the timings in DT because
> they are the only data we have. Choosing to ignore them is no longer an
> option because there's no way to initialize the panel without them. The
> driver implementation matches on the generic "panel-lvds" compatible and
> therefore can only ever rely on what's in the DT. There's no information
> about the required power sequencing or anything.

Mandatory for DT does not equate to the driver must use them. The
driver currently doesn't do much, but that doesn't mean it can't.
IIRC, when Laurent adds power sequencing support then that panel will
switch to matching on its specific compatible. The driver can match on
the specific compatible and it there's driver data with timings use
that. If not, fall back to using what is in DT.

Power supplies are similar. The DT may require them, but the OS
support for them is not yet in place and relies on everything being
powered on already.

> Essentially it does everything that I've been arguing against for the
> past 3.5 years.
>
>> > Now we have arrived at a point where apparently that decision has been
>> > revoked, and I don't understand what's changed. This puts me in a very
>> > difficult position. All of a sudden it's okay to do what everyone has
>> > been asking for the last three years, and I'm the jerk who told everyone
>> > that it couldn't be done.
>> >
>> > Maybe the discussions that we had back at the time are now far enough in
>> > the past that people have forgotten about the earlier failures. I still
>> > don't see how this new panel-lvds would be any more successful in
>> > solving the problems we failed to solve with simple-panel. The issues
>> > are still fundamentally the same. Now if this was a generic driver that
>> > dealt with a different subset of panels because they are different, that
>> > would've been okay with me. What I don't understand is why this has to
>> > deviate from the simple-panel binding in fundamental ways. Now we've got
>> > two bindings and we make life miserable for people because they have to
>> > choose between the two.
>> >
>> > Thierry
>> >
>> > [0]: https://sietch-tagr.blogspot.de/2016/04/display-panels-are-not-special.html
>>
>> I appreciate your excellent write-up very much. I've directed people
>> to it numerous times.
>
> This confuses me. You say that you agree with that write-up, and at the
> same time you've acked the panel-lvds binding. But they both contradict
> each other.

In general, I think we agree on most points. We always need a specific
compatible (I still have to argue that one). Having other properties
that are not related to describing connections is quite common. Having
specific properties vs. implying them from the compatible is not a
black and white situation. It's often a judgement call. For me, I
think timings falls on one side and power control falls on the other.

Rob
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel