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

[Emil Velikov <emil.l.velikov@gmail.com>]

On 10 April 2017 at 10:58, Lucas Stach <l.stach@pengutronix.de> wrote:
> Am Montag, den 10.04.2017, 09:17 +0200 schrieb Thierry Reding:
>> 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.
>
> I normally try to keep away from this "political" discussions,
> especially with regard to the panel stuff, but as this seems to pop up
> again and again, I feel the need to speak up.
>
Right, my intent did not plan as expected.

>> > On 7 April 2017 at 18:33, Thierry Reding <treding@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.
>
> I was personally opposed to the idea of specifying panels only by
> compatible and carrying around a "modedb" in the form of the
> simple-panel driver. Since then I've come to appreciate the flexibility
> that having a real driver for a panel provides.
>
> A panel is _much_ more than a simple mode. For LVDS panels it's in many
> cases a complete range of timings that will work with the panel, it's
> the enable sequencing with regulators and GPIOs, it's the size of the
> panel, so we get proper DPI information, it's a bus format describing
> all the oddities that vendors choose to implement when designing the
> physical interface. It provides proper sequencing of backlight
> enable/disable.
>
> I very much support Thierrys view, that all this should be encoded by
> the compatible. This allows us to add required properties to the panel
> if needed. It certainly saved us a lot of backward compat issues in
> imx-drm, that we could just add all the required information to the
> panel, so everything is consistent within one kernel version.
>
>> > 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.
>
> Although some people seems to feel different on that matter, but DT is
> supposed to be ABI. Newer kernels _must_ work on older DTs. Which means
> it needs to encode _all_ information on the panel.
>
> Personally I don't trust people, who are just enabling their one board
> to go through and validate that all the panel information is correct. As
> soon as a picture shows up they are going to ship the DT.
>
> With having to go through a panel driver, there is at least some basic
> review of the panel properties.
>
> Also speaking from experience with a board with different display
> configurations, where only the bootloader knows the specific attached
> panel, it's very convenient to have the bootloader only patch in the
> correct compatible, instead of all the panel properties. If any of those
> properties turns out to be wrong, I can fix it with a simple kernel
> update, instead of having to update the bootloader.
>
>> 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. 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.
>>
>> > 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.
>
> Yes, some old DTs still provide panel properties. That's no reason to
> not use the agreed upon way to describe panels in newer DTs. imx-drm
> still provides some support for the old way as this predates the panel
> stuff and we take DT backward compat serious, but newer kernels probably
> will start to complain about this. Please don't advocate this for new
> stuff.
>
> So to sum up, I completely support Thierrys way of handling panels and
> IMHO we can already see the brick wall the the "generic" (which is not
> generic at all, but very specific in that it doesn't require any of the
> handling a panel driver could provide) LVDS panel stuff is going to hit.
> Requiring properties "data-mirror" to describe uncommon bit orderings
> should be a sufficient warning sign.
>
If i understood things correctly, having the information within the
driver provides an extra amount of
 - verification - as one can simply add the information in DT without
properly checking that the panel works
 - flexibility - fixup the driver, as opposed to trying to fix the DT,
while preserving the ABI.

So, yes, having the timing information within the driver is good IMHO.

My closing thought from earlier was meant as:
If DT people do not see these as an issue and are OK with a)having DT
binding that are broken or b) breaking the DT ABI, so be it :-)

Either way, it's not my call so _please_ do not see any of my
questions as rocking the boat.

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