diff mbox series

[RESEND,v2,08/12] dt-bindings: add binding for generic eDP panel

Message ID 20190203185501.8958-9-anarsoul@gmail.com
State Changes Requested, archived
Headers show
Series Analogix ANX6345 RGB-(e)DP bridge support | expand

Checks

Context Check Description
robh/checkpatch success

Commit Message

Vasily Khoruzhick Feb. 3, 2019, 6:54 p.m. UTC
eDP panels usually have EDID EEPROM, so there's no need to define panel
width/height or any modes/timings in dts. But this panel still may have
regulator and/or backlight.

Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
---
 .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
 1 file changed, 7 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt

Comments

Thierry Reding Feb. 4, 2019, 7:43 a.m. UTC | #1
On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> eDP panels usually have EDID EEPROM, so there's no need to define panel
> width/height or any modes/timings in dts. But this panel still may have
> regulator and/or backlight.
> 
> Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> ---
>  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
>  1 file changed, 7 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt

Please don't try to make panels look more generic than they really are.
You're going to have to provide a compatible string for your device that
is more specific than "panel-edp". You claim that you don't need any
extra information that is panel specific, but you don't know that now.
We have in the past thought that we didn't need things like prepare
delay, but then we ran into situations where we did need them.

Just do what everybody else does. Provide a specific compatible string
and match on that in the panel-simple driver. Even if you can read all
the video timings from an EDID EEPROM, you can still provide a mode in
the panel descriptor to serve as a fallback if for example the EEPROM
is faulty on some device.

Thierry
Vasily Khoruzhick Feb. 4, 2019, 8:13 a.m. UTC | #2
On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > width/height or any modes/timings in dts. But this panel still may have
> > regulator and/or backlight.
> >
> > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > ---
> >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
>
> Please don't try to make panels look more generic than they really are.
> You're going to have to provide a compatible string for your device that
> is more specific than "panel-edp". You claim that you don't need any
> extra information that is panel specific, but you don't know that now.
> We have in the past thought that we didn't need things like prepare
> delay, but then we ran into situations where we did need them.
>
> Just do what everybody else does. Provide a specific compatible string
> and match on that in the panel-simple driver. Even if you can read all
> the video timings from an EDID EEPROM, you can still provide a mode in
> the panel descriptor to serve as a fallback if for example the EEPROM
> is faulty on some device.

Pinebook used several 768p panels that have slightly different timings
and recent batch uses 1080p panel.

What panel descriptor should I use as fallback?

> Thierry
Thierry Reding Feb. 4, 2019, 8:23 a.m. UTC | #3
On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > width/height or any modes/timings in dts. But this panel still may have
> > > regulator and/or backlight.
> > >
> > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > ---
> > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> >
> > Please don't try to make panels look more generic than they really are.
> > You're going to have to provide a compatible string for your device that
> > is more specific than "panel-edp". You claim that you don't need any
> > extra information that is panel specific, but you don't know that now.
> > We have in the past thought that we didn't need things like prepare
> > delay, but then we ran into situations where we did need them.
> >
> > Just do what everybody else does. Provide a specific compatible string
> > and match on that in the panel-simple driver. Even if you can read all
> > the video timings from an EDID EEPROM, you can still provide a mode in
> > the panel descriptor to serve as a fallback if for example the EEPROM
> > is faulty on some device.
> 
> Pinebook used several 768p panels that have slightly different timings
> and recent batch uses 1080p panel.
> 
> What panel descriptor should I use as fallback?

You don't use panel descriptors as fallback. The simple-panel driver
will bind to a panel device and use the corresponding descriptor. If
your device tree contains the correct information, the descriptor is
correct for the panel you have.

In other words you need to ensure that you have the correct panel in
device tree for the board that you're using. This is exactly the same
thing as for other devices.

One way to to this is to have separate device trees for each variant
of the board that you want to support. Another variant may be to have
a common device tree and then have some early firmware update the DTB
with the correct panel information.

Thierry
Daniel Vetter Feb. 4, 2019, 9:40 a.m. UTC | #4
On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > width/height or any modes/timings in dts. But this panel still may have
> > > > regulator and/or backlight.
> > > >
> > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > ---
> > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > >
> > > Please don't try to make panels look more generic than they really are.
> > > You're going to have to provide a compatible string for your device that
> > > is more specific than "panel-edp". You claim that you don't need any
> > > extra information that is panel specific, but you don't know that now.
> > > We have in the past thought that we didn't need things like prepare
> > > delay, but then we ran into situations where we did need them.
> > >
> > > Just do what everybody else does. Provide a specific compatible string
> > > and match on that in the panel-simple driver. Even if you can read all
> > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > is faulty on some device.
> > 
> > Pinebook used several 768p panels that have slightly different timings
> > and recent batch uses 1080p panel.
> > 
> > What panel descriptor should I use as fallback?
> 
> You don't use panel descriptors as fallback. The simple-panel driver
> will bind to a panel device and use the corresponding descriptor. If
> your device tree contains the correct information, the descriptor is
> correct for the panel you have.
> 
> In other words you need to ensure that you have the correct panel in
> device tree for the board that you're using. This is exactly the same
> thing as for other devices.
> 
> One way to to this is to have separate device trees for each variant
> of the board that you want to support. Another variant may be to have
> a common device tree and then have some early firmware update the DTB
> with the correct panel information.

This would defeat the point of edp, which is to standardize the mess of
panels (at least somewhat) and avoid having to change the DT/ACPI
tables/firmware for every board you ship. Also, we do have DP quirking
infrastructure already (using the OUI), I think if there's something that
doesn't work then we should quirk it there.

What does make sense though imo is if we try not to stuff the edp panel
into panel-simple, because it's anything like a simple dumb panel. There's
also some integration awkwardness since with this panel you need to do dp
aux/i2c transactions to get at the information (edid alone isn't good
enough for edp), and I'm not sure how exactly that's supposed to be
instantiated. Maybe a special function to instantiate an edp panel, which
takes both a DT node and the dp_aux controller would be much better,
instead of trying to auto-match against a DT compatible string and load a
panel driver which is almost all fake.

Or we teach dp_aux to register itself and somehow teach panel-edp how it
can get hold of the dp_aux channel it needs.

But fake mode in panel-simple sounds like the wrong approach. At least
from our experience with i915 (and I think other drivers supporting edp),
the standardization of panels for basic stuff at least worked.
Self-refresh seems an entirely different story unfortunately ... but again
quirking that for DP stuff should be done using the OUI I think.
-Daniel
Thierry Reding Feb. 4, 2019, 11:22 a.m. UTC | #5
On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > regulator and/or backlight.
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > ---
> > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > >
> > > > Please don't try to make panels look more generic than they really are.
> > > > You're going to have to provide a compatible string for your device that
> > > > is more specific than "panel-edp". You claim that you don't need any
> > > > extra information that is panel specific, but you don't know that now.
> > > > We have in the past thought that we didn't need things like prepare
> > > > delay, but then we ran into situations where we did need them.
> > > >
> > > > Just do what everybody else does. Provide a specific compatible string
> > > > and match on that in the panel-simple driver. Even if you can read all
> > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > is faulty on some device.
> > > 
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > > 
> > > What panel descriptor should I use as fallback?
> > 
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> > 
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> > 
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> This would defeat the point of edp, which is to standardize the mess of
> panels (at least somewhat) and avoid having to change the DT/ACPI
> tables/firmware for every board you ship. Also, we do have DP quirking
> infrastructure already (using the OUI), I think if there's something that
> doesn't work then we should quirk it there.

The problem is that while the attempt may have been to standardize, it
failed. It doesn't take into account any of the details such as timing
between things like powering up the display and enabling the backlight
or similar. I don't know how you'd want to "quirk" those kinds of
requirements because they are highly panel specific.

> What does make sense though imo is if we try not to stuff the edp panel
> into panel-simple, because it's anything like a simple dumb panel. There's
> also some integration awkwardness since with this panel you need to do dp
> aux/i2c transactions to get at the information (edid alone isn't good
> enough for edp), and I'm not sure how exactly that's supposed to be
> instantiated. Maybe a special function to instantiate an edp panel, which
> takes both a DT node and the dp_aux controller would be much better,
> instead of trying to auto-match against a DT compatible string and load a
> panel driver which is almost all fake.
> 
> Or we teach dp_aux to register itself and somehow teach panel-edp how it
> can get hold of the dp_aux channel it needs.

We already do that. drm_dp_aux registers as an I2C adapter that can be
used to read EDID EEPROMs using I2C-over-AUX transactions. We already
use that on some platforms.

Also note that simple-panel already supports getting video timings from
EDID. If a DDC link is present in DT, the driver will load the modes
from EDID and use them.

> But fake mode in panel-simple sounds like the wrong approach. At least
> from our experience with i915 (and I think other drivers supporting edp),
> the standardization of panels for basic stuff at least worked.
> Self-refresh seems an entirely different story unfortunately ... but again
> quirking that for DP stuff should be done using the OUI I think.

I can imagine that on x86 platforms OEMs can also easily hide some of
the necessary quirks in ACPI. None of that exists on ARM or other
architectures that solely rely on device tree to describe the device.
Since there's no way of specifying power sequences in DT (there have
been attempts in the past to make that work, but they failed miserably)
all we can do is match on a compatible string and encode the necessary
logic in the driver. Luckily we haven't run into anything too fancy up
to now and we've been able to make things work mostly with just a couple
of fairly generic parameters.

Also note that initially people thought this wasn't going to be
necessary and we only added the various "delay" parameters later on
because it did turn out that not all panels are created equal.

Thierry
Daniel Vetter Feb. 4, 2019, 3:59 p.m. UTC | #6
On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > regulator and/or backlight.
> > > > > >
> > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > >
> > > > > Please don't try to make panels look more generic than they really are.
> > > > > You're going to have to provide a compatible string for your device that
> > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > extra information that is panel specific, but you don't know that now.
> > > > > We have in the past thought that we didn't need things like prepare
> > > > > delay, but then we ran into situations where we did need them.
> > > > >
> > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > is faulty on some device.
> > > > 
> > > > Pinebook used several 768p panels that have slightly different timings
> > > > and recent batch uses 1080p panel.
> > > > 
> > > > What panel descriptor should I use as fallback?
> > > 
> > > You don't use panel descriptors as fallback. The simple-panel driver
> > > will bind to a panel device and use the corresponding descriptor. If
> > > your device tree contains the correct information, the descriptor is
> > > correct for the panel you have.
> > > 
> > > In other words you need to ensure that you have the correct panel in
> > > device tree for the board that you're using. This is exactly the same
> > > thing as for other devices.
> > > 
> > > One way to to this is to have separate device trees for each variant
> > > of the board that you want to support. Another variant may be to have
> > > a common device tree and then have some early firmware update the DTB
> > > with the correct panel information.
> > 
> > This would defeat the point of edp, which is to standardize the mess of
> > panels (at least somewhat) and avoid having to change the DT/ACPI
> > tables/firmware for every board you ship. Also, we do have DP quirking
> > infrastructure already (using the OUI), I think if there's something that
> > doesn't work then we should quirk it there.
> 
> The problem is that while the attempt may have been to standardize, it
> failed. It doesn't take into account any of the details such as timing
> between things like powering up the display and enabling the backlight
> or similar. I don't know how you'd want to "quirk" those kinds of
> requirements because they are highly panel specific.

Hm right, we get these from some firmware tables (and mix them with the
spec one, since some of the firmware values are nonsense). I don't even
know whether we can read the timings over dp aux somehow (you can power up
the panel with some pessimistic values to figure those out, and you only
need dp aux to work, which is much simpler than the entire panel).

> > What does make sense though imo is if we try not to stuff the edp panel
> > into panel-simple, because it's anything like a simple dumb panel. There's
> > also some integration awkwardness since with this panel you need to do dp
> > aux/i2c transactions to get at the information (edid alone isn't good
> > enough for edp), and I'm not sure how exactly that's supposed to be
> > instantiated. Maybe a special function to instantiate an edp panel, which
> > takes both a DT node and the dp_aux controller would be much better,
> > instead of trying to auto-match against a DT compatible string and load a
> > panel driver which is almost all fake.
> > 
> > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > can get hold of the dp_aux channel it needs.
> 
> We already do that. drm_dp_aux registers as an I2C adapter that can be
> used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> use that on some platforms.
> 
> Also note that simple-panel already supports getting video timings from
> EDID. If a DDC link is present in DT, the driver will load the modes
> from EDID and use them.

Could we extend this to dp aux somehow? For edp you need the dp aux (which
then gives you the ddc link automatically).

> > But fake mode in panel-simple sounds like the wrong approach. At least
> > from our experience with i915 (and I think other drivers supporting edp),
> > the standardization of panels for basic stuff at least worked.
> > Self-refresh seems an entirely different story unfortunately ... but again
> > quirking that for DP stuff should be done using the OUI I think.
> 
> I can imagine that on x86 platforms OEMs can also easily hide some of
> the necessary quirks in ACPI. None of that exists on ARM or other
> architectures that solely rely on device tree to describe the device.
> Since there's no way of specifying power sequences in DT (there have
> been attempts in the past to make that work, but they failed miserably)
> all we can do is match on a compatible string and encode the necessary
> logic in the driver. Luckily we haven't run into anything too fancy up
> to now and we've been able to make things work mostly with just a couple
> of fairly generic parameters.
> 
> Also note that initially people thought this wasn't going to be
> necessary and we only added the various "delay" parameters later on
> because it did turn out that not all panels are created equal.

Yeah this sucks, but oh well.
-Daniel
Vasily Khoruzhick Feb. 4, 2019, 4:11 p.m. UTC | #7
On Mon, Feb 4, 2019 at 12:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > Pinebook used several 768p panels that have slightly different timings
> > and recent batch uses 1080p panel.
> >
> > What panel descriptor should I use as fallback?
>
> You don't use panel descriptors as fallback. The simple-panel driver
> will bind to a panel device and use the corresponding descriptor. If
> your device tree contains the correct information, the descriptor is
> correct for the panel you have.
>
> In other words you need to ensure that you have the correct panel in
> device tree for the board that you're using. This is exactly the same
> thing as for other devices.
>
> One way to to this is to have separate device trees for each variant
> of the board that you want to support. Another variant may be to have
> a common device tree and then have some early firmware update the DTB
> with the correct panel information.

That defeats the purpose of using eDP panels. Panel can identify
itself and report what timings it supports.

If we use separate DTBs then users will have to figure out what panel
is installed in their hardware and use appropriate software image -
that's something I'd like to avoid.

I can add a descriptor for something like "pinebook eDP panel" if it
works for you, but it won't have any display timings in it.
Thierry Reding Feb. 4, 2019, 4:22 p.m. UTC | #8
On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > >
> > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > regulator and/or backlight.
> > > > > > >
> > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > ---
> > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > >  1 file changed, 7 insertions(+)
> > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > >
> > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > You're going to have to provide a compatible string for your device that
> > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > delay, but then we ran into situations where we did need them.
> > > > > >
> > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > is faulty on some device.
> > > > > 
> > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > and recent batch uses 1080p panel.
> > > > > 
> > > > > What panel descriptor should I use as fallback?
> > > > 
> > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > will bind to a panel device and use the corresponding descriptor. If
> > > > your device tree contains the correct information, the descriptor is
> > > > correct for the panel you have.
> > > > 
> > > > In other words you need to ensure that you have the correct panel in
> > > > device tree for the board that you're using. This is exactly the same
> > > > thing as for other devices.
> > > > 
> > > > One way to to this is to have separate device trees for each variant
> > > > of the board that you want to support. Another variant may be to have
> > > > a common device tree and then have some early firmware update the DTB
> > > > with the correct panel information.
> > > 
> > > This would defeat the point of edp, which is to standardize the mess of
> > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > infrastructure already (using the OUI), I think if there's something that
> > > doesn't work then we should quirk it there.
> > 
> > The problem is that while the attempt may have been to standardize, it
> > failed. It doesn't take into account any of the details such as timing
> > between things like powering up the display and enabling the backlight
> > or similar. I don't know how you'd want to "quirk" those kinds of
> > requirements because they are highly panel specific.
> 
> Hm right, we get these from some firmware tables (and mix them with the
> spec one, since some of the firmware values are nonsense). I don't even
> know whether we can read the timings over dp aux somehow (you can power up
> the panel with some pessimistic values to figure those out, and you only
> need dp aux to work, which is much simpler than the entire panel).
> 
> > > What does make sense though imo is if we try not to stuff the edp panel
> > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > also some integration awkwardness since with this panel you need to do dp
> > > aux/i2c transactions to get at the information (edid alone isn't good
> > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > takes both a DT node and the dp_aux controller would be much better,
> > > instead of trying to auto-match against a DT compatible string and load a
> > > panel driver which is almost all fake.
> > > 
> > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > can get hold of the dp_aux channel it needs.
> > 
> > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > use that on some platforms.
> > 
> > Also note that simple-panel already supports getting video timings from
> > EDID. If a DDC link is present in DT, the driver will load the modes
> > from EDID and use them.
> 
> Could we extend this to dp aux somehow? For edp you need the dp aux (which
> then gives you the ddc link automatically).

I suppose we could do that. We could introduce a new property that would
allow the panel driver to get at the struct drm_dp_aux that can access
the panel. I'm not sure how much that would buy us. I suppose the driver
could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
ddc-bus property in device tree. A drm_dp_aux object could also be used
to access DPCD if that's helpful.

The driver proposed here doesn't need access to DPCD, so I'm not sure
that would immediately help.

Thierry
Rob Herring Feb. 4, 2019, 4:27 p.m. UTC | #9
On Mon, Feb 4, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > width/height or any modes/timings in dts. But this panel still may have
> > > > regulator and/or backlight.
> > > >
> > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > ---
> > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > >
> > > Please don't try to make panels look more generic than they really are.
> > > You're going to have to provide a compatible string for your device that
> > > is more specific than "panel-edp". You claim that you don't need any
> > > extra information that is panel specific, but you don't know that now.
> > > We have in the past thought that we didn't need things like prepare
> > > delay, but then we ran into situations where we did need them.
> > >
> > > Just do what everybody else does. Provide a specific compatible string
> > > and match on that in the panel-simple driver. Even if you can read all
> > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > is faulty on some device.
> >
> > Pinebook used several 768p panels that have slightly different timings
> > and recent batch uses 1080p panel.
> >
> > What panel descriptor should I use as fallback?
>
> You don't use panel descriptors as fallback. The simple-panel driver
> will bind to a panel device and use the corresponding descriptor. If
> your device tree contains the correct information, the descriptor is
> correct for the panel you have.
>
> In other words you need to ensure that you have the correct panel in
> device tree for the board that you're using. This is exactly the same
> thing as for other devices.
>
> One way to to this is to have separate device trees for each variant
> of the board that you want to support. Another variant may be to have
> a common device tree and then have some early firmware update the DTB
> with the correct panel information.

That can be a pain to manage especially if panels are swapped run to
run with 2nd sources.

I think it is perfectly fine to have a generic-ish fallback as long as
it is just that, a fallback. If the panel has quirks, then you'd
better make sure the firmware is stuffing in the right compatibles or
that you can update the firmware.

Rob
Thierry Reding Feb. 4, 2019, 4:27 p.m. UTC | #10
On Mon, Feb 04, 2019 at 08:11:05AM -0800, Vasily Khoruzhick wrote:
> On Mon, Feb 4, 2019 at 12:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > >
> > > What panel descriptor should I use as fallback?
> >
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> >
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> >
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> That defeats the purpose of using eDP panels. Panel can identify
> itself and report what timings it supports.
> 
> If we use separate DTBs then users will have to figure out what panel
> is installed in their hardware and use appropriate software image -
> that's something I'd like to avoid.
> 
> I can add a descriptor for something like "pinebook eDP panel" if it
> works for you, but it won't have any display timings in it.

I'd like to point you to this:

	http://sietch-tagr.blogspot.com/2016/04/display-panels-are-not-special.html

it addresses some of your questions. Hopefully that will also help you
understand that this isn't only about display timings.

Thierry
Rob Herring Feb. 4, 2019, 4:39 p.m. UTC | #11
On Mon, Feb 4, 2019 at 10:11 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Mon, Feb 4, 2019 at 12:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > >
> > > What panel descriptor should I use as fallback?
> >
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> >
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> >
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
>
> That defeats the purpose of using eDP panels. Panel can identify
> itself and report what timings it supports.

If you are confident that this works for all panels, then the firmware
can identify the right panel and update the DTB with the correct
information. If this doesn't work in the firmware, then it is not
going to work in the kernel either and you are SOL without specific
panel information in the DT.

> If we use separate DTBs then users will have to figure out what panel
> is installed in their hardware and use appropriate software image -
> that's something I'd like to avoid.

I think Thierry meant either way this is a firmware problem. If you
have a SKU per device and panel type, then the firmware just picks a
dtb among a set.

Rob
Thierry Reding Feb. 4, 2019, 4:56 p.m. UTC | #12
On Mon, Feb 04, 2019 at 10:27:09AM -0600, Rob Herring wrote:
> On Mon, Feb 4, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > regulator and/or backlight.
> > > > >
> > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > ---
> > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > >  1 file changed, 7 insertions(+)
> > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > >
> > > > Please don't try to make panels look more generic than they really are.
> > > > You're going to have to provide a compatible string for your device that
> > > > is more specific than "panel-edp". You claim that you don't need any
> > > > extra information that is panel specific, but you don't know that now.
> > > > We have in the past thought that we didn't need things like prepare
> > > > delay, but then we ran into situations where we did need them.
> > > >
> > > > Just do what everybody else does. Provide a specific compatible string
> > > > and match on that in the panel-simple driver. Even if you can read all
> > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > is faulty on some device.
> > >
> > > Pinebook used several 768p panels that have slightly different timings
> > > and recent batch uses 1080p panel.
> > >
> > > What panel descriptor should I use as fallback?
> >
> > You don't use panel descriptors as fallback. The simple-panel driver
> > will bind to a panel device and use the corresponding descriptor. If
> > your device tree contains the correct information, the descriptor is
> > correct for the panel you have.
> >
> > In other words you need to ensure that you have the correct panel in
> > device tree for the board that you're using. This is exactly the same
> > thing as for other devices.
> >
> > One way to to this is to have separate device trees for each variant
> > of the board that you want to support. Another variant may be to have
> > a common device tree and then have some early firmware update the DTB
> > with the correct panel information.
> 
> That can be a pain to manage especially if panels are swapped run to
> run with 2nd sources.
> 
> I think it is perfectly fine to have a generic-ish fallback as long as
> it is just that, a fallback. If the panel has quirks, then you'd
> better make sure the firmware is stuffing in the right compatibles or
> that you can update the firmware.

simple-panel would probably work if you stuck in some mostly compatible
string and provided a ddc-i2c-bus property in the device tree node. The
generic-ish fallback case could be implemented by providing a fallback
compatible string (we used to have "simple-panel", which I think would
still be adequate for this I suppose) and adding a dummy descriptor in
the driver, perhaps one with pre-defined delays that could be adjusted
to work for all cases, or they could just be 0. At least that way we'd
be explicitly documenting that we support this as a fallback.

I'm not sure how you'd want to enforce that people provide the right
compatible strings that way. Currently there's no way to make your panel
work without adding a panel driver, so people are forced to write the
DT bindings and a driver, or add support to an existing one. If we open
this backdoor, I suspect many people will just take the easy route and
rely on the fallback. And then what do we do when we get bug reports
about panels not working, or requiring some quirks. How do we go and
find out what the right compatible strings are for each board, or how to
fix things for something we don't have access to.

This all sounds like a Pandora's box to me.

Thierry
Vasily Khoruzhick Feb. 4, 2019, 5:02 p.m. UTC | #13
On Mon, Feb 4, 2019 at 8:39 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, Feb 4, 2019 at 10:11 AM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> >
> > On Mon, Feb 4, 2019 at 12:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > >
> > > > Pinebook used several 768p panels that have slightly different timings
> > > > and recent batch uses 1080p panel.
> > > >
> > > > What panel descriptor should I use as fallback?
> > >
> > > You don't use panel descriptors as fallback. The simple-panel driver
> > > will bind to a panel device and use the corresponding descriptor. If
> > > your device tree contains the correct information, the descriptor is
> > > correct for the panel you have.
> > >
> > > In other words you need to ensure that you have the correct panel in
> > > device tree for the board that you're using. This is exactly the same
> > > thing as for other devices.
> > >
> > > One way to to this is to have separate device trees for each variant
> > > of the board that you want to support. Another variant may be to have
> > > a common device tree and then have some early firmware update the DTB
> > > with the correct panel information.
> >
> > That defeats the purpose of using eDP panels. Panel can identify
> > itself and report what timings it supports.
>
> If you are confident that this works for all panels, then the firmware
> can identify the right panel and update the DTB with the correct
> information. If this doesn't work in the firmware, then it is not
> going to work in the kernel either and you are SOL without specific
> panel information in the DT.

"firmware" is u-boot and on this platform it sits on the same physical
media as OS (it's either microsd or eMMC).

I guess u-boot can fill in timings for kernel, but I don't see much
point in it. If u-boot can't read EDID then same is true
for kernel.

> > If we use separate DTBs then users will have to figure out what panel
> > is installed in their hardware and use appropriate software image -
> > that's something I'd like to avoid.
>
> I think Thierry meant either way this is a firmware problem. If you
> have a SKU per device and panel type, then the firmware just picks a
> dtb among a set.

Unfortunately it's one SKU per multiple panels.

> Rob
Vasily Khoruzhick Feb. 4, 2019, 5:07 p.m. UTC | #14
On Mon, Feb 4, 2019 at 8:56 AM Thierry Reding <thierry.reding@gmail.com> wrote:

> > I think it is perfectly fine to have a generic-ish fallback as long as
> > it is just that, a fallback. If the panel has quirks, then you'd
> > better make sure the firmware is stuffing in the right compatibles or
> > that you can update the firmware.
>
> simple-panel would probably work if you stuck in some mostly compatible
> string and provided a ddc-i2c-bus property in the device tree node. The
> generic-ish fallback case could be implemented by providing a fallback
> compatible string (we used to have "simple-panel", which I think would
> still be adequate for this I suppose) and adding a dummy descriptor in
> the driver, perhaps one with pre-defined delays that could be adjusted
> to work for all cases, or they could just be 0. At least that way we'd
> be explicitly documenting that we support this as a fallback.
>
> I'm not sure how you'd want to enforce that people provide the right
> compatible strings that way. Currently there's no way to make your panel
> work without adding a panel driver, so people are forced to write the
> DT bindings and a driver, or add support to an existing one. If we open
> this backdoor, I suspect many people will just take the easy route and
> rely on the fallback. And then what do we do when we get bug reports
> about panels not working, or requiring some quirks. How do we go and
> find out what the right compatible strings are for each board, or how to
> fix things for something we don't have access to.
>
> This all sounds like a Pandora's box to me.

OK, just give me an option that will work on this platform with a
single software image (keep in mind that u-boot aka "firmware" is part
of this image) and that is acceptable for upstream and I'll try to
implement it.

> Thierry
Rob Herring Feb. 4, 2019, 8:23 p.m. UTC | #15
On Mon, Feb 4, 2019 at 10:56 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Mon, Feb 04, 2019 at 10:27:09AM -0600, Rob Herring wrote:
> > On Mon, Feb 4, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > >
> > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > regulator and/or backlight.
> > > > > >
> > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > ---
> > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > >  1 file changed, 7 insertions(+)
> > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > >
> > > > > Please don't try to make panels look more generic than they really are.
> > > > > You're going to have to provide a compatible string for your device that
> > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > extra information that is panel specific, but you don't know that now.
> > > > > We have in the past thought that we didn't need things like prepare
> > > > > delay, but then we ran into situations where we did need them.
> > > > >
> > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > is faulty on some device.
> > > >
> > > > Pinebook used several 768p panels that have slightly different timings
> > > > and recent batch uses 1080p panel.
> > > >
> > > > What panel descriptor should I use as fallback?
> > >
> > > You don't use panel descriptors as fallback. The simple-panel driver
> > > will bind to a panel device and use the corresponding descriptor. If
> > > your device tree contains the correct information, the descriptor is
> > > correct for the panel you have.
> > >
> > > In other words you need to ensure that you have the correct panel in
> > > device tree for the board that you're using. This is exactly the same
> > > thing as for other devices.
> > >
> > > One way to to this is to have separate device trees for each variant
> > > of the board that you want to support. Another variant may be to have
> > > a common device tree and then have some early firmware update the DTB
> > > with the correct panel information.
> >
> > That can be a pain to manage especially if panels are swapped run to
> > run with 2nd sources.
> >
> > I think it is perfectly fine to have a generic-ish fallback as long as
> > it is just that, a fallback. If the panel has quirks, then you'd
> > better make sure the firmware is stuffing in the right compatibles or
> > that you can update the firmware.
>
> simple-panel would probably work if you stuck in some mostly compatible
> string and provided a ddc-i2c-bus property in the device tree node. The
> generic-ish fallback case could be implemented by providing a fallback
> compatible string (we used to have "simple-panel", which I think would
> still be adequate for this I suppose) and adding a dummy descriptor in
> the driver, perhaps one with pre-defined delays that could be adjusted
> to work for all cases, or they could just be 0. At least that way we'd
> be explicitly documenting that we support this as a fallback.

I'd like something more specific than 'simple-panel' that at least
implies it has EDID and whatever else we think it should imply.

Looking into this a bit more, why don't we just do a connector here?
eDP has a standard connector (with power). It's just like other
connectors, but with power and no hotplug. If someone does their own
interface, then they should do their own connector or panel binding.

> I'm not sure how you'd want to enforce that people provide the right
> compatible strings that way. Currently there's no way to make your panel
> work without adding a panel driver, so people are forced to write the
> DT bindings and a driver, or add support to an existing one. If we open
> this backdoor, I suspect many people will just take the easy route and
> rely on the fallback. And then what do we do when we get bug reports
> about panels not working, or requiring some quirks. How do we go and
> find out what the right compatible strings are for each board, or how to
> fix things for something we don't have access to.

We'll simply reject anything that should be implied by compatible. And
now we can enforce with schema that DTs aren't populated with
fallbacks alone. Of course, we can't make anyone pass all schema
checks before shipping a dtb.

Rob
Vasily Khoruzhick Feb. 4, 2019, 8:26 p.m. UTC | #16
On Mon, Feb 4, 2019 at 12:23 PM Rob Herring <robh+dt@kernel.org> wrote:

> > simple-panel would probably work if you stuck in some mostly compatible
> > string and provided a ddc-i2c-bus property in the device tree node. The
> > generic-ish fallback case could be implemented by providing a fallback
> > compatible string (we used to have "simple-panel", which I think would
> > still be adequate for this I suppose) and adding a dummy descriptor in
> > the driver, perhaps one with pre-defined delays that could be adjusted
> > to work for all cases, or they could just be 0. At least that way we'd
> > be explicitly documenting that we support this as a fallback.
>
> I'd like something more specific than 'simple-panel' that at least
> implies it has EDID and whatever else we think it should imply.
>
> Looking into this a bit more, why don't we just do a connector here?
> eDP has a standard connector (with power). It's just like other
> connectors, but with power and no hotplug. If someone does their own
> interface, then they should do their own connector or panel binding.

Where do we put backlight in this case?

> > I'm not sure how you'd want to enforce that people provide the right
> > compatible strings that way. Currently there's no way to make your panel
> > work without adding a panel driver, so people are forced to write the
> > DT bindings and a driver, or add support to an existing one. If we open
> > this backdoor, I suspect many people will just take the easy route and
> > rely on the fallback. And then what do we do when we get bug reports
> > about panels not working, or requiring some quirks. How do we go and
> > find out what the right compatible strings are for each board, or how to
> > fix things for something we don't have access to.
>
> We'll simply reject anything that should be implied by compatible. And
> now we can enforce with schema that DTs aren't populated with
> fallbacks alone. Of course, we can't make anyone pass all schema
> checks before shipping a dtb.
>
> Rob
Rob Herring Feb. 4, 2019, 8:34 p.m. UTC | #17
On Mon, Feb 4, 2019 at 2:26 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Mon, Feb 4, 2019 at 12:23 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> > > simple-panel would probably work if you stuck in some mostly compatible
> > > string and provided a ddc-i2c-bus property in the device tree node. The
> > > generic-ish fallback case could be implemented by providing a fallback
> > > compatible string (we used to have "simple-panel", which I think would
> > > still be adequate for this I suppose) and adding a dummy descriptor in
> > > the driver, perhaps one with pre-defined delays that could be adjusted
> > > to work for all cases, or they could just be 0. At least that way we'd
> > > be explicitly documenting that we support this as a fallback.
> >
> > I'd like something more specific than 'simple-panel' that at least
> > implies it has EDID and whatever else we think it should imply.
> >
> > Looking into this a bit more, why don't we just do a connector here?
> > eDP has a standard connector (with power). It's just like other
> > connectors, but with power and no hotplug. If someone does their own
> > interface, then they should do their own connector or panel binding.
>
> Where do we put backlight in this case?

In the connector node. You'd need a backlight supply,
'backlight-enable-gpios' and backlight phandle to pwm-backlight node.

Rob
Daniel Vetter Feb. 5, 2019, 8:57 a.m. UTC | #18
On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > regulator and/or backlight.
> > > > > > > >
> > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > ---
> > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > >
> > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > >
> > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > is faulty on some device.
> > > > > > 
> > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > and recent batch uses 1080p panel.
> > > > > > 
> > > > > > What panel descriptor should I use as fallback?
> > > > > 
> > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > your device tree contains the correct information, the descriptor is
> > > > > correct for the panel you have.
> > > > > 
> > > > > In other words you need to ensure that you have the correct panel in
> > > > > device tree for the board that you're using. This is exactly the same
> > > > > thing as for other devices.
> > > > > 
> > > > > One way to to this is to have separate device trees for each variant
> > > > > of the board that you want to support. Another variant may be to have
> > > > > a common device tree and then have some early firmware update the DTB
> > > > > with the correct panel information.
> > > > 
> > > > This would defeat the point of edp, which is to standardize the mess of
> > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > infrastructure already (using the OUI), I think if there's something that
> > > > doesn't work then we should quirk it there.
> > > 
> > > The problem is that while the attempt may have been to standardize, it
> > > failed. It doesn't take into account any of the details such as timing
> > > between things like powering up the display and enabling the backlight
> > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > requirements because they are highly panel specific.
> > 
> > Hm right, we get these from some firmware tables (and mix them with the
> > spec one, since some of the firmware values are nonsense). I don't even
> > know whether we can read the timings over dp aux somehow (you can power up
> > the panel with some pessimistic values to figure those out, and you only
> > need dp aux to work, which is much simpler than the entire panel).
> > 
> > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > also some integration awkwardness since with this panel you need to do dp
> > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > takes both a DT node and the dp_aux controller would be much better,
> > > > instead of trying to auto-match against a DT compatible string and load a
> > > > panel driver which is almost all fake.
> > > > 
> > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > can get hold of the dp_aux channel it needs.
> > > 
> > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > use that on some platforms.
> > > 
> > > Also note that simple-panel already supports getting video timings from
> > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > from EDID and use them.
> > 
> > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > then gives you the ddc link automatically).
> 
> I suppose we could do that. We could introduce a new property that would
> allow the panel driver to get at the struct drm_dp_aux that can access
> the panel. I'm not sure how much that would buy us. I suppose the driver
> could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> ddc-bus property in device tree. A drm_dp_aux object could also be used
> to access DPCD if that's helpful.
> 
> The driver proposed here doesn't need access to DPCD, so I'm not sure
> that would immediately help.

You definitely need dp aux to drive edp. That's where a lot of the really
important stuff is stored, and it sounds like on non-broken panels even
the timings (we've never implemented that on i915 somehow).
-Daniel
Thierry Reding Feb. 5, 2019, 10:24 a.m. UTC | #19
On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > regulator and/or backlight.
> > > > > > > > >
> > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > ---
> > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > >
> > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > >
> > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > is faulty on some device.
> > > > > > > 
> > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > and recent batch uses 1080p panel.
> > > > > > > 
> > > > > > > What panel descriptor should I use as fallback?
> > > > > > 
> > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > your device tree contains the correct information, the descriptor is
> > > > > > correct for the panel you have.
> > > > > > 
> > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > thing as for other devices.
> > > > > > 
> > > > > > One way to to this is to have separate device trees for each variant
> > > > > > of the board that you want to support. Another variant may be to have
> > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > with the correct panel information.
> > > > > 
> > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > doesn't work then we should quirk it there.
> > > > 
> > > > The problem is that while the attempt may have been to standardize, it
> > > > failed. It doesn't take into account any of the details such as timing
> > > > between things like powering up the display and enabling the backlight
> > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > requirements because they are highly panel specific.
> > > 
> > > Hm right, we get these from some firmware tables (and mix them with the
> > > spec one, since some of the firmware values are nonsense). I don't even
> > > know whether we can read the timings over dp aux somehow (you can power up
> > > the panel with some pessimistic values to figure those out, and you only
> > > need dp aux to work, which is much simpler than the entire panel).
> > > 
> > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > panel driver which is almost all fake.
> > > > > 
> > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > can get hold of the dp_aux channel it needs.
> > > > 
> > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > use that on some platforms.
> > > > 
> > > > Also note that simple-panel already supports getting video timings from
> > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > from EDID and use them.
> > > 
> > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > then gives you the ddc link automatically).
> > 
> > I suppose we could do that. We could introduce a new property that would
> > allow the panel driver to get at the struct drm_dp_aux that can access
> > the panel. I'm not sure how much that would buy us. I suppose the driver
> > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > to access DPCD if that's helpful.
> > 
> > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > that would immediately help.
> 
> You definitely need dp aux to drive edp. That's where a lot of the really
> important stuff is stored, and it sounds like on non-broken panels even
> the timings (we've never implemented that on i915 somehow).

I'm not sure I understand what you're saying here. I haven't worked with
eDP panels in a while, but my recollection is that you can use DP AUX to
read video timings over EDID. We provide support for that by exporting a
DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
and then that I2C adapter can be used to read the EDID. I wasn't aware
that eDP panels additionally stored the video timings somewhere else.

What I meant above was that aside from the I2C-over-AUX for reading the
EDID, this driver doesn't do anything else with DP AUX in order to turn
the panel on. Looking at the eDP support we have on Tegra, there's a
DPCD register (DP_SET_POWER) that needs to be written in order to take
the sink device (i.e. panel) out of the power saving state. We do that
as part of the connector implementation rather than within the panel
driver. There are also additional registers such as DP_LINK_BW_SET that
need to be programmed. I think this is also relevant to regular DP and
detailed in the specification.

Given all the above, I'm beginning to think that Rob's right in that
perhaps we shouldn't be treating eDP panels as panels, but rather to
make them look more like DP monitors and make all this code part of the
connector implementation. I think pretty much the only differences to
regular DP are that we might require some lower-level resources that a
DP monitor would usually have built-in (reset or power GPIOs, power
supplies, backlight, ...).

I'm not sure if that's enough for eDP panels, though. For example the
AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
panel. But the driver also specifies a couple of additional delays which
suggests that either it violates the eDP specification or that the eDP
specification doesn't define any power sequencing delays that would've
been needed. Or perhaps these delays are specified somewhere and the
driver just doesn't use them?

Thierry
Daniel Vetter Feb. 5, 2019, 4:36 p.m. UTC | #20
On Tue, Feb 05, 2019 at 11:24:19AM +0100, Thierry Reding wrote:
> On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > > regulator and/or backlight.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > >
> > > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > >
> > > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > > is faulty on some device.
> > > > > > > > 
> > > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > > and recent batch uses 1080p panel.
> > > > > > > > 
> > > > > > > > What panel descriptor should I use as fallback?
> > > > > > > 
> > > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > > your device tree contains the correct information, the descriptor is
> > > > > > > correct for the panel you have.
> > > > > > > 
> > > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > > thing as for other devices.
> > > > > > > 
> > > > > > > One way to to this is to have separate device trees for each variant
> > > > > > > of the board that you want to support. Another variant may be to have
> > > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > > with the correct panel information.
> > > > > > 
> > > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > > doesn't work then we should quirk it there.
> > > > > 
> > > > > The problem is that while the attempt may have been to standardize, it
> > > > > failed. It doesn't take into account any of the details such as timing
> > > > > between things like powering up the display and enabling the backlight
> > > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > > requirements because they are highly panel specific.
> > > > 
> > > > Hm right, we get these from some firmware tables (and mix them with the
> > > > spec one, since some of the firmware values are nonsense). I don't even
> > > > know whether we can read the timings over dp aux somehow (you can power up
> > > > the panel with some pessimistic values to figure those out, and you only
> > > > need dp aux to work, which is much simpler than the entire panel).
> > > > 
> > > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > > panel driver which is almost all fake.
> > > > > > 
> > > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > > can get hold of the dp_aux channel it needs.
> > > > > 
> > > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > > use that on some platforms.
> > > > > 
> > > > > Also note that simple-panel already supports getting video timings from
> > > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > > from EDID and use them.
> > > > 
> > > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > > then gives you the ddc link automatically).
> > > 
> > > I suppose we could do that. We could introduce a new property that would
> > > allow the panel driver to get at the struct drm_dp_aux that can access
> > > the panel. I'm not sure how much that would buy us. I suppose the driver
> > > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > > to access DPCD if that's helpful.
> > > 
> > > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > > that would immediately help.
> > 
> > You definitely need dp aux to drive edp. That's where a lot of the really
> > important stuff is stored, and it sounds like on non-broken panels even
> > the timings (we've never implemented that on i915 somehow).
> 
> I'm not sure I understand what you're saying here. I haven't worked with
> eDP panels in a while, but my recollection is that you can use DP AUX to
> read video timings over EDID. We provide support for that by exporting a
> DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
> and then that I2C adapter can be used to read the EDID. I wasn't aware
> that eDP panels additionally stored the video timings somewhere else.

I didn't know either, but understood Vasily's comment that this is
possible. Haven't dug around in the specs though.

> What I meant above was that aside from the I2C-over-AUX for reading the
> EDID, this driver doesn't do anything else with DP AUX in order to turn
> the panel on. Looking at the eDP support we have on Tegra, there's a
> DPCD register (DP_SET_POWER) that needs to be written in order to take
> the sink device (i.e. panel) out of the power saving state. We do that
> as part of the connector implementation rather than within the panel
> driver. There are also additional registers such as DP_LINK_BW_SET that
> need to be programmed. I think this is also relevant to regular DP and
> detailed in the specification.
> 
> Given all the above, I'm beginning to think that Rob's right in that
> perhaps we shouldn't be treating eDP panels as panels, but rather to
> make them look more like DP monitors and make all this code part of the
> connector implementation. I think pretty much the only differences to
> regular DP are that we might require some lower-level resources that a
> DP monitor would usually have built-in (reset or power GPIOs, power
> supplies, backlight, ...).
> 
> I'm not sure if that's enough for eDP panels, though. For example the
> AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
> panel. But the driver also specifies a couple of additional delays which
> suggests that either it violates the eDP specification or that the eDP
> specification doesn't define any power sequencing delays that would've
> been needed. Or perhaps these delays are specified somewhere and the
> driver just doesn't use them?

I honestly don't know, but gut feeling also goes towards making edp
something else than a dumb panel. There's a lot of stuff you need to do
that overlaps with normal DP sinks, plus there's a lot of stuff (but not
everything, or at least it needs some quirking sometimes) that you're
supposed to autodiscover. But I'm not sure what's the best solution really
is, since for i915 all the quirking we need is in some i915-specific
tables (and I'm not even sure how much of that quirking is really
necessary, and how much just convenience for us because the fw already did
the hard work of poking the information out of the panel).
-Daniel
Vasily Khoruzhick Feb. 14, 2019, 7:51 p.m. UTC | #21
On Tue, Feb 5, 2019 at 8:36 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Feb 05, 2019 at 11:24:19AM +0100, Thierry Reding wrote:
> > On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > > > regulator and/or backlight.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > > >
> > > > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > > >
> > > > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > > > is faulty on some device.
> > > > > > > > >
> > > > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > > > and recent batch uses 1080p panel.
> > > > > > > > >
> > > > > > > > > What panel descriptor should I use as fallback?
> > > > > > > >
> > > > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > > > your device tree contains the correct information, the descriptor is
> > > > > > > > correct for the panel you have.
> > > > > > > >
> > > > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > > > thing as for other devices.
> > > > > > > >
> > > > > > > > One way to to this is to have separate device trees for each variant
> > > > > > > > of the board that you want to support. Another variant may be to have
> > > > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > > > with the correct panel information.
> > > > > > >
> > > > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > > > doesn't work then we should quirk it there.
> > > > > >
> > > > > > The problem is that while the attempt may have been to standardize, it
> > > > > > failed. It doesn't take into account any of the details such as timing
> > > > > > between things like powering up the display and enabling the backlight
> > > > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > > > requirements because they are highly panel specific.
> > > > >
> > > > > Hm right, we get these from some firmware tables (and mix them with the
> > > > > spec one, since some of the firmware values are nonsense). I don't even
> > > > > know whether we can read the timings over dp aux somehow (you can power up
> > > > > the panel with some pessimistic values to figure those out, and you only
> > > > > need dp aux to work, which is much simpler than the entire panel).
> > > > >
> > > > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > > > panel driver which is almost all fake.
> > > > > > >
> > > > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > > > can get hold of the dp_aux channel it needs.
> > > > > >
> > > > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > > > use that on some platforms.
> > > > > >
> > > > > > Also note that simple-panel already supports getting video timings from
> > > > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > > > from EDID and use them.
> > > > >
> > > > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > > > then gives you the ddc link automatically).
> > > >
> > > > I suppose we could do that. We could introduce a new property that would
> > > > allow the panel driver to get at the struct drm_dp_aux that can access
> > > > the panel. I'm not sure how much that would buy us. I suppose the driver
> > > > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > > > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > > > to access DPCD if that's helpful.
> > > >
> > > > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > > > that would immediately help.
> > >
> > > You definitely need dp aux to drive edp. That's where a lot of the really
> > > important stuff is stored, and it sounds like on non-broken panels even
> > > the timings (we've never implemented that on i915 somehow).
> >
> > I'm not sure I understand what you're saying here. I haven't worked with
> > eDP panels in a while, but my recollection is that you can use DP AUX to
> > read video timings over EDID. We provide support for that by exporting a
> > DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
> > and then that I2C adapter can be used to read the EDID. I wasn't aware
> > that eDP panels additionally stored the video timings somewhere else.
>
> I didn't know either, but understood Vasily's comment that this is
> possible. Haven't dug around in the specs though.

It probes link capabilities such as lanes number and rate, see
drm_dp_link_probe(),
I'm not sure if it's panel property or not.

> > What I meant above was that aside from the I2C-over-AUX for reading the
> > EDID, this driver doesn't do anything else with DP AUX in order to turn
> > the panel on. Looking at the eDP support we have on Tegra, there's a
> > DPCD register (DP_SET_POWER) that needs to be written in order to take
> > the sink device (i.e. panel) out of the power saving state. We do that
> > as part of the connector implementation rather than within the panel
> > driver. There are also additional registers such as DP_LINK_BW_SET that
> > need to be programmed. I think this is also relevant to regular DP and
> > detailed in the specification.
> >
> > Given all the above, I'm beginning to think that Rob's right in that
> > perhaps we shouldn't be treating eDP panels as panels, but rather to
> > make them look more like DP monitors and make all this code part of the
> > connector implementation. I think pretty much the only differences to
> > regular DP are that we might require some lower-level resources that a
> > DP monitor would usually have built-in (reset or power GPIOs, power
> > supplies, backlight, ...).
> >
> > I'm not sure if that's enough for eDP panels, though. For example the
> > AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
> > panel. But the driver also specifies a couple of additional delays which
> > suggests that either it violates the eDP specification or that the eDP
> > specification doesn't define any power sequencing delays that would've
> > been needed. Or perhaps these delays are specified somewhere and the
> > driver just doesn't use them?
>
> I honestly don't know, but gut feeling also goes towards making edp
> something else than a dumb panel. There's a lot of stuff you need to do
> that overlaps with normal DP sinks, plus there's a lot of stuff (but not
> everything, or at least it needs some quirking sometimes) that you're
> supposed to autodiscover. But I'm not sure what's the best solution really
> is, since for i915 all the quirking we need is in some i915-specific
> tables (and I'm not even sure how much of that quirking is really
> necessary, and how much just convenience for us because the fw already did
> the hard work of poking the information out of the panel).
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
Vasily Khoruzhick Feb. 14, 2019, 8:05 p.m. UTC | #22
On Tue, Feb 5, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
>
> On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > > >
> > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > > regulator and/or backlight.
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > > ---
> > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > >
> > > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > >
> > > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > > is faulty on some device.
> > > > > > > >
> > > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > > and recent batch uses 1080p panel.
> > > > > > > >
> > > > > > > > What panel descriptor should I use as fallback?
> > > > > > >
> > > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > > your device tree contains the correct information, the descriptor is
> > > > > > > correct for the panel you have.
> > > > > > >
> > > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > > thing as for other devices.
> > > > > > >
> > > > > > > One way to to this is to have separate device trees for each variant
> > > > > > > of the board that you want to support. Another variant may be to have
> > > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > > with the correct panel information.
> > > > > >
> > > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > > doesn't work then we should quirk it there.
> > > > >
> > > > > The problem is that while the attempt may have been to standardize, it
> > > > > failed. It doesn't take into account any of the details such as timing
> > > > > between things like powering up the display and enabling the backlight
> > > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > > requirements because they are highly panel specific.
> > > >
> > > > Hm right, we get these from some firmware tables (and mix them with the
> > > > spec one, since some of the firmware values are nonsense). I don't even
> > > > know whether we can read the timings over dp aux somehow (you can power up
> > > > the panel with some pessimistic values to figure those out, and you only
> > > > need dp aux to work, which is much simpler than the entire panel).
> > > >
> > > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > > panel driver which is almost all fake.
> > > > > >
> > > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > > can get hold of the dp_aux channel it needs.
> > > > >
> > > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > > use that on some platforms.
> > > > >
> > > > > Also note that simple-panel already supports getting video timings from
> > > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > > from EDID and use them.
> > > >
> > > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > > then gives you the ddc link automatically).
> > >
> > > I suppose we could do that. We could introduce a new property that would
> > > allow the panel driver to get at the struct drm_dp_aux that can access
> > > the panel. I'm not sure how much that would buy us. I suppose the driver
> > > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > > to access DPCD if that's helpful.
> > >
> > > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > > that would immediately help.
> >
> > You definitely need dp aux to drive edp. That's where a lot of the really
> > important stuff is stored, and it sounds like on non-broken panels even
> > the timings (we've never implemented that on i915 somehow).
>
> I'm not sure I understand what you're saying here. I haven't worked with
> eDP panels in a while, but my recollection is that you can use DP AUX to
> read video timings over EDID. We provide support for that by exporting a
> DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
> and then that I2C adapter can be used to read the EDID. I wasn't aware
> that eDP panels additionally stored the video timings somewhere else.
>
> What I meant above was that aside from the I2C-over-AUX for reading the
> EDID, this driver doesn't do anything else with DP AUX in order to turn
> the panel on. Looking at the eDP support we have on Tegra, there's a
> DPCD register (DP_SET_POWER) that needs to be written in order to take
> the sink device (i.e. panel) out of the power saving state. We do that
> as part of the connector implementation rather than within the panel
> driver. There are also additional registers such as DP_LINK_BW_SET that
> need to be programmed. I think this is also relevant to regular DP and
> detailed in the specification.
>
> Given all the above, I'm beginning to think that Rob's right in that
> perhaps we shouldn't be treating eDP panels as panels, but rather to
> make them look more like DP monitors and make all this code part of the
> connector implementation. I think pretty much the only differences to
> regular DP are that we might require some lower-level resources that a
> DP monitor would usually have built-in (reset or power GPIOs, power
> supplies, backlight, ...).

I spent some time poking drm_connector code and I came to conclusion
that it's not a good idea.
Basically edp-connector driver will duplicate simple-panel code and
will bring extra complexity
into bridge driver for no benefit at all.

Also currently there are no stand-alone connector drivers, they all
are part of display controller
driver.

One more thing to add is that I'm not sure that drm_connector is
suitable for managing power
and backlight -  I can't find appropriate callback in
drm_connector_funcs to enable power and/or
backlight. Basically there's nothing similar to enable() or disable()
from drm_bridge_funcs.

> I'm not sure if that's enough for eDP panels, though. For example the
> AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
> panel. But the driver also specifies a couple of additional delays which
> suggests that either it violates the eDP specification or that the eDP
> specification doesn't define any power sequencing delays that would've
> been needed. Or perhaps these delays are specified somewhere and the
> driver just doesn't use them?

Sigh. We can't foresee any bizarre behavior some hardware may or may
not have. Anyway,
can you propose something that can handle same hardware with different
edp panels via
single software image (u-boot aka firmware is part of software image)
and is acceptable
upstream?

Regards,
Vasily

> Thierry
Rob Herring Feb. 14, 2019, 8:38 p.m. UTC | #23
On Thu, Feb 14, 2019 at 2:04 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
>
> On Tue, Feb 5, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> >
> > On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > > > >
> > > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > > > regulator and/or backlight.
> > > > > > > > > > >
> > > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > > > ---
> > > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > > >
> > > > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > > >
> > > > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > > > is faulty on some device.
> > > > > > > > >
> > > > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > > > and recent batch uses 1080p panel.
> > > > > > > > >
> > > > > > > > > What panel descriptor should I use as fallback?
> > > > > > > >
> > > > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > > > your device tree contains the correct information, the descriptor is
> > > > > > > > correct for the panel you have.
> > > > > > > >
> > > > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > > > thing as for other devices.
> > > > > > > >
> > > > > > > > One way to to this is to have separate device trees for each variant
> > > > > > > > of the board that you want to support. Another variant may be to have
> > > > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > > > with the correct panel information.
> > > > > > >
> > > > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > > > doesn't work then we should quirk it there.
> > > > > >
> > > > > > The problem is that while the attempt may have been to standardize, it
> > > > > > failed. It doesn't take into account any of the details such as timing
> > > > > > between things like powering up the display and enabling the backlight
> > > > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > > > requirements because they are highly panel specific.
> > > > >
> > > > > Hm right, we get these from some firmware tables (and mix them with the
> > > > > spec one, since some of the firmware values are nonsense). I don't even
> > > > > know whether we can read the timings over dp aux somehow (you can power up
> > > > > the panel with some pessimistic values to figure those out, and you only
> > > > > need dp aux to work, which is much simpler than the entire panel).
> > > > >
> > > > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > > > panel driver which is almost all fake.
> > > > > > >
> > > > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > > > can get hold of the dp_aux channel it needs.
> > > > > >
> > > > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > > > use that on some platforms.
> > > > > >
> > > > > > Also note that simple-panel already supports getting video timings from
> > > > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > > > from EDID and use them.
> > > > >
> > > > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > > > then gives you the ddc link automatically).
> > > >
> > > > I suppose we could do that. We could introduce a new property that would
> > > > allow the panel driver to get at the struct drm_dp_aux that can access
> > > > the panel. I'm not sure how much that would buy us. I suppose the driver
> > > > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > > > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > > > to access DPCD if that's helpful.
> > > >
> > > > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > > > that would immediately help.
> > >
> > > You definitely need dp aux to drive edp. That's where a lot of the really
> > > important stuff is stored, and it sounds like on non-broken panels even
> > > the timings (we've never implemented that on i915 somehow).
> >
> > I'm not sure I understand what you're saying here. I haven't worked with
> > eDP panels in a while, but my recollection is that you can use DP AUX to
> > read video timings over EDID. We provide support for that by exporting a
> > DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
> > and then that I2C adapter can be used to read the EDID. I wasn't aware
> > that eDP panels additionally stored the video timings somewhere else.
> >
> > What I meant above was that aside from the I2C-over-AUX for reading the
> > EDID, this driver doesn't do anything else with DP AUX in order to turn
> > the panel on. Looking at the eDP support we have on Tegra, there's a
> > DPCD register (DP_SET_POWER) that needs to be written in order to take
> > the sink device (i.e. panel) out of the power saving state. We do that
> > as part of the connector implementation rather than within the panel
> > driver. There are also additional registers such as DP_LINK_BW_SET that
> > need to be programmed. I think this is also relevant to regular DP and
> > detailed in the specification.
> >
> > Given all the above, I'm beginning to think that Rob's right in that
> > perhaps we shouldn't be treating eDP panels as panels, but rather to
> > make them look more like DP monitors and make all this code part of the
> > connector implementation. I think pretty much the only differences to
> > regular DP are that we might require some lower-level resources that a
> > DP monitor would usually have built-in (reset or power GPIOs, power
> > supplies, backlight, ...).
>
> I spent some time poking drm_connector code and I came to conclusion
> that it's not a good idea.
> Basically edp-connector driver will duplicate simple-panel code and
> will bring extra complexity
> into bridge driver for no benefit at all.

I said this on irc, but for everyone's benefit, what's used in the
kernel and the binding don't have to be aligned. I still think
following a connector binding in DT makes sense even if the kernel
implementation is actually a panel driver. Really, there's no
difference in bindings between a connector node and panel node.

> Also currently there are no stand-alone connector drivers, they all
> are part of display controller
> driver.

Probably that is something to be refactored. I think we have lots of cases of:

if (is_connector)
    // call connector func
else
    // call panel func

Or maybe that was panel vs. bridge (then a connector)? It's been a
while since I looked at this. In any case, each device in the chain
shouldn't really have much knowledge as to what it is attached to.

> One more thing to add is that I'm not sure that drm_connector is
> suitable for managing power
> and backlight -  I can't find appropriate callback in
> drm_connector_funcs to enable power and/or
> backlight. Basically there's nothing similar to enable() or disable()
> from drm_bridge_funcs.

Nobody has put the 5V supply on HDMI on a switchable regulator? It may
need to be always on for HPD to work, but I wouldn't expect all board
designers to get that right.

> > I'm not sure if that's enough for eDP panels, though. For example the
> > AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
> > panel. But the driver also specifies a couple of additional delays which
> > suggests that either it violates the eDP specification or that the eDP
> > specification doesn't define any power sequencing delays that would've
> > been needed. Or perhaps these delays are specified somewhere and the
> > driver just doesn't use them?
>
> Sigh. We can't foresee any bizarre behavior some hardware may or may
> not have.

Bingo! You just summed up why we have specific compatibles.

> Anyway,
> can you propose something that can handle same hardware with different
> edp panels via
> single software image (u-boot aka firmware is part of software image)
> and is acceptable
> upstream?

compatible = "vendor,some-panel", "edp-connector";

Either you have to fixup the first string for the actual panel or have
some testing up front that you don't need to. The kernel can start
with only using the fallback string and if panel constraints turn out
to be specified sufficiently, we'll never need to use the panel
compatible.

Rob
Vasily Khoruzhick Feb. 14, 2019, 8:54 p.m. UTC | #24
On Thu, Feb 14, 2019 at 12:38 PM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Thu, Feb 14, 2019 at 2:04 PM Vasily Khoruzhick <anarsoul@gmail.com> wrote:
> >
> > On Tue, Feb 5, 2019 at 2:24 AM Thierry Reding <thierry.reding@gmail.com> wrote:
> > >
> > > On Tue, Feb 05, 2019 at 09:57:37AM +0100, Daniel Vetter wrote:
> > > > On Mon, Feb 04, 2019 at 05:22:58PM +0100, Thierry Reding wrote:
> > > > > On Mon, Feb 04, 2019 at 04:59:09PM +0100, Daniel Vetter wrote:
> > > > > > On Mon, Feb 04, 2019 at 12:22:18PM +0100, Thierry Reding wrote:
> > > > > > > On Mon, Feb 04, 2019 at 10:40:12AM +0100, Daniel Vetter wrote:
> > > > > > > > On Mon, Feb 04, 2019 at 09:23:59AM +0100, Thierry Reding wrote:
> > > > > > > > > On Mon, Feb 04, 2019 at 12:13:55AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > On Sun, Feb 3, 2019 at 11:43 PM Thierry Reding <thierry.reding@gmail.com> wrote:
> > > > > > > > > > >
> > > > > > > > > > > On Sun, Feb 03, 2019 at 10:54:57AM -0800, Vasily Khoruzhick wrote:
> > > > > > > > > > > > eDP panels usually have EDID EEPROM, so there's no need to define panel
> > > > > > > > > > > > width/height or any modes/timings in dts. But this panel still may have
> > > > > > > > > > > > regulator and/or backlight.
> > > > > > > > > > > >
> > > > > > > > > > > > Signed-off-by: Vasily Khoruzhick <anarsoul@gmail.com>
> > > > > > > > > > > > ---
> > > > > > > > > > > >  .../devicetree/bindings/display/panel/panel-edp.txt        | 7 +++++++
> > > > > > > > > > > >  1 file changed, 7 insertions(+)
> > > > > > > > > > > >  create mode 100644 Documentation/devicetree/bindings/display/panel/panel-edp.txt
> > > > > > > > > > >
> > > > > > > > > > > Please don't try to make panels look more generic than they really are.
> > > > > > > > > > > You're going to have to provide a compatible string for your device that
> > > > > > > > > > > is more specific than "panel-edp". You claim that you don't need any
> > > > > > > > > > > extra information that is panel specific, but you don't know that now.
> > > > > > > > > > > We have in the past thought that we didn't need things like prepare
> > > > > > > > > > > delay, but then we ran into situations where we did need them.
> > > > > > > > > > >
> > > > > > > > > > > Just do what everybody else does. Provide a specific compatible string
> > > > > > > > > > > and match on that in the panel-simple driver. Even if you can read all
> > > > > > > > > > > the video timings from an EDID EEPROM, you can still provide a mode in
> > > > > > > > > > > the panel descriptor to serve as a fallback if for example the EEPROM
> > > > > > > > > > > is faulty on some device.
> > > > > > > > > >
> > > > > > > > > > Pinebook used several 768p panels that have slightly different timings
> > > > > > > > > > and recent batch uses 1080p panel.
> > > > > > > > > >
> > > > > > > > > > What panel descriptor should I use as fallback?
> > > > > > > > >
> > > > > > > > > You don't use panel descriptors as fallback. The simple-panel driver
> > > > > > > > > will bind to a panel device and use the corresponding descriptor. If
> > > > > > > > > your device tree contains the correct information, the descriptor is
> > > > > > > > > correct for the panel you have.
> > > > > > > > >
> > > > > > > > > In other words you need to ensure that you have the correct panel in
> > > > > > > > > device tree for the board that you're using. This is exactly the same
> > > > > > > > > thing as for other devices.
> > > > > > > > >
> > > > > > > > > One way to to this is to have separate device trees for each variant
> > > > > > > > > of the board that you want to support. Another variant may be to have
> > > > > > > > > a common device tree and then have some early firmware update the DTB
> > > > > > > > > with the correct panel information.
> > > > > > > >
> > > > > > > > This would defeat the point of edp, which is to standardize the mess of
> > > > > > > > panels (at least somewhat) and avoid having to change the DT/ACPI
> > > > > > > > tables/firmware for every board you ship. Also, we do have DP quirking
> > > > > > > > infrastructure already (using the OUI), I think if there's something that
> > > > > > > > doesn't work then we should quirk it there.
> > > > > > >
> > > > > > > The problem is that while the attempt may have been to standardize, it
> > > > > > > failed. It doesn't take into account any of the details such as timing
> > > > > > > between things like powering up the display and enabling the backlight
> > > > > > > or similar. I don't know how you'd want to "quirk" those kinds of
> > > > > > > requirements because they are highly panel specific.
> > > > > >
> > > > > > Hm right, we get these from some firmware tables (and mix them with the
> > > > > > spec one, since some of the firmware values are nonsense). I don't even
> > > > > > know whether we can read the timings over dp aux somehow (you can power up
> > > > > > the panel with some pessimistic values to figure those out, and you only
> > > > > > need dp aux to work, which is much simpler than the entire panel).
> > > > > >
> > > > > > > > What does make sense though imo is if we try not to stuff the edp panel
> > > > > > > > into panel-simple, because it's anything like a simple dumb panel. There's
> > > > > > > > also some integration awkwardness since with this panel you need to do dp
> > > > > > > > aux/i2c transactions to get at the information (edid alone isn't good
> > > > > > > > enough for edp), and I'm not sure how exactly that's supposed to be
> > > > > > > > instantiated. Maybe a special function to instantiate an edp panel, which
> > > > > > > > takes both a DT node and the dp_aux controller would be much better,
> > > > > > > > instead of trying to auto-match against a DT compatible string and load a
> > > > > > > > panel driver which is almost all fake.
> > > > > > > >
> > > > > > > > Or we teach dp_aux to register itself and somehow teach panel-edp how it
> > > > > > > > can get hold of the dp_aux channel it needs.
> > > > > > >
> > > > > > > We already do that. drm_dp_aux registers as an I2C adapter that can be
> > > > > > > used to read EDID EEPROMs using I2C-over-AUX transactions. We already
> > > > > > > use that on some platforms.
> > > > > > >
> > > > > > > Also note that simple-panel already supports getting video timings from
> > > > > > > EDID. If a DDC link is present in DT, the driver will load the modes
> > > > > > > from EDID and use them.
> > > > > >
> > > > > > Could we extend this to dp aux somehow? For edp you need the dp aux (which
> > > > > > then gives you the ddc link automatically).
> > > > >
> > > > > I suppose we could do that. We could introduce a new property that would
> > > > > allow the panel driver to get at the struct drm_dp_aux that can access
> > > > > the panel. I'm not sure how much that would buy us. I suppose the driver
> > > > > could go and use that drm_dp_aux to do I2C-over-AUX and ignore any
> > > > > ddc-bus property in device tree. A drm_dp_aux object could also be used
> > > > > to access DPCD if that's helpful.
> > > > >
> > > > > The driver proposed here doesn't need access to DPCD, so I'm not sure
> > > > > that would immediately help.
> > > >
> > > > You definitely need dp aux to drive edp. That's where a lot of the really
> > > > important stuff is stored, and it sounds like on non-broken panels even
> > > > the timings (we've never implemented that on i915 somehow).
> > >
> > > I'm not sure I understand what you're saying here. I haven't worked with
> > > eDP panels in a while, but my recollection is that you can use DP AUX to
> > > read video timings over EDID. We provide support for that by exporting a
> > > DP AUX controller as I2C adapter (i.e. register with the I2C subsystem)
> > > and then that I2C adapter can be used to read the EDID. I wasn't aware
> > > that eDP panels additionally stored the video timings somewhere else.
> > >
> > > What I meant above was that aside from the I2C-over-AUX for reading the
> > > EDID, this driver doesn't do anything else with DP AUX in order to turn
> > > the panel on. Looking at the eDP support we have on Tegra, there's a
> > > DPCD register (DP_SET_POWER) that needs to be written in order to take
> > > the sink device (i.e. panel) out of the power saving state. We do that
> > > as part of the connector implementation rather than within the panel
> > > driver. There are also additional registers such as DP_LINK_BW_SET that
> > > need to be programmed. I think this is also relevant to regular DP and
> > > detailed in the specification.
> > >
> > > Given all the above, I'm beginning to think that Rob's right in that
> > > perhaps we shouldn't be treating eDP panels as panels, but rather to
> > > make them look more like DP monitors and make all this code part of the
> > > connector implementation. I think pretty much the only differences to
> > > regular DP are that we might require some lower-level resources that a
> > > DP monitor would usually have built-in (reset or power GPIOs, power
> > > supplies, backlight, ...).
> >
> > I spent some time poking drm_connector code and I came to conclusion
> > that it's not a good idea.
> > Basically edp-connector driver will duplicate simple-panel code and
> > will bring extra complexity
> > into bridge driver for no benefit at all.
>
> I said this on irc, but for everyone's benefit, what's used in the
> kernel and the binding don't have to be aligned. I still think
> following a connector binding in DT makes sense even if the kernel
> implementation is actually a panel driver. Really, there's no
> difference in bindings between a connector node and panel node.
>
> > Also currently there are no stand-alone connector drivers, they all
> > are part of display controller
> > driver.
>
> Probably that is something to be refactored. I think we have lots of cases of:
>
> if (is_connector)
>     // call connector func
> else
>     // call panel func
>
> Or maybe that was panel vs. bridge (then a connector)? It's been a
> while since I looked at this. In any case, each device in the chain
> shouldn't really have much knowledge as to what it is attached to.
>
> > One more thing to add is that I'm not sure that drm_connector is
> > suitable for managing power
> > and backlight -  I can't find appropriate callback in
> > drm_connector_funcs to enable power and/or
> > backlight. Basically there's nothing similar to enable() or disable()
> > from drm_bridge_funcs.
>
> Nobody has put the 5V supply on HDMI on a switchable regulator? It may
> need to be always on for HPD to work, but I wouldn't expect all board
> designers to get that right.
>
> > > I'm not sure if that's enough for eDP panels, though. For example the
> > > AUO B133HTN01 panel, used by the exynos5800-peach-pi, seems to be an eDP
> > > panel. But the driver also specifies a couple of additional delays which
> > > suggests that either it violates the eDP specification or that the eDP
> > > specification doesn't define any power sequencing delays that would've
> > > been needed. Or perhaps these delays are specified somewhere and the
> > > driver just doesn't use them?
> >
> > Sigh. We can't foresee any bizarre behavior some hardware may or may
> > not have.
>
> Bingo! You just summed up why we have specific compatibles.
>
> > Anyway,
> > can you propose something that can handle same hardware with different
> > edp panels via
> > single software image (u-boot aka firmware is part of software image)
> > and is acceptable
> > upstream?
>
> compatible = "vendor,some-panel", "edp-connector";
>
> Either you have to fixup the first string for the actual panel or have
> some testing up front that you don't need to.

Where do I take actual panel compatible from?

> The kernel can start
> with only using the fallback string and if panel constraints turn out
> to be specified sufficiently, we'll never need to use the panel
> compatible.

So what panel compatible you want me to use if vendor used at least 3
different panels?
Introduce 2 more dts files (3 in total) even if they all work just
fine with a single dts now?
Add one more dts once laptop vendor decides to change panel vendor?

>
> Rob
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/display/panel/panel-edp.txt b/Documentation/devicetree/bindings/display/panel/panel-edp.txt
new file mode 100644
index 000000000000..fc7a0868048e
--- /dev/null
+++ b/Documentation/devicetree/bindings/display/panel/panel-edp.txt
@@ -0,0 +1,7 @@ 
+Generic eDP Display Panel
+
+Required properties:
+  - compatible: "panel-edp"
+
+This binding is compatible with the simple-panel binding, which is specified
+in simple-panel.txt in this directory.