diff mbox series

drm/bridge: ps8640: Drop the ability of ps8640 to fetch the EDID

Message ID 20230612163256.1.I7b8f60b3fbfda068f9bf452d584dc934494bfbfa@changeid
State New
Headers
Series drm/bridge: ps8640: Drop the ability of ps8640 to fetch the EDID |

Commit Message

Doug Anderson June 12, 2023, 11:32 p.m. UTC 
  In order to read the EDID from an eDP panel, you not only need to
power on the bridge chip itself but also the panel. In the ps8640
driver, this was made to work by having the bridge chip manually power
the panel on by calling pre_enable() on everything connectorward on
the bridge chain. This worked OK, but...

...when trying to do the same thing on ti-sn65dsi86, feedback was that
this wasn't a great idea. As a result, we designed the "DP AUX"
bus. With the design we ended up with the panel driver itself was in
charge of reading the EDID. The panel driver could power itself on and
the bridge chip was able to power itself on because it implemented the
DP AUX bus.

Despite the fact that we came up with a new scheme, implemented in on
ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
the old code around. This was because the new scheme required a DT
change. Previously the panel was a simple "platform_device" and in DT
at the top level. With the new design the panel needs to be listed in
DT under the DP controller node. The old code allowed us to properly
fetch EDIDs with ps8640 with the old DTs.

Unfortunately, the old code stopped working as of commit 102e80d1fa2c
("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
are cases at bootup where connector->state->state is NULL and the
kernel crashed at:
* drm_atomic_bridge_chain_pre_enable
* drm_atomic_get_old_bridge_state
* drm_atomic_get_old_private_obj_state

A bit of digging was done to see if there was an easy fix but there
was nothing obvious. Instead, the only device using ps8640 the "old"
way had its DT updated so that the panel was no longer a simple
"platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
mt8173-elm: Move display to ps8640 auxiliary bus") and commit
113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
number in DT").

Let's delete the old, crashing code so nobody gets tempted to copy it
or figure out how it works (since it doesn't).

NOTE: from a device tree "purist" point of view, we're supposed to
keep old device trees working and this patch is technically "against
policy". Reasons I'm still proposing it anyway:
1. Officially, old mt8173-elm device trees worked via the "little
   white lie" approach. The DT would list an arbitrary/representative
   panel that would be used for power sequencing. The mode information
   in the panel driver would then be ignored / overridden by the EDID
   reading code in ps8640. I don't feel too terrible breaking DTs that
   contained the wrong "compatible" string to begin with. NOTE that
   any old device trees that _didn't_ lie about their compatible will
   still work because the mode information will come from the
   hardcoded panels in panel-edp.
2. The only users of the old code were Chromebooks and Chromebooks
   don't bake their DTs into the BIOS (they are bundled with the
   kernel). Thus we don't need to worry about breaking someone using
   an old DT with a new kernel.
3. The old code was crashing anyway. If someone wants to fix the old
   code instead of deleting it then they have my blessing, but without
   a proper fix the old code isn't useful.

I'll list this as "Fixing" the code that made the old code start
failing. There's not lots of reason to bring this back any further
than that.

Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/bridge/parade-ps8640.c | 79 --------------------------
 1 file changed, 79 deletions(-)

Comments

Sam Ravnborg June 13, 2023, 7:42 a.m. UTC | #1 
Hi Douglas,

On Mon, Jun 12, 2023 at 04:32:57PM -0700, Douglas Anderson wrote:
> In order to read the EDID from an eDP panel, you not only need to
> power on the bridge chip itself but also the panel. In the ps8640
> driver, this was made to work by having the bridge chip manually power
> the panel on by calling pre_enable() on everything connectorward on
> the bridge chain. This worked OK, but...
> 
> ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> this wasn't a great idea. As a result, we designed the "DP AUX"
> bus. With the design we ended up with the panel driver itself was in
> charge of reading the EDID. The panel driver could power itself on and
> the bridge chip was able to power itself on because it implemented the
> DP AUX bus.
> 
> Despite the fact that we came up with a new scheme, implemented in on
> ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> the old code around. This was because the new scheme required a DT
> change. Previously the panel was a simple "platform_device" and in DT
> at the top level. With the new design the panel needs to be listed in
> DT under the DP controller node. The old code allowed us to properly
> fetch EDIDs with ps8640 with the old DTs.
> 
> Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> are cases at bootup where connector->state->state is NULL and the
> kernel crashed at:
> * drm_atomic_bridge_chain_pre_enable
> * drm_atomic_get_old_bridge_state
> * drm_atomic_get_old_private_obj_state
> 
> A bit of digging was done to see if there was an easy fix but there
> was nothing obvious. Instead, the only device using ps8640 the "old"
> way had its DT updated so that the panel was no longer a simple
> "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> number in DT").
> 
> Let's delete the old, crashing code so nobody gets tempted to copy it
> or figure out how it works (since it doesn't).
> 
> NOTE: from a device tree "purist" point of view, we're supposed to
> keep old device trees working and this patch is technically "against
> policy". Reasons I'm still proposing it anyway:
> 1. Officially, old mt8173-elm device trees worked via the "little
>    white lie" approach. The DT would list an arbitrary/representative
>    panel that would be used for power sequencing. The mode information
>    in the panel driver would then be ignored / overridden by the EDID
>    reading code in ps8640. I don't feel too terrible breaking DTs that
>    contained the wrong "compatible" string to begin with. NOTE that
>    any old device trees that _didn't_ lie about their compatible will
>    still work because the mode information will come from the
>    hardcoded panels in panel-edp.
> 2. The only users of the old code were Chromebooks and Chromebooks
>    don't bake their DTs into the BIOS (they are bundled with the
>    kernel). Thus we don't need to worry about breaking someone using
>    an old DT with a new kernel.
> 3. The old code was crashing anyway. If someone wants to fix the old
>    code instead of deleting it then they have my blessing, but without
>    a proper fix the old code isn't useful.
> 
> I'll list this as "Fixing" the code that made the old code start
> failing. There's not lots of reason to bring this back any further
> than that.
> 
> Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thanks for fixing this!
Change looks good, and I like that it deletes stuff.

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
AngeloGioacchino Del Regno June 14, 2023, 8:22 a.m. UTC | #2 
Il 13/06/23 01:32, Douglas Anderson ha scritto:
> In order to read the EDID from an eDP panel, you not only need to
> power on the bridge chip itself but also the panel. In the ps8640
> driver, this was made to work by having the bridge chip manually power
> the panel on by calling pre_enable() on everything connectorward on
> the bridge chain. This worked OK, but...
> 
> ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> this wasn't a great idea. As a result, we designed the "DP AUX"
> bus. With the design we ended up with the panel driver itself was in
> charge of reading the EDID. The panel driver could power itself on and
> the bridge chip was able to power itself on because it implemented the
> DP AUX bus.
> 
> Despite the fact that we came up with a new scheme, implemented in on
> ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> the old code around. This was because the new scheme required a DT
> change. Previously the panel was a simple "platform_device" and in DT
> at the top level. With the new design the panel needs to be listed in
> DT under the DP controller node. The old code allowed us to properly
> fetch EDIDs with ps8640 with the old DTs.
> 
> Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> are cases at bootup where connector->state->state is NULL and the
> kernel crashed at:
> * drm_atomic_bridge_chain_pre_enable
> * drm_atomic_get_old_bridge_state
> * drm_atomic_get_old_private_obj_state
> 
> A bit of digging was done to see if there was an easy fix but there
> was nothing obvious. Instead, the only device using ps8640 the "old"
> way had its DT updated so that the panel was no longer a simple
> "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> number in DT").
> 
> Let's delete the old, crashing code so nobody gets tempted to copy it
> or figure out how it works (since it doesn't).
> 
> NOTE: from a device tree "purist" point of view, we're supposed to
> keep old device trees working and this patch is technically "against
> policy". Reasons I'm still proposing it anyway:
> 1. Officially, old mt8173-elm device trees worked via the "little
>     white lie" approach. The DT would list an arbitrary/representative
>     panel that would be used for power sequencing. The mode information
>     in the panel driver would then be ignored / overridden by the EDID
>     reading code in ps8640. I don't feel too terrible breaking DTs that
>     contained the wrong "compatible" string to begin with. NOTE that
>     any old device trees that _didn't_ lie about their compatible will
>     still work because the mode information will come from the
>     hardcoded panels in panel-edp.
> 2. The only users of the old code were Chromebooks and Chromebooks
>     don't bake their DTs into the BIOS (they are bundled with the
>     kernel). Thus we don't need to worry about breaking someone using
>     an old DT with a new kernel.
> 3. The old code was crashing anyway. If someone wants to fix the old
>     code instead of deleting it then they have my blessing, but without
>     a proper fix the old code isn't useful.
> 
> I'll list this as "Fixing" the code that made the old code start
> failing. There's not lots of reason to bring this back any further
> than that.

Hoping to see removal of non-aux EDID reading code from all drivers that can
support aux-bus is exactly why I moved Elm to the proper... aux-bus.. so...

Yes! Let's go!

> 
> Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")

...but this Fixes tag will cause this commit to be backported to kernel versions
before my commit moving Elm to aux-bus, and break display on those.

I would suggest to either find a different Fixes tag, or don't add any, since
technically this is a deprecation commit. We could imply that the old technique
is deprecated since kernel version X.Y and get away with it.

Otherwise, if you want it backported *anyway*, the safest way would be to Cc it
to stable and explicitly say which versions should it be backported to.

I really want to give my R-b tag to this one.

Cheers!
Angelo
Doug Anderson June 14, 2023, 9:31 p.m. UTC | #3 
Hi,

On Wed, Jun 14, 2023 at 1:22 AM AngeloGioacchino Del Regno
<angelogioacchino.delregno@collabora.com> wrote:
>
> Il 13/06/23 01:32, Douglas Anderson ha scritto:
> > In order to read the EDID from an eDP panel, you not only need to
> > power on the bridge chip itself but also the panel. In the ps8640
> > driver, this was made to work by having the bridge chip manually power
> > the panel on by calling pre_enable() on everything connectorward on
> > the bridge chain. This worked OK, but...
> >
> > ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> > this wasn't a great idea. As a result, we designed the "DP AUX"
> > bus. With the design we ended up with the panel driver itself was in
> > charge of reading the EDID. The panel driver could power itself on and
> > the bridge chip was able to power itself on because it implemented the
> > DP AUX bus.
> >
> > Despite the fact that we came up with a new scheme, implemented in on
> > ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> > the old code around. This was because the new scheme required a DT
> > change. Previously the panel was a simple "platform_device" and in DT
> > at the top level. With the new design the panel needs to be listed in
> > DT under the DP controller node. The old code allowed us to properly
> > fetch EDIDs with ps8640 with the old DTs.
> >
> > Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> > ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> > are cases at bootup where connector->state->state is NULL and the
> > kernel crashed at:
> > * drm_atomic_bridge_chain_pre_enable
> > * drm_atomic_get_old_bridge_state
> > * drm_atomic_get_old_private_obj_state
> >
> > A bit of digging was done to see if there was an easy fix but there
> > was nothing obvious. Instead, the only device using ps8640 the "old"
> > way had its DT updated so that the panel was no longer a simple
> > "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> > mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> > 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> > number in DT").
> >
> > Let's delete the old, crashing code so nobody gets tempted to copy it
> > or figure out how it works (since it doesn't).
> >
> > NOTE: from a device tree "purist" point of view, we're supposed to
> > keep old device trees working and this patch is technically "against
> > policy". Reasons I'm still proposing it anyway:
> > 1. Officially, old mt8173-elm device trees worked via the "little
> >     white lie" approach. The DT would list an arbitrary/representative
> >     panel that would be used for power sequencing. The mode information
> >     in the panel driver would then be ignored / overridden by the EDID
> >     reading code in ps8640. I don't feel too terrible breaking DTs that
> >     contained the wrong "compatible" string to begin with. NOTE that
> >     any old device trees that _didn't_ lie about their compatible will
> >     still work because the mode information will come from the
> >     hardcoded panels in panel-edp.
> > 2. The only users of the old code were Chromebooks and Chromebooks
> >     don't bake their DTs into the BIOS (they are bundled with the
> >     kernel). Thus we don't need to worry about breaking someone using
> >     an old DT with a new kernel.
> > 3. The old code was crashing anyway. If someone wants to fix the old
> >     code instead of deleting it then they have my blessing, but without
> >     a proper fix the old code isn't useful.
> >
> > I'll list this as "Fixing" the code that made the old code start
> > failing. There's not lots of reason to bring this back any further
> > than that.
>
> Hoping to see removal of non-aux EDID reading code from all drivers that can
> support aux-bus is exactly why I moved Elm to the proper... aux-bus.. so...
>
> Yes! Let's go!
>
> >
> > Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
>
> ...but this Fixes tag will cause this commit to be backported to kernel versions
> before my commit moving Elm to aux-bus, and break display on those.
>
> I would suggest to either find a different Fixes tag, or don't add any, since
> technically this is a deprecation commit. We could imply that the old technique
> is deprecated since kernel version X.Y and get away with it.
>
> Otherwise, if you want it backported *anyway*, the safest way would be to Cc it
> to stable and explicitly say which versions should it be backported to.

The problem is that, as I understand it, as of commit 102e80d1fa2c
("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"),
things are broken anyway and you'll get a crash at bootup. However, if
you start at that commit and apply ${SUBJECT} patch, things actually
end up being less broken. It won't crash anymore and on any boards
that actually have the display that's specified in the DT compatible
the screen should actually work. Thus even without your patch to move
things over to the aux-bus it's still an improvement to take
${SUBJECT} patch on any kernels that have that commit.

I don't have an 'elm' device easily accessible, but I can figure out
how to get one if needed to confirm that's true. However, maybe it's
easy for you or Pin-Yen to confirm.

If my understanding is incorrect, I have no objection to removing the
Fixes tag. I'd probably have to update the commit message a bunch too
because that was part of my justification for landing the patch in the
first place.
Icenowy Zheng June 15, 2023, 4:07 a.m. UTC | #4 
在 2023-06-14星期三的 14:31 -0700，Doug Anderson写道：
> Hi,
> 
> On Wed, Jun 14, 2023 at 1:22 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> > 
> > Il 13/06/23 01:32, Douglas Anderson ha scritto:
> > > In order to read the EDID from an eDP panel, you not only need to
> > > power on the bridge chip itself but also the panel. In the ps8640
> > > driver, this was made to work by having the bridge chip manually
> > > power
> > > the panel on by calling pre_enable() on everything connectorward
> > > on
> > > the bridge chain. This worked OK, but...
> > > 
> > > ...when trying to do the same thing on ti-sn65dsi86, feedback was
> > > that
> > > this wasn't a great idea. As a result, we designed the "DP AUX"
> > > bus. With the design we ended up with the panel driver itself was
> > > in
> > > charge of reading the EDID. The panel driver could power itself
> > > on and
> > > the bridge chip was able to power itself on because it
> > > implemented the
> > > DP AUX bus.
> > > 
> > > Despite the fact that we came up with a new scheme, implemented
> > > in on
> > > ti-sn65dsi86, and even implemented it on parade-ps8640, we still
> > > kept
> > > the old code around. This was because the new scheme required a
> > > DT
> > > change. Previously the panel was a simple "platform_device" and
> > > in DT
> > > at the top level. With the new design the panel needs to be
> > > listed in
> > > DT under the DP controller node. The old code allowed us to
> > > properly
> > > fetch EDIDs with ps8640 with the old DTs.
> > > 
> > > Unfortunately, the old code stopped working as of commit
> > > 102e80d1fa2c
> > > ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs").
> > > There
> > > are cases at bootup where connector->state->state is NULL and the
> > > kernel crashed at:
> > > * drm_atomic_bridge_chain_pre_enable
> > > * drm_atomic_get_old_bridge_state
> > > * drm_atomic_get_old_private_obj_state
> > > 
> > > A bit of digging was done to see if there was an easy fix but
> > > there
> > > was nothing obvious. Instead, the only device using ps8640 the
> > > "old"
> > > way had its DT updated so that the panel was no longer a simple
> > > "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> > > mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> > > 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel
> > > model
> > > number in DT").
> > > 
> > > Let's delete the old, crashing code so nobody gets tempted to
> > > copy it
> > > or figure out how it works (since it doesn't).
> > > 
> > > NOTE: from a device tree "purist" point of view, we're supposed
> > > to
> > > keep old device trees working and this patch is technically
> > > "against
> > > policy". Reasons I'm still proposing it anyway:
> > > 1. Officially, old mt8173-elm device trees worked via the "little
> > >     white lie" approach. The DT would list an
> > > arbitrary/representative
> > >     panel that would be used for power sequencing. The mode
> > > information
> > >     in the panel driver would then be ignored / overridden by the
> > > EDID
> > >     reading code in ps8640. I don't feel too terrible breaking
> > > DTs that
> > >     contained the wrong "compatible" string to begin with. NOTE
> > > that
> > >     any old device trees that _didn't_ lie about their compatible
> > > will
> > >     still work because the mode information will come from the
> > >     hardcoded panels in panel-edp.
> > > 2. The only users of the old code were Chromebooks and
> > > Chromebooks
> > >     don't bake their DTs into the BIOS (they are bundled with the
> > >     kernel). Thus we don't need to worry about breaking someone
> > > using
> > >     an old DT with a new kernel.
> > > 3. The old code was crashing anyway. If someone wants to fix the
> > > old
> > >     code instead of deleting it then they have my blessing, but
> > > without
> > >     a proper fix the old code isn't useful.
> > > 
> > > I'll list this as "Fixing" the code that made the old code start
> > > failing. There's not lots of reason to bring this back any
> > > further
> > > than that.
> > 
> > Hoping to see removal of non-aux EDID reading code from all drivers
> > that can
> > support aux-bus is exactly why I moved Elm to the proper... aux-
> > bus.. so...
> > 
> > Yes! Let's go!
> > 
> > > 
> > > Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of
> > > drm_bridge_funcs")
> > 
> > ...but this Fixes tag will cause this commit to be backported to
> > kernel versions
> > before my commit moving Elm to aux-bus, and break display on those.
> > 
> > I would suggest to either find a different Fixes tag, or don't add
> > any, since
> > technically this is a deprecation commit. We could imply that the
> > old technique
> > is deprecated since kernel version X.Y and get away with it.
> > 
> > Otherwise, if you want it backported *anyway*, the safest way would
> > be to Cc it
> > to stable and explicitly say which versions should it be backported
> > to.
> 
> The problem is that, as I understand it, as of commit 102e80d1fa2c
> ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"),
> things are broken anyway and you'll get a crash at bootup. However,
> if
> you start at that commit and apply ${SUBJECT} patch, things actually
> end up being less broken. It won't crash anymore and on any boards
> that actually have the display that's specified in the DT compatible
> the screen should actually work. Thus even without your patch to move
> things over to the aux-bus it's still an improvement to take
> ${SUBJECT} patch on any kernels that have that commit.
> 
> I don't have an 'elm' device easily accessible, but I can figure out
> how to get one if needed to confirm that's true. However, maybe it's
> easy for you or Pin-Yen to confirm.

Well I think hana also works in this situation, and elm is indeed
broken because the DT compatible is not the correct panel (I assume the
panel is used on some hana instead of elm).

> 
> If my understanding is incorrect, I have no objection to removing the
> Fixes tag. I'd probably have to update the commit message a bunch too
> because that was part of my justification for landing the patch in
> the
> first place.
Pin-yen Lin June 15, 2023, 8:46 a.m. UTC | #5 
Hi Doug,

On Thu, Jun 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Jun 14, 2023 at 1:22 AM AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com> wrote:
> >
> > Il 13/06/23 01:32, Douglas Anderson ha scritto:
> > > In order to read the EDID from an eDP panel, you not only need to
> > > power on the bridge chip itself but also the panel. In the ps8640
> > > driver, this was made to work by having the bridge chip manually power
> > > the panel on by calling pre_enable() on everything connectorward on
> > > the bridge chain. This worked OK, but...
> > >
> > > ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> > > this wasn't a great idea. As a result, we designed the "DP AUX"
> > > bus. With the design we ended up with the panel driver itself was in
> > > charge of reading the EDID. The panel driver could power itself on and
> > > the bridge chip was able to power itself on because it implemented the
> > > DP AUX bus.
> > >
> > > Despite the fact that we came up with a new scheme, implemented in on
> > > ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> > > the old code around. This was because the new scheme required a DT
> > > change. Previously the panel was a simple "platform_device" and in DT
> > > at the top level. With the new design the panel needs to be listed in
> > > DT under the DP controller node. The old code allowed us to properly
> > > fetch EDIDs with ps8640 with the old DTs.
> > >
> > > Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> > > ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> > > are cases at bootup where connector->state->state is NULL and the
> > > kernel crashed at:
> > > * drm_atomic_bridge_chain_pre_enable
> > > * drm_atomic_get_old_bridge_state
> > > * drm_atomic_get_old_private_obj_state
> > >
> > > A bit of digging was done to see if there was an easy fix but there
> > > was nothing obvious. Instead, the only device using ps8640 the "old"
> > > way had its DT updated so that the panel was no longer a simple
> > > "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> > > mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> > > 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> > > number in DT").
> > >
> > > Let's delete the old, crashing code so nobody gets tempted to copy it
> > > or figure out how it works (since it doesn't).
> > >
> > > NOTE: from a device tree "purist" point of view, we're supposed to
> > > keep old device trees working and this patch is technically "against
> > > policy". Reasons I'm still proposing it anyway:
> > > 1. Officially, old mt8173-elm device trees worked via the "little
> > >     white lie" approach. The DT would list an arbitrary/representative
> > >     panel that would be used for power sequencing. The mode information
> > >     in the panel driver would then be ignored / overridden by the EDID
> > >     reading code in ps8640. I don't feel too terrible breaking DTs that
> > >     contained the wrong "compatible" string to begin with. NOTE that
> > >     any old device trees that _didn't_ lie about their compatible will
> > >     still work because the mode information will come from the
> > >     hardcoded panels in panel-edp.
> > > 2. The only users of the old code were Chromebooks and Chromebooks
> > >     don't bake their DTs into the BIOS (they are bundled with the
> > >     kernel). Thus we don't need to worry about breaking someone using
> > >     an old DT with a new kernel.
> > > 3. The old code was crashing anyway. If someone wants to fix the old
> > >     code instead of deleting it then they have my blessing, but without
> > >     a proper fix the old code isn't useful.
> > >
> > > I'll list this as "Fixing" the code that made the old code start
> > > failing. There's not lots of reason to bring this back any further
> > > than that.
> >
> > Hoping to see removal of non-aux EDID reading code from all drivers that can
> > support aux-bus is exactly why I moved Elm to the proper... aux-bus.. so...
> >
> > Yes! Let's go!
> >
> > >
> > > Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
> >
> > ...but this Fixes tag will cause this commit to be backported to kernel versions
> > before my commit moving Elm to aux-bus, and break display on those.
> >
> > I would suggest to either find a different Fixes tag, or don't add any, since
> > technically this is a deprecation commit. We could imply that the old technique
> > is deprecated since kernel version X.Y and get away with it.
> >
> > Otherwise, if you want it backported *anyway*, the safest way would be to Cc it
> > to stable and explicitly say which versions should it be backported to.
>
> The problem is that, as I understand it, as of commit 102e80d1fa2c
> ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"),
> things are broken anyway and you'll get a crash at bootup. However, if
> you start at that commit and apply ${SUBJECT} patch, things actually
> end up being less broken. It won't crash anymore and on any boards
> that actually have the display that's specified in the DT compatible
> the screen should actually work. Thus even without your patch to move
> things over to the aux-bus it's still an improvement to take
> ${SUBJECT} patch on any kernels that have that commit.
>
> I don't have an 'elm' device easily accessible, but I can figure out
> how to get one if needed to confirm that's true. However, maybe it's
> easy for you or Pin-Yen to confirm.

The crash was there, but then commit 4fb912e5e190 ("drm/bridge:
Introduce pre_enable_prev_first to alter bridge init order") added a
NULL check on the state object in
drm_atomic_bridge_chain_pre_enable(), which prevents the kernel crash
on the latest kernel. And now the panel on "elm" Chromebook is
actually working without an "aux-bus" node seemingly because the
userspace is patient enough to keep retrying until the connector gets
its state initialized. My elm device crashes again after reverting
commit 4fb912e5e190.

However commit 4fb912e5e190 does not contain any fixes tag, so older
branches without it should be experiencing crashes without the
"aux-bus" node now.

Pin-yen

>
> If my understanding is incorrect, I have no objection to removing the
> Fixes tag. I'd probably have to update the commit message a bunch too
> because that was part of my justification for landing the patch in the
> first place.
Doug Anderson June 15, 2023, 2:30 p.m. UTC | #6 
Hi,

On Thu, Jun 15, 2023 at 1:47 AM Pin-yen Lin <treapking@chromium.org> wrote:
>
> Hi Doug,
>
> On Thu, Jun 15, 2023 at 5:31 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Jun 14, 2023 at 1:22 AM AngeloGioacchino Del Regno
> > <angelogioacchino.delregno@collabora.com> wrote:
> > >
> > > Il 13/06/23 01:32, Douglas Anderson ha scritto:
> > > > In order to read the EDID from an eDP panel, you not only need to
> > > > power on the bridge chip itself but also the panel. In the ps8640
> > > > driver, this was made to work by having the bridge chip manually power
> > > > the panel on by calling pre_enable() on everything connectorward on
> > > > the bridge chain. This worked OK, but...
> > > >
> > > > ...when trying to do the same thing on ti-sn65dsi86, feedback was that
> > > > this wasn't a great idea. As a result, we designed the "DP AUX"
> > > > bus. With the design we ended up with the panel driver itself was in
> > > > charge of reading the EDID. The panel driver could power itself on and
> > > > the bridge chip was able to power itself on because it implemented the
> > > > DP AUX bus.
> > > >
> > > > Despite the fact that we came up with a new scheme, implemented in on
> > > > ti-sn65dsi86, and even implemented it on parade-ps8640, we still kept
> > > > the old code around. This was because the new scheme required a DT
> > > > change. Previously the panel was a simple "platform_device" and in DT
> > > > at the top level. With the new design the panel needs to be listed in
> > > > DT under the DP controller node. The old code allowed us to properly
> > > > fetch EDIDs with ps8640 with the old DTs.
> > > >
> > > > Unfortunately, the old code stopped working as of commit 102e80d1fa2c
> > > > ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"). There
> > > > are cases at bootup where connector->state->state is NULL and the
> > > > kernel crashed at:
> > > > * drm_atomic_bridge_chain_pre_enable
> > > > * drm_atomic_get_old_bridge_state
> > > > * drm_atomic_get_old_private_obj_state
> > > >
> > > > A bit of digging was done to see if there was an easy fix but there
> > > > was nothing obvious. Instead, the only device using ps8640 the "old"
> > > > way had its DT updated so that the panel was no longer a simple
> > > > "platform_deice". See commit c2d94f72140a ("arm64: dts: mediatek:
> > > > mt8173-elm: Move display to ps8640 auxiliary bus") and commit
> > > > 113b5cc06f44 ("arm64: dts: mediatek: mt8173-elm: remove panel model
> > > > number in DT").
> > > >
> > > > Let's delete the old, crashing code so nobody gets tempted to copy it
> > > > or figure out how it works (since it doesn't).
> > > >
> > > > NOTE: from a device tree "purist" point of view, we're supposed to
> > > > keep old device trees working and this patch is technically "against
> > > > policy". Reasons I'm still proposing it anyway:
> > > > 1. Officially, old mt8173-elm device trees worked via the "little
> > > >     white lie" approach. The DT would list an arbitrary/representative
> > > >     panel that would be used for power sequencing. The mode information
> > > >     in the panel driver would then be ignored / overridden by the EDID
> > > >     reading code in ps8640. I don't feel too terrible breaking DTs that
> > > >     contained the wrong "compatible" string to begin with. NOTE that
> > > >     any old device trees that _didn't_ lie about their compatible will
> > > >     still work because the mode information will come from the
> > > >     hardcoded panels in panel-edp.
> > > > 2. The only users of the old code were Chromebooks and Chromebooks
> > > >     don't bake their DTs into the BIOS (they are bundled with the
> > > >     kernel). Thus we don't need to worry about breaking someone using
> > > >     an old DT with a new kernel.
> > > > 3. The old code was crashing anyway. If someone wants to fix the old
> > > >     code instead of deleting it then they have my blessing, but without
> > > >     a proper fix the old code isn't useful.
> > > >
> > > > I'll list this as "Fixing" the code that made the old code start
> > > > failing. There's not lots of reason to bring this back any further
> > > > than that.
> > >
> > > Hoping to see removal of non-aux EDID reading code from all drivers that can
> > > support aux-bus is exactly why I moved Elm to the proper... aux-bus.. so...
> > >
> > > Yes! Let's go!
> > >
> > > >
> > > > Fixes: 102e80d1fa2c ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs")
> > >
> > > ...but this Fixes tag will cause this commit to be backported to kernel versions
> > > before my commit moving Elm to aux-bus, and break display on those.
> > >
> > > I would suggest to either find a different Fixes tag, or don't add any, since
> > > technically this is a deprecation commit. We could imply that the old technique
> > > is deprecated since kernel version X.Y and get away with it.
> > >
> > > Otherwise, if you want it backported *anyway*, the safest way would be to Cc it
> > > to stable and explicitly say which versions should it be backported to.
> >
> > The problem is that, as I understand it, as of commit 102e80d1fa2c
> > ("drm/bridge: ps8640: Use atomic variants of drm_bridge_funcs"),
> > things are broken anyway and you'll get a crash at bootup. However, if
> > you start at that commit and apply ${SUBJECT} patch, things actually
> > end up being less broken. It won't crash anymore and on any boards
> > that actually have the display that's specified in the DT compatible
> > the screen should actually work. Thus even without your patch to move
> > things over to the aux-bus it's still an improvement to take
> > ${SUBJECT} patch on any kernels that have that commit.
> >
> > I don't have an 'elm' device easily accessible, but I can figure out
> > how to get one if needed to confirm that's true. However, maybe it's
> > easy for you or Pin-Yen to confirm.
>
> The crash was there, but then commit 4fb912e5e190 ("drm/bridge:
> Introduce pre_enable_prev_first to alter bridge init order") added a
> NULL check on the state object in
> drm_atomic_bridge_chain_pre_enable(), which prevents the kernel crash
> on the latest kernel. And now the panel on "elm" Chromebook is
> actually working without an "aux-bus" node seemingly because the
> userspace is patient enough to keep retrying until the connector gets
> its state initialized. My elm device crashes again after reverting
> commit 4fb912e5e190.

Oh, right! I forgot about that. Commit 4fb912e5e190 ("drm/bridge:
Introduce pre_enable_prev_first to alter bridge init order") as a side
effect caused the crash not to happen, but that also essentially made
the pre-enable a "no-op".

Hmmm, maybe I'll re-post this patch and add that extra note in and
remove the Fixes tag just to keep it from being controversial. I'll
plan to do that ~tomorrow unless there are objections.

-Doug
diff mbox series

Patch

diff --git a/drivers/gpu/drm/bridge/parade-ps8640.c b/drivers/gpu/drm/bridge/parade-ps8640.c
index 8801cdd033b5..8161b1a1a4b1 100644
--- a/drivers/gpu/drm/bridge/parade-ps8640.c
+++ b/drivers/gpu/drm/bridge/parade-ps8640.c
@@ -105,7 +105,6 @@  struct ps8640 {
 	struct gpio_desc *gpio_reset;
 	struct gpio_desc *gpio_powerdown;
 	struct device_link *link;
-	struct edid *edid;
 	bool pre_enabled;
 	bool need_post_hpd_delay;
 };
@@ -155,23 +154,6 @@  static inline struct ps8640 *aux_to_ps8640(struct drm_dp_aux *aux)
 	return container_of(aux, struct ps8640, aux);
 }
 
-static bool ps8640_of_panel_on_aux_bus(struct device *dev)
-{
-	struct device_node *bus, *panel;
-
-	bus = of_get_child_by_name(dev->of_node, "aux-bus");
-	if (!bus)
-		return false;
-
-	panel = of_get_child_by_name(bus, "panel");
-	of_node_put(bus);
-	if (!panel)
-		return false;
-	of_node_put(panel);
-
-	return true;
-}
-
 static int _ps8640_wait_hpd_asserted(struct ps8640 *ps_bridge, unsigned long wait_us)
 {
 	struct regmap *map = ps_bridge->regmap[PAGE2_TOP_CNTL];
@@ -539,50 +521,6 @@  static void ps8640_bridge_detach(struct drm_bridge *bridge)
 		device_link_del(ps_bridge->link);
 }
 
-static struct edid *ps8640_bridge_get_edid(struct drm_bridge *bridge,
-					   struct drm_connector *connector)
-{
-	struct ps8640 *ps_bridge = bridge_to_ps8640(bridge);
-	struct device *dev = &ps_bridge->page[PAGE0_DP_CNTL]->dev;
-	bool poweroff = !ps_bridge->pre_enabled;
-
-	if (!ps_bridge->edid) {
-		/*
-		 * When we end calling get_edid() triggered by an ioctl, i.e
-		 *
-		 *   drm_mode_getconnector (ioctl)
-		 *     -> drm_helper_probe_single_connector_modes
-		 *        -> drm_bridge_connector_get_modes
-		 *           -> ps8640_bridge_get_edid
-		 *
-		 * We need to make sure that what we need is enabled before
-		 * reading EDID, for this chip, we need to do a full poweron,
-		 * otherwise it will fail.
-		 */
-		if (poweroff)
-			drm_atomic_bridge_chain_pre_enable(bridge,
-							   connector->state->state);
-
-		ps_bridge->edid = drm_get_edid(connector,
-					       ps_bridge->page[PAGE0_DP_CNTL]->adapter);
-
-		/*
-		 * If we call the get_edid() function without having enabled the
-		 * chip before, return the chip to its original power state.
-		 */
-		if (poweroff)
-			drm_atomic_bridge_chain_post_disable(bridge,
-							     connector->state->state);
-	}
-
-	if (!ps_bridge->edid) {
-		dev_err(dev, "Failed to get EDID\n");
-		return NULL;
-	}
-
-	return drm_edid_duplicate(ps_bridge->edid);
-}
-
 static void ps8640_runtime_disable(void *data)
 {
 	pm_runtime_dont_use_autosuspend(data);
@@ -592,7 +530,6 @@  static void ps8640_runtime_disable(void *data)
 static const struct drm_bridge_funcs ps8640_bridge_funcs = {
 	.attach = ps8640_bridge_attach,
 	.detach = ps8640_bridge_detach,
-	.get_edid = ps8640_bridge_get_edid,
 	.atomic_post_disable = ps8640_atomic_post_disable,
 	.atomic_pre_enable = ps8640_atomic_pre_enable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
@@ -705,14 +642,6 @@  static int ps8640_probe(struct i2c_client *client)
 	ps_bridge->bridge.of_node = dev->of_node;
 	ps_bridge->bridge.type = DRM_MODE_CONNECTOR_eDP;
 
-	/*
-	 * In the device tree, if panel is listed under aux-bus of the bridge
-	 * node, panel driver should be able to retrieve EDID by itself using
-	 * aux-bus. So let's not set DRM_BRIDGE_OP_EDID here.
-	 */
-	if (!ps8640_of_panel_on_aux_bus(&client->dev))
-		ps_bridge->bridge.ops = DRM_BRIDGE_OP_EDID;
-
 	/*
 	 * Get MIPI DSI resources early. These can return -EPROBE_DEFER so
 	 * we want to get them out of the way sooner.
@@ -777,13 +706,6 @@  static int ps8640_probe(struct i2c_client *client)
 	return ret;
 }
 
-static void ps8640_remove(struct i2c_client *client)
-{
-	struct ps8640 *ps_bridge = i2c_get_clientdata(client);
-
-	kfree(ps_bridge->edid);
-}
-
 static const struct of_device_id ps8640_match[] = {
 	{ .compatible = "parade,ps8640" },
 	{ }
@@ -792,7 +714,6 @@  MODULE_DEVICE_TABLE(of, ps8640_match);
 
 static struct i2c_driver ps8640_driver = {
 	.probe = ps8640_probe,
-	.remove = ps8640_remove,
 	.driver = {
 		.name = "ps8640",
 		.of_match_table = ps8640_match,