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.
It's not easy to get rid of these warnings. Until we know that all DRM
modeset drivers used with panel-simple and panel-edp are properly
calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
then the panel drivers _need_ to disable/unprepare themselves in order
to power off the panel cleanly. However, there are lots of DRM modeset
drivers used with panel-edp and panel-simple and it's hard to know
when we've got them all. Since the warning happens only on the drivers
that _are_ updated there's nothing to encourage broken DRM modeset
drivers to get fixed.
In order to flip the warning to the proper place, we need to know
which modeset drivers are going to shutdown properly. Though ugly, do
this by creating a list of everyone that shuts down properly. This
allows us to generate a warning for the correct case and also lets us
get rid of the warning for drivers that are shutting down properly.
Maintaining this list is ugly, but the idea is that it's only short
term. Once everyone is converted we can delete the list and call it
done. The list is ugly enough and adding to it is annoying enough that
people should push to make this happen.
Implement this all in a shared "header" file included by the two panel
drivers that need it. This avoids us adding an new exports while still
allowing the panel drivers to be modules. The code waste should be
small and, as per above, the whole solution is temporary.
Signed-off-by: Douglas Anderson <[email protected]>
---
I came up with this idea to help us move forward since otherwise I
couldn't see how we were ever going to fix panel-simple and panel-edp
since they're used by so many DRM Modeset drivers. It's a bit ugly but
I don't hate it. What do others think?
This is at the end of the series so even if folks hate it we could
still land the rest of the series.
This was a "bonus" extra patch I added at the end of v3 of the series
("drm/panel: Remove most store/double-check of prepared/enabled
state") [1]. There, I had the note: "I came up with this idea to help
us move forward since otherwise I couldn't see how we were ever going
to fix panel-simple and panel-edp since they're used by so many DRM
Modeset drivers. It's a bit ugly but I don't hate it. What do others
think?"
As requested by Neil, now that the rest of the series has landed I'm
sending this as a standalone patch so it can get more attention. I'm
starting it back at "v1". There is no change between v1 and the one
sent previously except for a typo fix in an error message: Can't't =>
Can't
[1] https://lore.kernel.org/r/[email protected]
drivers/gpu/drm/drm_panel.c | 12 ++
.../gpu/drm/panel/panel-drm-shutdown-check.h | 151 ++++++++++++++++++
drivers/gpu/drm/panel/panel-edp.c | 19 +--
drivers/gpu/drm/panel/panel-simple.c | 19 +--
4 files changed, 169 insertions(+), 32 deletions(-)
create mode 100644 drivers/gpu/drm/panel/panel-drm-shutdown-check.h
diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
index cfbe020de54e..df3f15f4625e 100644
--- a/drivers/gpu/drm/drm_panel.c
+++ b/drivers/gpu/drm/drm_panel.c
@@ -161,6 +161,12 @@ int drm_panel_unprepare(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ /*
+ * If you're seeing this warning, you either need to add your driver
+ * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
+ * or panel-simple) or you need to remove the manual call to
+ * drm_panel_unprepare() in your panel driver.
+ */
if (!panel->prepared) {
dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
return 0;
@@ -245,6 +251,12 @@ int drm_panel_disable(struct drm_panel *panel)
if (!panel)
return -EINVAL;
+ /*
+ * If you're seeing this warning, you either need to add your driver
+ * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
+ * or panel-simple) or you need to remove the manual call to
+ * drm_panel_disable() in your panel driver.
+ */
if (!panel->enabled) {
dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
return 0;
diff --git a/drivers/gpu/drm/panel/panel-drm-shutdown-check.h b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
new file mode 100644
index 000000000000..dfa8197e09fb
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
@@ -0,0 +1,151 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright 2024 Google Inc.
+ *
+ * This header is a temporary solution and is intended to be included
+ * directly by panel-edp.c and panel-simple.c.
+ *
+ * This header is needed because panel-edp and panel-simple are used by a
+ * wide variety of DRM drivers and it's hard to know for sure if all of the
+ * DRM drivers used by those panel drivers are properly calling
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
+ * shutdown/remove time.
+ *
+ * The plan for this header file:
+ * - Land it and hope that the warning print will encourage DRM drivers to
+ * get fixed.
+ * - Eventually move to a WARN() splat for extra encouragement.
+ * - Assume that everyone has been fixed and remove this header file.
+ */
+
+#ifndef __PANEL_DRM_SHUTDOWN_CHECK_H__
+#define __PANEL_DRM_SHUTDOWN_CHECK_H__
+
+#include <drm/drm_bridge.h>
+#include <drm/drm_drv.h>
+
+/*
+ * This is a list of all DRM drivers that appear to properly call
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
+ * shutdown and remove time.
+ *
+ * We can't detect this dynamically and are stuck with a list because the panel
+ * driver's shutdown() call might be called _before_ the DRM driver's
+ * shutdown() call.
+ *
+ * NOTE: no verification has been done to confirm that the below drivers
+ * are actually _used_ with panel-simple or panel-edp, only that these drivers
+ * appear to be shutting down properly. It doesn't hurt to have extra drivers
+ * listed here as long as the list doesn't contain any drivers that are
+ * missing the shutdown calls.
+ */
+static const char * const drm_drivers_that_shutdown[] = {
+ "armada-drm",
+ "aspeed-gfx-drm",
+ "ast",
+ "atmel-hlcdc",
+ "bochs-drm",
+ "cirrus",
+ "exynos",
+ "fsl-dcu-drm",
+ "gm12u320",
+ "gud",
+ "hdlcd",
+ "hibmc",
+ "hx8357d",
+ "hyperv_drm",
+ "ili9163",
+ "ili9225",
+ "ili9341",
+ "ili9486",
+ "imx-dcss",
+ "imx-drm",
+ "imx-lcdc",
+ "imx-lcdif",
+ "ingenic-drm",
+ "kirin",
+ "komeda",
+ "logicvc-drm",
+ "loongson",
+ "mali-dp",
+ "mcde",
+ "meson",
+ "mgag200",
+ "mi0283qt",
+ "msm",
+ "mxsfb-drm",
+ "omapdrm",
+ "panel-mipi-dbi",
+ "pl111",
+ "qxl",
+ "rcar-du",
+ "repaper",
+ "rockchip",
+ "rzg2l-du",
+ "ssd130x",
+ "st7586",
+ "st7735r",
+ "sti",
+ "stm",
+ "sun4i-drm",
+ "tidss",
+ "tilcdc",
+ "tve200",
+ "vboxvideo",
+ "zynqmp-dpsub",
+ ""
+};
+
+static void panel_shutdown_if_drm_driver_needs_fixing(struct drm_panel *panel)
+{
+ struct drm_bridge *bridge;
+ const struct drm_driver *driver;
+ const char * const *driver_name;
+
+ /*
+ * Look for a bridge that shares the DT node of this panel. That only
+ * works if we've been linked up with a panel_bridge.
+ */
+ bridge = of_drm_find_bridge(panel->dev->of_node);
+ if (bridge && bridge->dev && bridge->dev->driver) {
+ /*
+ * If the DRM driver for the bridge is known to be fine then
+ * we're done.
+ */
+ driver = bridge->dev->driver;
+ for (driver_name = drm_drivers_that_shutdown; *driver_name; driver_name++) {
+ if (strcmp(*driver_name, driver->name) == 0)
+ return;
+ }
+
+ /*
+ * If you see the message below then:
+ * 1. Make sure your DRM driver is properly calling
+ * drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
+ * at shutdown time.
+ * 2. Add your driver to the list.
+ */
+ dev_warn(panel->dev,
+ "DRM driver appears buggy; manually disable/unprepare\n");
+ } else {
+ /*
+ * If you see the message below then your setup needs to
+ * be moved to using a panel_bridge. This often happens
+ * by calling devm_drm_of_get_bridge(). Having a panel without
+ * an associated panel_bridge is deprecated.
+ */
+ dev_warn(panel->dev,
+ "Can't find DRM driver; manually disable/unprepare\n");
+ }
+
+ /*
+ * If we don't know if a DRM driver is properly shutting things down
+ * then we'll manually call the disable/unprepare. This is always a
+ * safe thing to do (in that it won't cause you to crash), but it
+ * does generate a warning.
+ */
+ drm_panel_disable(panel);
+ drm_panel_unprepare(panel);
+}
+
+#endif
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 67ab6915d6e4..26f89858df9d 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -42,6 +42,8 @@
#include <drm/drm_edid.h>
#include <drm/drm_panel.h>
+#include "panel-drm-shutdown-check.h"
+
/**
* struct panel_delay - Describes delays for a simple panel.
*/
@@ -948,22 +950,7 @@ static void panel_edp_shutdown(struct device *dev)
{
struct panel_edp *panel = dev_get_drvdata(dev);
- /*
- * NOTE: the following two calls don't really belong here. It is the
- * responsibility of a correctly written DRM modeset driver to call
- * 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().
- */
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
}
static void panel_edp_remove(struct device *dev)
diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
index 8345ed891f5a..f505bc27e5ae 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -42,6 +42,8 @@
#include <drm/drm_panel.h>
#include <drm/drm_of.h>
+#include "panel-drm-shutdown-check.h"
+
/**
* struct panel_desc - Describes a simple panel.
*/
@@ -720,22 +722,7 @@ static void panel_simple_shutdown(struct device *dev)
{
struct panel_simple *panel = dev_get_drvdata(dev);
- /*
- * NOTE: the following two calls don't really belong here. It is the
- * responsibility of a correctly written DRM modeset driver to call
- * 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().
- */
- drm_panel_disable(&panel->base);
- drm_panel_unprepare(&panel->base);
+ panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
}
static void panel_simple_remove(struct device *dev)
--
2.45.2.505.gda0bf45e8d-goog
Hi,
On Tue, Jun 11, 2024 at 07:48:51AM GMT, Douglas Anderson wrote:
> 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.
>
> It's not easy to get rid of these warnings. Until we know that all DRM
> modeset drivers used with panel-simple and panel-edp are properly
> calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> then the panel drivers _need_ to disable/unprepare themselves in order
> to power off the panel cleanly. However, there are lots of DRM modeset
> drivers used with panel-edp and panel-simple and it's hard to know
> when we've got them all. Since the warning happens only on the drivers
> that _are_ updated there's nothing to encourage broken DRM modeset
> drivers to get fixed.
>
> In order to flip the warning to the proper place, we need to know
> which modeset drivers are going to shutdown properly. Though ugly, do
> this by creating a list of everyone that shuts down properly. This
> allows us to generate a warning for the correct case and also lets us
> get rid of the warning for drivers that are shutting down properly.
>
> Maintaining this list is ugly, but the idea is that it's only short
> term. Once everyone is converted we can delete the list and call it
> done. The list is ugly enough and adding to it is annoying enough that
> people should push to make this happen.
>
> Implement this all in a shared "header" file included by the two panel
> drivers that need it. This avoids us adding an new exports while still
> allowing the panel drivers to be modules. The code waste should be
> small and, as per above, the whole solution is temporary.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> I came up with this idea to help us move forward since otherwise I
> couldn't see how we were ever going to fix panel-simple and panel-edp
> since they're used by so many DRM Modeset drivers. It's a bit ugly but
> I don't hate it. What do others think?
I don't think it's the right approach, even more so since we're so close
now to having it in every driver.
I ran the coccinelle script we started with, and here are the results:
./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
Looking at the drivers by hand, it seems consistent.
Moving forward, I think having a collection of coccinelle scripts that
we ask new driver authors to run or put them in CI somehow would be a
better path. We have other similar candidates that can't really be dealt
with any other way, like not using drmm_ memory allocations, or not
using drm_dev_enter / drm_dev_exit.
Maxime
On Tue, Jun 11, 2024 at 07:48:51AM -0700, Douglas Anderson wrote:
> 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.
>
> It's not easy to get rid of these warnings. Until we know that all DRM
> modeset drivers used with panel-simple and panel-edp are properly
> calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> then the panel drivers _need_ to disable/unprepare themselves in order
> to power off the panel cleanly. However, there are lots of DRM modeset
> drivers used with panel-edp and panel-simple and it's hard to know
> when we've got them all. Since the warning happens only on the drivers
> that _are_ updated there's nothing to encourage broken DRM modeset
> drivers to get fixed.
>
> In order to flip the warning to the proper place, we need to know
> which modeset drivers are going to shutdown properly. Though ugly, do
> this by creating a list of everyone that shuts down properly. This
> allows us to generate a warning for the correct case and also lets us
> get rid of the warning for drivers that are shutting down properly.
>
> Maintaining this list is ugly, but the idea is that it's only short
> term. Once everyone is converted we can delete the list and call it
> done. The list is ugly enough and adding to it is annoying enough that
> people should push to make this happen.
>
> Implement this all in a shared "header" file included by the two panel
> drivers that need it. This avoids us adding an new exports while still
> allowing the panel drivers to be modules. The code waste should be
> small and, as per above, the whole solution is temporary.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
> I came up with this idea to help us move forward since otherwise I
> couldn't see how we were ever going to fix panel-simple and panel-edp
> since they're used by so many DRM Modeset drivers. It's a bit ugly but
> I don't hate it. What do others think?
I think it's terrible :-)
Why does something like this now work?
drm_panel_shutdown_fixup(panel)
{
/* if you get warnings here, fix your main drm driver to call
* drm_atomic_helper_shutdown()
*/
if (WARN_ON(panel->enabled))
drm_panel_disable(panel);
if (WARN_ON(panel->prepared))
drm_panel_unprepare(panel);
}
And then call that little helper in the relevant panel drivers? Also feel
free to bikeshed the name and maybe put a more lengthly explainer into the
kerneldoc for that ...
Or am I completely missing the point here?
-Sima
>
> This is at the end of the series so even if folks hate it we could
> still land the rest of the series.
> This was a "bonus" extra patch I added at the end of v3 of the series
> ("drm/panel: Remove most store/double-check of prepared/enabled
> state") [1]. There, I had the note: "I came up with this idea to help
> us move forward since otherwise I couldn't see how we were ever going
> to fix panel-simple and panel-edp since they're used by so many DRM
> Modeset drivers. It's a bit ugly but I don't hate it. What do others
> think?"
>
> As requested by Neil, now that the rest of the series has landed I'm
> sending this as a standalone patch so it can get more attention. I'm
> starting it back at "v1". There is no change between v1 and the one
> sent previously except for a typo fix in an error message: Can't't =>
> Can't
>
> [1] https://lore.kernel.org/r/[email protected]
>
> drivers/gpu/drm/drm_panel.c | 12 ++
> .../gpu/drm/panel/panel-drm-shutdown-check.h | 151 ++++++++++++++++++
> drivers/gpu/drm/panel/panel-edp.c | 19 +--
> drivers/gpu/drm/panel/panel-simple.c | 19 +--
> 4 files changed, 169 insertions(+), 32 deletions(-)
> create mode 100644 drivers/gpu/drm/panel/panel-drm-shutdown-check.h
>
> diff --git a/drivers/gpu/drm/drm_panel.c b/drivers/gpu/drm/drm_panel.c
> index cfbe020de54e..df3f15f4625e 100644
> --- a/drivers/gpu/drm/drm_panel.c
> +++ b/drivers/gpu/drm/drm_panel.c
> @@ -161,6 +161,12 @@ int drm_panel_unprepare(struct drm_panel *panel)
> if (!panel)
> return -EINVAL;
>
> + /*
> + * If you're seeing this warning, you either need to add your driver
> + * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
> + * or panel-simple) or you need to remove the manual call to
> + * drm_panel_unprepare() in your panel driver.
> + */
> if (!panel->prepared) {
> dev_warn(panel->dev, "Skipping unprepare of already unprepared panel\n");
> return 0;
> @@ -245,6 +251,12 @@ int drm_panel_disable(struct drm_panel *panel)
> if (!panel)
> return -EINVAL;
>
> + /*
> + * If you're seeing this warning, you either need to add your driver
> + * to "drm_drivers_that_shutdown" (if you're seeing it with panel-edp
> + * or panel-simple) or you need to remove the manual call to
> + * drm_panel_disable() in your panel driver.
> + */
> if (!panel->enabled) {
> dev_warn(panel->dev, "Skipping disable of already disabled panel\n");
> return 0;
> diff --git a/drivers/gpu/drm/panel/panel-drm-shutdown-check.h b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
> new file mode 100644
> index 000000000000..dfa8197e09fb
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-drm-shutdown-check.h
> @@ -0,0 +1,151 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright 2024 Google Inc.
> + *
> + * This header is a temporary solution and is intended to be included
> + * directly by panel-edp.c and panel-simple.c.
> + *
> + * This header is needed because panel-edp and panel-simple are used by a
> + * wide variety of DRM drivers and it's hard to know for sure if all of the
> + * DRM drivers used by those panel drivers are properly calling
> + * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
> + * shutdown/remove time.
> + *
> + * The plan for this header file:
> + * - Land it and hope that the warning print will encourage DRM drivers to
> + * get fixed.
> + * - Eventually move to a WARN() splat for extra encouragement.
> + * - Assume that everyone has been fixed and remove this header file.
> + */
> +
> +#ifndef __PANEL_DRM_SHUTDOWN_CHECK_H__
> +#define __PANEL_DRM_SHUTDOWN_CHECK_H__
> +
> +#include <drm/drm_bridge.h>
> +#include <drm/drm_drv.h>
> +
> +/*
> + * This is a list of all DRM drivers that appear to properly call
> + * drm_atomic_helper_shutdown() or drm_helper_force_disable_all() at
> + * shutdown and remove time.
> + *
> + * We can't detect this dynamically and are stuck with a list because the panel
> + * driver's shutdown() call might be called _before_ the DRM driver's
> + * shutdown() call.
> + *
> + * NOTE: no verification has been done to confirm that the below drivers
> + * are actually _used_ with panel-simple or panel-edp, only that these drivers
> + * appear to be shutting down properly. It doesn't hurt to have extra drivers
> + * listed here as long as the list doesn't contain any drivers that are
> + * missing the shutdown calls.
> + */
> +static const char * const drm_drivers_that_shutdown[] = {
> + "armada-drm",
> + "aspeed-gfx-drm",
> + "ast",
> + "atmel-hlcdc",
> + "bochs-drm",
> + "cirrus",
> + "exynos",
> + "fsl-dcu-drm",
> + "gm12u320",
> + "gud",
> + "hdlcd",
> + "hibmc",
> + "hx8357d",
> + "hyperv_drm",
> + "ili9163",
> + "ili9225",
> + "ili9341",
> + "ili9486",
> + "imx-dcss",
> + "imx-drm",
> + "imx-lcdc",
> + "imx-lcdif",
> + "ingenic-drm",
> + "kirin",
> + "komeda",
> + "logicvc-drm",
> + "loongson",
> + "mali-dp",
> + "mcde",
> + "meson",
> + "mgag200",
> + "mi0283qt",
> + "msm",
> + "mxsfb-drm",
> + "omapdrm",
> + "panel-mipi-dbi",
> + "pl111",
> + "qxl",
> + "rcar-du",
> + "repaper",
> + "rockchip",
> + "rzg2l-du",
> + "ssd130x",
> + "st7586",
> + "st7735r",
> + "sti",
> + "stm",
> + "sun4i-drm",
> + "tidss",
> + "tilcdc",
> + "tve200",
> + "vboxvideo",
> + "zynqmp-dpsub",
> + ""
> +};
> +
> +static void panel_shutdown_if_drm_driver_needs_fixing(struct drm_panel *panel)
> +{
> + struct drm_bridge *bridge;
> + const struct drm_driver *driver;
> + const char * const *driver_name;
> +
> + /*
> + * Look for a bridge that shares the DT node of this panel. That only
> + * works if we've been linked up with a panel_bridge.
> + */
> + bridge = of_drm_find_bridge(panel->dev->of_node);
> + if (bridge && bridge->dev && bridge->dev->driver) {
> + /*
> + * If the DRM driver for the bridge is known to be fine then
> + * we're done.
> + */
> + driver = bridge->dev->driver;
> + for (driver_name = drm_drivers_that_shutdown; *driver_name; driver_name++) {
> + if (strcmp(*driver_name, driver->name) == 0)
> + return;
> + }
> +
> + /*
> + * If you see the message below then:
> + * 1. Make sure your DRM driver is properly calling
> + * drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> + * at shutdown time.
> + * 2. Add your driver to the list.
> + */
> + dev_warn(panel->dev,
> + "DRM driver appears buggy; manually disable/unprepare\n");
> + } else {
> + /*
> + * If you see the message below then your setup needs to
> + * be moved to using a panel_bridge. This often happens
> + * by calling devm_drm_of_get_bridge(). Having a panel without
> + * an associated panel_bridge is deprecated.
> + */
> + dev_warn(panel->dev,
> + "Can't find DRM driver; manually disable/unprepare\n");
> + }
> +
> + /*
> + * If we don't know if a DRM driver is properly shutting things down
> + * then we'll manually call the disable/unprepare. This is always a
> + * safe thing to do (in that it won't cause you to crash), but it
> + * does generate a warning.
> + */
> + drm_panel_disable(panel);
> + drm_panel_unprepare(panel);
> +}
> +
> +#endif
> diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
> index 67ab6915d6e4..26f89858df9d 100644
> --- a/drivers/gpu/drm/panel/panel-edp.c
> +++ b/drivers/gpu/drm/panel/panel-edp.c
> @@ -42,6 +42,8 @@
> #include <drm/drm_edid.h>
> #include <drm/drm_panel.h>
>
> +#include "panel-drm-shutdown-check.h"
> +
> /**
> * struct panel_delay - Describes delays for a simple panel.
> */
> @@ -948,22 +950,7 @@ static void panel_edp_shutdown(struct device *dev)
> {
> struct panel_edp *panel = dev_get_drvdata(dev);
>
> - /*
> - * NOTE: the following two calls don't really belong here. It is the
> - * responsibility of a correctly written DRM modeset driver to call
> - * 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().
> - */
> - drm_panel_disable(&panel->base);
> - drm_panel_unprepare(&panel->base);
> + panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
> }
>
> static void panel_edp_remove(struct device *dev)
> diff --git a/drivers/gpu/drm/panel/panel-simple.c b/drivers/gpu/drm/panel/panel-simple.c
> index 8345ed891f5a..f505bc27e5ae 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -42,6 +42,8 @@
> #include <drm/drm_panel.h>
> #include <drm/drm_of.h>
>
> +#include "panel-drm-shutdown-check.h"
> +
> /**
> * struct panel_desc - Describes a simple panel.
> */
> @@ -720,22 +722,7 @@ static void panel_simple_shutdown(struct device *dev)
> {
> struct panel_simple *panel = dev_get_drvdata(dev);
>
> - /*
> - * NOTE: the following two calls don't really belong here. It is the
> - * responsibility of a correctly written DRM modeset driver to call
> - * 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().
> - */
> - drm_panel_disable(&panel->base);
> - drm_panel_unprepare(&panel->base);
> + panel_shutdown_if_drm_driver_needs_fixing(&panel->base);
> }
>
> static void panel_simple_remove(struct device *dev)
> --
> 2.45.2.505.gda0bf45e8d-goog
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi,
On Wed, Jun 12, 2024 at 1:09 AM Maxime Ripard <[email protected]> wrote:
>
> Hi,
>
> On Tue, Jun 11, 2024 at 07:48:51AM GMT, Douglas Anderson wrote:
> > 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.
> >
> > It's not easy to get rid of these warnings. Until we know that all DRM
> > modeset drivers used with panel-simple and panel-edp are properly
> > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > then the panel drivers _need_ to disable/unprepare themselves in order
> > to power off the panel cleanly. However, there are lots of DRM modeset
> > drivers used with panel-edp and panel-simple and it's hard to know
> > when we've got them all. Since the warning happens only on the drivers
> > that _are_ updated there's nothing to encourage broken DRM modeset
> > drivers to get fixed.
> >
> > In order to flip the warning to the proper place, we need to know
> > which modeset drivers are going to shutdown properly. Though ugly, do
> > this by creating a list of everyone that shuts down properly. This
> > allows us to generate a warning for the correct case and also lets us
> > get rid of the warning for drivers that are shutting down properly.
> >
> > Maintaining this list is ugly, but the idea is that it's only short
> > term. Once everyone is converted we can delete the list and call it
> > done. The list is ugly enough and adding to it is annoying enough that
> > people should push to make this happen.
> >
> > Implement this all in a shared "header" file included by the two panel
> > drivers that need it. This avoids us adding an new exports while still
> > allowing the panel drivers to be modules. The code waste should be
> > small and, as per above, the whole solution is temporary.
> >
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
> > I came up with this idea to help us move forward since otherwise I
> > couldn't see how we were ever going to fix panel-simple and panel-edp
> > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > I don't hate it. What do others think?
>
> I don't think it's the right approach, even more so since we're so close
> now to having it in every driver.
>
> I ran the coccinelle script we started with, and here are the results:
>
> ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> ./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> ./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
Sure, although I think we agreed even back when we talked about this
last that your coccinelle script wasn't guaranteed to catch every
driver. ...so I guess the question is: are we willing to accept that
we'll stop disabling panels at shutdown for any drivers that might
were missed. For instance, looking at it by hand (which also could
miss things), I previously thought that we also might need:
* nouveau
* tegra
* amdgpu
* sprd
* gma500
* radeon
I sent patches for those drivers but they don't go through drm-misc
and some of the drivers had a lot of abstraction layers and were hard
to reason about. I'm also not 100% confident that all of those drivers
really are affected--they'd have to be used with panel-simple or
panel-edp...
In any case, having some sort of warning that would give us a
definitive answer would be nice. My proposed patch would give us that
warning. I could even jump to a WARN_ON right from the start.
My proposed patch is self-admittedly super ugly and is also designed
to be temporary, so I don't think of this as giving up right before
crossing the finish line but instead accepting a tiny bit of temporary
ugliness to make sure that we don't accidentally regress anyone. I
would really hope it would be obvious to anyone writing / reviewing
drivers that the function I introduced isn't intended for anyone but
panel-simple and panel-edp.
-Doug
Hi,
On Wed, Jun 12, 2024 at 1:58 AM Daniel Vetter <[email protected]> wrote:
>
> On Tue, Jun 11, 2024 at 07:48:51AM -0700, Douglas Anderson wrote:
> > 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.
> >
> > It's not easy to get rid of these warnings. Until we know that all DRM
> > modeset drivers used with panel-simple and panel-edp are properly
> > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > then the panel drivers _need_ to disable/unprepare themselves in order
> > to power off the panel cleanly. However, there are lots of DRM modeset
> > drivers used with panel-edp and panel-simple and it's hard to know
> > when we've got them all. Since the warning happens only on the drivers
> > that _are_ updated there's nothing to encourage broken DRM modeset
> > drivers to get fixed.
> >
> > In order to flip the warning to the proper place, we need to know
> > which modeset drivers are going to shutdown properly. Though ugly, do
> > this by creating a list of everyone that shuts down properly. This
> > allows us to generate a warning for the correct case and also lets us
> > get rid of the warning for drivers that are shutting down properly.
> >
> > Maintaining this list is ugly, but the idea is that it's only short
> > term. Once everyone is converted we can delete the list and call it
> > done. The list is ugly enough and adding to it is annoying enough that
> > people should push to make this happen.
> >
> > Implement this all in a shared "header" file included by the two panel
> > drivers that need it. This avoids us adding an new exports while still
> > allowing the panel drivers to be modules. The code waste should be
> > small and, as per above, the whole solution is temporary.
> >
> > Signed-off-by: Douglas Anderson <[email protected]>
> > ---
> > I came up with this idea to help us move forward since otherwise I
> > couldn't see how we were ever going to fix panel-simple and panel-edp
> > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > I don't hate it. What do others think?
>
> I think it's terrible :-)
Well, we're in agreement. It is terrible. However, in my opinion it's
still a reasonable way to move forward.
> Why does something like this now work?
>
> drm_panel_shutdown_fixup(panel)
> {
> /* if you get warnings here, fix your main drm driver to call
> * drm_atomic_helper_shutdown()
> */
> if (WARN_ON(panel->enabled))
> drm_panel_disable(panel);
>
> if (WARN_ON(panel->prepared))
> drm_panel_unprepare(panel);
> }
>
> And then call that little helper in the relevant panel drivers? Also feel
> free to bikeshed the name and maybe put a more lengthly explainer into the
> kerneldoc for that ...
>
> Or am I completely missing the point here?
The problem is that the ordering is wrong, I think. Even if the OS was
calling driver shutdown functions in the perfect order (which I'm not
convinced about since panels aren't always child "struct device"s of
the DRM device), the OS should be calling panel shutdown _before_
shutting down the DRM device. That means that with your suggestion:
1. Shutdown starts and panel is on.
2. OS calls panel shutdown call, which prints warnings because panel
is still on.
3. OS calls DRM driver shutdown call, which prints warnings because
someone else turned the panel off.
:-P
Certainly if I goofed and the above is wrong then let me know--I did
my experiments on this many months ago and didn't try repeating them
again now.
In any case, the only way I could figure out around this was some sort
of list. As mentioned in the commit message, it's super ugly and
intended to be temporary. Once we solve all the current in-tree
drivers, I'd imagine that long term for new DRM drivers this kind of
thing would be discovered during bringup of new boards. Usually during
bringup of new boards EEs measure timing signals and complain if
they're not right. If some EE cared and said we weren't disabling the
panel correctly at shutdown time then we'd know there was a problem.
-Doug
On Wed, Jun 12, 2024 at 07:49:31AM GMT, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 1:58 AM Daniel Vetter <[email protected]> wrote:
> >
> > On Tue, Jun 11, 2024 at 07:48:51AM -0700, Douglas Anderson wrote:
> > > 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.
> > >
> > > It's not easy to get rid of these warnings. Until we know that all DRM
> > > modeset drivers used with panel-simple and panel-edp are properly
> > > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > > then the panel drivers _need_ to disable/unprepare themselves in order
> > > to power off the panel cleanly. However, there are lots of DRM modeset
> > > drivers used with panel-edp and panel-simple and it's hard to know
> > > when we've got them all. Since the warning happens only on the drivers
> > > that _are_ updated there's nothing to encourage broken DRM modeset
> > > drivers to get fixed.
> > >
> > > In order to flip the warning to the proper place, we need to know
> > > which modeset drivers are going to shutdown properly. Though ugly, do
> > > this by creating a list of everyone that shuts down properly. This
> > > allows us to generate a warning for the correct case and also lets us
> > > get rid of the warning for drivers that are shutting down properly.
> > >
> > > Maintaining this list is ugly, but the idea is that it's only short
> > > term. Once everyone is converted we can delete the list and call it
> > > done. The list is ugly enough and adding to it is annoying enough that
> > > people should push to make this happen.
> > >
> > > Implement this all in a shared "header" file included by the two panel
> > > drivers that need it. This avoids us adding an new exports while still
> > > allowing the panel drivers to be modules. The code waste should be
> > > small and, as per above, the whole solution is temporary.
> > >
> > > Signed-off-by: Douglas Anderson <[email protected]>
> > > ---
> > > I came up with this idea to help us move forward since otherwise I
> > > couldn't see how we were ever going to fix panel-simple and panel-edp
> > > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > > I don't hate it. What do others think?
> >
> > I think it's terrible :-)
>
> Well, we're in agreement. It is terrible. However, in my opinion it's
> still a reasonable way to move forward.
>
>
> > Why does something like this now work?
> >
> > drm_panel_shutdown_fixup(panel)
> > {
> > /* if you get warnings here, fix your main drm driver to call
> > * drm_atomic_helper_shutdown()
> > */
> > if (WARN_ON(panel->enabled))
> > drm_panel_disable(panel);
> >
> > if (WARN_ON(panel->prepared))
> > drm_panel_unprepare(panel);
> > }
> >
> > And then call that little helper in the relevant panel drivers? Also feel
> > free to bikeshed the name and maybe put a more lengthly explainer into the
> > kerneldoc for that ...
> >
> > Or am I completely missing the point here?
>
> The problem is that the ordering is wrong, I think. Even if the OS was
> calling driver shutdown functions in the perfect order (which I'm not
> convinced about since panels aren't always child "struct device"s of
> the DRM device), the OS should be calling panel shutdown _before_
> shutting down the DRM device. That means that with your suggestion:
>
> 1. Shutdown starts and panel is on.
>
> 2. OS calls panel shutdown call, which prints warnings because panel
> is still on.
>
> 3. OS calls DRM driver shutdown call, which prints warnings because
> someone else turned the panel off.
>
> :-P
>
> Certainly if I goofed and the above is wrong then let me know--I did
> my experiments on this many months ago and didn't try repeating them
> again now.
>
> In any case, the only way I could figure out around this was some sort
> of list. As mentioned in the commit message, it's super ugly and
> intended to be temporary. Once we solve all the current in-tree
> drivers, I'd imagine that long term for new DRM drivers this kind of
> thing would be discovered during bringup of new boards. Usually during
> bringup of new boards EEs measure timing signals and complain if
> they're not right. If some EE cared and said we weren't disabling the
> panel correctly at shutdown time then we'd know there was a problem.
Based on a discussion we had today With Sima on IRC, I think there's
another way forward.
We were actually discussing refcount'ing the panels to avoid lifetime
issues. It would require some API overhaul to have a function to
allocate the drm_panel structure and init'ing the refcount, plus some to
get / put the references.
Having this refcount would mean that we also get a release function now,
called when the panel is free'd.
Could we warn if the panel is still prepared/enabled and is about to be
freed?
It would require to switch panel-simple and panel-edp to that new API,
but it should be easy enough.
Maxime
Hi,
On Wed, Jun 12, 2024 at 8:03 AM Maxime Ripard <[email protected]> wrote:
>
> > > Why does something like this now work?
> > >
> > > drm_panel_shutdown_fixup(panel)
> > > {
> > > /* if you get warnings here, fix your main drm driver to call
> > > * drm_atomic_helper_shutdown()
> > > */
> > > if (WARN_ON(panel->enabled))
> > > drm_panel_disable(panel);
> > >
> > > if (WARN_ON(panel->prepared))
> > > drm_panel_unprepare(panel);
> > > }
> > >
> > > And then call that little helper in the relevant panel drivers? Also feel
> > > free to bikeshed the name and maybe put a more lengthly explainer into the
> > > kerneldoc for that ...
> > >
> > > Or am I completely missing the point here?
> >
> > The problem is that the ordering is wrong, I think. Even if the OS was
> > calling driver shutdown functions in the perfect order (which I'm not
> > convinced about since panels aren't always child "struct device"s of
> > the DRM device), the OS should be calling panel shutdown _before_
> > shutting down the DRM device. That means that with your suggestion:
> >
> > 1. Shutdown starts and panel is on.
> >
> > 2. OS calls panel shutdown call, which prints warnings because panel
> > is still on.
> >
> > 3. OS calls DRM driver shutdown call, which prints warnings because
> > someone else turned the panel off.
> >
> > :-P
> >
> > Certainly if I goofed and the above is wrong then let me know--I did
> > my experiments on this many months ago and didn't try repeating them
> > again now.
> >
> > In any case, the only way I could figure out around this was some sort
> > of list. As mentioned in the commit message, it's super ugly and
> > intended to be temporary. Once we solve all the current in-tree
> > drivers, I'd imagine that long term for new DRM drivers this kind of
> > thing would be discovered during bringup of new boards. Usually during
> > bringup of new boards EEs measure timing signals and complain if
> > they're not right. If some EE cared and said we weren't disabling the
> > panel correctly at shutdown time then we'd know there was a problem.
>
> Based on a discussion we had today With Sima on IRC, I think there's
> another way forward.
>
> We were actually discussing refcount'ing the panels to avoid lifetime
> issues. It would require some API overhaul to have a function to
> allocate the drm_panel structure and init'ing the refcount, plus some to
> get / put the references.
>
> Having this refcount would mean that we also get a release function now,
> called when the panel is free'd.
>
> Could we warn if the panel is still prepared/enabled and is about to be
> freed?
>
> It would require to switch panel-simple and panel-edp to that new API,
> but it should be easy enough.
I think there are two problems here:
1. The problem is at shutdown here. Memory isn't freed at shutdown
time. This isn't a lifetime issue. No release functions are involved
in shutdown and we don't free memory then.
2. As I tried to point out, even if we were guaranteed the correct
order it still doesn't help us. In other words: if all device links
were perfect and all ordering was proper then the panel should get
shutdown _before_ the DRM device. That means we can't put a check in
the panel code to see if the DRM device has been shutdown.
-Doug
Hi,
On Wed, Jun 12, 2024 at 8:11 AM Daniel Vetter <[email protected]> wrote:
>
> > The problem is that the ordering is wrong, I think. Even if the OS was
> > calling driver shutdown functions in the perfect order (which I'm not
> > convinced about since panels aren't always child "struct device"s of
> > the DRM device), the OS should be calling panel shutdown _before_
> > shutting down the DRM device. That means that with your suggestion:
> >
> > 1. Shutdown starts and panel is on.
> >
> > 2. OS calls panel shutdown call, which prints warnings because panel
> > is still on.
> >
> > 3. OS calls DRM driver shutdown call, which prints warnings because
> > someone else turned the panel off.
>
> Uh, that's a _much_ more fundamental issue.
>
> The fix for that is telling the driver core about this dependency with
> device_link_add. Unfortuantely, despite years of me trying to push for
> this, drm_bridge and drm_panel still don't automatically add these,
> because the situation is a really complex mess.
>
> Probably need to read dri-devel archives for all the past attempts around
> device_link_add.
>
> But the solution is definitely not to have a manually tracked list, what's
> very architectural unsound way to tackle this problem.
>
> > Certainly if I goofed and the above is wrong then let me know--I did
> > my experiments on this many months ago and didn't try repeating them
> > again now.
>
> Oh the issue is very real and known since years. It also wreaks module
> unload and driver unbinding, since currently nothing makes sure your
> drm_panel lives longer than your drm_device.
In this case I'm mostly worried about the device "shutdown" call, so
it's not quite a lifetime issue but it is definitely related.
As per my reply to Maxime, though, I'd expect that if all ordering
issues were fixed and things were perfect then we'd still have a
problem. Specifically it would seem pretty wrong to me to say that the
panel is the "parent" of the DRM device, right? So if the panel is the
"child" of the DRM device that means it'll get shutdown first and that
means that the panel's shutdown call cannot be used to tell whether
the DRM device's shutdown call behaved properly.
> > In any case, the only way I could figure out around this was some sort
> > of list. As mentioned in the commit message, it's super ugly and
> > intended to be temporary. Once we solve all the current in-tree
> > drivers, I'd imagine that long term for new DRM drivers this kind of
> > thing would be discovered during bringup of new boards. Usually during
> > bringup of new boards EEs measure timing signals and complain if
> > they're not right. If some EE cared and said we weren't disabling the
> > panel correctly at shutdown time then we'd know there was a problem.
>
> You've stepped into an entire hornets nest with this device dependency
> issue unfortunately, I'm afraid :-/
As you've said, you've been working on this problem for years. Solving
the device link problem doesn't help me, but even if it did it's
really not fundamental to the problem here. The only need is to get a
warning printed out so we know for sure which DRM drivers need to be
updated before deleting the old crufty code. Blocking that on a
difficult / years-long struggle might not be the best.
That all being said, I'm also totally OK with any of the following:
1. Dropping my patch and just accepting that we will have warnings
printed out for all DRM drivers that do things correctly and have no
warnings for broken DRM drivers.
2. Someone else posting / landing a patch to remove the hacky "disable
/ unprepare" for panel-simple and panel-edp and asserting that they
don't care if they break any DRM drivers that are still broken. I
don't want to be involved in authoring or landing this patch, but I
won't scream loudly if others want to do it.
3. Someone else taking over trying to solve this problem.
...mostly this work is janitorial and I'm trying to help move the DRM
framework forward and get rid of cruft, so if it's going to cause too
much conflict I'm fine just stepping back.
-Doug
On Wed, Jun 12, 2024 at 07:49:31AM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 1:58 AM Daniel Vetter <[email protected]> wrote:
> >
> > On Tue, Jun 11, 2024 at 07:48:51AM -0700, Douglas Anderson wrote:
> > > 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.
> > >
> > > It's not easy to get rid of these warnings. Until we know that all DRM
> > > modeset drivers used with panel-simple and panel-edp are properly
> > > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > > then the panel drivers _need_ to disable/unprepare themselves in order
> > > to power off the panel cleanly. However, there are lots of DRM modeset
> > > drivers used with panel-edp and panel-simple and it's hard to know
> > > when we've got them all. Since the warning happens only on the drivers
> > > that _are_ updated there's nothing to encourage broken DRM modeset
> > > drivers to get fixed.
> > >
> > > In order to flip the warning to the proper place, we need to know
> > > which modeset drivers are going to shutdown properly. Though ugly, do
> > > this by creating a list of everyone that shuts down properly. This
> > > allows us to generate a warning for the correct case and also lets us
> > > get rid of the warning for drivers that are shutting down properly.
> > >
> > > Maintaining this list is ugly, but the idea is that it's only short
> > > term. Once everyone is converted we can delete the list and call it
> > > done. The list is ugly enough and adding to it is annoying enough that
> > > people should push to make this happen.
> > >
> > > Implement this all in a shared "header" file included by the two panel
> > > drivers that need it. This avoids us adding an new exports while still
> > > allowing the panel drivers to be modules. The code waste should be
> > > small and, as per above, the whole solution is temporary.
> > >
> > > Signed-off-by: Douglas Anderson <[email protected]>
> > > ---
> > > I came up with this idea to help us move forward since otherwise I
> > > couldn't see how we were ever going to fix panel-simple and panel-edp
> > > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > > I don't hate it. What do others think?
> >
> > I think it's terrible :-)
>
> Well, we're in agreement. It is terrible. However, in my opinion it's
> still a reasonable way to move forward.
>
>
> > Why does something like this now work?
> >
> > drm_panel_shutdown_fixup(panel)
> > {
> > /* if you get warnings here, fix your main drm driver to call
> > * drm_atomic_helper_shutdown()
> > */
> > if (WARN_ON(panel->enabled))
> > drm_panel_disable(panel);
> >
> > if (WARN_ON(panel->prepared))
> > drm_panel_unprepare(panel);
> > }
> >
> > And then call that little helper in the relevant panel drivers? Also feel
> > free to bikeshed the name and maybe put a more lengthly explainer into the
> > kerneldoc for that ...
> >
> > Or am I completely missing the point here?
>
> The problem is that the ordering is wrong, I think. Even if the OS was
> calling driver shutdown functions in the perfect order (which I'm not
> convinced about since panels aren't always child "struct device"s of
> the DRM device), the OS should be calling panel shutdown _before_
> shutting down the DRM device. That means that with your suggestion:
>
> 1. Shutdown starts and panel is on.
>
> 2. OS calls panel shutdown call, which prints warnings because panel
> is still on.
>
> 3. OS calls DRM driver shutdown call, which prints warnings because
> someone else turned the panel off.
Uh, that's a _much_ more fundamental issue.
The fix for that is telling the driver core about this dependency with
device_link_add. Unfortuantely, despite years of me trying to push for
this, drm_bridge and drm_panel still don't automatically add these,
because the situation is a really complex mess.
Probably need to read dri-devel archives for all the past attempts around
device_link_add.
But the solution is definitely not to have a manually tracked list, what's
very architectural unsound way to tackle this problem.
> Certainly if I goofed and the above is wrong then let me know--I did
> my experiments on this many months ago and didn't try repeating them
> again now.
Oh the issue is very real and known since years. It also wreaks module
unload and driver unbinding, since currently nothing makes sure your
drm_panel lives longer than your drm_device.
> In any case, the only way I could figure out around this was some sort
> of list. As mentioned in the commit message, it's super ugly and
> intended to be temporary. Once we solve all the current in-tree
> drivers, I'd imagine that long term for new DRM drivers this kind of
> thing would be discovered during bringup of new boards. Usually during
> bringup of new boards EEs measure timing signals and complain if
> they're not right. If some EE cared and said we weren't disabling the
> panel correctly at shutdown time then we'd know there was a problem.
You've stepped into an entire hornets nest with this device dependency
issue unfortunately, I'm afraid :-/
Cheers, Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi,
On Wed, Jun 12, 2024 at 8:13 AM Daniel Vetter <[email protected]> wrote:
>
> > > I ran the coccinelle script we started with, and here are the results:
> > >
> > > ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> > > ./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> > > ./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
> >
> > Sure, although I think we agreed even back when we talked about this
> > last that your coccinelle script wasn't guaranteed to catch every
> > driver. ...so I guess the question is: are we willing to accept that
> > we'll stop disabling panels at shutdown for any drivers that might
> > were missed. For instance, looking at it by hand (which also could
> > miss things), I previously thought that we also might need:
> >
> > * nouveau
> > * tegra
> > * amdgpu
> > * sprd
> > * gma500
> > * radeon
> >
> > I sent patches for those drivers but they don't go through drm-misc
> > and some of the drivers had a lot of abstraction layers and were hard
> > to reason about. I'm also not 100% confident that all of those drivers
> > really are affected--they'd have to be used with panel-simple or
> > panel-edp...
>
> Aside from amdgpu and radeon they're all in -misc now, and Alex is
> generally fairly responsive.
Ah, nice! They weren't when I sent the patches ages ago. I guess I
should go ahead and repost things and maybe they'll get some traction.
> > In any case, having some sort of warning that would give us a
> > definitive answer would be nice. My proposed patch would give us that
> > warning. I could even jump to a WARN_ON right from the start.
>
> Yeah we defo want some warning to at least check this at runtime.
Yeah, my patch today currently just has a "dev_warn", but the question
is whether it would get more attention with a full on WARN_ON(). I
know WARN_ON() can be pretty controversial.
-Doug
On Wed, Jun 12, 2024 at 5:11 PM Daniel Vetter <[email protected]> wrote:
> On Wed, Jun 12, 2024 at 07:49:31AM -0700, Doug Anderson wrote:
(...)
> > The problem is that the ordering is wrong, I think. Even if the OS was
> > calling driver shutdown functions in the perfect order (which I'm not
> > convinced about since panels aren't always child "struct device"s of
> > the DRM device), the OS should be calling panel shutdown _before_
> > shutting down the DRM device. That means that with your suggestion:
> >
> > 1. Shutdown starts and panel is on.
> >
> > 2. OS calls panel shutdown call, which prints warnings because panel
> > is still on.
> >
> > 3. OS calls DRM driver shutdown call, which prints warnings because
> > someone else turned the panel off.
>
> Uh, that's a _much_ more fundamental issue.
>
> The fix for that is telling the driver core about this dependency with
> device_link_add. Unfortuantely, despite years of me trying to push for
> this, drm_bridge and drm_panel still don't automatically add these,
> because the situation is a really complex mess.
>
> Probably need to read dri-devel archives for all the past attempts around
> device_link_add.
I think involving Saravana Kannan in the discussions around this
is the right thing to do, because he knows how to get devicelinks
to do the right thing.
If we can describe what devicelink needs to do to get this ordering
right, I'm pretty sure Saravana can tell us how to do it.
Yours,
Linus Walleij
Sima,
On Wed, Jun 12, 2024 at 8:13 AM Daniel Vetter <[email protected]> wrote:
>
> > > I ran the coccinelle script we started with, and here are the results:
> > >
> > > ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> > > ./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> > > ./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
> >
> > Sure, although I think we agreed even back when we talked about this
> > last that your coccinelle script wasn't guaranteed to catch every
> > driver. ...so I guess the question is: are we willing to accept that
> > we'll stop disabling panels at shutdown for any drivers that might
> > were missed. For instance, looking at it by hand (which also could
> > miss things), I previously thought that we also might need:
> >
> > * nouveau
> > * tegra
> > * amdgpu
> > * sprd
> > * gma500
> > * radeon
> >
> > I sent patches for those drivers but they don't go through drm-misc
> > and some of the drivers had a lot of abstraction layers and were hard
> > to reason about. I'm also not 100% confident that all of those drivers
> > really are affected--they'd have to be used with panel-simple or
> > panel-edp...
>
> Aside from amdgpu and radeon they're all in -misc now, and Alex is
> generally fairly responsive.
Sorry for not keeping up with things, but can you point to where this
was documented or what patch changed things so that these drivers went
through drm-misc? From the MAINTAINERS file I see commit 5a44d50f0072
("MAINTAINERS: Update drm-misc entry to match all drivers") and that
shows several of these drivers as "X:". As far as I can tell that
means that they _aren't_ handled by drm-misc, right? Maybe the
decision was made elsewhere and MAINTAINERS was just not updated, or
I'm not looking at the right place? I checked drm-misc-next and
drm/next and, for instance, "tegra" and "kmb" still show as excluded.
-Doug
Hi,
On Wed, Jun 12, 2024 at 9:47 AM Linus Walleij <[email protected]> wrote:
>
> On Wed, Jun 12, 2024 at 5:11 PM Daniel Vetter <[email protected]> wrote:
> > On Wed, Jun 12, 2024 at 07:49:31AM -0700, Doug Anderson wrote:
> (...)
> > > The problem is that the ordering is wrong, I think. Even if the OS was
> > > calling driver shutdown functions in the perfect order (which I'm not
> > > convinced about since panels aren't always child "struct device"s of
> > > the DRM device), the OS should be calling panel shutdown _before_
> > > shutting down the DRM device. That means that with your suggestion:
> > >
> > > 1. Shutdown starts and panel is on.
> > >
> > > 2. OS calls panel shutdown call, which prints warnings because panel
> > > is still on.
> > >
> > > 3. OS calls DRM driver shutdown call, which prints warnings because
> > > someone else turned the panel off.
> >
> > Uh, that's a _much_ more fundamental issue.
> >
> > The fix for that is telling the driver core about this dependency with
> > device_link_add. Unfortuantely, despite years of me trying to push for
> > this, drm_bridge and drm_panel still don't automatically add these,
> > because the situation is a really complex mess.
> >
> > Probably need to read dri-devel archives for all the past attempts around
> > device_link_add.
>
> I think involving Saravana Kannan in the discussions around this
> is the right thing to do, because he knows how to get devicelinks
> to do the right thing.
>
> If we can describe what devicelink needs to do to get this ordering
> right, I'm pretty sure Saravana can tell us how to do it.
I'm really not convinced that hacking with device links in order to
get the shutdown notification in the right order is correct, though.
The idea is that after we're confident that all DRM modeset drivers
are calling shutdown properly that we should _remove_ any code
handling shutdown from panel-edp and panel-simple. They should just
get disabled as part of DRM's shutdown. That means that if we messed
with devicelinks just to get a different shutdown order that it was
just for a short term thing.
That being said, one could argue that having device links between the
DRM device and the panel is the right thing long term anyway and that
may well be. I guess the issue is that it's not necessarily obvious
how the "parent/child" or "supplier/consumer" relationship works w/
DRM devices, especially panels. My instinct says that the panel
logically is a "child" or "consumer" of the DRM device and thus
inserting the correct long term device link would mean we'd get
shutdown notification in the wrong order. It would be hard to argue
that the panel is the "parent" of a DRM device, but I guess you could
call it a "supplier"? ...but it's also a "consumer" of some other
stuff, like the pixels being output and also (perhaps) the DP AUX bus.
All this complexity is why the DRM framework tends to use its own
logic for things like prepare/enable instead of using what Linux gives
you. I'm sure Saravanah can also tell you about all the crazy device
link circular dependencies that DRM has thrown him through...
In any case, I guess I'll continue asserting that I'm not going to try
to solve this problem. If folks don't like my patch and there's no
suggestion other than solving years-old problems then I'm happy to
live with the way things are and hope that someone eventually comes
along and solves it.
-Doug
On Wed, Jun 12, 2024 at 07:39:01AM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, Jun 12, 2024 at 1:09 AM Maxime Ripard <[email protected]> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 11, 2024 at 07:48:51AM GMT, Douglas Anderson wrote:
> > > 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.
> > >
> > > It's not easy to get rid of these warnings. Until we know that all DRM
> > > modeset drivers used with panel-simple and panel-edp are properly
> > > calling drm_atomic_helper_shutdown() or drm_helper_force_disable_all()
> > > then the panel drivers _need_ to disable/unprepare themselves in order
> > > to power off the panel cleanly. However, there are lots of DRM modeset
> > > drivers used with panel-edp and panel-simple and it's hard to know
> > > when we've got them all. Since the warning happens only on the drivers
> > > that _are_ updated there's nothing to encourage broken DRM modeset
> > > drivers to get fixed.
> > >
> > > In order to flip the warning to the proper place, we need to know
> > > which modeset drivers are going to shutdown properly. Though ugly, do
> > > this by creating a list of everyone that shuts down properly. This
> > > allows us to generate a warning for the correct case and also lets us
> > > get rid of the warning for drivers that are shutting down properly.
> > >
> > > Maintaining this list is ugly, but the idea is that it's only short
> > > term. Once everyone is converted we can delete the list and call it
> > > done. The list is ugly enough and adding to it is annoying enough that
> > > people should push to make this happen.
> > >
> > > Implement this all in a shared "header" file included by the two panel
> > > drivers that need it. This avoids us adding an new exports while still
> > > allowing the panel drivers to be modules. The code waste should be
> > > small and, as per above, the whole solution is temporary.
> > >
> > > Signed-off-by: Douglas Anderson <[email protected]>
> > > ---
> > > I came up with this idea to help us move forward since otherwise I
> > > couldn't see how we were ever going to fix panel-simple and panel-edp
> > > since they're used by so many DRM Modeset drivers. It's a bit ugly but
> > > I don't hate it. What do others think?
> >
> > I don't think it's the right approach, even more so since we're so close
> > now to having it in every driver.
> >
> > I ran the coccinelle script we started with, and here are the results:
> >
> > ./drivers/gpu/drm/vmwgfx/vmwgfx_drv.c:1640:25-39: ERROR: KMS driver vmw_pci_driver is missing shutdown implementation
> > ./drivers/gpu/drm/kmb/kmb_drv.c:621:30-49: ERROR: KMS driver kmb_platform_driver is missing shutdown implementation
> > ./drivers/gpu/drm/tiny/arcpgu.c:422:30-52: ERROR: KMS driver arcpgu_platform_driver is missing shutdown implementation
>
> Sure, although I think we agreed even back when we talked about this
> last that your coccinelle script wasn't guaranteed to catch every
> driver. ...so I guess the question is: are we willing to accept that
> we'll stop disabling panels at shutdown for any drivers that might
> were missed. For instance, looking at it by hand (which also could
> miss things), I previously thought that we also might need:
>
> * nouveau
> * tegra
> * amdgpu
> * sprd
> * gma500
> * radeon
>
> I sent patches for those drivers but they don't go through drm-misc
> and some of the drivers had a lot of abstraction layers and were hard
> to reason about. I'm also not 100% confident that all of those drivers
> really are affected--they'd have to be used with panel-simple or
> panel-edp...
Aside from amdgpu and radeon they're all in -misc now, and Alex is
generally fairly responsive.
> In any case, having some sort of warning that would give us a
> definitive answer would be nice. My proposed patch would give us that
> warning. I could even jump to a WARN_ON right from the start.
Yeah we defo want some warning to at least check this at runtime.
> My proposed patch is self-admittedly super ugly and is also designed
> to be temporary, so I don't think of this as giving up right before
> crossing the finish line but instead accepting a tiny bit of temporary
> ugliness to make sure that we don't accidentally regress anyone. I
> would really hope it would be obvious to anyone writing / reviewing
> drivers that the function I introduced isn't intended for anyone but
> panel-simple and panel-edp.
See my other reply for the proper design fix, and my apologies for what
you stepped into here :-/
-Sima
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch