Re: [PATCH AUTOSEL 5.14 001/252] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC

From: Doug Anderson <dianders@chromium.org>
To: Sasha Levin <sashal@kernel.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	"# 4.0+" <stable@vger.kernel.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	dri-devel <dri-devel@lists.freedesktop.org>
Subject: Re: [PATCH AUTOSEL 5.14 001/252] drm/bridge: ti-sn65dsi86: Don't read EDID blob over DDC
Date: Thu, 9 Sep 2021 06:44:21 -0700	[thread overview]
Message-ID: <CAD=FV=U2dGjeEzp+K1vnLTj8oPJ-GKBTTKz2XQ1OZ7QF_sTHuw@mail.gmail.com> (raw)
In-Reply-To: <20210909014623.128976-1-sashal@kernel.org>

Hi,

On Wed, Sep 8, 2021 at 6:46 PM Sasha Levin <sashal@kernel.org> wrote:
>
> From: Douglas Anderson <dianders@chromium.org>
>
> [ Upstream commit a70e558c151043ce46a5e5999f4310e0b3551f57 ]
>
> This is really just a revert of commit 58074b08c04a ("drm/bridge:
> ti-sn65dsi86: Read EDID blob over DDC"), resolving conflicts.
>
> The old code failed to read the EDID properly in a very important
> case: before the bridge's pre_enable() was called. The way things need
> to work:
> 1. Read the EDID.
> 2. Based on the EDID, decide on video settings and pixel clock.
> 3. Enable the bridge w/ the desired settings.
>
> The way things were working:
> 1. Try to read the EDID but fail; fall back to hardcoded values.
> 2. Based on hardcoded values, decide on video settings and pixel clock.
> 3. Enable the bridge w/ the desired settings.
> 4. Try again to read the EDID, it works now!
> 5. Realize that the hardcoded settings weren't quite right.
> 6. Disable / reenable the bridge w/ the right settings.
>
> The reasons for the failures were twofold:
> a) Since we never ran the bridge chip's pre-enable then we never set
>    the bit to ignore HPD. This meant the bridge chip didn't even _try_
>    to go out on the bus and communicate with the panel.
> b) Even if we fixed things to ignore HPD, the EDID still wouldn't read
>    if the panel wasn't on.
>
> Instead of reverting the code, we could fix it to set the HPD bit and
> also power on the panel. However, it also works nicely to just let the
> panel code read the EDID. Now that we've split the driver up we can
> expose the DDC AUX channel bus to the panel node. The panel can take
> charge of reading the EDID.
>
> NOTE: in order for things to work, anyone that needs to read the EDID
> will need to instantiate their panel using the new DP AUX bus (AKA by
> listing their panel under the "aux-bus" node of the bridge chip in the
> device tree).
>
> In the future if we want to use the bridge chip to provide a full
> external DP port (which won't have a panel) then we will have to
> conditinally add EDID reading back in.
>
> Suggested-by: Andrzej Hajda <a.hajda@samsung.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Link: https://patchwork.freedesktop.org/patch/msgid/20210611101711.v10.9.I9330684c25f65bb318eff57f0616500f83eac3cc@changeid
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  drivers/gpu/drm/bridge/ti-sn65dsi86.c | 22 ----------------------
>  1 file changed, 22 deletions(-)

I would suggest against backporting this one unless you're going to
backport the whole pile of DP AUX bus patches, which probably doesn't
make sense for stable. Even though the old EDID reading was broken for
the first read, it still worked for later reads. ...and the first read
didn't crash or anything--it just timed out.

-Doug