Hi, On Thu, Jun 15, 2023 at 1:47 AM Pin-yen Lin <treapking@xxxxxxxxxxxx> wrote: > > Hi Doug, > > On Thu, Jun 15, 2023 at 5:31 AM Doug Anderson <dianders@xxxxxxxxxxxx> wrote: > > > > Hi, > > > > On Wed, Jun 14, 2023 at 1:22 AM AngeloGioacchino Del Regno > > <angelogioacchino.delregno@xxxxxxxxxxxxx> 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