[PATCH] drm/panel: panel-simple: Support panel-dpi

[Rob Herring]

On Fri, Mar 8, 2019 at 3:45 AM Thierry Reding <thierry.reding at gmail.com> wrote:
>
> On Thu, Mar 07, 2019 at 11:10:30AM +0100, Maxime Ripard wrote:
> > The kernel has a device tree binding for panel-dpi which allows for the
> > panel timings to be described in the device-tree, however it wasn't
> > supported so far except in a (small) number of KMS drivers that had an
> > ad-hoc solution for this (omapdrm for example).
>
> I'm growing really tired of having to repeat these discussions...
>
> As far as I can tell, this binding was never reviewed by device tree
> maintainers, so I'm not sure whether there's concensus that this should
> be proliferated. Adding Rob and the device tree mailing list for a wider
> audience.
>
> > Just like we've seen with panel-lvds, and even though the current dogma is
> > to set the timings within the driver, having them in the device tree
> > provides a number of benefits.
>
> I don't think there was concensus on that. But Rob acked it, so I guess
> he thought it was acceptable.
>
> Rob, can we use this thread as an opportunity to write down some of the
> rules regarding this? We've discussed this numerous times in the past
> but there still doesn't seem to be concensus.
>
> I know you're very tired of this as well, but perhaps we can bite the
> bullet now and produce clear documentation and guidelines that we can
> point people at (or put in an obvious location so that people find it
> themselves) in the future, so that we don't have to keep having this
> discussion.
>
> Summarizing what our latest discussion on this was:
>
>   * Generic compatible strings should typically be used as a fallback
>     only. Exceptions could be made if there's a specific standard that
>     is sufficiently strict to not require any quirks, and hence avoid
>     the need for panel-specific compatible strings.
>
>   * So in general device tree nodes for panels need to have a specific
>     compatible string that uniquely identifies that panel. This is in
>     line with existing practice for other devices and a good idea in
>     general so that we can implement quirks if necessary.
>
>   * Panel nodes can optionally also list a generic compatible string, in
>     addition to the specific compatible string, that drivers could match
>     to support devices which are not specifically supported yet but that
>     may already work anyway.
>
>     I'm not sure that's really practical. In the past we've seen that a
>     panel can work fine on one board but break on another because the
>     runtime execution timing is such that necessary delays in the power
>     sequence are noticeable on one but not another. I also suspect that
>     in some cases shortcuts were taken because things happened to work,
>     even if perhaps there was intermittent garbage on the screen because
>     the power sequence wasn't respected.

If there's some problem with a panel working, then we just use the
more specific compatible and deal with it in the driver. What's the
issue here?

A bigger issue is if later you need to tweak the timings, do you
update the timings in DT or add timings to kernel?

>   * Mechanisms that probe information from a panel at runtime (such as
>     EDID) are to see preferential treatment. In other words, if a DDC
>     channel exists to a panel, the driver should parse video timings
>     from the EDID.

That pretty well summarizes things.

>
> One thing that's not clear to me is whether or not we want to allow
> video timings to be specified in DT. I used to think that we didn't,
> because the video timings are implied by the specific compatible string
> (which we already determined is mandatory anyway), but the panel-lvds
> bindings suggest that from a device tree perspective this would be fine?
> I also notice that the panel-lvds doesn't make any provisions for power
> sequences. The same is true for panel-dpi. Are both LVDS and DPI panels
> always guaranteed not to need any specific power sequences?

As long as the specific compatible is there, I'd don't really care
that much. I think we've found over time with the LVDS binding that
the specific compatible ends up being needed.

I'm fine with timings in DT, but I'm on the fence whether a
'panel-dpi' compatible is a good thing or not. At least with LVDS,
that implies something about the interface. For DPI, there is no
standard really (MIPI does define something, but following MIPI is
pretty optional). There's lots of ways to wire up the data lines. It
could be a continual addition of timing flags which I don't want to
see. My controller has fine grained clock controls and I need to
control the duty cycle or delay the pixel clock some number of ns, for
example. I think most of that goes away with LVDS.

> If ultimately we decide that video timings in DT are okay, then how are
> we going to reconcile that with existing bindings? By definition the
> video timings would have to be optional, otherwise we'd be breaking ABI
> for existing device trees. But if they are optional, then we're back to
> square one, because we need to rely on the specific compatible string
> to get the bindings if they aren't present in DT. And then having them
> in DT is really just redundant. Except perhaps in order to support the
> cases that Maxime described where you want to do some really fine tuning
> to meet EMI requirements or some such.

For existing compatibles, they'd have to remain optional. For new
compatibles, it would be by choice. I don't want to see a bunch of
let's move the timing info to DT and remove from the driver patches.

> > The first is that it allows third party users to enable a random panel
> > without having to modify and recompile their kernel of choice. This might
> > sound like what we're trying to avoid in the first place, but it
> > significantly lowers the barrier of entry, both technical and practical.
>
> I think you're exaggerating. Modifying the kernel and rebuilding it is
> not significantly more difficult than doing the same for a DTB.

Agreed.

> > Indeed, users might not have the knowledge on how to recompile and modify a
> > kernel by their own, or might not have any documentation on the panel
> > itself which would prevent any inclusion.
>
> If you don't have documentation, how are you going to know what the
> video timings are? Or how will you know how to wire the board up to your
> board, or what the power sequence is that you need to follow.
>
> > But moderns systems also tend to move to mechanisms like secure boot which
> > would prevent that kernel, provided that the kernel was able to do that,
> > from running in the system, unless you would know how (and be able) to
> > install custom keys into your system.
> >
> > While the DT itself might have the same constraints, mechanisms like the
> > overlays allows to circumvent it.
>
> I'm not sure secure boot is a very good argument. You usually see that
> on closed devices, and those are not typically devices where you can go
> and swap out the panel with another random one.

Secure boot that allows changing the DT, but not the kernel? That
doesn't sound very secure.

> > Another thing that panel-dpi allows to address is EMC, where even though
> > the timings described in the driver could be functional on the board and
> > for the panel, it would be better to use another arbitrary frequency on a
> > particular board to increase the spread of the EM emissions.
>
> That's a valid point, and there have been proposals in the past to allow
> timings to be overridden by DT to allow such fine tuning. I think such a
> mechanism is generally fine, but it also implies that the video timings
> in DT would be optional, so it usually doesn't give people what they
> really want, which is to add support for arbitrary panels to the kernel.
>
> I want to explain why I'm being so reluctant to merge support for this,
> so that perhaps people better understand where this is coming from. If
> we allow arbitrary panels to be supported in this way, we can get into a
> situation where someone makes a device, upstreams support for it, using
> video timings specified in DT without a specific compatible string for
> the panel node and then burn that DTB into a ROM on the device and ship
> it. Now consider what would happen if some time down the road we get a
> bug report that the panel on that device no longer works. We've nicely
> painted ourselves into a corner, because we can't tell people to fix the
> DTB (it's in a ROM) and we can't add a quirk in the kernel because we
> don't have a way of identifying the panel. So what do we tell them?
> Tough luck?

Yes.

We can enforce that panels have a more specific compatible either thru
schema or in the driver as Laurent suggests.

Rob