[PATCH 0/2] Drop EDID cache for it6505/anx7625 when the bridge is powered off

[PATCH 0/2] Drop EDID cache for it6505/anx7625 when the bridge is powered off

[Pin-yen Lin]

This mainly fixes the use case when the user changes the external monitor
when the system is suspended. Without this series, both of the bridges
will skip the EDID read and returned the cached one after resume.

Apart from that, we also observed a DP-to-HDMI bridge expects an EDID read
after it's powered on. This patch also works around the problem by always
triggering the EDID read after the system resume.


Pin-yen Lin (2):
  drm/bridge: anx7625: Drop EDID cache on bridge power off
  drm/bridge: it6505: Drop EDID cache on bridge power off

 drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
 drivers/gpu/drm/bridge/ite-it6505.c       | 2 ++
 2 files changed, 3 insertions(+)

-- 
2.46.0.792.g87dc391469-goog

[PATCH 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off

[Pin-yen Lin]

The bridge might miss the display change events when it's powered off.
This happens when a user changes the external monitor when the system
is suspended and the embedded controller doesn't not wake AP up.

It's also observed that one DP-to-HDMI bridge doesn't work correctly
when there is no EDID read after it is powered on.

Drop the cache to force an EDID read after system resume to fix this.

Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>
---

 drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/bridge/analogix/anx7625.c b/drivers/gpu/drm/bridge/analogix/anx7625.c
index 88e4aa5830f3..b4c1f00f1c3d 100644
--- a/drivers/gpu/drm/bridge/analogix/anx7625.c
+++ b/drivers/gpu/drm/bridge/analogix/anx7625.c
@@ -2561,6 +2561,7 @@ static int __maybe_unused anx7625_runtime_pm_suspend(struct device *dev)
 	mutex_lock(&ctx->lock);
 
 	anx7625_stop_dp_work(ctx);
+	anx7625_remove_edid(ctx);
 	anx7625_power_standby(ctx);
 
 	mutex_unlock(&ctx->lock);
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 11:51:46AM GMT, Pin-yen Lin wrote:
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
> 
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
> 
> Drop the cache to force an EDID read after system resume to fix this.
> 
> Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
> 
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
>  1 file changed, 1 insertion(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
-- 
With best wishes
Dmitry

Re: [PATCH 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off

[Doug Anderson]

Hi,

On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
>  1 file changed, 1 insertion(+)

I'm not totally sure if it matters, but I wonder if you should change
this to only delete the EDID cache if you're in DP mode and not eDP
mode? For eDP mode the panel is not allowed to change and re-reading
it needlessly seems like it would slow down things like
suspend/resume. I think this would only matter if someone were using
eDP panels in the "old" way (not under the aux-bus) because we don't
set the "DRM_BRIDGE_OP_EDID" when we see "aux-bus", so maybe we don't
care that much but still...

Other than that, I know that there have been discussions in the past
about EDID caches but I can't quite remember all the details. I know
that panel-edp.c still caches it, so we must have concluded that it's
at least fine/reasonable for panels. I don't remember whether caching
is encouraged / suggested for external displays, though. Do you happen
to know if it even makes a difference there (in other words, do you
actually see multiple calls to read the EDID when you plug in a DP
display)?

-Doug

Re: [PATCH 1/2] drm/bridge: anx7625: Drop EDID cache on bridge power off

[Pin-yen Lin]

Hi,

On Wed, Sep 25, 2024 at 12:43 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin <treapking@chromium.org> wrote:
> >
> > The bridge might miss the display change events when it's powered off.
> > This happens when a user changes the external monitor when the system
> > is suspended and the embedded controller doesn't not wake AP up.
> >
> > It's also observed that one DP-to-HDMI bridge doesn't work correctly
> > when there is no EDID read after it is powered on.
> >
> > Drop the cache to force an EDID read after system resume to fix this.
> >
> > Fixes: 8bdfc5dae4e3 ("drm/bridge: anx7625: Add anx7625 MIPI DSI/DPI to DP")
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> > ---
> >
> >  drivers/gpu/drm/bridge/analogix/anx7625.c | 1 +
> >  1 file changed, 1 insertion(+)
>
> I'm not totally sure if it matters, but I wonder if you should change
> this to only delete the EDID cache if you're in DP mode and not eDP
> mode? For eDP mode the panel is not allowed to change and re-reading
> it needlessly seems like it would slow down things like
> suspend/resume. I think this would only matter if someone were using
> eDP panels in the "old" way (not under the aux-bus) because we don't
> set the "DRM_BRIDGE_OP_EDID" when we see "aux-bus", so maybe we don't
> care that much but still...

I'll update this in v2.
>
> Other than that, I know that there have been discussions in the past
> about EDID caches but I can't quite remember all the details. I know
> that panel-edp.c still caches it, so we must have concluded that it's
> at least fine/reasonable for panels. I don't remember whether caching
> is encouraged / suggested for external displays, though. Do you happen
> to know if it even makes a difference there (in other words, do you
> actually see multiple calls to read the EDID when you plug in a DP
> display)?

At least on ChromeOS, Chrome triggers two EDID reads when I plug in an
external monitor. I'm not sure if this is common for other DRM masters
though.
>
> -Doug

Regards,
Pin-yen

[PATCH 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off

[Pin-yen Lin]

The bridge might miss the display change events when it's powered off.
This happens when a user changes the external monitor when the system
is suspended and the embedded controller doesn't not wake AP up.

It's also observed that one DP-to-HDMI bridge doesn't work correctly
when there is no EDID read after it is powered on.

Drop the cache to force an EDID read after system resume to fix this.

Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
Signed-off-by: Pin-yen Lin <treapking@chromium.org>

---

 drivers/gpu/drm/bridge/ite-it6505.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ite-it6505.c b/drivers/gpu/drm/bridge/ite-it6505.c
index 1e1c06fdf206..bb449efac2f4 100644
--- a/drivers/gpu/drm/bridge/ite-it6505.c
+++ b/drivers/gpu/drm/bridge/ite-it6505.c
@@ -3101,6 +3101,8 @@ static __maybe_unused int it6505_bridge_suspend(struct device *dev)
 {
 	struct it6505 *it6505 = dev_get_drvdata(dev);
 
+	it6505_remove_edid(it6505);
+
 	return it6505_poweroff(it6505);
 }
 
-- 
2.46.0.792.g87dc391469-goog

Re: [PATCH 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off

[Dmitry Baryshkov]

On Tue, Sep 24, 2024 at 11:51:47AM GMT, Pin-yen Lin wrote:
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
> 
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
> 
> Drop the cache to force an EDID read after system resume to fix this.
> 
> Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>
> 
> ---
> 
>  drivers/gpu/drm/bridge/ite-it6505.c | 2 ++
>  1 file changed, 2 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off

[Doug Anderson]

Hi,

On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin <treapking@chromium.org> wrote:
>
> The bridge might miss the display change events when it's powered off.
> This happens when a user changes the external monitor when the system
> is suspended and the embedded controller doesn't not wake AP up.
>
> It's also observed that one DP-to-HDMI bridge doesn't work correctly
> when there is no EDID read after it is powered on.
>
> Drop the cache to force an EDID read after system resume to fix this.
>
> Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
> Signed-off-by: Pin-yen Lin <treapking@chromium.org>

Ah, I guess this answers my question in the previous patch about
whether caching was important even for external displays since this
driver only supports external DP and the commit you mention in "Fixes"
says that caching was important.

So this looks reasonable. One thing I wonder is if you're totally
guaranteed to get a PM Runtime suspend whenever you get an unplug /
replug of a display. I tried to dig a little bit but I'm not super
familiar with this bridge and it looks complicated enough that I guess
I'll have to trust that it's fine. So...

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH 2/2] drm/bridge: it6505: Drop EDID cache on bridge power off

[Pin-yen Lin]

Hi,

On Wed, Sep 25, 2024 at 1:48 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 23, 2024 at 8:53 PM Pin-yen Lin <treapking@chromium.org> wrote:
> >
> > The bridge might miss the display change events when it's powered off.
> > This happens when a user changes the external monitor when the system
> > is suspended and the embedded controller doesn't not wake AP up.
> >
> > It's also observed that one DP-to-HDMI bridge doesn't work correctly
> > when there is no EDID read after it is powered on.
> >
> > Drop the cache to force an EDID read after system resume to fix this.
> >
> > Fixes: 11feaef69d0c ("drm/bridge: it6505: Add caching for EDID")
> > Signed-off-by: Pin-yen Lin <treapking@chromium.org>
>
> Ah, I guess this answers my question in the previous patch about
> whether caching was important even for external displays since this
> driver only supports external DP and the commit you mention in "Fixes"
> says that caching was important.
>
> So this looks reasonable. One thing I wonder is if you're totally
> guaranteed to get a PM Runtime suspend whenever you get an unplug /
> replug of a display. I tried to dig a little bit but I'm not super
> familiar with this bridge and it looks complicated enough that I guess
> I'll have to trust that it's fine. So...

it6505_remove_edid() is also called when the bridge reads HPD low in
the IRQ handler or the DPCD sink count changes to 0 after a HPD_IRQ
signal. The assumption here is that if the bridge is still powered on,
then it should be aware of the monitor change events and drop the EDID
cache when needed. This patch addresses the scenario where monitor
changes are not communicated to the bridge when it is powered off.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Regards,
Pin-yen