[PATCH v2 0/8] drm: make leftover drivers call drm_atomic_helper_shutdown() at the right times

[PATCH v2 0/8] drm: make leftover drivers call drm_atomic_helper_shutdown() at the right times

[Douglas Anderson]

This patch series is the leftovers of a patch series sent in September
2023 [1] in an attempt to get some of the patches landed finally.

This patch series originally came about after a _long_ discussion
between me and Maxime Ripard in response to a different patch I sent
out [2]. As part of that discussion, we realized that it would be good
if DRM drivers consistently called drm_atomic_helper_shutdown()
properly at shutdown and driver remove time as it's documented that
they should do. The eventual goal of this would be to enable removing
some hacky code from panel drivers where they had to hook into
shutdown themselves because the DRM driver wasn't calling them.

It turns out that quite a lot of drivers seemed to be missing
drm_atomic_helper_shutdown() in one or both places that it was
supposed to be. This patch series attempts to fix all the drivers that
I was able to identify.

NOTE: fixing this wasn't exactly cookie cutter. Each driver has its
own unique way of setting itself up and tearing itself down. Some
drivers also use the component model, which adds extra fun. I've made
my best guess at solving this and I've run a bunch of compile tests
(specifically, allmodconfig for amd64, arm64, and powerpc). That being
said, these code changes are not totally trivial and I've done zero
real testing on them. Making these patches was also a little mind
numbing and I'm certain my eyes glazed over at several points when
writing them. What I'm trying to say is to please double-check that I
didn't do anything too silly, like cast your driver's drvdata to the
wrong type. Even better, test these patches!

Apparently most of these drivers now land through drm-misc [3], so
hopefully they can land. The two that don't (amdgpu and radeon) are
the ones I'm most ucertain about anyway so I've stuck them at the end.
If I've totally buggered those up feel free to take my patch as a bug
report and submit your own proper fix. ...or if there's some reason
that we don't need to do anything for those drivers then let me know
and we can drop them.

I'd like to call out a few drivers that I _didn't_ fix in this series
and why. If any of these drivers should be fixed then please yell.
- DRM drivers backed by usb_driver (like gud, gm12u320, udl): I didn't
  add the call to drm_atomic_helper_shutdown() at shutdown time
  because there's no ".shutdown" callback for them USB drivers. Given
  that USB is hotpluggable, I'm assuming that they are robust against
  this and the special shutdown callback isn't needed.
- ofdrm and simpledrm: These didn't have drm_atomic_helper_shutdown()
  in either shutdown or remove, but I didn't add it. I think that's OK
  since they're sorta special and not really directly controlling
  hardware power sequencing.
- virtio, vkms, vmwgfx, xen: I believe these are all virtual (thus
  they wouldn't directly drive a panel) and adding the shutdown
  didn't look straightforward, so I skipped them.

I've let each patch in the series get CCed straight from
get_maintainer. That means not everyone will have received every patch
but everyone should be on the cover letter. I know some people dislike
this but when touching this many drivers there's not much
choice. dri-devel and lkml have been CCed and lore/lei exist, so
hopefully that's enough for folks. I'm happy to add people to the
whole series for future posts.

[1] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org
[2] https://lore.kernel.org/r/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
[3] https://lore.kernel.org/r/Zmm6_27GikpmT3HQ@phenom.ffwll.local

Changes in v2:
- Gathered whatever hadn't landed, rebased, and reposted.

Douglas Anderson (8):
  drm/kmb: Call drm_atomic_helper_shutdown() at shutdown time
  drm/nouveau: Call drm_atomic_helper_shutdown() or equiv at shutdown
    time
  drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time
  drm/arcpgu: Call drm_atomic_helper_shutdown() at shutdown time
  drm/sprd: Call drm_atomic_helper_shutdown() at remove time
  drm/gma500: Call drm_helper_force_disable_all() at shutdown/remove
    time
  drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove
    time
  drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 ++
 drivers/gpu/drm/gma500/psb_drv.c           |  8 ++++++++
 drivers/gpu/drm/kmb/kmb_drv.c              |  6 ++++++
 drivers/gpu/drm/nouveau/nouveau_display.c  |  9 +++++++++
 drivers/gpu/drm/nouveau/nouveau_display.h  |  1 +
 drivers/gpu/drm/nouveau/nouveau_drm.c      | 13 +++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drv.h      |  1 +
 drivers/gpu/drm/nouveau/nouveau_platform.c |  6 ++++++
 drivers/gpu/drm/radeon/radeon_drv.c        |  7 ++++++-
 drivers/gpu/drm/sprd/sprd_drm.c            |  4 +++-
 drivers/gpu/drm/tegra/drm.c                |  6 ++++++
 drivers/gpu/drm/tiny/arcpgu.c              |  6 ++++++
 14 files changed, 78 insertions(+), 2 deletions(-)

-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 1/8] drm/kmb: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

(no changes since v1)

 drivers/gpu/drm/kmb/kmb_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 169b83987ce2..73d82b4d33e7 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -475,6 +475,11 @@ static void kmb_remove(struct platform_device *pdev)
 	drm_atomic_helper_shutdown(drm);
 }
 
+static void kmb_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static int kmb_probe(struct platform_device *pdev)
 {
 	struct device *dev = get_device(&pdev->dev);
@@ -621,6 +626,7 @@ static SIMPLE_DEV_PM_OPS(kmb_pm_ops, kmb_pm_suspend, kmb_pm_resume);
 static struct platform_driver kmb_platform_driver = {
 	.probe = kmb_probe,
 	.remove_new = kmb_remove,
+	.shutdown = kmb_shutdown,
 	.driver = {
 		.name = "kmb-drm",
 		.pm = &kmb_pm_ops,
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 2/8] drm/nouveau: Call drm_atomic_helper_shutdown() or equiv at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() (or
drm_helper_force_disable_all() if not using atomic) at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested. I made my best guess about
how to fit this into the existing code. If someone wishes a different
style, please yell.

(no changes since v1)

 drivers/gpu/drm/nouveau/nouveau_display.c  |  9 +++++++++
 drivers/gpu/drm/nouveau/nouveau_display.h  |  1 +
 drivers/gpu/drm/nouveau/nouveau_drm.c      | 13 +++++++++++++
 drivers/gpu/drm/nouveau/nouveau_drv.h      |  1 +
 drivers/gpu/drm/nouveau/nouveau_platform.c |  6 ++++++
 5 files changed, 30 insertions(+)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
index d4725a968827..15da55c382f1 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -645,6 +645,15 @@ nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime)
 	disp->fini(dev, runtime, suspend);
 }
 
+void
+nouveau_display_shutdown(struct drm_device *dev)
+{
+	if (drm_drv_uses_atomic_modeset(dev))
+		drm_atomic_helper_shutdown(dev);
+	else
+		drm_helper_force_disable_all(dev);
+}
+
 static void
 nouveau_display_create_properties(struct drm_device *dev)
 {
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h b/drivers/gpu/drm/nouveau/nouveau_display.h
index 2ab2ddb1eadf..9df62e833cda 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -47,6 +47,7 @@ void nouveau_display_destroy(struct drm_device *dev);
 int  nouveau_display_init(struct drm_device *dev, bool resume, bool runtime);
 void nouveau_display_hpd_resume(struct drm_device *dev);
 void nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime);
+void nouveau_display_shutdown(struct drm_device *dev);
 int  nouveau_display_suspend(struct drm_device *dev, bool runtime);
 void nouveau_display_resume(struct drm_device *dev, bool runtime);
 int  nouveau_display_vblank_enable(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c b/drivers/gpu/drm/nouveau/nouveau_drm.c
index a947e1d5f309..b41154c9b9cc 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -894,6 +894,18 @@ nouveau_drm_remove(struct pci_dev *pdev)
 	pci_disable_device(pdev);
 }
 
+void
+nouveau_drm_device_shutdown(struct drm_device *dev)
+{
+	nouveau_display_shutdown(dev);
+}
+
+static void
+nouveau_drm_shutdown(struct pci_dev *pdev)
+{
+	nouveau_drm_device_shutdown(pci_get_drvdata(pdev));
+}
+
 static int
 nouveau_do_suspend(struct drm_device *dev, bool runtime)
 {
@@ -1361,6 +1373,7 @@ nouveau_drm_pci_driver = {
 	.id_table = nouveau_drm_pci_table,
 	.probe = nouveau_drm_probe,
 	.remove = nouveau_drm_remove,
+	.shutdown = nouveau_drm_shutdown,
 	.driver.pm = &nouveau_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 25fca98a20bc..78a91686006b 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -327,6 +327,7 @@ struct drm_device *
 nouveau_platform_device_create(const struct nvkm_device_tegra_func *,
 			       struct platform_device *, struct nvkm_device **);
 void nouveau_drm_device_remove(struct drm_device *dev);
+void nouveau_drm_device_shutdown(struct drm_device *dev);
 
 #define NV_PRINTK(l,c,f,a...) do {                                             \
 	struct nouveau_cli *_cli = (c);                                        \
diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c b/drivers/gpu/drm/nouveau/nouveau_platform.c
index bf2dc7567ea4..511f3a0c6ee9 100644
--- a/drivers/gpu/drm/nouveau/nouveau_platform.c
+++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
@@ -49,6 +49,11 @@ static void nouveau_platform_remove(struct platform_device *pdev)
 	nouveau_drm_device_remove(dev);
 }
 
+static void nouveau_platform_shutdown(struct platform_device *pdev)
+{
+	nouveau_drm_device_shutdown(platform_get_drvdata(pdev));
+}
+
 #if IS_ENABLED(CONFIG_OF)
 static const struct nvkm_device_tegra_func gk20a_platform_data = {
 	.iommu_bit = 34,
@@ -93,4 +98,5 @@ struct platform_driver nouveau_platform_driver = {
 	},
 	.probe = nouveau_platform_probe,
 	.remove_new = nouveau_platform_remove,
+	.shutdown = nouveau_platform_shutdown,
 };
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 3/8] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

(no changes since v1)

 drivers/gpu/drm/tegra/drm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 03d1c76aec2d..d9f0728c3afd 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1330,6 +1330,11 @@ static int host1x_drm_remove(struct host1x_device *dev)
 	return 0;
 }
 
+static void host1x_drm_shutdown(struct host1x_device *dev)
+{
+	drm_atomic_helper_shutdown(dev_get_drvdata(&dev->dev));
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int host1x_drm_suspend(struct device *dev)
 {
@@ -1398,6 +1403,7 @@ static struct host1x_driver host1x_drm_driver = {
 	},
 	.probe = host1x_drm_probe,
 	.remove = host1x_drm_remove,
+	.shutdown = host1x_drm_shutdown,
 	.subdevs = host1x_drm_subdevs,
 };
 
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 4/8] drm/arcpgu: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

(no changes since v1)

 drivers/gpu/drm/tiny/arcpgu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index 4f8f3172379e..85b21f5aac55 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -412,6 +412,11 @@ static void arcpgu_remove(struct platform_device *pdev)
 	arcpgu_unload(drm);
 }
 
+static void arcpgu_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id arcpgu_of_table[] = {
 	{.compatible = "snps,arcpgu"},
 	{}
@@ -422,6 +427,7 @@ MODULE_DEVICE_TABLE(of, arcpgu_of_table);
 static struct platform_driver arcpgu_platform_driver = {
 	.probe = arcpgu_probe,
 	.remove_new = arcpgu_remove,
+	.shutdown = arcpgu_shutdown,
 	.driver = {
 		   .name = "arcpgu",
 		   .of_match_table = arcpgu_of_table,
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 5/8] drm/sprd: Call drm_atomic_helper_shutdown() at remove time

[Douglas Anderson]

Based on grepping through the source code, this driver appears to be
missing a call to drm_atomic_helper_shutdown() at remove time. Let's
add it.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS driver remove comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

While at it, let's also fix it so that if the driver's bind fails or
if a driver gets unbound that the drvdata gets set to NULL. This will
make sure we can't get confused during a later shutdown().

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

While making this patch, I noticed that the bind() function of this
driver is using "devm". That's probably a bug. As per kernel docs [1]
"the lifetime of the aggregate driver does not align with any of the
underlying struct device instances. Therefore devm cannot be used and
all resources acquired or allocated in this callback must be
explicitly released in the unbind callback". Fixing that is outside
the scope of this commit.

[1] https://docs.kernel.org/driver-api/component.html

(no changes since v1)

 drivers/gpu/drm/sprd/sprd_drm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
index a74cd0caf645..d4453430dd1f 100644
--- a/drivers/gpu/drm/sprd/sprd_drm.c
+++ b/drivers/gpu/drm/sprd/sprd_drm.c
@@ -114,6 +114,7 @@ static int sprd_drm_bind(struct device *dev)
 	drm_kms_helper_poll_fini(drm);
 err_unbind_all:
 	component_unbind_all(drm->dev, drm);
+	platform_set_drvdata(pdev, NULL);
 	return ret;
 }
 
@@ -122,10 +123,11 @@ static void sprd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 
 	drm_dev_unregister(drm);
-
 	drm_kms_helper_poll_fini(drm);
+	drm_atomic_helper_shutdown(drm);
 
 	component_unbind_all(drm->dev, drm);
+	dev_set_drvdata(dev, NULL);
 }
 
 static const struct component_master_ops drm_component_ops = {
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 6/8] drm/gma500: Call drm_helper_force_disable_all() at shutdown/remove time

[Douglas Anderson]

Based on grepping through the source code, this driver appears to be
missing a call to drm_atomic_helper_shutdown(), or in this case the
non-atomic equivalent drm_helper_force_disable_all(), at system
shutdown time and at driver remove time. This is important because
drm_helper_force_disable_all() will cause panels to get disabled
cleanly which may be important for their power sequencing. Future
changes will remove any custom powering off in individual panel
drivers so the DRM drivers need to start getting this right.

The fact that we should call drm_atomic_helper_shutdown(), or in this
case the non-atomic equivalent drm_helper_force_disable_all(), in the
case of OS shutdown/restart comes straight out of the kernel doc
"driver instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

(no changes since v1)

 drivers/gpu/drm/gma500/psb_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 8b64f61ffaf9..a5a399bbe8f5 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -20,6 +20,7 @@
 #include <acpi/video.h>
 
 #include <drm/drm.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
@@ -485,6 +486,12 @@ static void psb_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
+	drm_helper_force_disable_all(dev);
+}
+
+static void psb_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_helper_force_disable_all(pci_get_drvdata(pdev));
 }
 
 static DEFINE_RUNTIME_DEV_PM_OPS(psb_pm_ops, gma_power_suspend, gma_power_resume, NULL);
@@ -521,6 +528,7 @@ static struct pci_driver psb_pci_driver = {
 	.id_table = pciidlist,
 	.probe = psb_pci_probe,
 	.remove = psb_pci_remove,
+	.shutdown = psb_pci_shutdown,
 	.driver.pm = &psb_pm_ops,
 };
 
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 7/8] drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove time

[Douglas Anderson]

Based on grepping through the source code, this driver appears to be
missing a call to drm_atomic_helper_shutdown(), or in this case the
non-atomic equivalent drm_helper_force_disable_all(), at system
shutdown time and at driver remove time. This is important because
drm_helper_force_disable_all() will cause panels to get disabled
cleanly which may be important for their power sequencing. Future
changes will remove any custom powering off in individual panel
drivers so the DRM drivers need to start getting this right.

The fact that we should call drm_atomic_helper_shutdown(), or in this
case the non-atomic equivalent drm_helper_force_disable_all(), in the
case of OS shutdown/restart comes straight out of the kernel doc
"driver instance overview" in drm_drv.c.

NOTE: in order to get things inserted in the right place, I had to
replace the old/deprecated drm_put_dev() function with the equivalent
new calls.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Xinhui Pan <Xinhui.Pan@amd.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I honestly have no idea if I got this patch right. The shutdown()
function already had some special case logic for PPC, Loongson, and
VMs and I don't 100% for sure know how this interacts with
those. Everything here is just compile tested.

(no changes since v1)

 drivers/gpu/drm/radeon/radeon_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 7bf08164140e..9ea7f163a731 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -38,6 +38,7 @@
 #include <linux/pci.h>
 
 #include <drm/drm_aperture.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
@@ -330,7 +331,9 @@ radeon_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_put_dev(dev);
+	drm_dev_unregister(dev);
+	drm_helper_force_disable_all(dev);
+	drm_dev_put(dev);
 }
 
 static void
@@ -341,6 +344,8 @@ radeon_pci_shutdown(struct pci_dev *pdev)
 	 */
 	if (radeon_device_is_virtual())
 		radeon_pci_remove(pdev);
+	else
+		drm_helper_force_disable_all(pci_get_drvdata(pdev));
 
 #if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
 	/*
-- 
2.45.2.505.gda0bf45e8d-goog

[PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.

The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Xinhui Pan <Xinhui.Pan@amd.com>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

...and further, I'd say that this patch is more of a plea for help
than a patch I think is actually right. I'm _fairly_ certain that
drm/amdgpu needs this call at shutdown time but the logic is a bit
hard for me to follow. I'd appreciate if anyone who actually knows
what this should look like could illuminate me, or perhaps even just
post a patch themselves!

(no changes since v1)

 drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index f87d53e183c3..c202a1d5ff5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1197,6 +1197,7 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_device *bdev)
 int amdgpu_device_init(struct amdgpu_device *adev,
 		       uint32_t flags);
 void amdgpu_device_fini_hw(struct amdgpu_device *adev);
+void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
 void amdgpu_device_fini_sw(struct amdgpu_device *adev);
 
 int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 861ccff78af9..a8c4b8412e04 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4531,6 +4531,16 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 
 }
 
+void amdgpu_device_shutdown_hw(struct amdgpu_device *adev)
+{
+	if (adev->mode_info.mode_config_initialized) {
+		if (!drm_drv_uses_atomic_modeset(adev_to_drm(adev)))
+			drm_helper_force_disable_all(adev_to_drm(adev));
+		else
+			drm_atomic_helper_shutdown(adev_to_drm(adev));
+	}
+}
+
 void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 {
 	int idx;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index ea14f1c8f430..b34bf9259d5c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2409,6 +2409,8 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct amdgpu_device *adev = drm_to_adev(dev);
 
+	amdgpu_device_shutdown_hw(adev);
+
 	if (amdgpu_ras_intr_triggered())
 		return;
 
-- 
2.45.2.505.gda0bf45e8d-goog

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Alex Deucher]

On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Based on grepping through the source code this driver appears to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown
> time. Among other things, this means that if a panel is in use that it
> won't be cleanly powered off at system shutdown time.
>
> The fact that we should call drm_atomic_helper_shutdown() in the case
> of OS shutdown/restart comes straight out of the kernel doc "driver
> instance overview" in drm_drv.c.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> ...and further, I'd say that this patch is more of a plea for help
> than a patch I think is actually right. I'm _fairly_ certain that
> drm/amdgpu needs this call at shutdown time but the logic is a bit
> hard for me to follow. I'd appreciate if anyone who actually knows
> what this should look like could illuminate me, or perhaps even just
> post a patch themselves!

I'm not sure this patch makes sense or not.  The driver doesn't really
do a formal tear down in its shutdown routine, it just quiesces the
hardware.  What are the actual requirements of the shutdown function?
In the past when we did a full driver tear down in shutdown, it
delayed the shutdown sequence and users complained.

Alex

>
> (no changes since v1)
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 ++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index f87d53e183c3..c202a1d5ff5f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1197,6 +1197,7 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_device *bdev)
>  int amdgpu_device_init(struct amdgpu_device *adev,
>                        uint32_t flags);
>  void amdgpu_device_fini_hw(struct amdgpu_device *adev);
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev);
>
>  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 861ccff78af9..a8c4b8412e04 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4531,6 +4531,16 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
>
>  }
>
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev)
> +{
> +       if (adev->mode_info.mode_config_initialized) {
> +               if (!drm_drv_uses_atomic_modeset(adev_to_drm(adev)))
> +                       drm_helper_force_disable_all(adev_to_drm(adev));
> +               else
> +                       drm_atomic_helper_shutdown(adev_to_drm(adev));
> +       }
> +}
> +
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev)
>  {
>         int idx;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index ea14f1c8f430..b34bf9259d5c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2409,6 +2409,8 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
>         struct drm_device *dev = pci_get_drvdata(pdev);
>         struct amdgpu_device *adev = drm_to_adev(dev);
>
> +       amdgpu_device_shutdown_hw(adev);
> +
>         if (amdgpu_ras_intr_triggered())
>                 return;
>
> --
> 2.45.2.505.gda0bf45e8d-goog
>

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,


On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> >
> > Based on grepping through the source code this driver appears to be
> > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > time. Among other things, this means that if a panel is in use that it
> > won't be cleanly powered off at system shutdown time.
> >
> > The fact that we should call drm_atomic_helper_shutdown() in the case
> > of OS shutdown/restart comes straight out of the kernel doc "driver
> > instance overview" in drm_drv.c.
> >
> > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > Cc: Christian König <christian.koenig@amd.com>
> > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This commit is only compile-time tested.
> >
> > ...and further, I'd say that this patch is more of a plea for help
> > than a patch I think is actually right. I'm _fairly_ certain that
> > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > hard for me to follow. I'd appreciate if anyone who actually knows
> > what this should look like could illuminate me, or perhaps even just
> > post a patch themselves!
>
> I'm not sure this patch makes sense or not.  The driver doesn't really
> do a formal tear down in its shutdown routine, it just quiesces the
> hardware.  What are the actual requirements of the shutdown function?
> In the past when we did a full driver tear down in shutdown, it
> delayed the shutdown sequence and users complained.

The "inspiration" for this patch is to handle panels properly.
Specifically, panels often have several power/enable signals going to
them and often have requirements that these signals are powered off in
the proper order with the proper delays between them. While we can't
always do so when the system crashes / reboots in an uncontrolled way,
panel manufacturers / HW Engineers get upset if we don't power things
off properly during an orderly shutdown/reboot. When panels are
powered off badly it can cause garbage on the screen and, so I've been
told, can even cause long term damage to the panels over time.

In Linux, some panel drivers have tried to ensure a proper poweroff of
the panel by handling the shutdown() call themselves. However, this is
ugly and panel maintainers want panel drivers to stop doing it. We
have removed the code doing this from most panels now [1]. Instead the
assumption is that the DRM modeset drivers should be calling
drm_atomic_helper_shutdown() which will make sure panels get an
orderly shutdown.

For a lot more details, see the cover letter [2] which then contains
links to even more discussions about the topic.

[1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
[2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Alex Deucher]

On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
>
> On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > >
> > > Based on grepping through the source code this driver appears to be
> > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > time. Among other things, this means that if a panel is in use that it
> > > won't be cleanly powered off at system shutdown time.
> > >
> > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > instance overview" in drm_drv.c.
> > >
> > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Christian König <christian.koenig@amd.com>
> > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > ---
> > > This commit is only compile-time tested.
> > >
> > > ...and further, I'd say that this patch is more of a plea for help
> > > than a patch I think is actually right. I'm _fairly_ certain that
> > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > what this should look like could illuminate me, or perhaps even just
> > > post a patch themselves!
> >
> > I'm not sure this patch makes sense or not.  The driver doesn't really
> > do a formal tear down in its shutdown routine, it just quiesces the
> > hardware.  What are the actual requirements of the shutdown function?
> > In the past when we did a full driver tear down in shutdown, it
> > delayed the shutdown sequence and users complained.
>
> The "inspiration" for this patch is to handle panels properly.
> Specifically, panels often have several power/enable signals going to
> them and often have requirements that these signals are powered off in
> the proper order with the proper delays between them. While we can't
> always do so when the system crashes / reboots in an uncontrolled way,
> panel manufacturers / HW Engineers get upset if we don't power things
> off properly during an orderly shutdown/reboot. When panels are
> powered off badly it can cause garbage on the screen and, so I've been
> told, can even cause long term damage to the panels over time.
>
> In Linux, some panel drivers have tried to ensure a proper poweroff of
> the panel by handling the shutdown() call themselves. However, this is
> ugly and panel maintainers want panel drivers to stop doing it. We
> have removed the code doing this from most panels now [1]. Instead the
> assumption is that the DRM modeset drivers should be calling
> drm_atomic_helper_shutdown() which will make sure panels get an
> orderly shutdown.
>
> For a lot more details, see the cover letter [2] which then contains
> links to even more discussions about the topic.
>
> [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org

I don't think it's an issue.  We quiesce the hardware as if we were
about to suspend the system (e.g., S3).  For the display hardware we
call drm_atomic_helper_suspend() as part of that sequence.

Alex

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> >
> > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Based on grepping through the source code this driver appears to be
> > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > time. Among other things, this means that if a panel is in use that it
> > > > won't be cleanly powered off at system shutdown time.
> > > >
> > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > instance overview" in drm_drv.c.
> > > >
> > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > Cc: Christian König <christian.koenig@amd.com>
> > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > ---
> > > > This commit is only compile-time tested.
> > > >
> > > > ...and further, I'd say that this patch is more of a plea for help
> > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > what this should look like could illuminate me, or perhaps even just
> > > > post a patch themselves!
> > >
> > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > do a formal tear down in its shutdown routine, it just quiesces the
> > > hardware.  What are the actual requirements of the shutdown function?
> > > In the past when we did a full driver tear down in shutdown, it
> > > delayed the shutdown sequence and users complained.
> >
> > The "inspiration" for this patch is to handle panels properly.
> > Specifically, panels often have several power/enable signals going to
> > them and often have requirements that these signals are powered off in
> > the proper order with the proper delays between them. While we can't
> > always do so when the system crashes / reboots in an uncontrolled way,
> > panel manufacturers / HW Engineers get upset if we don't power things
> > off properly during an orderly shutdown/reboot. When panels are
> > powered off badly it can cause garbage on the screen and, so I've been
> > told, can even cause long term damage to the panels over time.
> >
> > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > the panel by handling the shutdown() call themselves. However, this is
> > ugly and panel maintainers want panel drivers to stop doing it. We
> > have removed the code doing this from most panels now [1]. Instead the
> > assumption is that the DRM modeset drivers should be calling
> > drm_atomic_helper_shutdown() which will make sure panels get an
> > orderly shutdown.
> >
> > For a lot more details, see the cover letter [2] which then contains
> > links to even more discussions about the topic.
> >
> > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
>
> I don't think it's an issue.  We quiesce the hardware as if we were
> about to suspend the system (e.g., S3).  For the display hardware we
> call drm_atomic_helper_suspend() as part of that sequence.

OK. It's no skin off my teeth and we can drop this patch if you're
convinced it's not needed. From the point of view of someone who has
no experience with this driver it seems weird to me that it would use
drm_atomic_helper_suspend() at shutdown time instead of the documented
drm_atomic_helper_shutdown(), but if it works for everyone then I'm
not gonna complain.

-Doug

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Alex Deucher]

On Tue, Jun 18, 2024 at 7:53 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > >
> > > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > >
> > > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > > >
> > > > > Based on grepping through the source code this driver appears to be
> > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > > time. Among other things, this means that if a panel is in use that it
> > > > > won't be cleanly powered off at system shutdown time.
> > > > >
> > > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > > instance overview" in drm_drv.c.
> > > > >
> > > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > ---
> > > > > This commit is only compile-time tested.
> > > > >
> > > > > ...and further, I'd say that this patch is more of a plea for help
> > > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > > what this should look like could illuminate me, or perhaps even just
> > > > > post a patch themselves!
> > > >
> > > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > > do a formal tear down in its shutdown routine, it just quiesces the
> > > > hardware.  What are the actual requirements of the shutdown function?
> > > > In the past when we did a full driver tear down in shutdown, it
> > > > delayed the shutdown sequence and users complained.
> > >
> > > The "inspiration" for this patch is to handle panels properly.
> > > Specifically, panels often have several power/enable signals going to
> > > them and often have requirements that these signals are powered off in
> > > the proper order with the proper delays between them. While we can't
> > > always do so when the system crashes / reboots in an uncontrolled way,
> > > panel manufacturers / HW Engineers get upset if we don't power things
> > > off properly during an orderly shutdown/reboot. When panels are
> > > powered off badly it can cause garbage on the screen and, so I've been
> > > told, can even cause long term damage to the panels over time.
> > >
> > > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > > the panel by handling the shutdown() call themselves. However, this is
> > > ugly and panel maintainers want panel drivers to stop doing it. We
> > > have removed the code doing this from most panels now [1]. Instead the
> > > assumption is that the DRM modeset drivers should be calling
> > > drm_atomic_helper_shutdown() which will make sure panels get an
> > > orderly shutdown.
> > >
> > > For a lot more details, see the cover letter [2] which then contains
> > > links to even more discussions about the topic.
> > >
> > > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
> >
> > I don't think it's an issue.  We quiesce the hardware as if we were
> > about to suspend the system (e.g., S3).  For the display hardware we
> > call drm_atomic_helper_suspend() as part of that sequence.
>
> OK. It's no skin off my teeth and we can drop this patch if you're
> convinced it's not needed. From the point of view of someone who has
> no experience with this driver it seems weird to me that it would use
> drm_atomic_helper_suspend() at shutdown time instead of the documented
> drm_atomic_helper_shutdown(), but if it works for everyone then I'm
> not gonna complain.

I think the problem is that it is not clear exactly what the
expectations are around the PCI shutdown callback.  The documentation
says:

"Hook into reboot_notifier_list (kernel/sys.c). Intended to stop any
idling DMA operations. Useful for enabling wake-on-lan (NIC) or
changing the power state of a device before reboot. e.g.
drivers/net/e100.c."

We tried a full driver teardown in the shutdown callback and it added
a lot of latency that really wasn't needed since the system was just
going into a reboot or power down.  The best middle ground was to just
leverage our hw level suspend code to quiesce the hardware.  Adding
complexity to call drm_atomic_helper_suspend() vs
drm_atomic_helper_shutdown() doesn't seem worth it since the functions
do pretty much the same thing (both call
drm_atomic_helper_disable_all()).  Maybe it's better to update the
documentation to recommend drm_atomic_helper_suspend() if drivers want
to leverage their suspend code?

Alex

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Alex Deucher]

On Wed, Jun 19, 2024 at 9:50 AM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Tue, Jun 18, 2024 at 7:53 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > >
> > > > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > >
> > > > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > > > >
> > > > > > Based on grepping through the source code this driver appears to be
> > > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > > > time. Among other things, this means that if a panel is in use that it
> > > > > > won't be cleanly powered off at system shutdown time.
> > > > > >
> > > > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > > > instance overview" in drm_drv.c.
> > > > > >
> > > > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > ---
> > > > > > This commit is only compile-time tested.
> > > > > >
> > > > > > ...and further, I'd say that this patch is more of a plea for help
> > > > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > > > what this should look like could illuminate me, or perhaps even just
> > > > > > post a patch themselves!
> > > > >
> > > > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > > > do a formal tear down in its shutdown routine, it just quiesces the
> > > > > hardware.  What are the actual requirements of the shutdown function?
> > > > > In the past when we did a full driver tear down in shutdown, it
> > > > > delayed the shutdown sequence and users complained.
> > > >
> > > > The "inspiration" for this patch is to handle panels properly.
> > > > Specifically, panels often have several power/enable signals going to
> > > > them and often have requirements that these signals are powered off in
> > > > the proper order with the proper delays between them. While we can't
> > > > always do so when the system crashes / reboots in an uncontrolled way,
> > > > panel manufacturers / HW Engineers get upset if we don't power things
> > > > off properly during an orderly shutdown/reboot. When panels are
> > > > powered off badly it can cause garbage on the screen and, so I've been
> > > > told, can even cause long term damage to the panels over time.
> > > >
> > > > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > > > the panel by handling the shutdown() call themselves. However, this is
> > > > ugly and panel maintainers want panel drivers to stop doing it. We
> > > > have removed the code doing this from most panels now [1]. Instead the
> > > > assumption is that the DRM modeset drivers should be calling
> > > > drm_atomic_helper_shutdown() which will make sure panels get an
> > > > orderly shutdown.
> > > >
> > > > For a lot more details, see the cover letter [2] which then contains
> > > > links to even more discussions about the topic.
> > > >
> > > > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > > > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
> > >
> > > I don't think it's an issue.  We quiesce the hardware as if we were
> > > about to suspend the system (e.g., S3).  For the display hardware we
> > > call drm_atomic_helper_suspend() as part of that sequence.
> >
> > OK. It's no skin off my teeth and we can drop this patch if you're
> > convinced it's not needed. From the point of view of someone who has
> > no experience with this driver it seems weird to me that it would use
> > drm_atomic_helper_suspend() at shutdown time instead of the documented
> > drm_atomic_helper_shutdown(), but if it works for everyone then I'm
> > not gonna complain.
>
> I think the problem is that it is not clear exactly what the
> expectations are around the PCI shutdown callback.  The documentation
> says:
>
> "Hook into reboot_notifier_list (kernel/sys.c). Intended to stop any
> idling DMA operations. Useful for enabling wake-on-lan (NIC) or
> changing the power state of a device before reboot. e.g.
> drivers/net/e100.c."

Arguably, there is no requirement to even touch the display hardware
at all.  In theory you could just leave the display hardware as is in
the current state.  The system will either be rebooting or powering
down anyway.

Alex

>
> We tried a full driver teardown in the shutdown callback and it added
> a lot of latency that really wasn't needed since the system was just
> going into a reboot or power down.  The best middle ground was to just
> leverage our hw level suspend code to quiesce the hardware.  Adding
> complexity to call drm_atomic_helper_suspend() vs
> drm_atomic_helper_shutdown() doesn't seem worth it since the functions
> do pretty much the same thing (both call
> drm_atomic_helper_disable_all()).  Maybe it's better to update the
> documentation to recommend drm_atomic_helper_suspend() if drivers want
> to leverage their suspend code?
>
> Alex

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5839 bytes --]

Hi,

On Wed, Jun 19, 2024 at 09:53:12AM GMT, Alex Deucher wrote:
> On Wed, Jun 19, 2024 at 9:50 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > On Tue, Jun 18, 2024 at 7:53 PM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > >
> > > > > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > > >
> > > > > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > > > > >
> > > > > > > Based on grepping through the source code this driver appears to be
> > > > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > > > > time. Among other things, this means that if a panel is in use that it
> > > > > > > won't be cleanly powered off at system shutdown time.
> > > > > > >
> > > > > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > > > > instance overview" in drm_drv.c.
> > > > > > >
> > > > > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > ---
> > > > > > > This commit is only compile-time tested.
> > > > > > >
> > > > > > > ...and further, I'd say that this patch is more of a plea for help
> > > > > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > > > > what this should look like could illuminate me, or perhaps even just
> > > > > > > post a patch themselves!
> > > > > >
> > > > > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > > > > do a formal tear down in its shutdown routine, it just quiesces the
> > > > > > hardware.  What are the actual requirements of the shutdown function?
> > > > > > In the past when we did a full driver tear down in shutdown, it
> > > > > > delayed the shutdown sequence and users complained.
> > > > >
> > > > > The "inspiration" for this patch is to handle panels properly.
> > > > > Specifically, panels often have several power/enable signals going to
> > > > > them and often have requirements that these signals are powered off in
> > > > > the proper order with the proper delays between them. While we can't
> > > > > always do so when the system crashes / reboots in an uncontrolled way,
> > > > > panel manufacturers / HW Engineers get upset if we don't power things
> > > > > off properly during an orderly shutdown/reboot. When panels are
> > > > > powered off badly it can cause garbage on the screen and, so I've been
> > > > > told, can even cause long term damage to the panels over time.
> > > > >
> > > > > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > > > > the panel by handling the shutdown() call themselves. However, this is
> > > > > ugly and panel maintainers want panel drivers to stop doing it. We
> > > > > have removed the code doing this from most panels now [1]. Instead the
> > > > > assumption is that the DRM modeset drivers should be calling
> > > > > drm_atomic_helper_shutdown() which will make sure panels get an
> > > > > orderly shutdown.
> > > > >
> > > > > For a lot more details, see the cover letter [2] which then contains
> > > > > links to even more discussions about the topic.
> > > > >
> > > > > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > > > > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
> > > >
> > > > I don't think it's an issue.  We quiesce the hardware as if we were
> > > > about to suspend the system (e.g., S3).  For the display hardware we
> > > > call drm_atomic_helper_suspend() as part of that sequence.
> > >
> > > OK. It's no skin off my teeth and we can drop this patch if you're
> > > convinced it's not needed. From the point of view of someone who has
> > > no experience with this driver it seems weird to me that it would use
> > > drm_atomic_helper_suspend() at shutdown time instead of the documented
> > > drm_atomic_helper_shutdown(), but if it works for everyone then I'm
> > > not gonna complain.
> >
> > I think the problem is that it is not clear exactly what the
> > expectations are around the PCI shutdown callback.  The documentation
> > says:
> >
> > "Hook into reboot_notifier_list (kernel/sys.c). Intended to stop any
> > idling DMA operations. Useful for enabling wake-on-lan (NIC) or
> > changing the power state of a device before reboot. e.g.
> > drivers/net/e100.c."
> 
> Arguably, there is no requirement to even touch the display hardware
> at all.  In theory you could just leave the display hardware as is in
> the current state.  The system will either be rebooting or powering
> down anyway.

I think it mostly boils down to a cultural mismatch :)

Doug works on panel for ARM systems, where devices need (and need to
handle) much more resources than what's typical on a system with an AMD
GPU.

So, for the kind of hardware Doug usually deals with, we definitely need
the shutdown hook to make sure the regulators, GPIOs, etc. supplying the
panel are properly shutdown.

And panels usually tied to AMD GPUs probably don't need any of that.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Alex Deucher]

On Thu, Jun 20, 2024 at 3:10 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> Hi,
>
> On Wed, Jun 19, 2024 at 09:53:12AM GMT, Alex Deucher wrote:
> > On Wed, Jun 19, 2024 at 9:50 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > >
> > > On Tue, Jun 18, 2024 at 7:53 PM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > >
> > > > > On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > >
> > > > > > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > > > >
> > > > > > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > > > > > >
> > > > > > > > Based on grepping through the source code this driver appears to be
> > > > > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > > > > > time. Among other things, this means that if a panel is in use that it
> > > > > > > > won't be cleanly powered off at system shutdown time.
> > > > > > > >
> > > > > > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > > > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > > > > > instance overview" in drm_drv.c.
> > > > > > > >
> > > > > > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > > ---
> > > > > > > > This commit is only compile-time tested.
> > > > > > > >
> > > > > > > > ...and further, I'd say that this patch is more of a plea for help
> > > > > > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > > > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > > > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > > > > > what this should look like could illuminate me, or perhaps even just
> > > > > > > > post a patch themselves!
> > > > > > >
> > > > > > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > > > > > do a formal tear down in its shutdown routine, it just quiesces the
> > > > > > > hardware.  What are the actual requirements of the shutdown function?
> > > > > > > In the past when we did a full driver tear down in shutdown, it
> > > > > > > delayed the shutdown sequence and users complained.
> > > > > >
> > > > > > The "inspiration" for this patch is to handle panels properly.
> > > > > > Specifically, panels often have several power/enable signals going to
> > > > > > them and often have requirements that these signals are powered off in
> > > > > > the proper order with the proper delays between them. While we can't
> > > > > > always do so when the system crashes / reboots in an uncontrolled way,
> > > > > > panel manufacturers / HW Engineers get upset if we don't power things
> > > > > > off properly during an orderly shutdown/reboot. When panels are
> > > > > > powered off badly it can cause garbage on the screen and, so I've been
> > > > > > told, can even cause long term damage to the panels over time.
> > > > > >
> > > > > > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > > > > > the panel by handling the shutdown() call themselves. However, this is
> > > > > > ugly and panel maintainers want panel drivers to stop doing it. We
> > > > > > have removed the code doing this from most panels now [1]. Instead the
> > > > > > assumption is that the DRM modeset drivers should be calling
> > > > > > drm_atomic_helper_shutdown() which will make sure panels get an
> > > > > > orderly shutdown.
> > > > > >
> > > > > > For a lot more details, see the cover letter [2] which then contains
> > > > > > links to even more discussions about the topic.
> > > > > >
> > > > > > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > > > > > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
> > > > >
> > > > > I don't think it's an issue.  We quiesce the hardware as if we were
> > > > > about to suspend the system (e.g., S3).  For the display hardware we
> > > > > call drm_atomic_helper_suspend() as part of that sequence.
> > > >
> > > > OK. It's no skin off my teeth and we can drop this patch if you're
> > > > convinced it's not needed. From the point of view of someone who has
> > > > no experience with this driver it seems weird to me that it would use
> > > > drm_atomic_helper_suspend() at shutdown time instead of the documented
> > > > drm_atomic_helper_shutdown(), but if it works for everyone then I'm
> > > > not gonna complain.
> > >
> > > I think the problem is that it is not clear exactly what the
> > > expectations are around the PCI shutdown callback.  The documentation
> > > says:
> > >
> > > "Hook into reboot_notifier_list (kernel/sys.c). Intended to stop any
> > > idling DMA operations. Useful for enabling wake-on-lan (NIC) or
> > > changing the power state of a device before reboot. e.g.
> > > drivers/net/e100.c."
> >
> > Arguably, there is no requirement to even touch the display hardware
> > at all.  In theory you could just leave the display hardware as is in
> > the current state.  The system will either be rebooting or powering
> > down anyway.
>
> I think it mostly boils down to a cultural mismatch :)
>
> Doug works on panel for ARM systems, where devices need (and need to
> handle) much more resources than what's typical on a system with an AMD
> GPU.
>
> So, for the kind of hardware Doug usually deals with, we definitely need
> the shutdown hook to make sure the regulators, GPIOs, etc. supplying the
> panel are properly shutdown.
>
> And panels usually tied to AMD GPUs probably don't need any of that.

Makes sense.  I think drm_atomic_helper_suspend() is a viable
alternative if drivers want to leverage their existing suspend code.
I could write up a doc patch unless there is reason to prefer the
shutdown variant.

Alex

Re: [PATCH v2 8/8] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6843 bytes --]

On Thu, Jun 20, 2024 at 09:00:23AM GMT, Alex Deucher wrote:
> On Thu, Jun 20, 2024 at 3:10 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > Hi,
> >
> > On Wed, Jun 19, 2024 at 09:53:12AM GMT, Alex Deucher wrote:
> > > On Wed, Jun 19, 2024 at 9:50 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > >
> > > > On Tue, Jun 18, 2024 at 7:53 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > On Tue, Jun 18, 2024 at 3:00 PM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Jun 18, 2024 at 5:40 PM Doug Anderson <dianders@chromium.org> wrote:
> > > > > > >
> > > > > > > Hi,
> > > > > > >
> > > > > > >
> > > > > > > On Mon, Jun 17, 2024 at 8:01 AM Alex Deucher <alexdeucher@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Wed, Jun 12, 2024 at 6:37 PM Douglas Anderson <dianders@chromium.org> wrote:
> > > > > > > > >
> > > > > > > > > Based on grepping through the source code this driver appears to be
> > > > > > > > > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > > > > > > > > time. Among other things, this means that if a panel is in use that it
> > > > > > > > > won't be cleanly powered off at system shutdown time.
> > > > > > > > >
> > > > > > > > > The fact that we should call drm_atomic_helper_shutdown() in the case
> > > > > > > > > of OS shutdown/restart comes straight out of the kernel doc "driver
> > > > > > > > > instance overview" in drm_drv.c.
> > > > > > > > >
> > > > > > > > > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > > > > > > > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > > > > > > > Cc: Christian König <christian.koenig@amd.com>
> > > > > > > > > Cc: Xinhui Pan <Xinhui.Pan@amd.com>
> > > > > > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > > > > > > > > ---
> > > > > > > > > This commit is only compile-time tested.
> > > > > > > > >
> > > > > > > > > ...and further, I'd say that this patch is more of a plea for help
> > > > > > > > > than a patch I think is actually right. I'm _fairly_ certain that
> > > > > > > > > drm/amdgpu needs this call at shutdown time but the logic is a bit
> > > > > > > > > hard for me to follow. I'd appreciate if anyone who actually knows
> > > > > > > > > what this should look like could illuminate me, or perhaps even just
> > > > > > > > > post a patch themselves!
> > > > > > > >
> > > > > > > > I'm not sure this patch makes sense or not.  The driver doesn't really
> > > > > > > > do a formal tear down in its shutdown routine, it just quiesces the
> > > > > > > > hardware.  What are the actual requirements of the shutdown function?
> > > > > > > > In the past when we did a full driver tear down in shutdown, it
> > > > > > > > delayed the shutdown sequence and users complained.
> > > > > > >
> > > > > > > The "inspiration" for this patch is to handle panels properly.
> > > > > > > Specifically, panels often have several power/enable signals going to
> > > > > > > them and often have requirements that these signals are powered off in
> > > > > > > the proper order with the proper delays between them. While we can't
> > > > > > > always do so when the system crashes / reboots in an uncontrolled way,
> > > > > > > panel manufacturers / HW Engineers get upset if we don't power things
> > > > > > > off properly during an orderly shutdown/reboot. When panels are
> > > > > > > powered off badly it can cause garbage on the screen and, so I've been
> > > > > > > told, can even cause long term damage to the panels over time.
> > > > > > >
> > > > > > > In Linux, some panel drivers have tried to ensure a proper poweroff of
> > > > > > > the panel by handling the shutdown() call themselves. However, this is
> > > > > > > ugly and panel maintainers want panel drivers to stop doing it. We
> > > > > > > have removed the code doing this from most panels now [1]. Instead the
> > > > > > > assumption is that the DRM modeset drivers should be calling
> > > > > > > drm_atomic_helper_shutdown() which will make sure panels get an
> > > > > > > orderly shutdown.
> > > > > > >
> > > > > > > For a lot more details, see the cover letter [2] which then contains
> > > > > > > links to even more discussions about the topic.
> > > > > > >
> > > > > > > [1] https://lore.kernel.org/r/20240605002401.2848541-1-dianders@chromium.org
> > > > > > > [2] https://lore.kernel.org/r/20240612222435.3188234-1-dianders@chromium.org
> > > > > >
> > > > > > I don't think it's an issue.  We quiesce the hardware as if we were
> > > > > > about to suspend the system (e.g., S3).  For the display hardware we
> > > > > > call drm_atomic_helper_suspend() as part of that sequence.
> > > > >
> > > > > OK. It's no skin off my teeth and we can drop this patch if you're
> > > > > convinced it's not needed. From the point of view of someone who has
> > > > > no experience with this driver it seems weird to me that it would use
> > > > > drm_atomic_helper_suspend() at shutdown time instead of the documented
> > > > > drm_atomic_helper_shutdown(), but if it works for everyone then I'm
> > > > > not gonna complain.
> > > >
> > > > I think the problem is that it is not clear exactly what the
> > > > expectations are around the PCI shutdown callback.  The documentation
> > > > says:
> > > >
> > > > "Hook into reboot_notifier_list (kernel/sys.c). Intended to stop any
> > > > idling DMA operations. Useful for enabling wake-on-lan (NIC) or
> > > > changing the power state of a device before reboot. e.g.
> > > > drivers/net/e100.c."
> > >
> > > Arguably, there is no requirement to even touch the display hardware
> > > at all.  In theory you could just leave the display hardware as is in
> > > the current state.  The system will either be rebooting or powering
> > > down anyway.
> >
> > I think it mostly boils down to a cultural mismatch :)
> >
> > Doug works on panel for ARM systems, where devices need (and need to
> > handle) much more resources than what's typical on a system with an AMD
> > GPU.
> >
> > So, for the kind of hardware Doug usually deals with, we definitely need
> > the shutdown hook to make sure the regulators, GPIOs, etc. supplying the
> > panel are properly shutdown.
> >
> > And panels usually tied to AMD GPUs probably don't need any of that.
> 
> Makes sense.  I think drm_atomic_helper_suspend() is a viable
> alternative if drivers want to leverage their existing suspend code.
> I could write up a doc patch unless there is reason to prefer the
> shutdown variant.

It could be worth adding that mention in the _shutdown() doc indeed, but
I'm not sure we need to enforce anything at this point. I'd rather have
either, at the driver's authors discretion.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 3/8] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 892 bytes --]

On Thu Jun 13, 2024 at 12:23 AM CEST, Douglas Anderson wrote:
> Based on grepping through the source code this driver appears to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown
> time. Among other things, this means that if a panel is in use that it
> won't be cleanly powered off at system shutdown time.
>
> The fact that we should call drm_atomic_helper_shutdown() in the case
> of OS shutdown/restart comes straight out of the kernel doc "driver
> instance overview" in drm_drv.c.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> (no changes since v1)
>
>  drivers/gpu/drm/tegra/drm.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/8] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Wed, Jun 26, 2024 at 11:58 PM Thierry Reding
<thierry.reding@gmail.com> wrote:
>
> On Thu Jun 13, 2024 at 12:23 AM CEST, Douglas Anderson wrote:
> > Based on grepping through the source code this driver appears to be
> > missing a call to drm_atomic_helper_shutdown() at system shutdown
> > time. Among other things, this means that if a panel is in use that it
> > won't be cleanly powered off at system shutdown time.
> >
> > The fact that we should call drm_atomic_helper_shutdown() in the case
> > of OS shutdown/restart comes straight out of the kernel doc "driver
> > instance overview" in drm_drv.c.
> >
> > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This commit is only compile-time tested.
> >
> > (no changes since v1)
> >
> >  drivers/gpu/drm/tegra/drm.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
>
> Acked-by: Thierry Reding <treding@nvidia.com>

Pushed this one to drm-misc-next:

[3/8] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time
      commit: bc5846d3d3dff9f057e2897a736b51584785da30