Message-ID: <86ad3ffb-fbe2-9bed-751d-684994b71e9d@collabora.com>
Date:   Wed, 14 Jun 2023 10:22:01 +0200
From:   AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>
To:     Douglas Anderson <dianders@...omium.org>,
        dri-devel@...ts.freedesktop.org
Cc:     Maxime Ripard <mripard@...nel.org>,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Pin-yen Lin <treapking@...omium.org>,
        Sam Ravnborg <sam@...nborg.org>,
        Icenowy Zheng <uwu@...nowy.me>,
        linux-mediatek@...ts.infradead.org,
        Andrzej Hajda <andrzej.hajda@...el.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Dave Stevenson <dave.stevenson@...pberrypi.com>,
        David Airlie <airlied@...il.com>,
        Jernej Skrabec <jernej.skrabec@...il.com>,
        Jonas Karlman <jonas@...boo.se>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Neil Armstrong <neil.armstrong@...aro.org>,
        Robert Foss <rfoss@...nel.org>,
        linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/bridge: ps8640: Drop the ability of ps8640 to fetch
 the EDID

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

Powered by blists - more mailing lists