| Message ID | 20220822180729.1.I8ac5abe3a4c1c6fd5c061686c6e883c22f69022c@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Revert "drm: bridge: analogix/dp: add panel prepare/unprepare in suspend/resume time" | expand |
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.
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
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
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
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);
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(-)