kevin/linux
1
Fork 0
Code

drm/panel: Avoid warnings w/ panel-simple/panel-edp at shutdown

Browse Source 
At shutdown if you've got a _properly_ coded DRM modeset driver then
you'll get these two warnings at shutdown time:

  Skipping disable of already disabled panel
  Skipping unprepare of already unprepared panel

These warnings are ugly and sound concerning, but they're actually a
sign of a properly working system. That's not great.

We're not ready to get rid of the calls to drm_panel_disable() and
drm_panel_unprepare() because we're not 100% convinced that all DRM
modeset drivers are properly calling drm_atomic_helper_shutdown() or
drm_helper_force_disable_all() at the right times. However, having the
warning show up for correctly working systems is bad.

As a bit of a workaround, add some "if" tests to try to avoid the
warning on correctly working systems. Also add some comments and
update the TODO items in the hopes that future developers won't be too
confused by what's going on here.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Acked-by: Neil Armstrong <neil.armstrong@linaro.org>
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
Link: https://patchwork.freedesktop.org/patch/msgid/20240621134427.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid
This commit is contained in:
Douglas Anderson 2024-06-21 13:44:29 -07:00
parent 1f7ce4316d
commit f00bfaca70
4 changed files with 66 additions and 35 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

31
Documentation/gpu/todo.rst
View File

@ -475,25 +475,22 @@ Remove disable/unprepare in remove/shutdown in panel-simple and panel-edp
As of commit d2aacaf07395 ("drm/panel: Check for already prepared/enabled in
drm_panel"), we have a check in the drm_panel core to make sure nobody
double-calls prepare/enable/disable/unprepare. Eventually that should probably
be turned into a WARN_ON() or somehow made louder, but right now we actually
expect it to trigger and so we don't want it to be too loud.
be turned into a WARN_ON() or somehow made louder.
Specifically, that warning will trigger for panel-edp and panel-simple at
shutdown time because those panels hardcode a call to drm_panel_disable()
and drm_panel_unprepare() at shutdown and remove time that they call regardless
of panel state. On systems with a properly coded DRM modeset driver that
calls drm_atomic_helper_shutdown() this is pretty much guaranteed to cause
the warning to fire.
At the moment, we expect that we may still encounter the warnings in the
drm_panel core when using panel-simple and panel-edp. Since those panel
drivers are used with a lot of different DRM modeset drivers they still
make an extra effort to disable/unprepare the panel themsevles at shutdown
time. Specifically we could still encounter those warnings if the panel
driver gets shutdown() _before_ the DRM modeset driver and the DRM modeset
driver properly calls drm_atomic_helper_shutdown() in its own shutdown()
callback. Warnings could be avoided in such a case by using something like
device links to ensure that the panel gets shutdown() after the DRM modeset
driver.
Unfortunately we can't safely remove the calls in panel-edp and panel-simple
until we're sure that all DRM modeset drivers that are used with those panels
properly call drm_atomic_helper_shutdown(). This TODO item is to validate
that all DRM modeset drivers used with panel-edp and panel-simple properly
call drm_atomic_helper_shutdown() and then remove the calls to
disable/unprepare from those panels. Alternatively, this TODO item could be
removed by convincing stakeholders that those calls are fine and downgrading
the error message in drm_panel_disable() / drm_panel_unprepare() to a
debug-level message.
Once all DRM modeset drivers are known to shutdown properly, the extra
calls to disable/unprepare in remove/shutdown in panel-simple and panel-edp
should be removed and this TODO item marked complete.
Contact: Douglas Anderson <dianders@chromium.org>

18
drivers/gpu/drm/drm_panel.c
View File

@ -161,6 +161,15 @@ int drm_panel_unprepare(struct drm_panel *panel)
if (!panel)
return -EINVAL;
/*
* If you are seeing the warning below it likely means one of two things:
* - Your panel driver incorrectly calls drm_panel_unprepare() in its
* shutdown routine. You should delete this.
* - You are using panel-edp or panel-simple and your DRM modeset
* driver's shutdown() callback happened after the panel's shutdown().
* In this case the warning is harmless though ideally you should
* figure out how to reverse the order of the shutdown() callbacks.
*/
if (!panel->prepared) {
dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
return 0;
@ -245,6 +254,15 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
/*
* If you are seeing the warning below it likely means one of two things:
* - Your panel driver incorrectly calls drm_panel_disable() in its
* shutdown routine. You should delete this.
* - You are using panel-edp or panel-simple and your DRM modeset
* driver's shutdown() callback happened after the panel's shutdown().
* In this case the warning is harmless though ideally you should
* figure out how to reverse the order of the shutdown() callbacks.
*/
if (!panel->enabled) {
dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
return 0;

26
drivers/gpu/drm/panel/panel-edp.c
View File

@ -954,16 +954,24 @@ static void panel_edp_shutdown(struct device *dev)
* drm_atomic_helper_shutdown() at shutdown time and that should
* cause the panel to be disabled / unprepared if needed. For now,
* however, we'll keep these calls due to the sheer number of
* different DRM modeset drivers used with panel-edp. The fact that
* we're calling these and _also_ the drm_atomic_helper_shutdown()
* will try to disable/unprepare means that we can get a warning about
* trying to disable/unprepare an already disabled/unprepared panel,
* but that's something we'll have to live with until we've confirmed
* that all DRM modeset drivers are properly calling
* drm_atomic_helper_shutdown().
* different DRM modeset drivers used with panel-edp. Once we've
* confirmed that all DRM modeset drivers using this panel properly
* call drm_atomic_helper_shutdown() we can simply delete the two
* calls below.
*
* TO BE EXPLICIT: THE CALLS BELOW SHOULDN'T BE COPIED TO ANY NEW
* PANEL DRIVERS.
*
* FIXME: If we're still haven't figured out if all DRM modeset
* drivers properly call drm_atomic_helper_shutdown() but we _have_
* managed to make sure that DRM modeset drivers get their shutdown()
* callback before the panel's shutdown() callback (perhaps using
* device link), we could add a WARN_ON here to help move forward.
*/
drm_panel_disable(&panel->base);
drm_panel_unprepare(&panel->base);
if (panel->base.enabled)
drm_panel_disable(&panel->base);
if (panel->base.prepared)
drm_panel_unprepare(&panel->base);
}
static void panel_edp_remove(struct device *dev)

26
drivers/gpu/drm/panel/panel-simple.c
View File

@ -726,16 +726,24 @@ static void panel_simple_shutdown(struct device *dev)
* drm_atomic_helper_shutdown() at shutdown time and that should
* cause the panel to be disabled / unprepared if needed. For now,
* however, we'll keep these calls due to the sheer number of
* different DRM modeset drivers used with panel-simple. The fact that
* we're calling these and _also_ the drm_atomic_helper_shutdown()
* will try to disable/unprepare means that we can get a warning about
* trying to disable/unprepare an already disabled/unprepared panel,
* but that's something we'll have to live with until we've confirmed
* that all DRM modeset drivers are properly calling
* drm_atomic_helper_shutdown().
* different DRM modeset drivers used with panel-simple. Once we've
* confirmed that all DRM modeset drivers using this panel properly
* call drm_atomic_helper_shutdown() we can simply delete the two
* calls below.
*
* TO BE EXPLICIT: THE CALLS BELOW SHOULDN'T BE COPIED TO ANY NEW
* PANEL DRIVERS.
*
* FIXME: If we're still haven't figured out if all DRM modeset
* drivers properly call drm_atomic_helper_shutdown() but we _have_
* managed to make sure that DRM modeset drivers get their shutdown()
* callback before the panel's shutdown() callback (perhaps using
* device link), we could add a WARN_ON here to help move forward.
*/
drm_panel_disable(&panel->base);
drm_panel_unprepare(&panel->base);
if (panel->base.enabled)
drm_panel_disable(&panel->base);
if (panel->base.prepared)
drm_panel_unprepare(&panel->base);
}
static void panel_simple_remove(struct device *dev)