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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 7881 bytes --]

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.

  * 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.

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?

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.

> 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.

> 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.

> 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?

The root cause here is that there are no standards and designers just do
whatever they want because somebody will somehow make things work. While
that may work fine for products where the OEM provides a custom kernel,
it's a maintenance nightmare for upstream. So I think we need to be more
strict in what we accept and perhaps even help shape some form of
standard that will avoid any of the common pitfalls we know about.

I'd be interested to know whether this problem exists on any of the
other architectures, because this seems to me to be specific to the ARM
ecosystem. There's a myriad of x86 systems out there that use panels, so
I wonder how we solve these problems for those, or whether they simply
don't exist there because people have put more thought into system
designs?

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Laurent Pinchart]

Hi Thierry,

On Fri, Mar 08, 2019 at 10:44:57AM +0100, Thierry Reding 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.

The history indeed shows a less than ideal situation. I don't want or
plan to use omapdrm as a precedent to push for this, but as part of an
effort to remove the omapdrm-specific panel-dpi driver, I'm trying to
find a solution. I added support to the panel-simple driver for one
particular panel used by one particular OMAP board in order to port it
away from the omapdrm-specific panel code, but that will have a hard
time scaling. I'm thus interested in the outcome of this discussion.

> > 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.

That's a good idea, thank you for proposing it. Let's try to reach an
agreement and document it to avoid repeating the same story in 6 months.

> 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.

I'd say most cases instead of some cases :-) We have lots of panels for
which no public datasheet is available, and the timings added to
panel-simple were just copied from random device tree sources part of
obscure BSPs. It's then hard to do better than "if it works, ship it"
:-(

>   * 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.
> 
> 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?

They're not, and if a power sequence is needed, we need custom code
matching a specific compatible string.

> 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.
> 
> > 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.
> 
> > 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.

As explained above, we often get them from random device tree sources,
or possibly random forks of panel-simple in BSPs. Those will contain a
particular timing that happens to work with the system at hand. Adding
them to panel-simple isn't very difficult, but when a different system
will try to use the same panel, it may find out that the timings then
available in panel-simple don't work, because the two systems require
different timings both within the range of timings suppported by the
panel. Patching the timings in panel-simple with the values obtained
from the BSP for the second system will solve the problem, but
potentially break the first system.

The problem really stems from the fact that documentation is missing.
Moving timings to DT is a way to work around that, but it clearly more a
workaround than a solution for this specific issue. The question is if
we can find a better solution, or a better workaround.

> > 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.
> 
> > 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 think we need such a mechanism. We may not need a full override
though, possibly just a restricted range of pixel clock frequencies. I
can't tell yet what would be needed exactly, but EMC is a valid concern,
and we had to add explicit pixel clock frequencies for cameras in DTs
for this reason. Let's keep this in mind in our design.

> 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?

Could the panel-simple (or panel-dpi) driver reject panels that have a
single compatible string, forcing the DT author to add a specific one ?
Of course that wouldn't prevent someone from writing

	/* Don't remove foo,bar or the driver will not work */
	compatible = "foo,bar", "panel-dpi";

> The root cause here is that there are no standards and designers just do
> whatever they want because somebody will somehow make things work. While
> that may work fine for products where the OEM provides a custom kernel,
> it's a maintenance nightmare for upstream. So I think we need to be more
> strict in what we accept and perhaps even help shape some form of
> standard that will avoid any of the common pitfalls we know about.

I agree with you, but I would argue that having timings in C code can
also becomes a maintenance nightmare when the same panel is used on
multiple systems, as explained above.

> I'd be interested to know whether this problem exists on any of the
> other architectures, because this seems to me to be specific to the ARM
> ecosystem. There's a myriad of x86 systems out there that use panels, so
> I wonder how we solve these problems for those, or whether they simply
> don't exist there because people have put more thought into system
> designs?

Aren't timings stored in ACPI, at least in some cases ?

-- 
Regards,

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 15229 bytes --]

On Fri, Mar 08, 2019 at 01:14:30PM +0200, Laurent Pinchart wrote:
> Hi Thierry,
> 
> On Fri, Mar 08, 2019 at 10:44:57AM +0100, Thierry Reding 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.
> 
> The history indeed shows a less than ideal situation. I don't want or
> plan to use omapdrm as a precedent to push for this, but as part of an
> effort to remove the omapdrm-specific panel-dpi driver, I'm trying to
> find a solution. I added support to the panel-simple driver for one
> particular panel used by one particular OMAP board in order to port it
> away from the omapdrm-specific panel code, but that will have a hard
> time scaling. I'm thus interested in the outcome of this discussion.
> 
> > > 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.
> 
> That's a good idea, thank you for proposing it. Let's try to reach an
> agreement and document it to avoid repeating the same story in 6 months.
> 
> > 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.
> 
> I'd say most cases instead of some cases :-) We have lots of panels for
> which no public datasheet is available, and the timings added to
> panel-simple were just copied from random device tree sources part of
> obscure BSPs. It's then hard to do better than "if it works, ship it"
> :-(

But that's precisely one of the problems with panels. People think that
they are just really simple things that you connect and that if you feed
them a signal with the right timing that they will just work. But it's
actually much more difficult than that. If you look at the git history
of the simple panel driver, you'll notice that we've had to add a bunch
of information that it didn't have when we started out. These were all
things that we didn't need at first because they had been baked into the
designs and assumptions abounded. Then as panels started to be more
broadly used, additional parameters had to be introduced to account for
the differences between boards.

Imagine what a nightmare that would've been if we had only specified the
data in DT. There'd be all sorts of backwards-compatibility code that we
would have had to maintain forever.

And I'm talking the worst kinds of quirks, like matching on the
top-level compatible string to find out what board we're running on
(because we wouldn't have a more accurate way of identifying the panel)
and then adding quirks for that in SoC specific drivers so that they can
default to the right polarities and whatnot for that specific board.

> >   * 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.
> > 
> > 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?
> 
> They're not, and if a power sequence is needed, we need custom code
> matching a specific compatible string.

I suspected as much. So it's not really different from simple-panel in
the end.

> > 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.
> > 
> > > 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.
> > 
> > > 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.
> 
> As explained above, we often get them from random device tree sources,
> or possibly random forks of panel-simple in BSPs. Those will contain a
> particular timing that happens to work with the system at hand. Adding
> them to panel-simple isn't very difficult, but when a different system
> will try to use the same panel, it may find out that the timings then
> available in panel-simple don't work, because the two systems require
> different timings both within the range of timings suppported by the
> panel. Patching the timings in panel-simple with the values obtained
> from the BSP for the second system will solve the problem, but
> potentially break the first system.

We do have a mechanism of dealing with this. simple-panel can take video
timings instead of a display mode. Video timings are basically a
transcript of the (minimum, typical, maximum) values that you typically
see tabulated in datasheets.

Using these timings a display driver should be free to adjust values in
whatever way it deems necessary to meet its limitations. Don't support a
front porch less than 5? Fix it to 5, or any other convenient value, and
adjust other parameters within their ranges to find a combination that
works.

> The problem really stems from the fact that documentation is missing.
> Moving timings to DT is a way to work around that, but it clearly more a
> workaround than a solution for this specific issue. The question is if
> we can find a better solution, or a better workaround.

We're certainly not going to get a better solution if we simply accept
the status quo. What I want to see is people taking this more seriously
and think about panels the same way they think about other components in
the system. Panels really aren't special.

> > > 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.
> > 
> > > 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 think we need such a mechanism. We may not need a full override
> though, possibly just a restricted range of pixel clock frequencies. I
> can't tell yet what would be needed exactly, but EMC is a valid concern,
> and we had to add explicit pixel clock frequencies for cameras in DTs
> for this reason. Let's keep this in mind in our design.

We already have that with video timings. They allow you to specify a
valid range for the pixel clock frequency and any of the other
parameters. Display drivers can then use this to adjust timings as they
deem necessary to match their restrictions.

> > 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?
> 
> Could the panel-simple (or panel-dpi) driver reject panels that have a
> single compatible string, forcing the DT author to add a specific one ?
> Of course that wouldn't prevent someone from writing
> 
> 	/* Don't remove foo,bar or the driver will not work */
> 	compatible = "foo,bar", "panel-dpi";

I think Rob had some ideas of how to enforce this using DTC validation
so that it can be caught before this is merged.

> > The root cause here is that there are no standards and designers just do
> > whatever they want because somebody will somehow make things work. While
> > that may work fine for products where the OEM provides a custom kernel,
> > it's a maintenance nightmare for upstream. So I think we need to be more
> > strict in what we accept and perhaps even help shape some form of
> > standard that will avoid any of the common pitfalls we know about.
> 
> I agree with you, but I would argue that having timings in C code can
> also becomes a maintenance nightmare when the same panel is used on
> multiple systems, as explained above.

I think that's mostly a matter of perspective. Yes, it's certainly more
complicated to make drivers derive a suitable set of video timings from
the ranges that a panel supports. However, blindly allowing device tree
to specify the timings doesn't give the kernel any way of validating
that the values even remotely make sense.

And I know I keep repeating myself, you need more than just video
timings to describe a panel.

> > I'd be interested to know whether this problem exists on any of the
> > other architectures, because this seems to me to be specific to the ARM
> > ecosystem. There's a myriad of x86 systems out there that use panels, so
> > I wonder how we solve these problems for those, or whether they simply
> > don't exist there because people have put more thought into system
> > designs?
> 
> Aren't timings stored in ACPI, at least in some cases ?

I vaguely remember working on an Intel Atom based module where the LVDS
connector had DDC lines and the timings were parsed from the EDID, but I
suppose ACPI might be another way to store it. ACPI would also give you
a good way to encapsulate power sequences, which EDID doesn't give you.

I think x86 clearly has an edge here in terms of platform. There are
standard interfaces that allow you to hide these specifics. With device
tree we don't really have that. It's just too low-level. If we had a
standard (and complete) way of describing panels in DT, then we could
maybe achieve something similar, but attempts to do that have failed in
the past, so I don't have high hopes for DT being the solution in this
case.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Maxime Ripard]

[-- Attachment #1.1: Type: text/plain, Size: 16842 bytes --]

On Fri, Mar 08, 2019 at 03:11:37PM +0100, Thierry Reding wrote:
> On Fri, Mar 08, 2019 at 01:14:30PM +0200, Laurent Pinchart wrote:
> > Hi Thierry,
> > 
> > On Fri, Mar 08, 2019 at 10:44:57AM +0100, Thierry Reding 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.
> > 
> > The history indeed shows a less than ideal situation. I don't want or
> > plan to use omapdrm as a precedent to push for this, but as part of an
> > effort to remove the omapdrm-specific panel-dpi driver, I'm trying to
> > find a solution. I added support to the panel-simple driver for one
> > particular panel used by one particular OMAP board in order to port it
> > away from the omapdrm-specific panel code, but that will have a hard
> > time scaling. I'm thus interested in the outcome of this discussion.
> > 
> > > > 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.
> > 
> > That's a good idea, thank you for proposing it. Let's try to reach an
> > agreement and document it to avoid repeating the same story in 6 months.
> > 
> > > 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.
> > 
> > I'd say most cases instead of some cases :-) We have lots of panels for
> > which no public datasheet is available, and the timings added to
> > panel-simple were just copied from random device tree sources part of
> > obscure BSPs. It's then hard to do better than "if it works, ship it"
> > :-(
> 
> But that's precisely one of the problems with panels. People think that
> they are just really simple things that you connect and that if you feed
> them a signal with the right timing that they will just work. But it's
> actually much more difficult than that. If you look at the git history
> of the simple panel driver, you'll notice that we've had to add a bunch
> of information that it didn't have when we started out. These were all
> things that we didn't need at first because they had been baked into the
> designs and assumptions abounded. Then as panels started to be more
> broadly used, additional parameters had to be introduced to account for
> the differences between boards.
> 
> Imagine what a nightmare that would've been if we had only specified the
> data in DT. There'd be all sorts of backwards-compatibility code that we
> would have had to maintain forever.
> 
> And I'm talking the worst kinds of quirks, like matching on the
> top-level compatible string to find out what board we're running on
> (because we wouldn't have a more accurate way of identifying the panel)
> and then adding quirks for that in SoC specific drivers so that they can
> default to the right polarities and whatnot for that specific board.

One could also argue that it would be a bug in the DT, and that you'd
need to update it to fix it. Just like we update the firmware on x86
platforms on a regular basis to fix all kind of bugs and wrong data.

> > >   * 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.
> > > 
> > > 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?
> > 
> > They're not, and if a power sequence is needed, we need custom code
> > matching a specific compatible string.
> 
> I suspected as much. So it's not really different from simple-panel in
> the end.
> 
> > > 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.
> > > 
> > > > 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.
> > > 
> > > > 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.
> > 
> > As explained above, we often get them from random device tree sources,
> > or possibly random forks of panel-simple in BSPs. Those will contain a
> > particular timing that happens to work with the system at hand. Adding
> > them to panel-simple isn't very difficult, but when a different system
> > will try to use the same panel, it may find out that the timings then
> > available in panel-simple don't work, because the two systems require
> > different timings both within the range of timings suppported by the
> > panel. Patching the timings in panel-simple with the values obtained
> > from the BSP for the second system will solve the problem, but
> > potentially break the first system.
> 
> We do have a mechanism of dealing with this. simple-panel can take video
> timings instead of a display mode. Video timings are basically a
> transcript of the (minimum, typical, maximum) values that you typically
> see tabulated in datasheets.
> 
> Using these timings a display driver should be free to adjust values in
> whatever way it deems necessary to meet its limitations. Don't support a
> front porch less than 5? Fix it to 5, or any other convenient value, and
> adjust other parameters within their ranges to find a combination that
> works.

It's not as simple as you put it to use though, and omapdrm is
basically the only in-tree user so far.

> > The problem really stems from the fact that documentation is missing.
> > Moving timings to DT is a way to work around that, but it clearly more a
> > workaround than a solution for this specific issue. The question is if
> > we can find a better solution, or a better workaround.
> 
> We're certainly not going to get a better solution if we simply accept
> the status quo. What I want to see is people taking this more seriously
> and think about panels the same way they think about other components in
> the system. Panels really aren't special.
> 
> > > > 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.
> > > 
> > > > 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 think we need such a mechanism. We may not need a full override
> > though, possibly just a restricted range of pixel clock frequencies. I
> > can't tell yet what would be needed exactly, but EMC is a valid concern,
> > and we had to add explicit pixel clock frequencies for cameras in DTs
> > for this reason. Let's keep this in mind in our design.
> 
> We already have that with video timings. They allow you to specify a
> valid range for the pixel clock frequency and any of the other
> parameters. Display drivers can then use this to adjust timings as they
> deem necessary to match their restrictions.
> 
> > > 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?
> > 
> > Could the panel-simple (or panel-dpi) driver reject panels that have a
> > single compatible string, forcing the DT author to add a specific one ?
> > Of course that wouldn't prevent someone from writing
> > 
> > 	/* Don't remove foo,bar or the driver will not work */
> > 	compatible = "foo,bar", "panel-dpi";
> 
> I think Rob had some ideas of how to enforce this using DTC validation
> so that it can be caught before this is merged.
> 
> > > The root cause here is that there are no standards and designers just do
> > > whatever they want because somebody will somehow make things work. While
> > > that may work fine for products where the OEM provides a custom kernel,
> > > it's a maintenance nightmare for upstream. So I think we need to be more
> > > strict in what we accept and perhaps even help shape some form of
> > > standard that will avoid any of the common pitfalls we know about.
> > 
> > I agree with you, but I would argue that having timings in C code can
> > also becomes a maintenance nightmare when the same panel is used on
> > multiple systems, as explained above.
> 
> I think that's mostly a matter of perspective. Yes, it's certainly more
> complicated to make drivers derive a suitable set of video timings from
> the ranges that a panel supports. However, blindly allowing device tree
> to specify the timings doesn't give the kernel any way of validating
> that the values even remotely make sense.
> 
> And I know I keep repeating myself, you need more than just video
> timings to describe a panel.

The way I see it, we're mostly in agreement. We do agree on the fact
that yes, panels can be complicated and in that case we should make a
driver. We agree on the fact that panel can have bugs too. We agree on
the fact that EDIDs should be the preferred way to retrieve panels.

The only thing that we seem to have a different view is that you're
saying that since it's complicated on some panels, we can't have a
generic solution, while my view is that since it's not complicated on
most panels, then we should have a simpler solution. That's it,
really.

However, that difference has a quite dramatic side-effect, since it
closes a number of use-cases, and if we don't address them, then
there's no surprise really if that discussion keeps coming up.

Making a simple-panel driver for each and every driver, even for the
most simple ones, introduces a very significant overhead. Contributing
it introduces yet another overhead, which might not even be possible
in the first place because one might be running an older version of
the kernel. Even if people were contributing, I'm pretty sure that if
someone came up saying "I have no idea what this panel is, I don't
have this datasheet, but the panel works with those timings (I think)"
we would reject it (rightfully so). While you can keep your overlay to
yourself, and it's pretty easy to manage.

The fact that most BSP's has (or wants to have) a similar mechanism
also points out that it's one pain point most have.

We also agree on the fact that panel-dpi doesn't express enough to
cover most cases, but everything that has been discussed so far can be
adressed pretty easily. Bus width, polarities, and so on can be
expressed using the OF-graph for example, and it's what v4l2 uses, so
I don't really see it as a potential obstacle.

Maxime

-- 
Maxime Ripard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Sam Ravnborg]

> 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),

We often have two users of the timings for a simple panel.
First we have the bootloader that may present something on the
panel - next step it then the kernel.

Bootloaders such as U-boot and barebox supports devicetree.
So with the timings specified in the devicetree there are three
users that can use the timings, and it is simple to share the
timing specifications.

As it is now one has to patch the kernel to add a panel to panel-simple,
and add timing to device tree to let barebox use it.

So it would be good once and for all to have the rules specified.
And the preferred solution is to have timing in the devicetree
so we can use it both in the kernel and in the bootloaders.

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

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

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2253 bytes --]

On Fri, Mar 08, 2019 at 02:01:48PM +0100, Sam Ravnborg wrote:
> > 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),
> 
> We often have two users of the timings for a simple panel.
> First we have the bootloader that may present something on the
> panel - next step it then the kernel.
> 
> Bootloaders such as U-boot and barebox supports devicetree.
> So with the timings specified in the devicetree there are three
> users that can use the timings, and it is simple to share the
> timing specifications.

I think this is not true in practice. As far as I know U-Boot and Linux
don't share the device tree. So we wouldn't actually be sharing the
video timings, we'd be duplicating them. And whether we duplicate them
in code or DT isn't really all that different.

> As it is now one has to patch the kernel to add a panel to panel-simple,
> and add timing to device tree to let barebox use it.
> 
> So it would be good once and for all to have the rules specified.
> And the preferred solution is to have timing in the devicetree
> so we can use it both in the kernel and in the bootloaders.

This is *exactly* the same argument that I've heard many times before.
And it is still overly simplistic. Video timings are just one part of
the description of the panel. In most cases you need at least also a
power sequence. You also typically want additional data like physical
dimensions of the visible area so that you can compute the DPI, etc.

The status quo of deriving all of this information from the compatible
string gives us all the flexibility that we need in order to add such
information as we find it to be necessary. If we accept video timings
to be defined in DT, people are just blindly going to use that and not
think about things like physical dimensions or power sequences. And
then we'll just end up in a situation that we can't properly fix
anymore.

From that perspective the status quo is the preferred solution because
it actually allows us to fix things.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Sam Ravnborg]

Hi Thierry.
On Fri, Mar 08, 2019 at 02:39:24PM +0100, Thierry Reding wrote:
> On Fri, Mar 08, 2019 at 02:01:48PM +0100, Sam Ravnborg wrote:
> > > 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),
> > 
> > We often have two users of the timings for a simple panel.
> > First we have the bootloader that may present something on the
> > panel - next step it then the kernel.
> > 
> > Bootloaders such as U-boot and barebox supports devicetree.
> > So with the timings specified in the devicetree there are three
> > users that can use the timings, and it is simple to share the
> > timing specifications.
> 
> I think this is not true in practice. As far as I know U-Boot and Linux
> don't share the device tree. So we wouldn't actually be sharing the
> video timings, we'd be duplicating them. And whether we duplicate them
> in code or DT isn't really all that different.
U-boot copies selected DT files from the kernel to U-boot.
barebox has a (sanitized?) set of DT files from the kernel, with
barebox specifics added on top of it. This receives updates from the kernel
more or less for each kernel rc.
The origin of the DT files are the kernel, but they keep copies
for various reasons. And their update process from the kernel differs.

In other words - barebox uses the kernel DT files in practice.
U-boot in practice have their own copy (as I see it - did not look to close)

> > As it is now one has to patch the kernel to add a panel to panel-simple,
> > and add timing to device tree to let barebox use it.
> > 
> > So it would be good once and for all to have the rules specified.
> > And the preferred solution is to have timing in the devicetree
> > so we can use it both in the kernel and in the bootloaders.
> 
> This is *exactly* the same argument that I've heard many times before.
> And it is still overly simplistic. Video timings are just one part of
> the description of the panel. In most cases you need at least also a
> power sequence.
There are panels that are compatibe with panel-simple and there are the
other panels.
My comment is solely for the panel-simple compatible panels, where
we already know stuff like power sequence and such.

There are today a lot of panels in that group and in my tree there
is patches waiting to add another three panels.
This is panels that "just works" with barebox with timings(*) specified
in the DT and for the kernel requires a patch to panle-simple.

There are obviously a lot of panels that have additioanl requirements,
but then this is not "panle-simple" compatible panels and outside the
scope of my request.

I hope this clarifies it.

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

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

[Rob Herring]

On Fri, Mar 8, 2019 at 11:12 AM Sam Ravnborg <sam@ravnborg.org> wrote:
>
> Hi Thierry.
> On Fri, Mar 08, 2019 at 02:39:24PM +0100, Thierry Reding wrote:
> > On Fri, Mar 08, 2019 at 02:01:48PM +0100, Sam Ravnborg wrote:
> > > > 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),
> > >
> > > We often have two users of the timings for a simple panel.
> > > First we have the bootloader that may present something on the
> > > panel - next step it then the kernel.
> > >
> > > Bootloaders such as U-boot and barebox supports devicetree.
> > > So with the timings specified in the devicetree there are three
> > > users that can use the timings, and it is simple to share the
> > > timing specifications.
> >
> > I think this is not true in practice. As far as I know U-Boot and Linux
> > don't share the device tree. So we wouldn't actually be sharing the
> > video timings, we'd be duplicating them. And whether we duplicate them
> > in code or DT isn't really all that different.
> U-boot copies selected DT files from the kernel to U-boot.
> barebox has a (sanitized?) set of DT files from the kernel, with
> barebox specifics added on top of it. This receives updates from the kernel
> more or less for each kernel rc.
> The origin of the DT files are the kernel, but they keep copies
> for various reasons. And their update process from the kernel differs.
>
> In other words - barebox uses the kernel DT files in practice.
> U-boot in practice have their own copy (as I see it - did not look to close)

While barebox does a more automated sync, u-boot does do ad-hoc
copying of dts files from the kernel. And while it's typically a dtb
for u-boot and a dtb for the kernel, there is effort to make that be
the same dtb.

> > > As it is now one has to patch the kernel to add a panel to panel-simple,
> > > and add timing to device tree to let barebox use it.
> > >
> > > So it would be good once and for all to have the rules specified.
> > > And the preferred solution is to have timing in the devicetree
> > > so we can use it both in the kernel and in the bootloaders.
> >
> > This is *exactly* the same argument that I've heard many times before.
> > And it is still overly simplistic. Video timings are just one part of
> > the description of the panel. In most cases you need at least also a
> > power sequence.
> There are panels that are compatibe with panel-simple and there are the
> other panels.
> My comment is solely for the panel-simple compatible panels, where
> we already know stuff like power sequence and such.
>
> There are today a lot of panels in that group and in my tree there
> is patches waiting to add another three panels.
> This is panels that "just works" with barebox with timings(*) specified
> in the DT and for the kernel requires a patch to panle-simple.
>
> There are obviously a lot of panels that have additioanl requirements,
> but then this is not "panle-simple" compatible panels and outside the
> scope of my request.

Part of the problem with panel-simple is that it works at the
beginning and not as a platform progresses. Initially perhaps there is
no power control, but then that's added later. I've seen all to often
folks claiming panels "follow the simple-panel binding" and yet it
turns out that the panel has multiple power rails.

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

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

[Rob Herring]

On Fri, Mar 8, 2019 at 3:45 AM Thierry Reding <thierry.reding@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 6892 bytes --]

Rob Herring <robh+dt@kernel.org> writes:

> On Fri, Mar 8, 2019 at 3:45 AM Thierry Reding <thierry.reding@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.

I love how EDID keeps getting brought up in discussions how how to
handle non-EDID devices.  If anyone had EDIDs, we'd use them because
that's easy, but we don't.

>> 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.

I think now that we have 85 struct panel_desc in panel-simple.c, we can
just go look at what's actually needed to drive panels.  Detailed power
timing requirements are infrequently needed, being specified for 23 of
those.  I keep hearing that more detailed timing is necessary, so I
looked at the other non-DSI, non-SPI panels in tree (since those should
always require a driver for their register writes) and found a total of:

- panel-arm-versatile: has register writes to a syscon
- olinuxino: reads modes from an eeprom.
- seiko-43wvf1g: has two power rails to manage
- panel-lvds: is basically doing the thing people are asking panel-dpi
  to do but for lvds.

If DT is supposed to "describe the hardware" as people keep telling me,
then I think we have strong evidence of commonality between hardware
that could be described in data instead of code, using panel-simple's
data structures as the model.

>> 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.

Of course.  (and same for requiring a panel-specific compatible)

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

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

[Rob Herring]

On Fri, Mar 8, 2019 at 5:05 PM Eric Anholt <eric@anholt.net> wrote:
>
> Rob Herring <robh+dt@kernel.org> writes:
>
> > On Fri, Mar 8, 2019 at 3:45 AM Thierry Reding <thierry.reding@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.
>
> I love how EDID keeps getting brought up in discussions how how to
> handle non-EDID devices.  If anyone had EDIDs, we'd use them because
> that's easy, but we don't.

This is mainly in reference to recent discussions on eDP panels which
do have EDID and even a standard connector including power (though I'd
guess not required). So a generic-ish compatible makes sense in that
context as it has some meaning behind it.

> >> 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.
>
> I think now that we have 85 struct panel_desc in panel-simple.c, we can
> just go look at what's actually needed to drive panels.  Detailed power
> timing requirements are infrequently needed, being specified for 23 of
> those.

One thing to consider is whether power timing was added later or in
general if the panels' data changed at all. Having incomplete bindings
that have to evolve is one part of what we try to avoid.

And do any of the others (not 23) just not have power control at all
(because firmware turns on power or the platform doesn't have power
control yet)?

> I keep hearing that more detailed timing is necessary, so I
> looked at the other non-DSI, non-SPI panels in tree (since those should
> always require a driver for their register writes) and found a total of:
>
> - panel-arm-versatile: has register writes to a syscon
> - olinuxino: reads modes from an eeprom.
> - seiko-43wvf1g: has two power rails to manage
> - panel-lvds: is basically doing the thing people are asking panel-dpi
>   to do but for lvds.
>
> If DT is supposed to "describe the hardware" as people keep telling me,
> then I think we have strong evidence of commonality between hardware
> that could be described in data instead of code, using panel-simple's
> data structures as the model.

"describe the h/w" also means the h/w description doesn't evolve
because the h/w itself doesn't evolve.

That being said, I'm not against moving things to DT if that's the
right answer. Sometimes we don't know and it is better to put data in
the kernel until it is clear what should and shouldn't go in DT.

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