Re: [PATCH] drm/bridge: ps8640: Drop the ability of ps8640 to fetch the EDID

[Doug Anderson <dianders@chromium.org>]

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