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@xxxxxxxxxxxx> 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@xxxxxxxxxx/
> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
> ---
>
>  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@xxxxxxxxxxxxxx/

...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@xxxxxxxxxxxx>

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.