[PATCH v7 00/10] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus

[PATCH v7 00/10] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus

[Douglas Anderson]

The primary goal of this series is to try to properly fix EDID reading
for eDP panels using the ti-sn65dsi86 bridge.

Previously we had a patch that added EDID reading but it turned out
not to work at bootup. This caused some extra churn at bootup as we
tried (and failed) to read the EDID several times and also ended up
forcing us to use the hardcoded mode at boot. With this patch series I
believe EDID reading is reliable at boot now and we never use the
hardcoded mode.

High level note: in this series the EDID reading is driven by the
panel driver, not by the bridge chip driver. I believe this makes a
reasonable amount of sense since the panel driver already _could_
drive reading the EDID if provided with the DDC bus and in future
planned work we'll want to give the panel driver the DDC bus (to make
decisions based on EDID) and the AUX bus (to control the
backlight). There are also planned patches from Laurent to make
ti-sn65dsi86 able to drive full DP monitors. In that case the bridge
chip will still be in charge of reading the EDID, but it's not hard to
make this dynamic.

This series is the logical successor to the 3-part series containing
the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
if refclk") [1].

This patch was tested against drm-misc-next commit 60a6b73dd821
("drm/ingenic: Fix pixclock rate for 24-bit serial panels") on a
sc7180-trogdor-lazor device.

At v7 now, this patch series grew a bit from v6 because it introduces
the DP AUX bus.

Between v2 and v3, high-level view of changes:
- stop doing the EDID caching in the core.

Between v3 and v4, high-level view of changes:
- EDID reading is actually driven by the panel driver now. See above.
- Lots of chicken-and-egg problems solved w/ sub-devices.

Between v4 and v5, high-level view of changes.
- Some of the early patches landed, so dropped from series.
- New pm_runtime_disable() fix (fixed a patch that already landed).
- Added Bjorn's tags to most patches
- Fixed problems when building as a module.
- Reordered debugfs patch and fixed error handling there.
- Dropped last patch. I'm not convinced it's safe w/out more work.

Between v5 and v6, high-level view of changes:
- Added the patch ("drm/dp: Allow an early call to register DDC i2c
  bus")
- Many patches had been landed, so only a few "controversial" ones
  left.

Between v6 and v7, high-level view of changes:
- New AUX DP bus!

[1] https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/

Changes in v7:
- pm_runtime_dont_use_autosuspend() fix new for v7.
- List hpd properties bindings patch new for v7.
- ti-sn65dsi86: Add aux-bus child patch new for v7.
- Patch introducing the DP AUX bus is new for v7.
- Patch to allow panel-simple to be DP AUX EP new for v7.
- Patch using the DP AUX for DDC new for v7.
- Remove use of now-dropped drm_dp_aux_register_ddc() call.
- Beefed up commit message in context of the DP AUX bus.
- Set the proper sub-device "dev" pointer in the AUX structure.
- Patch to support for DP AUX bus on ti-sn65dsi86 new for v7.
- Adjusted commit message to talk about DP AUX bus.
- Panel now under bridge chip instead of getting a link to ddc-i2c

Changes in v6:
- Use new drm_dp_aux_register_ddc() calls.

Douglas Anderson (10):
  drm/panel: panel-simple: Add missing pm_runtime_dont_use_autosuspend()
    calls
  dt-bindings: display: simple: List hpd properties in panel-simple
  dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child
  drm: Introduce the DP AUX bus
  drm/panel: panel-simple: Allow panel-simple be a DP AUX endpoint
    device
  drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC
  drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
  drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus
  drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
  arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip

 .../bindings/display/bridge/ti,sn65dsi86.yaml |  22 +-
 .../bindings/display/panel/panel-simple.yaml  |   2 +
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  30 +-
 drivers/gpu/drm/Kconfig                       |   5 +
 drivers/gpu/drm/Makefile                      |   2 +
 drivers/gpu/drm/bridge/Kconfig                |   1 +
 drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 111 ++++--
 drivers/gpu/drm/drm_dp_aux_bus.c              | 322 ++++++++++++++++++
 drivers/gpu/drm/panel/Kconfig                 |   1 +
 drivers/gpu/drm/panel/panel-simple.c          |  66 +++-
 include/drm/drm_dp_aux_bus.h                  |  57 ++++
 11 files changed, 563 insertions(+), 56 deletions(-)
 create mode 100644 drivers/gpu/drm/drm_dp_aux_bus.c
 create mode 100644 include/drm/drm_dp_aux_bus.h

-- 
2.31.1.751.gd2f1c929bd-goog

[PATCH v7 02/10] dt-bindings: display: simple: List hpd properties in panel-simple

[Douglas Anderson]

These are described in panel-common.yaml but if I don't list them in
panel-simple then I get yells when running 'dt_binding_check' in a
future patch. List them along with other properties that seem to be
listed in panel-simple for similar reasons.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I didn't spend tons of time digging to see if there was supposed to be
a better way of doing this. If there is, feel free to yell.

Changes in v7:
- List hpd properties bindings patch new for v7.

 .../devicetree/bindings/display/panel/panel-simple.yaml         | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
index b3797ba2698b..4a0a5e1ee252 100644
--- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
+++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
@@ -298,6 +298,8 @@ properties:
   enable-gpios: true
   port: true
   power-supply: true
+  no-hpd: true
+  hpd-gpios: true
 
 additionalProperties: false
 
-- 
2.31.1.751.gd2f1c929bd-goog

[PATCH v7 03/10] dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child

[Douglas Anderson]

We want to be able to list an eDP panel as a child of a ti-sn65dsi86
node to represent the fact that the panel is connected to the bridge's
DP AUX bus. Though the panel and the bridge chip are connected in
several ways, the DP AUX bus is the primary control interface between
the two and thus makes the most sense to model in device tree
hierarchy.

Listing a panel in this way makes it possible for the panel driver to
easily get access to the DP AUX bus that it resides on, which can be
useful to help in auto-detecting the panel and for turning on various
bits.

NOTE: it's still possible to continue using the bridge chip and point
to a panel that _isn't_ listed as a child of the bridge chip (since
it's worked that way previously), but that should be deprecated since
there is no downside to listing the panel under the bridge chip.

The idea for this bus's design was hashed out over IRC [1].

[1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2021-05-11

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Possibly we might want something fancier that could be included by
other eDP controller bindings. If we want to do this, I'd love to be
pointed at a good example to follow.

Changes in v7:
- ti-sn65dsi86: Add aux-bus child patch new for v7.

 .../bindings/display/bridge/ti,sn65dsi86.yaml | 22 ++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
index 26932d2e86ab..51f5a29e216c 100644
--- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
+++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
@@ -70,6 +70,11 @@ properties:
     const: 1
     description: See ../../pwm/pwm.yaml for description of the cell formats.
 
+  aux-bus:
+    description:
+      It is recommended that you place your panel under the aux-bus node
+      here to represent the control hierarchy.
+
   ports:
     $ref: /schemas/graph.yaml#/properties/ports
 
@@ -201,11 +206,26 @@ examples:
 
           port@1 {
             reg = <1>;
-            endpoint {
+            sn65dsi86_out: endpoint {
               remote-endpoint = <&panel_in_edp>;
             };
           };
         };
+
+        aux-bus {
+          panel {
+            compatible = "boe,nv133fhm-n62";
+            power-supply = <&pp3300_dx_edp>;
+            backlight = <&backlight>;
+            hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+
+            port {
+              panel_in_edp: endpoint {
+                remote-endpoint = <&sn65dsi86_out>;
+              };
+            };
+          };
+        };
       };
     };
   - |
-- 
2.31.1.751.gd2f1c929bd-goog

[PATCH v7 10/10] arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip

[Douglas Anderson]

Putting the panel under the bridge chip (under the aux-bus node)
allows the panel driver to get access to the DP AUX bus, enabling all
sorts of fabulous new features.

While we're at this, get rid of a level of hierarchy for the panel
node. It doesn't need "ports / port" and can just have a "port" child.

For Linux, this patch has a hard requirement on the patches adding DP
AUX bus support to the ti-sn65dsi86 bridge chip driver. See the patch
("drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus").

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v7:
- Panel now under bridge chip instead of getting a link to ddc-i2c

 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi | 30 ++++++++++----------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 24d293ef56d7..c76afd857b54 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -260,21 +260,6 @@ max98357a: audio-codec-0 {
 		#sound-dai-cells = <0>;
 	};
 
-	panel: panel {
-		/* Compatible will be filled in per-board */
-		power-supply = <&pp3300_dx_edp>;
-		backlight = <&backlight>;
-		hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
-
-		ports {
-			port {
-				panel_in_edp: endpoint {
-					remote-endpoint = <&sn65dsi86_out>;
-				};
-			};
-		};
-	};
-
 	pwmleds {
 		compatible = "pwm-leds";
 		keyboard_backlight: keyboard-backlight {
@@ -674,6 +659,21 @@ sn65dsi86_out: endpoint {
 				};
 			};
 		};
+
+		aux-bus {
+			panel: panel {
+				/* Compatible will be filled in per-board */
+				power-supply = <&pp3300_dx_edp>;
+				backlight = <&backlight>;
+				hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+
+				port {
+					panel_in_edp: endpoint {
+						remote-endpoint = <&sn65dsi86_out>;
+					};
+				};
+			};
+		};
 	};
 };
 
-- 
2.31.1.751.gd2f1c929bd-goog

Re: [PATCH v7 02/10] dt-bindings: display: simple: List hpd properties in panel-simple

[Rob Herring]

On Mon, May 17, 2021 at 3:09 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> These are described in panel-common.yaml but if I don't list them in
> panel-simple then I get yells when running 'dt_binding_check' in a
> future patch. List them along with other properties that seem to be
> listed in panel-simple for similar reasons.

If you have HPD, is it still a simple panel? I don't see this as an
omission because the use of these properties for simple panels was
never documented IIRC.

Not saying we can't add them, but justify it as an addition, not just
fixing a warning.

>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I didn't spend tons of time digging to see if there was supposed to be
> a better way of doing this. If there is, feel free to yell.

That's the right way to do it unless you want to allow all common
properties, then we'd use unevaluatedProperties instead of
additionalProperties.


>
> Changes in v7:
> - List hpd properties bindings patch new for v7.
>
>  .../devicetree/bindings/display/panel/panel-simple.yaml         | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> index b3797ba2698b..4a0a5e1ee252 100644
> --- a/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> +++ b/Documentation/devicetree/bindings/display/panel/panel-simple.yaml
> @@ -298,6 +298,8 @@ properties:
>    enable-gpios: true
>    port: true
>    power-supply: true
> +  no-hpd: true
> +  hpd-gpios: true
>
>  additionalProperties: false
>
> --
> 2.31.1.751.gd2f1c929bd-goog
>

Re: [PATCH v7 02/10] dt-bindings: display: simple: List hpd properties in panel-simple

[Doug Anderson]

Hi,

On Tue, May 18, 2021 at 5:42 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Mon, May 17, 2021 at 3:09 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > These are described in panel-common.yaml but if I don't list them in
> > panel-simple then I get yells when running 'dt_binding_check' in a
> > future patch. List them along with other properties that seem to be
> > listed in panel-simple for similar reasons.
>
> If you have HPD, is it still a simple panel? I don't see this as an
> omission because the use of these properties for simple panels was
> never documented IIRC.

I would say so. It is currently supported by panel-simple in Linux. Of
course, you could make the argument that panel-simple is no longer
really "simple" because of things like this...

I guess I'd say this: I believe that the HPD properties eventually
belong in the generic "edp-panel" that I'm still planning to post (and
which will be based on this series). I justified that previously [1]
by talking about the fact that there's a single timing diagram that
(as far as I've been able to tell) is fairly universal in panel specs.
It's a complicated timing diagram showing some two dozen timings (and
includes HPD!), but if you support all the timings then you've
supported pretty much all panels. IMO the original intent of
"simple-panel" was to specify a panel that's just like all the other
panels w/ a few parameters.

NOTE: I'd also say that for nearly all eDP panels HPD is important,
but in many designs HPD is handled "magically" and not specified in
the device tree. This is because it goes to a dedicated location on
the eDP controller / bridge chip. I added the HPD GPIO support (and
no-hpd) to simple-panel because my bridge chip has a fairly useless
HPD line and we don't use it. Even though the fact that we need the
HPD specified like this is a function of our bridge chip, back in the
day I was told that the property belonged in the panel and so that's
where it lives.


> Not saying we can't add them, but justify it as an addition, not just
> fixing a warning.

Sure, I'll beef up the commit message.


> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I didn't spend tons of time digging to see if there was supposed to be
> > a better way of doing this. If there is, feel free to yell.
>
> That's the right way to do it unless you want to allow all common
> properties, then we'd use unevaluatedProperties instead of
> additionalProperties.

Ah, perfect. Thanks!

[1] https://lore.kernel.org/r/CAD=FV=VZYOMPwQZzWdhJGh5cjJWw_EcM-wQVEivZ-bdGXjPrEQ@mail.gmail.com/


-Doug

Re: [PATCH v7 03/10] dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child

[Rob Herring]

On Mon, May 17, 2021 at 01:09:00PM -0700, Douglas Anderson wrote:
> We want to be able to list an eDP panel as a child of a ti-sn65dsi86
> node to represent the fact that the panel is connected to the bridge's
> DP AUX bus. Though the panel and the bridge chip are connected in
> several ways, the DP AUX bus is the primary control interface between
> the two and thus makes the most sense to model in device tree
> hierarchy.
> 
> Listing a panel in this way makes it possible for the panel driver to
> easily get access to the DP AUX bus that it resides on, which can be
> useful to help in auto-detecting the panel and for turning on various
> bits.
> 
> NOTE: it's still possible to continue using the bridge chip and point
> to a panel that _isn't_ listed as a child of the bridge chip (since
> it's worked that way previously), but that should be deprecated since
> there is no downside to listing the panel under the bridge chip.
> 
> The idea for this bus's design was hashed out over IRC [1].
> 
> [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2021-05-11
> 
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Possibly we might want something fancier that could be included by
> other eDP controller bindings. If we want to do this, I'd love to be
> pointed at a good example to follow.
> 
> Changes in v7:
> - ti-sn65dsi86: Add aux-bus child patch new for v7.
> 
>  .../bindings/display/bridge/ti,sn65dsi86.yaml | 22 ++++++++++++++++++-
>  1 file changed, 21 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> index 26932d2e86ab..51f5a29e216c 100644
> --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> @@ -70,6 +70,11 @@ properties:
>      const: 1
>      description: See ../../pwm/pwm.yaml for description of the cell formats.
>  
> +  aux-bus:

As this is a node:

type: object

> +    description:
> +      It is recommended that you place your panel under the aux-bus node
> +      here to represent the control hierarchy.
> +
>    ports:
>      $ref: /schemas/graph.yaml#/properties/ports
>  
> @@ -201,11 +206,26 @@ examples:
>  
>            port@1 {
>              reg = <1>;
> -            endpoint {
> +            sn65dsi86_out: endpoint {
>                remote-endpoint = <&panel_in_edp>;
>              };
>            };
>          };
> +
> +        aux-bus {
> +          panel {

We should perhaps have a separate aux-bus schema. Something should 
define the child node is 'panel' and nothing else. Though perhaps 
connectors are valid too?

> +            compatible = "boe,nv133fhm-n62";
> +            power-supply = <&pp3300_dx_edp>;
> +            backlight = <&backlight>;
> +            hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
> +
> +            port {
> +              panel_in_edp: endpoint {
> +                remote-endpoint = <&sn65dsi86_out>;
> +              };
> +            };
> +          };
> +        };
>        };
>      };
>    - |
> -- 
> 2.31.1.751.gd2f1c929bd-goog
> 

Re: [PATCH v7 03/10] dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child

[Doug Anderson]

Hi,

On Wed, May 19, 2021 at 1:02 PM Rob Herring <robh@kernel.org> wrote:
>
> On Mon, May 17, 2021 at 01:09:00PM -0700, Douglas Anderson wrote:
> > We want to be able to list an eDP panel as a child of a ti-sn65dsi86
> > node to represent the fact that the panel is connected to the bridge's
> > DP AUX bus. Though the panel and the bridge chip are connected in
> > several ways, the DP AUX bus is the primary control interface between
> > the two and thus makes the most sense to model in device tree
> > hierarchy.
> >
> > Listing a panel in this way makes it possible for the panel driver to
> > easily get access to the DP AUX bus that it resides on, which can be
> > useful to help in auto-detecting the panel and for turning on various
> > bits.
> >
> > NOTE: it's still possible to continue using the bridge chip and point
> > to a panel that _isn't_ listed as a child of the bridge chip (since
> > it's worked that way previously), but that should be deprecated since
> > there is no downside to listing the panel under the bridge chip.
> >
> > The idea for this bus's design was hashed out over IRC [1].
> >
> > [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2021-05-11
> >
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > Possibly we might want something fancier that could be included by
> > other eDP controller bindings. If we want to do this, I'd love to be
> > pointed at a good example to follow.
> >
> > Changes in v7:
> > - ti-sn65dsi86: Add aux-bus child patch new for v7.
> >
> >  .../bindings/display/bridge/ti,sn65dsi86.yaml | 22 ++++++++++++++++++-
> >  1 file changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > index 26932d2e86ab..51f5a29e216c 100644
> > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > @@ -70,6 +70,11 @@ properties:
> >      const: 1
> >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> >
> > +  aux-bus:
>
> As this is a node:
>
> type: object
>
> > +    description:
> > +      It is recommended that you place your panel under the aux-bus node
> > +      here to represent the control hierarchy.
> > +
> >    ports:
> >      $ref: /schemas/graph.yaml#/properties/ports
> >
> > @@ -201,11 +206,26 @@ examples:
> >
> >            port@1 {
> >              reg = <1>;
> > -            endpoint {
> > +            sn65dsi86_out: endpoint {
> >                remote-endpoint = <&panel_in_edp>;
> >              };
> >            };
> >          };
> > +
> > +        aux-bus {
> > +          panel {
>
> We should perhaps have a separate aux-bus schema.

Yeah. Before spending lots of time digging into how to do this I
wanted to see if anyone was going to give me a big-old NAK on the
whole approach. ;-)

I guess I'd make a file called "dp-aux-bus.yaml" (maybe right under
bindings/display?) and then I'd include it like this:

aux-bus:
  $ref: "../dp-aux-bus.yaml#"


> Something should
> define the child node is 'panel' and nothing else.

At the moment the code also requires that the node name is 'aux-bus'.
Any objections to that?


> Though perhaps
> connectors are valid too?

They might be. We could always add it later?

Re: [PATCH v7 00/10] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus

[Lyude Paul]

JFYI I haven't had a chance yet but I'm hoping to look at this this week

On Mon, 2021-05-17 at 13:08 -0700, Douglas Anderson wrote:
> The primary goal of this series is to try to properly fix EDID reading
> for eDP panels using the ti-sn65dsi86 bridge.
> 
> Previously we had a patch that added EDID reading but it turned out
> not to work at bootup. This caused some extra churn at bootup as we
> tried (and failed) to read the EDID several times and also ended up
> forcing us to use the hardcoded mode at boot. With this patch series I
> believe EDID reading is reliable at boot now and we never use the
> hardcoded mode.
> 
> High level note: in this series the EDID reading is driven by the
> panel driver, not by the bridge chip driver. I believe this makes a
> reasonable amount of sense since the panel driver already _could_
> drive reading the EDID if provided with the DDC bus and in future
> planned work we'll want to give the panel driver the DDC bus (to make
> decisions based on EDID) and the AUX bus (to control the
> backlight). There are also planned patches from Laurent to make
> ti-sn65dsi86 able to drive full DP monitors. In that case the bridge
> chip will still be in charge of reading the EDID, but it's not hard to
> make this dynamic.
> 
> This series is the logical successor to the 3-part series containing
> the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
> if refclk") [1].
> 
> This patch was tested against drm-misc-next commit 60a6b73dd821
> ("drm/ingenic: Fix pixclock rate for 24-bit serial panels") on a
> sc7180-trogdor-lazor device.
> 
> At v7 now, this patch series grew a bit from v6 because it introduces
> the DP AUX bus.
> 
> Between v2 and v3, high-level view of changes:
> - stop doing the EDID caching in the core.
> 
> Between v3 and v4, high-level view of changes:
> - EDID reading is actually driven by the panel driver now. See above.
> - Lots of chicken-and-egg problems solved w/ sub-devices.
> 
> Between v4 and v5, high-level view of changes.
> - Some of the early patches landed, so dropped from series.
> - New pm_runtime_disable() fix (fixed a patch that already landed).
> - Added Bjorn's tags to most patches
> - Fixed problems when building as a module.
> - Reordered debugfs patch and fixed error handling there.
> - Dropped last patch. I'm not convinced it's safe w/out more work.
> 
> Between v5 and v6, high-level view of changes:
> - Added the patch ("drm/dp: Allow an early call to register DDC i2c
>   bus")
> - Many patches had been landed, so only a few "controversial" ones
>   left.
> 
> Between v6 and v7, high-level view of changes:
> - New AUX DP bus!
> 
> [1]
> https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
> 
> Changes in v7:
> - pm_runtime_dont_use_autosuspend() fix new for v7.
> - List hpd properties bindings patch new for v7.
> - ti-sn65dsi86: Add aux-bus child patch new for v7.
> - Patch introducing the DP AUX bus is new for v7.
> - Patch to allow panel-simple to be DP AUX EP new for v7.
> - Patch using the DP AUX for DDC new for v7.
> - Remove use of now-dropped drm_dp_aux_register_ddc() call.
> - Beefed up commit message in context of the DP AUX bus.
> - Set the proper sub-device "dev" pointer in the AUX structure.
> - Patch to support for DP AUX bus on ti-sn65dsi86 new for v7.
> - Adjusted commit message to talk about DP AUX bus.
> - Panel now under bridge chip instead of getting a link to ddc-i2c
> 
> Changes in v6:
> - Use new drm_dp_aux_register_ddc() calls.
> 
> Douglas Anderson (10):
>   drm/panel: panel-simple: Add missing pm_runtime_dont_use_autosuspend()
>     calls
>   dt-bindings: display: simple: List hpd properties in panel-simple
>   dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child
>   drm: Introduce the DP AUX bus
>   drm/panel: panel-simple: Allow panel-simple be a DP AUX endpoint
>     device
>   drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC
>   drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
>   drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus
>   drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
>   arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip
> 
>  .../bindings/display/bridge/ti,sn65dsi86.yaml |  22 +-
>  .../bindings/display/panel/panel-simple.yaml  |   2 +
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  30 +-
>  drivers/gpu/drm/Kconfig                       |   5 +
>  drivers/gpu/drm/Makefile                      |   2 +
>  drivers/gpu/drm/bridge/Kconfig                |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 111 ++++--
>  drivers/gpu/drm/drm_dp_aux_bus.c              | 322 ++++++++++++++++++
>  drivers/gpu/drm/panel/Kconfig                 |   1 +
>  drivers/gpu/drm/panel/panel-simple.c          |  66 +++-
>  include/drm/drm_dp_aux_bus.h                  |  57 ++++
>  11 files changed, 563 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_dp_aux_bus.c
>  create mode 100644 include/drm/drm_dp_aux_bus.h
> 

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

Re: [PATCH v7 03/10] dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child

[Rob Herring]

On Wed, May 19, 2021 at 4:06 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, May 19, 2021 at 1:02 PM Rob Herring <robh@kernel.org> wrote:
> >
> > On Mon, May 17, 2021 at 01:09:00PM -0700, Douglas Anderson wrote:
> > > We want to be able to list an eDP panel as a child of a ti-sn65dsi86
> > > node to represent the fact that the panel is connected to the bridge's
> > > DP AUX bus. Though the panel and the bridge chip are connected in
> > > several ways, the DP AUX bus is the primary control interface between
> > > the two and thus makes the most sense to model in device tree
> > > hierarchy.
> > >
> > > Listing a panel in this way makes it possible for the panel driver to
> > > easily get access to the DP AUX bus that it resides on, which can be
> > > useful to help in auto-detecting the panel and for turning on various
> > > bits.
> > >
> > > NOTE: it's still possible to continue using the bridge chip and point
> > > to a panel that _isn't_ listed as a child of the bridge chip (since
> > > it's worked that way previously), but that should be deprecated since
> > > there is no downside to listing the panel under the bridge chip.
> > >
> > > The idea for this bus's design was hashed out over IRC [1].
> > >
> > > [1] https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&date=2021-05-11
> > >
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > Possibly we might want something fancier that could be included by
> > > other eDP controller bindings. If we want to do this, I'd love to be
> > > pointed at a good example to follow.
> > >
> > > Changes in v7:
> > > - ti-sn65dsi86: Add aux-bus child patch new for v7.
> > >
> > >  .../bindings/display/bridge/ti,sn65dsi86.yaml | 22 ++++++++++++++++++-
> > >  1 file changed, 21 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > index 26932d2e86ab..51f5a29e216c 100644
> > > --- a/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > +++ b/Documentation/devicetree/bindings/display/bridge/ti,sn65dsi86.yaml
> > > @@ -70,6 +70,11 @@ properties:
> > >      const: 1
> > >      description: See ../../pwm/pwm.yaml for description of the cell formats.
> > >
> > > +  aux-bus:
> >
> > As this is a node:
> >
> > type: object
> >
> > > +    description:
> > > +      It is recommended that you place your panel under the aux-bus node
> > > +      here to represent the control hierarchy.
> > > +
> > >    ports:
> > >      $ref: /schemas/graph.yaml#/properties/ports
> > >
> > > @@ -201,11 +206,26 @@ examples:
> > >
> > >            port@1 {
> > >              reg = <1>;
> > > -            endpoint {
> > > +            sn65dsi86_out: endpoint {
> > >                remote-endpoint = <&panel_in_edp>;
> > >              };
> > >            };
> > >          };
> > > +
> > > +        aux-bus {
> > > +          panel {
> >
> > We should perhaps have a separate aux-bus schema.
>
> Yeah. Before spending lots of time digging into how to do this I
> wanted to see if anyone was going to give me a big-old NAK on the
> whole approach. ;-)
>
> I guess I'd make a file called "dp-aux-bus.yaml" (maybe right under
> bindings/display?) and then I'd include it like this:
>
> aux-bus:
>   $ref: "../dp-aux-bus.yaml#"

Right.

> > Something should
> > define the child node is 'panel' and nothing else.
>
> At the moment the code also requires that the node name is 'aux-bus'.
> Any objections to that?

No. There's 2 ways to do that. The above does and adding $nodename in
dp-aux-bus.yaml will. The latter also means the schema will be applied
automatically to any node named 'aux-bus'. That means the schema will
be applied twice unless you have 'select: false'. The main advantage
of the latter case is it gets applied even without all the users
converted to schema.

> > Though perhaps
> > connectors are valid too?
>
> They might be. We could always add it later?

Sure.

Rob

Re: [PATCH v7 00/10] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus

[Lyude Paul]

For patches 5, and 6:

Reviewed-by: Lyude Paul <lyude@redhat.com>

This week got really busy so I wasn't able to look at the rest of them, but next
week is going to be a lot less busy so I should be able to look at them then

On Mon, 2021-05-17 at 13:08 -0700, Douglas Anderson wrote:
> The primary goal of this series is to try to properly fix EDID reading
> for eDP panels using the ti-sn65dsi86 bridge.
> 
> Previously we had a patch that added EDID reading but it turned out
> not to work at bootup. This caused some extra churn at bootup as we
> tried (and failed) to read the EDID several times and also ended up
> forcing us to use the hardcoded mode at boot. With this patch series I
> believe EDID reading is reliable at boot now and we never use the
> hardcoded mode.
> 
> High level note: in this series the EDID reading is driven by the
> panel driver, not by the bridge chip driver. I believe this makes a
> reasonable amount of sense since the panel driver already _could_
> drive reading the EDID if provided with the DDC bus and in future
> planned work we'll want to give the panel driver the DDC bus (to make
> decisions based on EDID) and the AUX bus (to control the
> backlight). There are also planned patches from Laurent to make
> ti-sn65dsi86 able to drive full DP monitors. In that case the bridge
> chip will still be in charge of reading the EDID, but it's not hard to
> make this dynamic.
> 
> This series is the logical successor to the 3-part series containing
> the patch ("drm/bridge: ti-sn65dsi86: Properly get the EDID, but only
> if refclk") [1].
> 
> This patch was tested against drm-misc-next commit 60a6b73dd821
> ("drm/ingenic: Fix pixclock rate for 24-bit serial panels") on a
> sc7180-trogdor-lazor device.
> 
> At v7 now, this patch series grew a bit from v6 because it introduces
> the DP AUX bus.
> 
> Between v2 and v3, high-level view of changes:
> - stop doing the EDID caching in the core.
> 
> Between v3 and v4, high-level view of changes:
> - EDID reading is actually driven by the panel driver now. See above.
> - Lots of chicken-and-egg problems solved w/ sub-devices.
> 
> Between v4 and v5, high-level view of changes.
> - Some of the early patches landed, so dropped from series.
> - New pm_runtime_disable() fix (fixed a patch that already landed).
> - Added Bjorn's tags to most patches
> - Fixed problems when building as a module.
> - Reordered debugfs patch and fixed error handling there.
> - Dropped last patch. I'm not convinced it's safe w/out more work.
> 
> Between v5 and v6, high-level view of changes:
> - Added the patch ("drm/dp: Allow an early call to register DDC i2c
>   bus")
> - Many patches had been landed, so only a few "controversial" ones
>   left.
> 
> Between v6 and v7, high-level view of changes:
> - New AUX DP bus!
> 
> [1]
> https://lore.kernel.org/r/20210304155144.3.I60a7fb23ce4589006bc95c64ab8d15c74b876e68@changeid/
> 
> Changes in v7:
> - pm_runtime_dont_use_autosuspend() fix new for v7.
> - List hpd properties bindings patch new for v7.
> - ti-sn65dsi86: Add aux-bus child patch new for v7.
> - Patch introducing the DP AUX bus is new for v7.
> - Patch to allow panel-simple to be DP AUX EP new for v7.
> - Patch using the DP AUX for DDC new for v7.
> - Remove use of now-dropped drm_dp_aux_register_ddc() call.
> - Beefed up commit message in context of the DP AUX bus.
> - Set the proper sub-device "dev" pointer in the AUX structure.
> - Patch to support for DP AUX bus on ti-sn65dsi86 new for v7.
> - Adjusted commit message to talk about DP AUX bus.
> - Panel now under bridge chip instead of getting a link to ddc-i2c
> 
> Changes in v6:
> - Use new drm_dp_aux_register_ddc() calls.
> 
> Douglas Anderson (10):
>   drm/panel: panel-simple: Add missing pm_runtime_dont_use_autosuspend()
>     calls
>   dt-bindings: display: simple: List hpd properties in panel-simple
>   dt-bindings: drm/bridge: ti-sn65dsi86: Add aux-bus child
>   drm: Introduce the DP AUX bus
>   drm/panel: panel-simple: Allow panel-simple be a DP AUX endpoint
>     device
>   drm/panel: panel-simple: Stash DP AUX bus; allow using it for DDC
>   drm/bridge: ti-sn65dsi86: Promote the AUX channel to its own sub-dev
>   drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus
>   drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
>   arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip
> 
>  .../bindings/display/bridge/ti,sn65dsi86.yaml |  22 +-
>  .../bindings/display/panel/panel-simple.yaml  |   2 +
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  |  30 +-
>  drivers/gpu/drm/Kconfig                       |   5 +
>  drivers/gpu/drm/Makefile                      |   2 +
>  drivers/gpu/drm/bridge/Kconfig                |   1 +
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c         | 111 ++++--
>  drivers/gpu/drm/drm_dp_aux_bus.c              | 322 ++++++++++++++++++
>  drivers/gpu/drm/panel/Kconfig                 |   1 +
>  drivers/gpu/drm/panel/panel-simple.c          |  66 +++-
>  include/drm/drm_dp_aux_bus.h                  |  57 ++++
>  11 files changed, 563 insertions(+), 56 deletions(-)
>  create mode 100644 drivers/gpu/drm/drm_dp_aux_bus.c
>  create mode 100644 include/drm/drm_dp_aux_bus.h
> 

-- 
Sincerely,
   Lyude Paul (she/her)
   Software Engineer at Red Hat
   
Note: I deal with a lot of emails and have a lot of bugs on my plate. If you've
asked me a question, are waiting for a review/merge on a patch, etc. and I
haven't responded in a while, please feel free to send me another email to check
on my status. I don't bite!

Re: [PATCH v7 02/10] dt-bindings: display: simple: List hpd properties in panel-simple

[Linus Walleij]

On Tue, May 18, 2021 at 3:58 PM Doug Anderson <dianders@chromium.org> wrote:
> On Tue, May 18, 2021 at 5:42 AM Rob Herring <robh+dt@kernel.org> wrote:
> > On Mon, May 17, 2021 at 3:09 PM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > These are described in panel-common.yaml but if I don't list them in
> > > panel-simple then I get yells when running 'dt_binding_check' in a
> > > future patch. List them along with other properties that seem to be
> > > listed in panel-simple for similar reasons.
> >
> > If you have HPD, is it still a simple panel? I don't see this as an
> > omission because the use of these properties for simple panels was
> > never documented IIRC.
>
> I would say so. It is currently supported by panel-simple in Linux. Of
> course, you could make the argument that panel-simple is no longer
> really "simple" because of things like this...

I think it should be renamed panel-panacea at this point. I think
I pointed it out before. But not my pick so I rest my case.

Yours,
Linus Walleij

Re: [PATCH v7 10/10] arm64: dts: qcom: sc7180-trogdor: Move panel under the bridge chip

[Linus Walleij]

On Mon, May 17, 2021 at 10:09 PM Douglas Anderson <dianders@chromium.org> wrote:

> Putting the panel under the bridge chip (under the aux-bus node)
> allows the panel driver to get access to the DP AUX bus, enabling all
> sorts of fabulous new features.
>
> While we're at this, get rid of a level of hierarchy for the panel
> node. It doesn't need "ports / port" and can just have a "port" child.
>
> For Linux, this patch has a hard requirement on the patches adding DP
> AUX bus support to the ti-sn65dsi86 bridge chip driver. See the patch
> ("drm/bridge: ti-sn65dsi86: Add support for the DP AUX bus").
>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

This is really looking good.
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [PATCH v7 00/10] drm: Fix EDID reading on ti-sn65dsi86 by introducing the DP AUX bus

[Doug Anderson]

Hi,

On Fri, May 21, 2021 at 4:07 PM Lyude Paul <lyude@redhat.com> wrote:
>
> For patches 5, and 6:
>
> Reviewed-by: Lyude Paul <lyude@redhat.com>
>
> This week got really busy so I wasn't able to look at the rest of them, but next
> week is going to be a lot less busy so I should be able to look at them then

Thanks for your review on the two patches and for letting me know your
plans. I know that I still need to spin the bindings patches with Rob
Herring's feedback, but I won't do that until I know you're done
reviewing just to avoid spamming everyone an extra time. Assuming no
emergency comes around and slams me, I should be able to react/respond
fairly quickly this week M-Th, though I'm taking Friday off.

BTW: if anyone reading this happens to have 10 minutes, I'd sorta like
to get patch #1 in this series landed sooner rather than later and
it's a dead-simple fix. If I see a review of that one, I'll apply it
to drm-misc before sending out the next version of the series. ;-)

-Doug