[PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[Brian Norris]

This reverts commit 211f276ed3d96e964d2d1106a198c7f4a4b3f4c0.

For quite some time, core DRM helpers already ensure that any relevant
connectors/CRTCs/etc. are disabled, as well as their associated
components (e.g., bridges) when suspending the system. Thus,
analogix_dp_bridge_{enable,disable}() already get called, which in turn
call drm_panel_{prepare,unprepare}(). This makes these drm_panel_*()
calls redundant.

Besides redundancy, there are a few problems with this handling:

(1) drm_panel_{prepare,unprepare}() are *not* reference-counted APIs and
are not in general designed to be handled by multiple callers --
although some panel drivers have a coarse 'prepared' flag that mitigates
some damage, at least. So at a minimum this is redundant and confusing,
but in some cases, this could be actively harmful.

(2) The error-handling is a bit non-standard. We ignored errors in
suspend(), but handled errors in resume(). And recently, people noticed
that the clk handling is unbalanced in error paths, and getting *that*
right is not actually trivial, given the current way errors are mostly
ignored.

(3) In the particular way analogix_dp_{suspend,resume}() get used (e.g.,
in rockchip_dp_*(), as a late/early callback), we don't necessarily have
a proper PM relationship between the DP/bridge device and the panel
device. So while the DP bridge gets resumed, the panel's parent device
(e.g., platform_device) may still be suspended, and so any prepare()
calls may fail.

So remove the superfluous, possibly-harmful suspend()/resume() handling
of panel state.

Fixes: 211f276ed3d9 ("drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time")
Link: https://lore.kernel.org/all/Yv2CPBD3Picg%2FgVe@google.com/
Signed-off-by: Brian Norris <briannorris@chromium.org>
---

 drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 8aadcc0aa90b..df9370e0ff23 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1864,12 +1864,6 @@ EXPORT_SYMBOL_GPL(analogix_dp_remove);
 int analogix_dp_suspend(struct analogix_dp_device *dp)
 {
 	clk_disable_unprepare(dp->clock);
-
-	if (dp->plat_data->panel) {
-		if (drm_panel_unprepare(dp->plat_data->panel))
-			DRM_ERROR("failed to turnoff the panel\n");
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_suspend);
@@ -1884,13 +1878,6 @@ int analogix_dp_resume(struct analogix_dp_device *dp)
 		return ret;
 	}
 
-	if (dp->plat_data->panel) {
-		if (drm_panel_prepare(dp->plat_data->panel)) {
-			DRM_ERROR("failed to setup the panel\n");
-			return -EBUSY;
-		}
-	}
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(analogix_dp_resume);
-- 
2.37.2.609.g9ff673ca1a-goog

Re: [PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[Doug Anderson]

Hi,

On Mon, Aug 22, 2022 at 6:08 PM Brian Norris <briannorris@chromium.org> wrote:
>
> This reverts commit 211f276ed3d96e964d2d1106a198c7f4a4b3f4c0.
>
> For quite some time, core DRM helpers already ensure that any relevant
> connectors/CRTCs/etc. are disabled, as well as their associated
> components (e.g., bridges) when suspending the system. Thus,
> analogix_dp_bridge_{enable,disable}() already get called, which in turn
> call drm_panel_{prepare,unprepare}(). This makes these drm_panel_*()
> calls redundant.
>
> Besides redundancy, there are a few problems with this handling:
>
> (1) drm_panel_{prepare,unprepare}() are *not* reference-counted APIs and
> are not in general designed to be handled by multiple callers --
> although some panel drivers have a coarse 'prepared' flag that mitigates
> some damage, at least. So at a minimum this is redundant and confusing,
> but in some cases, this could be actively harmful.
>
> (2) The error-handling is a bit non-standard. We ignored errors in
> suspend(), but handled errors in resume(). And recently, people noticed
> that the clk handling is unbalanced in error paths, and getting *that*
> right is not actually trivial, given the current way errors are mostly
> ignored.
>
> (3) In the particular way analogix_dp_{suspend,resume}() get used (e.g.,
> in rockchip_dp_*(), as a late/early callback), we don't necessarily have
> a proper PM relationship between the DP/bridge device and the panel
> device. So while the DP bridge gets resumed, the panel's parent device
> (e.g., platform_device) may still be suspended, and so any prepare()
> calls may fail.
>
> So remove the superfluous, possibly-harmful suspend()/resume() handling
> of panel state.
>
> Fixes: 211f276ed3d9 ("drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time")
> Link: https://lore.kernel.org/all/Yv2CPBD3Picg%2FgVe@google.com/
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
>
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 13 -------------
>  1 file changed, 13 deletions(-)

I thought it was strange that the patch being reverted had my
Tested-by, so I tracked that down at least. Looks like that's from:

https://lore.kernel.org/lkml/CAD=FV=XXESzA6n6MNEGH1Kbh7CeL8xWX8CifFLVf91+0JyFcJQ@mail.gmail.com/

...where I tested the whole stack of 17 patches together. That means
that my Tested-by was legit but it wasn't as if I tested that one
patch and confirmed that it was useful / needed.

Your argument here sounds right to me. The panel should get prepared
through the normal means (analogix_dp_bridge_pre_enable()) and
unprepared through normal means (analogix_dp_bridge_disable()). ...and
whenever the Analogix gets moved to "panel bridge" then that will move
to just being part of the bridge chain. Having these calls directly in
the suspend/resume seems weird/wrong.

So while I'm not an expert enough in the quirks of the Analogix DP
driver to say for certain that your revert won't cause any problems at
all, if problems come up they should probably be fixed in a way that
doesn't involve re-adding these direct calls to the suspend/resume
callbacks. Thus:

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

Given that this is _not_ an area that I'm an expert in nor is it an
area where the consequences are super easy to analyze, I'm a little
hesitant to apply this to drm-misc-next myself. Ideally someone more
familiar with the driver would do it. However, if nobody steps up
after a few weeks and nobody has yelled about this patch, I'll apply
it.

Re: [PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[Brian Norris]

Hi,

On Thu, Aug 25, 2022 at 10:37 AM Doug Anderson <dianders@chromium.org> wrote:
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks :)

> Given that this is _not_ an area that I'm an expert in nor is it an
> area where the consequences are super easy to analyze, I'm a little
> hesitant to apply this to drm-misc-next myself. Ideally someone more
> familiar with the driver would do it. However, if nobody steps up
> after a few weeks and nobody has yelled about this patch, I'll apply
> it.

I'll take this opportunity to correct Andrzej's email address (my bad;
I need to use the up-to-date MAINTAINERS / .mailmap when generating CC
lists), in case he's one such expert.

Brian

Re: [PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[Brian Norris]

On Thu, Aug 25, 2022 at 11:06 AM Brian Norris <briannorris@chromium.org> wrote:
> On Thu, Aug 25, 2022 at 10:37 AM Doug Anderson <dianders@chromium.org> wrote:
> > Given that this is _not_ an area that I'm an expert in nor is it an
> > area where the consequences are super easy to analyze, I'm a little
> > hesitant to apply this to drm-misc-next myself. Ideally someone more
> > familiar with the driver would do it. However, if nobody steps up
> > after a few weeks and nobody has yelled about this patch, I'll apply
> > it.

For this particular patch, I'd be interested in whether Zhang Zekun
has any feedback (even a Tested-by?), since they were patching this
function in the first place, which is why I paid attention:

Subject: [PATCH -next] drm/bridge: Add missing clk_disable_unprepare()
in analogix_dp_resume()
https://lore.kernel.org/lkml/20220816064231.60473-1-zhangzekun11@huawei.com/

But in absence of that...it has now been a few weeks :)

I'll also mark this to come back to again in a week or two, in case
somebody is still hoping to wait longer.

Regards,
Brian

Re: [PATCH] Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time"

[Doug Anderson]

Hi,

On Mon, Sep 12, 2022 at 11:48 AM Brian Norris <briannorris@chromium.org> wrote:
>
> On Thu, Aug 25, 2022 at 11:06 AM Brian Norris <briannorris@chromium.org> wrote:
> > On Thu, Aug 25, 2022 at 10:37 AM Doug Anderson <dianders@chromium.org> wrote:
> > > Given that this is _not_ an area that I'm an expert in nor is it an
> > > area where the consequences are super easy to analyze, I'm a little
> > > hesitant to apply this to drm-misc-next myself. Ideally someone more
> > > familiar with the driver would do it. However, if nobody steps up
> > > after a few weeks and nobody has yelled about this patch, I'll apply
> > > it.
>
> For this particular patch, I'd be interested in whether Zhang Zekun
> has any feedback (even a Tested-by?), since they were patching this
> function in the first place, which is why I paid attention:
>
> Subject: [PATCH -next] drm/bridge: Add missing clk_disable_unprepare()
> in analogix_dp_resume()
> https://lore.kernel.org/lkml/20220816064231.60473-1-zhangzekun11@huawei.com/
>
> But in absence of that...it has now been a few weeks :)
>
> I'll also mark this to come back to again in a week or two, in case
> somebody is still hoping to wait longer.

At this point people have had plenty of time and plenty of warnings to
speak up if they cared. Since this is a fix, I threw this in
drm-misc-fixes.

cc62d98bd56d Revert "drm: bridge: analogix/dp: add panel
prepare/unprepare in suspend/resume time"

If anyone comes along with a late objection, we can always revert the revert.

-Doug