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

Wed Apr 12 17:42:32 UTC 2017

On Wed, Apr 12, 2017 at 5:44 PM, Thierry Reding <treding at nvidia.com> wrote:
> On Tue, Apr 11, 2017 at 01:56:35PM -0500, Rob Herring wrote:
>> On Mon, Apr 10, 2017 at 2:27 PM, Dave Airlie <airlied at gmail.com> wrote:
>> > On 10 April 2017 at 19:03, Laurent Pinchart
>> > <laurent.pinchart at ideasonboard.com> wrote:
>> >> Hi Thierry,
>> >>
>> >> On Monday 10 Apr 2017 09:17:59 Thierry Reding wrote:
>> >>> On Sun, Apr 09, 2017 at 01:31:40PM +0100, Emil Velikov wrote:
>> >>> > Hi Thierry,
>> >>> >
>> >>> > I don't mean to stir up anything, just voicing "my 2c" as they say.
>> >>> >
>> >>> > On 7 April 2017 at 18:33, Thierry Reding <treding at nvidia.com> wrote:
>> >>> > > Ever since the simple-panel binding was introduced, which is now about
>> >>> > > 3 1/2 years ago,
>> >>> >
>> >>> > Do you have a link to these discussions? Your blog article does not
>> >>> > have any links and I only found the "Runtime Interpreted Power
>> >>> > Sequences" thread.
>> >>> > That in itself does not cover the pros/cons of storing HW information*
>> >>> > within DT.
>> >>>
>> >>> There's some discussion here:
>> >>>
>> >>>       https://lists.freedesktop.org/archives/dri-devel/2013-November/049509.html
>> >>>
>> >>> which continues here:
>> >>>
>> >>>       https://lists.freedesktop.org/archives/dri-devel/2013-December/050082.html
>> >>>
>> >>> There are a couple of earlier threads, though, that discuss similar
>> >>> issues. This one seems to be the earliest I can find that is publicly
>> >>> archived:
>> >>>
>> >>>       http://www.spinics.net/lists/devicetree/msg08497.html
>> >>>
>> >>> Going over all these threads again wasn't a very pleasant experience. I
>> >>> realize how much time I already spent discussing these, and I don't have
>> >>> any desire to repeat that discussion.
>> >>>
>> >>> We've had these differences ever since the very beginning. So we're now
>> >>> again going in circles.
>> >>>
>> >>> The main concern back at the time was that having to specify timings in
>> >>> the driver would result in a complete mess because we have zillions of
>> >>> panels that we need to support. That's turned out to be a complete non-
>> >>> issue. We've got something on the order of 50 or 60 drivers supported in
>> >>> the simple-panel driver, and for everything that's more complicated we
>> >>> have a handful of separate drivers, all fairly simple as well.
>> >>>
>> >>> So while I understand why people want to put all this information into
>> >>> DT, we've repeatedly discussed the disadvantages that this would have.
>> >>> And while we were never able to get everyone to agree, the current
>> >>> solution has had enough agreement that we merged it. And it turned out
>> >>> to be good enough. There's nothing in panel-lvds that I can see that
>> >>> fundamentally changes this.
>> >>>
>> >>> > Personally, the idea of having hardware information* in DT does not
>> >>> > sound all that bad. The simple panel driver(s) can use those
>> >>> > properties and any panels that require anything more complex will
>> >>> > still need their own driver.
>> >>>
>> >>> Again, the point is that you're going to have to modify the driver in
>> >>> any case, because you need to support the new compatible string. Without
>> >>> that compatible string you have zero information about the panel, and
>> >>> matching on a generic one isn't going to give you a working panel.
>> >>
>> >> It will *if* the panel doesn't need any device-specific handling. In all other
>> >> cases I agree with you, panel-specific code will be needed in the kernel (to
>> >> handle power sequences for instance).
>> >>
>> >>> So if you're already going to have to support a panel in a driver, why not
>> >>> go all the way and fully describe its capabilities and properties? We do it
>> >>> for all other devices. Panels are not at all special.
>> >>
>> >> That we agree on, panels are not special. They're not the only devices that
>> >> store in DT information that could be hardcoded in the driver based on the
>> >> compatible string. We have many devices whose compatible string contains the
>> >> SoC version. Driver could then hardcode interrupts or clocks without any need
>> >> to specific them in DT. We don't do that as it would be more complex to
>> >> handle.
>> >>
>> >> Regarding timings, I've long hesitated (albeit I confess I was more on the
>> >> side of specifying them in DT) until Rob Herring convinced me with the generic
>> >> rule that adding information in DT that are generally exposed by devices
>> >> themselves makes sense. Displays traditionally expose video mode information
>> >> through EDID, which is effectively device firmware. When a panel is integrated
>> >> in a system, and the system designer decides to save money by removing the
>> >> EDID I2C EEPROM, moving that piece of device firmware data to system firmware
>> >> makes sense to me.
>> >>
>> >> I certainly won't try to revive the power sequence discussions, I don't
>> >> believe it belongs to DT.
>> >>
>> >>> > For better or for worse, there's already a handful of drivers and
>> >>> > bindings that rely on/provide these. Using that information
>> >>> > consistently across the board, would be of a benefit, IMHO.
>> >>>
>> >>> Would you mind pointing out which ones these are? I'm aware of only a
>> >>> couple that seemed to have sneeked in because people were trying to
>> >>> side-step adding drm/panel support for their boards, so I don't think
>> >>> that qualifies as a reason to rethink how drm/panel works.
>> >
>> > Okay I'm afraid I agree with Thierry here.
>> >
>> > I don't want mode timings or EDID in DT files, I'm pretty sure I was one of the
>> > people who helped decide that just having a compatible string and modes in
>> > a driver makes sense. So if we have imported code to be the opposite of this
>> > please work on removing it.
>> >
>> > If you move EDID from over the wire into DT it's not really tied to the panel
>> > anymore, and is now just a complicated way of specifying the modes from the
>> > panel documentation.
>> >
>> > EDID and DDC are a way for a panel to communicate to the host system,
>> > timing constraints and mode info, if you remove the i2c link, why bother
>> > encoding stuff in an EDID?
>>
>> EDID is a way to make the h/w discoverable as is DT. Both are just
>> data. Both are OS independent data. Only the mechanism of providing
>> the data differs. I can envision putting DT overlays into EEPROMs on
>> add-on boards which then would even have the same mechanism.
>>
>> This is not an either or decision. DT can provide data and the OS can
>> decide whether it uses it or not as long as we continue with specific
>> compatible strings.
>
> How does the OS decide not to use the data provided by DT? What criteria
> should be used to determine that the DT can't be trusted? Where do we
> get fallback data from?
>
> What we did with the panel-simple driver was to mandate that there is a
> mode (later display timing ranges) hard-coded in the driver. This means
> that for each supported panel we have trusted data that we know works
> with it. But we also made provisions to get data from an EDID if it is
> present. However there are also cases where the EDID can't be accessed,
> or where it is corrupt. For such cases we can always fallback to the
> hard-coded data in the driver.
>
> There are no such mechanisms for panel-lvds. On the other hand I don't
> see anything in panel-lvds that couldn't be implemented as part of the
> panel-simple driver. We already have ways to describe everything it does
> and more.

I think I have some relevant experience here, since for i915.ko and
dsi panels we have a fully generic description in VBT. And if you make
DT into a real machine desription, then I expect you'll roughly end up
in a similar position.

For the "when should you trust the firmware data" issue: We have tons
of dmi-based quirks (which more or less equals the board id). No, it's
not pretty.

For the mode description: We do have the full mode in VBT. We've had
that for a long time in lvds, and used it as the override EDID (except
when blacklisted with a dmi-based entry) even when there was one on
the i2c bus. Probably lots of littel integration issues that were
"fixed" by frobbing the mode in VBT. The only panel type where we
don't depend much upon VBT is eDP. But even there there's stuff like
link training values in the VBT, to avoid link training on boot-up an
optimize them more.

What's even more fun in the context of power sequences is that we also
have that in the VBT, as some kind of bytecode. For PWMs, gpios and
i2c buses we have the equivalent of phandles that point at the right
endpoint (which is provided by some different pmic driver entirely
independent of i915.ko in some cases), and load ordering is handled
with deferred probing.

I'm not advertisting that we do this in DT, just saying that there's
at least 1 system which does this (well, the equivalent in a
proprietary format), it's in upstream and it's shipping. Food for
thought :-)

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch