[RFT PATCH 0/6] drm: drm-misc drivers call drm_atomic_helper_shutdown() at the right times

[RFT PATCH 0/6] drm: drm-misc drivers call drm_atomic_helper_shutdown() at the right times

[Douglas Anderson]

NOTE: in order to avoid email sending limits on the cover letter, I've
split this patch series in two. Patches that target drm-misc and ones
that don't. The cover letter of the two is identical other than this note.

This patch series came about after a _long_ discussion between me and
Maxime Ripard in response to a different patch I sent out [1]. 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!

I've organized this series like this:
1. One patch for all simple cases of just needing a call at shutdown
   time for drivers that go through drm-misc.
2. A separate patch for "drm/vc4", even though it goes through
   drm-misc, since I wanted to leave an extra note for that one.
3. Patches for drivers that just needed a call at shutdown time for
   drivers that _don't_ go through drm-misc.
4. Patches for the few drivers that had the call at shutdown time but
   lacked it at remove time.
5. One patch for all simple cases of needing a call at shutdown and
   remove (or unbind) time for drivers that go through drm-misc.
6. A separate patch for "drm/hisilicon/kirin", even though it goes
   through drm-misc, since I wanted to leave an extra note for that
   one.
7. Patches for drivers that needed a call at shutdown and remove (or
   unbind) time for drivers that _don't_ go through drm-misc.

I've labeled this patch series as RFT (request for testing) to help
call attention to the fact that I didn't personally test any of these
patches.

If you're a maintainer of one of these drivers and you think that the
patch for your driver looks fabulous, you've tested it, and you'd like
to land it right away then please do. For non-drm-misc drivers there
are no dependencies here. Some of the drm-misc drivers depend on the
first patch, AKA ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL)
should be a noop"). I've tried to call this out but it also should be
obvious once you know to look for it.

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 driers 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/lkml/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid


Douglas Anderson (6):
  drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop
  drm: Call drm_atomic_helper_shutdown() at shutdown time for misc
    drivers
  drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time
  drm/ssd130x: Call drm_atomic_helper_shutdown() at remove time
  drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for
    misc drivers
  drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at
    shutdown/unbind time

 .../gpu/drm/arm/display/komeda/komeda_drv.c   |  9 +++++
 .../gpu/drm/arm/display/komeda/komeda_kms.c   |  7 ++++
 .../gpu/drm/arm/display/komeda/komeda_kms.h   |  1 +
 drivers/gpu/drm/arm/hdlcd_drv.c               |  6 ++++
 drivers/gpu/drm/arm/malidp_drv.c              |  6 ++++
 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c       |  7 ++++
 drivers/gpu/drm/ast/ast_drv.c                 |  6 ++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  6 ++++
 drivers/gpu/drm/drm_atomic_helper.c           |  3 ++
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c     |  8 +++++
 .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  6 ++++
 .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c   |  9 +++++
 drivers/gpu/drm/hyperv/hyperv_drm_drv.c       |  6 ++++
 drivers/gpu/drm/logicvc/logicvc_drm.c         |  9 +++++
 drivers/gpu/drm/loongson/lsdc_drv.c           |  6 ++++
 drivers/gpu/drm/mcde/mcde_drv.c               |  9 +++++
 drivers/gpu/drm/mgag200/mgag200_drv.c         |  8 +++++
 drivers/gpu/drm/omapdrm/omap_drv.c            |  8 +++++
 drivers/gpu/drm/pl111/pl111_drv.c             |  7 ++++
 drivers/gpu/drm/qxl/qxl_drv.c                 |  7 ++++
 drivers/gpu/drm/solomon/ssd130x.c             |  1 +
 drivers/gpu/drm/sti/sti_drv.c                 |  7 ++++
 drivers/gpu/drm/stm/drv.c                     |  7 ++++
 drivers/gpu/drm/sun4i/sun4i_drv.c             |  6 ++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c           | 11 +++++-
 drivers/gpu/drm/tiny/bochs.c                  |  6 ++++
 drivers/gpu/drm/tiny/cirrus.c                 |  6 ++++
 drivers/gpu/drm/tve200/tve200_drv.c           |  7 ++++
 drivers/gpu/drm/vboxvideo/vbox_drv.c          | 10 ++++++
 drivers/gpu/drm/vc4/vc4_drv.c                 | 36 ++++++++++++-------
 30 files changed, 217 insertions(+), 14 deletions(-)

-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 1/6] drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop

[Douglas Anderson]

As with other places in the Linux kernel--kfree(NULL) being the most
famous example--it's convenient to treat being passed a NULL argument
as a noop in cleanup functions. Let's make
drm_atomic_helper_shutdown() work like this.

This is convenient for DRM devices that use the "component" model. On
these devices we want shutdown to be a noop if the bind() call of the
component hasn't been called yet. As long as drivers are careful to
make sure the drvdata is NULL whenever the driver is not bound then we
can just do a simple call to drm_atomic_helper_shutdown() with the
drvdata at shutdown time.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/drm_atomic_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 292e38eb6218..71d399397107 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3339,6 +3339,9 @@ void drm_atomic_helper_shutdown(struct drm_device *dev)
 	struct drm_modeset_acquire_ctx ctx;
 	int ret;
 
+	if (dev == NULL)
+		return;
+
 	DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret);
 
 	ret = drm_atomic_helper_disable_all(dev, &ctx);
-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Douglas Anderson]

Based on grepping through the source code these drivers appear 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.

All of the drivers in this patch were fairly straightforward to fix
since they already had a call to drm_atomic_helper_shutdown() at
remove/unbind time but were just lacking one at system shutdown. The
only hitch is that some of these drivers use the component model to
register/unregister their DRM devices. The shutdown callback is part
of the original device. The typical solution here, based on how other
DRM drivers do this, is to keep track of whether the device is bound
based on drvdata. In most cases the drvdata is the drm_device, so we
can just make sure it is NULL when the device is not bound. In some
drivers, this required minor code changes. To make things simpler,
drm_atomic_helper_shutdown() has been modified to consider a NULL
drm_device as a noop in the patch ("drm/atomic-helper:
drm_atomic_helper_shutdown(NULL) should be a noop").

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

Note that checkpatch yells that "drivers/gpu/drm/tiny/cirrus.c" is
marked as 'obsolete', but it seems silly not to include the fix if
it's already been written. If someone wants me to take that out,
though, I can.

 drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 9 +++++++++
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 7 +++++++
 drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 1 +
 drivers/gpu/drm/arm/hdlcd_drv.c                 | 6 ++++++
 drivers/gpu/drm/arm/malidp_drv.c                | 6 ++++++
 drivers/gpu/drm/ast/ast_drv.c                   | 6 ++++++
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 6 ++++++
 drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c       | 8 ++++++++
 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++++++
 drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 6 ++++++
 drivers/gpu/drm/logicvc/logicvc_drm.c           | 9 +++++++++
 drivers/gpu/drm/loongson/lsdc_drv.c             | 6 ++++++
 drivers/gpu/drm/mcde/mcde_drv.c                 | 9 +++++++++
 drivers/gpu/drm/omapdrm/omap_drv.c              | 8 ++++++++
 drivers/gpu/drm/qxl/qxl_drv.c                   | 7 +++++++
 drivers/gpu/drm/sti/sti_drv.c                   | 7 +++++++
 drivers/gpu/drm/sun4i/sun4i_drv.c               | 6 ++++++
 drivers/gpu/drm/tiny/bochs.c                    | 6 ++++++
 drivers/gpu/drm/tiny/cirrus.c                   | 6 ++++++
 19 files changed, 125 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
index cb2a2be24c5f..cc57ea4e13ae 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_drv.c
@@ -45,6 +45,14 @@ static void komeda_platform_remove(struct platform_device *pdev)
 	devm_kfree(dev, mdrv);
 }
 
+static void komeda_platform_shutdown(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct komeda_drv *mdrv = dev_get_drvdata(dev);
+
+	komeda_kms_shutdown(mdrv->kms);
+}
+
 static int komeda_platform_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
@@ -142,6 +150,7 @@ static const struct dev_pm_ops komeda_pm_ops = {
 static struct platform_driver komeda_platform_driver = {
 	.probe	= komeda_platform_probe,
 	.remove_new = komeda_platform_remove,
+	.shutdown = komeda_platform_shutdown,
 	.driver	= {
 		.name = "komeda",
 		.of_match_table	= komeda_of_match,
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 9299026701f3..fe46b0ebefea 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -340,3 +340,10 @@ void komeda_kms_detach(struct komeda_kms_dev *kms)
 	komeda_kms_cleanup_private_objs(kms);
 	drm->dev_private = NULL;
 }
+
+void komeda_kms_shutdown(struct komeda_kms_dev *kms)
+{
+	struct drm_device *drm = &kms->base;
+
+	drm_atomic_helper_shutdown(drm);
+}
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
index 6ef655326357..a4048724564d 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.h
@@ -190,5 +190,6 @@ void komeda_crtc_flush_and_wait_for_flip_done(struct komeda_crtc *kcrtc,
 
 struct komeda_kms_dev *komeda_kms_attach(struct komeda_dev *mdev);
 void komeda_kms_detach(struct komeda_kms_dev *kms);
+void komeda_kms_shutdown(struct komeda_kms_dev *kms);
 
 #endif /*_KOMEDA_KMS_H_*/
diff --git a/drivers/gpu/drm/arm/hdlcd_drv.c b/drivers/gpu/drm/arm/hdlcd_drv.c
index aa06f9838015..32be9e370049 100644
--- a/drivers/gpu/drm/arm/hdlcd_drv.c
+++ b/drivers/gpu/drm/arm/hdlcd_drv.c
@@ -372,6 +372,11 @@ static void hdlcd_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &hdlcd_master_ops);
 }
 
+static void hdlcd_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id  hdlcd_of_match[] = {
 	{ .compatible	= "arm,hdlcd" },
 	{},
@@ -399,6 +404,7 @@ static SIMPLE_DEV_PM_OPS(hdlcd_pm_ops, hdlcd_pm_suspend, hdlcd_pm_resume);
 static struct platform_driver hdlcd_platform_driver = {
 	.probe		= hdlcd_probe,
 	.remove_new	= hdlcd_remove,
+	.shutdown	= hdlcd_shutdown,
 	.driver	= {
 		.name = "hdlcd",
 		.pm = &hdlcd_pm_ops,
diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index 62329d5dd992..6682131d2910 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -941,6 +941,11 @@ static void malidp_platform_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &malidp_master_ops);
 }
 
+static void malidp_platform_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static int __maybe_unused malidp_pm_suspend(struct device *dev)
 {
 	struct drm_device *drm = dev_get_drvdata(dev);
@@ -982,6 +987,7 @@ static const struct dev_pm_ops malidp_pm_ops = {
 static struct platform_driver malidp_platform_driver = {
 	.probe		= malidp_platform_probe,
 	.remove_new	= malidp_platform_remove,
+	.shutdown	= malidp_platform_shutdown,
 	.driver	= {
 		.name = "mali-dp",
 		.pm = &malidp_pm_ops,
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index e1224ef4ad83..cf5b754f044c 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -125,6 +125,11 @@ static void ast_pci_remove(struct pci_dev *pdev)
 	drm_atomic_helper_shutdown(dev);
 }
 
+static void ast_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 static int ast_drm_freeze(struct drm_device *dev)
 {
 	int error;
@@ -209,6 +214,7 @@ static struct pci_driver ast_pci_driver = {
 	.id_table = ast_pciidlist,
 	.probe = ast_pci_probe,
 	.remove = ast_pci_remove,
+	.shutdown = ast_pci_shutdown,
 	.driver.pm = &ast_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index fa0f9a93d50d..84c54e8622d1 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -782,6 +782,11 @@ static void atmel_hlcdc_dc_drm_remove(struct platform_device *pdev)
 	drm_dev_put(ddev);
 }
 
+static void atmel_hlcdc_dc_drm_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static int atmel_hlcdc_dc_drm_suspend(struct device *dev)
 {
 	struct drm_device *drm_dev = dev_get_drvdata(dev);
@@ -825,6 +830,7 @@ static const struct of_device_id atmel_hlcdc_dc_of_match[] = {
 static struct platform_driver atmel_hlcdc_dc_platform_driver = {
 	.probe	= atmel_hlcdc_dc_drm_probe,
 	.remove_new = atmel_hlcdc_dc_drm_remove,
+	.shutdown = atmel_hlcdc_dc_drm_shutdown,
 	.driver	= {
 		.name	= "atmel-hlcdc-display-controller",
 		.pm	= pm_sleep_ptr(&atmel_hlcdc_dc_drm_pm_ops),
diff --git a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
index a395f93449f3..ab6c0c6cd0e2 100644
--- a/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
+++ b/drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c
@@ -356,9 +356,17 @@ static void fsl_dcu_drm_remove(struct platform_device *pdev)
 	clk_unregister(fsl_dev->pix_clk);
 }
 
+static void fsl_dcu_drm_shutdown(struct platform_device *pdev)
+{
+	struct fsl_dcu_drm_device *fsl_dev = platform_get_drvdata(pdev);
+
+	drm_atomic_helper_shutdown(fsl_dev->drm);
+}
+
 static struct platform_driver fsl_dcu_drm_platform_driver = {
 	.probe		= fsl_dcu_drm_probe,
 	.remove_new	= fsl_dcu_drm_remove,
+	.shutdown	= fsl_dcu_drm_shutdown,
 	.driver		= {
 		.name	= "fsl-dcu",
 		.pm	= &fsl_dcu_drm_pm_ops,
diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
index 8a98fa276e8a..57c21ec452b7 100644
--- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c
@@ -357,6 +357,11 @@ static void hibmc_pci_remove(struct pci_dev *pdev)
 	hibmc_unload(dev);
 }
 
+static void hibmc_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 static const struct pci_device_id hibmc_pci_table[] = {
 	{ PCI_VDEVICE(HUAWEI, 0x1711) },
 	{0,}
@@ -367,6 +372,7 @@ static struct pci_driver hibmc_pci_driver = {
 	.id_table =	hibmc_pci_table,
 	.probe =	hibmc_pci_probe,
 	.remove =	hibmc_pci_remove,
+	.shutdown =	hibmc_pci_shutdown,
 	.driver.pm =    &hibmc_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
index 8026118c6e03..58b0b46a21e6 100644
--- a/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
+++ b/drivers/gpu/drm/hyperv/hyperv_drm_drv.c
@@ -178,6 +178,11 @@ static void hyperv_vmbus_remove(struct hv_device *hdev)
 	vmbus_free_mmio(hv->mem->start, hv->fb_size);
 }
 
+static void hyperv_vmbus_shutdown(struct hv_device *hdev)
+{
+	drm_atomic_helper_shutdown(hv_get_drvdata(hdev));
+}
+
 static int hyperv_vmbus_suspend(struct hv_device *hdev)
 {
 	struct drm_device *dev = hv_get_drvdata(hdev);
@@ -220,6 +225,7 @@ static struct hv_driver hyperv_hv_driver = {
 	.id_table = hyperv_vmbus_tbl,
 	.probe = hyperv_vmbus_probe,
 	.remove = hyperv_vmbus_remove,
+	.shutdown = hyperv_vmbus_shutdown,
 	.suspend = hyperv_vmbus_suspend,
 	.resume = hyperv_vmbus_resume,
 	.driver = {
diff --git a/drivers/gpu/drm/logicvc/logicvc_drm.c b/drivers/gpu/drm/logicvc/logicvc_drm.c
index 749debd3d6a5..01a37e28c080 100644
--- a/drivers/gpu/drm/logicvc/logicvc_drm.c
+++ b/drivers/gpu/drm/logicvc/logicvc_drm.c
@@ -482,6 +482,14 @@ static void logicvc_drm_remove(struct platform_device *pdev)
 	of_reserved_mem_device_release(dev);
 }
 
+static void logicvc_drm_shutdown(struct platform_device *pdev)
+{
+	struct logicvc_drm *logicvc = platform_get_drvdata(pdev);
+	struct drm_device *drm_dev = &logicvc->drm_dev;
+
+	drm_atomic_helper_shutdown(drm_dev);
+}
+
 static const struct of_device_id logicvc_drm_of_table[] = {
 	{ .compatible = "xylon,logicvc-3.02.a-display" },
 	{ .compatible = "xylon,logicvc-4.01.a-display" },
@@ -492,6 +500,7 @@ MODULE_DEVICE_TABLE(of, logicvc_drm_of_table);
 static struct platform_driver logicvc_drm_platform_driver = {
 	.probe		= logicvc_drm_probe,
 	.remove_new	= logicvc_drm_remove,
+	.shutdown	= logicvc_drm_shutdown,
 	.driver		= {
 		.name		= "logicvc-drm",
 		.of_match_table	= logicvc_drm_of_table,
diff --git a/drivers/gpu/drm/loongson/lsdc_drv.c b/drivers/gpu/drm/loongson/lsdc_drv.c
index 188ec82afcfb..89ccc0c43169 100644
--- a/drivers/gpu/drm/loongson/lsdc_drv.c
+++ b/drivers/gpu/drm/loongson/lsdc_drv.c
@@ -327,6 +327,11 @@ static void lsdc_pci_remove(struct pci_dev *pdev)
 	drm_atomic_helper_shutdown(ddev);
 }
 
+static void lsdc_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 static int lsdc_drm_freeze(struct drm_device *ddev)
 {
 	struct lsdc_device *ldev = to_lsdc(ddev);
@@ -447,6 +452,7 @@ struct pci_driver lsdc_pci_driver = {
 	.id_table = lsdc_pciid_list,
 	.probe = lsdc_pci_probe,
 	.remove = lsdc_pci_remove,
+	.shutdown = lsdc_pci_shutdown,
 	.driver.pm = &lsdc_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/mcde/mcde_drv.c b/drivers/gpu/drm/mcde/mcde_drv.c
index a2572fb311f0..10c06440c7e7 100644
--- a/drivers/gpu/drm/mcde/mcde_drv.c
+++ b/drivers/gpu/drm/mcde/mcde_drv.c
@@ -459,6 +459,14 @@ static void mcde_remove(struct platform_device *pdev)
 	regulator_disable(mcde->epod);
 }
 
+static void mcde_shutdown(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	if (drm->registered)
+		drm_atomic_helper_shutdown(drm);
+}
+
 static const struct of_device_id mcde_of_match[] = {
 	{
 		.compatible = "ste,mcde",
@@ -473,6 +481,7 @@ static struct platform_driver mcde_driver = {
 	},
 	.probe = mcde_probe,
 	.remove_new = mcde_remove,
+	.shutdown = mcde_shutdown,
 };
 
 static struct platform_driver *const component_drivers[] = {
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index afeeb7737552..b2835b3ea6f5 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -817,6 +817,13 @@ static void pdev_remove(struct platform_device *pdev)
 	kfree(priv);
 }
 
+static void pdev_shutdown(struct platform_device *pdev)
+{
+	struct omap_drm_private *priv = platform_get_drvdata(pdev);
+
+	drm_atomic_helper_shutdown(priv->ddev);
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int omap_drm_suspend(struct device *dev)
 {
@@ -846,6 +853,7 @@ static struct platform_driver pdev = {
 	},
 	.probe = pdev_probe,
 	.remove_new = pdev_remove,
+	.shutdown = pdev_shutdown,
 };
 
 static struct platform_driver * const drivers[] = {
diff --git a/drivers/gpu/drm/qxl/qxl_drv.c b/drivers/gpu/drm/qxl/qxl_drv.c
index b30ede1cf62d..a4144c62ca93 100644
--- a/drivers/gpu/drm/qxl/qxl_drv.c
+++ b/drivers/gpu/drm/qxl/qxl_drv.c
@@ -163,6 +163,12 @@ qxl_pci_remove(struct pci_dev *pdev)
 		vga_put(pdev, VGA_RSRC_LEGACY_IO);
 }
 
+static void
+qxl_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 DEFINE_DRM_GEM_FOPS(qxl_fops);
 
 static int qxl_drm_freeze(struct drm_device *dev)
@@ -269,6 +275,7 @@ static struct pci_driver qxl_pci_driver = {
 	 .id_table = pciidlist,
 	 .probe = qxl_pci_probe,
 	 .remove = qxl_pci_remove,
+	 .shutdown = qxl_pci_shutdown,
 	 .driver.pm = &qxl_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/sti/sti_drv.c b/drivers/gpu/drm/sti/sti_drv.c
index 2390c1bb6596..4bab93c4fefd 100644
--- a/drivers/gpu/drm/sti/sti_drv.c
+++ b/drivers/gpu/drm/sti/sti_drv.c
@@ -174,6 +174,7 @@ static void sti_cleanup(struct drm_device *ddev)
 	drm_atomic_helper_shutdown(ddev);
 	drm_mode_config_cleanup(ddev);
 	component_unbind_all(ddev->dev, ddev);
+	dev_set_drvdata(ddev->dev, NULL);
 	kfree(private);
 	ddev->dev_private = NULL;
 }
@@ -253,6 +254,11 @@ static void sti_platform_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &sti_ops);
 }
 
+static void sti_platform_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id sti_dt_ids[] = {
 	{ .compatible = "st,sti-display-subsystem", },
 	{ /* end node */ },
@@ -262,6 +268,7 @@ MODULE_DEVICE_TABLE(of, sti_dt_ids);
 static struct platform_driver sti_platform_driver = {
 	.probe = sti_platform_probe,
 	.remove_new = sti_platform_remove,
+	.shutdown = sti_platform_shutdown,
 	.driver = {
 		.name = DRIVER_NAME,
 		.of_match_table = sti_dt_ids,
diff --git a/drivers/gpu/drm/sun4i/sun4i_drv.c b/drivers/gpu/drm/sun4i/sun4i_drv.c
index 6a8dfc022d3c..35d7a7ffd208 100644
--- a/drivers/gpu/drm/sun4i/sun4i_drv.c
+++ b/drivers/gpu/drm/sun4i/sun4i_drv.c
@@ -413,6 +413,11 @@ static void sun4i_drv_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &sun4i_drv_master_ops);
 }
 
+static void sun4i_drv_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id sun4i_drv_of_table[] = {
 	{ .compatible = "allwinner,sun4i-a10-display-engine" },
 	{ .compatible = "allwinner,sun5i-a10s-display-engine" },
@@ -437,6 +442,7 @@ MODULE_DEVICE_TABLE(of, sun4i_drv_of_table);
 static struct platform_driver sun4i_drv_platform_driver = {
 	.probe		= sun4i_drv_probe,
 	.remove_new	= sun4i_drv_remove,
+	.shutdown	= sun4i_drv_shutdown,
 	.driver		= {
 		.name		= "sun4i-drm",
 		.of_match_table	= sun4i_drv_of_table,
diff --git a/drivers/gpu/drm/tiny/bochs.c b/drivers/gpu/drm/tiny/bochs.c
index d254679a136e..c23c9f0cf49c 100644
--- a/drivers/gpu/drm/tiny/bochs.c
+++ b/drivers/gpu/drm/tiny/bochs.c
@@ -690,6 +690,11 @@ static void bochs_pci_remove(struct pci_dev *pdev)
 	drm_dev_put(dev);
 }
 
+static void bochs_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 static const struct pci_device_id bochs_pci_tbl[] = {
 	{
 		.vendor      = 0x1234,
@@ -720,6 +725,7 @@ static struct pci_driver bochs_pci_driver = {
 	.id_table =	bochs_pci_tbl,
 	.probe =	bochs_pci_probe,
 	.remove =	bochs_pci_remove,
+	.shutdown =	bochs_pci_shutdown,
 	.driver.pm =    &bochs_pm_ops,
 };
 
diff --git a/drivers/gpu/drm/tiny/cirrus.c b/drivers/gpu/drm/tiny/cirrus.c
index 594bc472862f..c5c34cd2edc1 100644
--- a/drivers/gpu/drm/tiny/cirrus.c
+++ b/drivers/gpu/drm/tiny/cirrus.c
@@ -727,6 +727,11 @@ static void cirrus_pci_remove(struct pci_dev *pdev)
 	drm_atomic_helper_shutdown(dev);
 }
 
+static void cirrus_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
+}
+
 static const struct pci_device_id pciidlist[] = {
 	{
 		.vendor    = PCI_VENDOR_ID_CIRRUS,
@@ -748,6 +753,7 @@ static struct pci_driver cirrus_pci_driver = {
 	.id_table = pciidlist,
 	.probe = cirrus_pci_probe,
 	.remove = cirrus_pci_remove,
+	.shutdown = cirrus_pci_shutdown,
 };
 
 drm_module_pci_driver(cirrus_pci_driver)
-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 3/6] drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code these drivers appear 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

Though this patch could be squashed into the patch ("drm: Call
drm_atomic_helper_shutdown() at shutdown time for misc drivers"), I
kept it separate to call attention to this driver. While writing this
patch, I noticed that the bind() function is using "devm" and thus
assumes it doesn't need to do much explicit error handling. That's
actually 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

 drivers/gpu/drm/vc4/vc4_drv.c | 36 ++++++++++++++++++++++-------------
 1 file changed, 23 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_drv.c b/drivers/gpu/drm/vc4/vc4_drv.c
index 1b3531374967..c133e96b8aca 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.c
+++ b/drivers/gpu/drm/vc4/vc4_drv.c
@@ -324,21 +324,21 @@ static int vc4_drm_bind(struct device *dev)
 	if (!is_vc5) {
 		ret = drmm_mutex_init(drm, &vc4->bin_bo_lock);
 		if (ret)
-			return ret;
+			goto err;
 
 		ret = vc4_bo_cache_init(drm);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	ret = drmm_mode_config_init(drm);
 	if (ret)
-		return ret;
+		goto err;
 
 	if (!is_vc5) {
 		ret = vc4_gem_init(drm);
 		if (ret)
-			return ret;
+			goto err;
 	}
 
 	node = of_find_compatible_node(NULL, NULL, "raspberrypi,bcm2835-firmware");
@@ -346,13 +346,15 @@ static int vc4_drm_bind(struct device *dev)
 		firmware = rpi_firmware_get(node);
 		of_node_put(node);
 
-		if (!firmware)
-			return -EPROBE_DEFER;
+		if (!firmware) {
+			ret = -EPROBE_DEFER;
+			goto err;
+		}
 	}
 
 	ret = drm_aperture_remove_framebuffers(driver);
 	if (ret)
-		return ret;
+		goto err;
 
 	if (firmware) {
 		ret = rpi_firmware_property(firmware,
@@ -366,32 +368,33 @@ static int vc4_drm_bind(struct device *dev)
 
 	ret = component_bind_all(dev, drm);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = devm_add_action_or_reset(dev, vc4_component_unbind_all, vc4);
 	if (ret)
-		return ret;
+		goto err;
 
 	ret = vc4_plane_create_additional_planes(drm);
 	if (ret)
-		goto unbind_all;
+		goto err;
 
 	ret = vc4_kms_load(drm);
 	if (ret < 0)
-		goto unbind_all;
+		goto err;
 
 	drm_for_each_crtc(crtc, drm)
 		vc4_crtc_disable_at_boot(crtc);
 
 	ret = drm_dev_register(drm, 0);
 	if (ret < 0)
-		goto unbind_all;
+		goto err;
 
 	drm_fbdev_dma_setup(drm, 16);
 
 	return 0;
 
-unbind_all:
+err:
+	platform_set_drvdata(pdev, NULL);
 	return ret;
 }
 
@@ -401,6 +404,7 @@ static void vc4_drm_unbind(struct device *dev)
 
 	drm_dev_unplug(drm);
 	drm_atomic_helper_shutdown(drm);
+	dev_set_drvdata(dev, NULL);
 }
 
 static const struct component_master_ops vc4_drm_ops = {
@@ -444,6 +448,11 @@ static void vc4_platform_drm_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &vc4_drm_ops);
 }
 
+static void vc4_platform_drm_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id vc4_of_match[] = {
 	{ .compatible = "brcm,bcm2711-vc5", },
 	{ .compatible = "brcm,bcm2835-vc4", },
@@ -455,6 +464,7 @@ MODULE_DEVICE_TABLE(of, vc4_of_match);
 static struct platform_driver vc4_platform_driver = {
 	.probe		= vc4_platform_drm_probe,
 	.remove_new	= vc4_platform_drm_remove,
+	.shutdown	= vc4_platform_drm_shutdown,
 	.driver		= {
 		.name	= "vc4-drm",
 		.of_match_table = vc4_of_match,
-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 4/6] drm/ssd130x: 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.

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

NOTE: I'm not 100% sure this is the correct thing to do, but I _think_
so. Please shout if you know better.

 drivers/gpu/drm/solomon/ssd130x.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/solomon/ssd130x.c b/drivers/gpu/drm/solomon/ssd130x.c
index 5a80b228d18c..dc06fd82fd30 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -1131,6 +1131,7 @@ EXPORT_SYMBOL_GPL(ssd130x_probe);
 void ssd130x_remove(struct ssd130x_device *ssd130x)
 {
 	drm_dev_unplug(&ssd130x->drm);
+	drm_atomic_helper_shutdown(&ssd130x->drm);
 }
 EXPORT_SYMBOL_GPL(ssd130x_remove);
 
-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 5/6] drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for misc drivers

[Douglas Anderson]

Based on grepping through the source code these drivers appear to be
missing a call to drm_atomic_helper_shutdown() at system shutdown time
and at driver remove (or unbind) 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 and at driver remove (or unbind) time comes
straight out of the kernel doc "driver instance overview" in
drm_drv.c.

A few notes about these fixes:
- I confirmed that these drivers were all DRIVER_MODESET type drivers,
  which I believe makes this relevant.
- I confirmed that these drivers were all DRIVER_ATOMIC.
- When adding drm_atomic_helper_shutdown() to the remove/unbind path,
  I added it after drm_kms_helper_poll_fini() when the driver had
  it. This seemed to be what other drivers did. If
  drm_kms_helper_poll_fini() wasn't there I added it straight after
  drm_dev_unregister().
- This patch deals with drivers using the component model in similar
  ways as the patch ("drm: Call drm_atomic_helper_shutdown() at
  shutdown time for misc drivers")
- These fixes rely on the patch ("drm/atomic-helper:
  drm_atomic_helper_shutdown(NULL) should be a noop") to simplify
  shutdown.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

 drivers/gpu/drm/aspeed/aspeed_gfx_drv.c |  7 +++++++
 drivers/gpu/drm/mgag200/mgag200_drv.c   |  8 ++++++++
 drivers/gpu/drm/pl111/pl111_drv.c       |  7 +++++++
 drivers/gpu/drm/stm/drv.c               |  7 +++++++
 drivers/gpu/drm/tilcdc/tilcdc_drv.c     | 11 ++++++++++-
 drivers/gpu/drm/tve200/tve200_drv.c     |  7 +++++++
 drivers/gpu/drm/vboxvideo/vbox_drv.c    | 10 ++++++++++
 7 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
index d207b03f8357..78122b35a0cb 100644
--- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
+++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
@@ -358,11 +358,18 @@ static void aspeed_gfx_remove(struct platform_device *pdev)
 	sysfs_remove_group(&pdev->dev.kobj, &aspeed_sysfs_attr_group);
 	drm_dev_unregister(drm);
 	aspeed_gfx_unload(drm);
+	drm_atomic_helper_shutdown(drm);
+}
+
+static void aspeed_gfx_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
 }
 
 static struct platform_driver aspeed_gfx_platform_driver = {
 	.probe		= aspeed_gfx_probe,
 	.remove_new	= aspeed_gfx_remove,
+	.shutdown	= aspeed_gfx_shutdown,
 	.driver = {
 		.name = "aspeed_gfx",
 		.of_match_table = aspeed_gfx_match,
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
index abddf37f0ea1..2fb18b782b05 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.c
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
@@ -10,6 +10,7 @@
 #include <linux/pci.h>
 
 #include <drm/drm_aperture.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fbdev_generic.h>
 #include <drm/drm_file.h>
@@ -278,6 +279,12 @@ static void mgag200_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
+	drm_atomic_helper_shutdown(dev);
+}
+
+static void mgag200_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
 }
 
 static struct pci_driver mgag200_pci_driver = {
@@ -285,6 +292,7 @@ static struct pci_driver mgag200_pci_driver = {
 	.id_table = mgag200_pciidlist,
 	.probe = mgag200_pci_probe,
 	.remove = mgag200_pci_remove,
+	.shutdown = mgag200_pci_shutdown,
 };
 
 drm_module_pci_driver_if_modeset(mgag200_pci_driver, mgag200_modeset);
diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
index ba3b5b5f0cdf..02e6b74d5016 100644
--- a/drivers/gpu/drm/pl111/pl111_drv.c
+++ b/drivers/gpu/drm/pl111/pl111_drv.c
@@ -323,12 +323,18 @@ static void pl111_amba_remove(struct amba_device *amba_dev)
 	struct pl111_drm_dev_private *priv = drm->dev_private;
 
 	drm_dev_unregister(drm);
+	drm_atomic_helper_shutdown(drm);
 	if (priv->panel)
 		drm_panel_bridge_remove(priv->bridge);
 	drm_dev_put(drm);
 	of_reserved_mem_device_release(dev);
 }
 
+static void pl111_amba_shutdown(struct amba_device *amba_dev)
+{
+	drm_atomic_helper_shutdown(amba_get_drvdata(amba_dev));
+}
+
 /*
  * This early variant lacks the 565 and 444 pixel formats.
  */
@@ -431,6 +437,7 @@ static struct amba_driver pl111_amba_driver __maybe_unused = {
 	},
 	.probe = pl111_amba_probe,
 	.remove = pl111_amba_remove,
+	.shutdown = pl111_amba_shutdown,
 	.id_table = pl111_id_table,
 };
 
diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
index c68c831136c9..e8523abef27a 100644
--- a/drivers/gpu/drm/stm/drv.c
+++ b/drivers/gpu/drm/stm/drv.c
@@ -114,6 +114,7 @@ static void drv_unload(struct drm_device *ddev)
 	DRM_DEBUG("%s\n", __func__);
 
 	drm_kms_helper_poll_fini(ddev);
+	drm_atomic_helper_shutdown(ddev);
 	ltdc_unload(ddev);
 }
 
@@ -225,6 +226,11 @@ static void stm_drm_platform_remove(struct platform_device *pdev)
 	drm_dev_put(ddev);
 }
 
+static void stm_drm_platform_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id drv_dt_ids[] = {
 	{ .compatible = "st,stm32-ltdc"},
 	{ /* end node */ },
@@ -234,6 +240,7 @@ MODULE_DEVICE_TABLE(of, drv_dt_ids);
 static struct platform_driver stm_drm_platform_driver = {
 	.probe = stm_drm_platform_probe,
 	.remove_new = stm_drm_platform_remove,
+	.shutdown = stm_drm_platform_shutdown,
 	.driver = {
 		.name = "stm32-display",
 		.of_match_table = drv_dt_ids,
diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
index fe56beea3e93..8ebd7134ee21 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
@@ -175,6 +175,7 @@ static void tilcdc_fini(struct drm_device *dev)
 		drm_dev_unregister(dev);
 
 	drm_kms_helper_poll_fini(dev);
+	drm_atomic_helper_shutdown(dev);
 	tilcdc_irq_uninstall(dev);
 	drm_mode_config_cleanup(dev);
 
@@ -389,6 +390,7 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
 
 init_failed:
 	tilcdc_fini(ddev);
+	platform_set_drvdata(pdev, NULL);
 
 	return ret;
 }
@@ -537,7 +539,8 @@ static void tilcdc_unbind(struct device *dev)
 	if (!ddev->dev_private)
 		return;
 
-	tilcdc_fini(dev_get_drvdata(dev));
+	tilcdc_fini(ddev);
+	dev_set_drvdata(dev, NULL);
 }
 
 static const struct component_master_ops tilcdc_comp_ops = {
@@ -582,6 +585,11 @@ static int tilcdc_pdev_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void tilcdc_pdev_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id tilcdc_of_match[] = {
 		{ .compatible = "ti,am33xx-tilcdc", },
 		{ .compatible = "ti,da850-tilcdc", },
@@ -592,6 +600,7 @@ MODULE_DEVICE_TABLE(of, tilcdc_of_match);
 static struct platform_driver tilcdc_platform_driver = {
 	.probe      = tilcdc_pdev_probe,
 	.remove     = tilcdc_pdev_remove,
+	.shutdown   = tilcdc_pdev_shutdown,
 	.driver     = {
 		.name   = "tilcdc",
 		.pm     = pm_sleep_ptr(&tilcdc_pm_ops),
diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
index 0bb56d063536..acce210e2554 100644
--- a/drivers/gpu/drm/tve200/tve200_drv.c
+++ b/drivers/gpu/drm/tve200/tve200_drv.c
@@ -242,6 +242,7 @@ static void tve200_remove(struct platform_device *pdev)
 	struct tve200_drm_dev_private *priv = drm->dev_private;
 
 	drm_dev_unregister(drm);
+	drm_atomic_helper_shutdown(drm);
 	if (priv->panel)
 		drm_panel_bridge_remove(priv->bridge);
 	drm_mode_config_cleanup(drm);
@@ -249,6 +250,11 @@ static void tve200_remove(struct platform_device *pdev)
 	drm_dev_put(drm);
 }
 
+static void tve200_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id tve200_of_match[] = {
 	{
 		.compatible = "faraday,tve200",
@@ -263,6 +269,7 @@ static struct platform_driver tve200_driver = {
 	},
 	.probe = tve200_probe,
 	.remove_new = tve200_remove,
+	.shutdown = tve200_shutdown,
 };
 drm_module_platform_driver(tve200_driver);
 
diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
index 4fee15c97c34..047b95812334 100644
--- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
+++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
@@ -12,6 +12,7 @@
 #include <linux/vt_kern.h>
 
 #include <drm/drm_aperture.h>
+#include <drm/drm_atomic_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_fbdev_generic.h>
 #include <drm/drm_file.h>
@@ -97,11 +98,19 @@ static void vbox_pci_remove(struct pci_dev *pdev)
 	struct vbox_private *vbox = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(&vbox->ddev);
+	drm_atomic_helper_shutdown(&vbox->ddev);
 	vbox_irq_fini(vbox);
 	vbox_mode_fini(vbox);
 	vbox_hw_fini(vbox);
 }
 
+static void vbox_pci_shutdown(struct pci_dev *pdev)
+{
+	struct vbox_private *vbox = pci_get_drvdata(pdev);
+
+	drm_atomic_helper_shutdown(&vbox->ddev);
+}
+
 static int vbox_pm_suspend(struct device *dev)
 {
 	struct vbox_private *vbox = dev_get_drvdata(dev);
@@ -165,6 +174,7 @@ static struct pci_driver vbox_pci_driver = {
 	.id_table = pciidlist,
 	.probe = vbox_pci_probe,
 	.remove = vbox_pci_remove,
+	.shutdown = vbox_pci_shutdown,
 	.driver.pm = pm_sleep_ptr(&vbox_pm_ops),
 };
 
-- 
2.42.0.283.g2d96d420d3-goog

[RFT PATCH 6/6] drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at shutdown/unbind 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
and at driver unbind 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 and at driver remove (or unbind) time comes
straight out of the kernel doc "driver instance overview" in
drm_drv.c.

I have attempted to put this in the right place at unbind time. In
most other DRM drivers the call is made right after the call to
drm_kms_helper_poll_fini(), so I've put it there. That means that this
call will also be made in the case that we hit errors in bind, since
kirin_drm_kms_cleanup() is called both in the bind error path and in
unbind. I believe this is harmless even though it's not needed in the
bind error path.

For handling shutdown, we rely on the common technique of seeing if
the drvdata is NULL to know whether we need to call
drm_atomic_helper_shutdown(). This makes it important to make sure
that the drvdata is NULL if bind failed or if unbind was called. We
don't need the actual check for NULL and we'll rely on the patch
("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
noop").

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

 drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
index e8c77bcc6dae..75292a2f4644 100644
--- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
+++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c
@@ -206,6 +206,7 @@ static int kirin_drm_kms_init(struct drm_device *dev,
 static int kirin_drm_kms_cleanup(struct drm_device *dev)
 {
 	drm_kms_helper_poll_fini(dev);
+	drm_atomic_helper_shutdown(dev);
 	kirin_drm_private_cleanup(dev);
 	drm_mode_config_cleanup(dev);
 
@@ -244,6 +245,7 @@ static int kirin_drm_bind(struct device *dev)
 	kirin_drm_kms_cleanup(drm_dev);
 err_drm_dev_put:
 	drm_dev_put(drm_dev);
+	dev_set_drvdata(dev, NULL);
 
 	return ret;
 }
@@ -255,6 +257,7 @@ static void kirin_drm_unbind(struct device *dev)
 	drm_dev_unregister(drm_dev);
 	kirin_drm_kms_cleanup(drm_dev);
 	drm_dev_put(drm_dev);
+	dev_set_drvdata(dev, NULL);
 }
 
 static const struct component_master_ops kirin_drm_ops = {
@@ -284,6 +287,11 @@ static void kirin_drm_platform_remove(struct platform_device *pdev)
 	component_master_del(&pdev->dev, &kirin_drm_ops);
 }
 
+static void kirin_drm_platform_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id kirin_drm_dt_ids[] = {
 	{ .compatible = "hisilicon,hi6220-ade",
 	  .data = &ade_driver_data,
@@ -295,6 +303,7 @@ MODULE_DEVICE_TABLE(of, kirin_drm_dt_ids);
 static struct platform_driver kirin_drm_platform_driver = {
 	.probe = kirin_drm_platform_probe,
 	.remove_new = kirin_drm_platform_remove,
+	.shutdown = kirin_drm_platform_shutdown,
 	.driver = {
 		.name = "kirin-drm",
 		.of_match_table = kirin_drm_dt_ids,
-- 
2.42.0.283.g2d96d420d3-goog

Re: [RFT PATCH 1/6] drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:52 -0700, Douglas Anderson wrote:
> As with other places in the Linux kernel--kfree(NULL) being the most
> famous example--it's convenient to treat being passed a NULL argument
> as a noop in cleanup functions. Let's make
> drm_atomic_helper_shutdown() work like this.
> 
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:53 -0700, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear 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.
> 
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 3/6] drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:54 -0700, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear 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.
> 
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 4/6] drm/ssd130x: Call drm_atomic_helper_shutdown() at remove time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:55 -0700, Douglas Anderson wrote:
> 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
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 5/6] drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for misc drivers

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:56 -0700, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown time
> and at driver remove (or unbind) 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.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 6/6] drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:39:57 -0700, 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
> and at driver unbind 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.
> 
> [ ... ]

Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Jernej Škrabec]

Dne sobota, 02. september 2023 ob 01:39:53 CEST je Douglas Anderson 
napisal(a):
> Based on grepping through the source code these drivers appear 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.
> 
> All of the drivers in this patch were fairly straightforward to fix
> since they already had a call to drm_atomic_helper_shutdown() at
> remove/unbind time but were just lacking one at system shutdown. The
> only hitch is that some of these drivers use the component model to
> register/unregister their DRM devices. The shutdown callback is part
> of the original device. The typical solution here, based on how other
> DRM drivers do this, is to keep track of whether the device is bound
> based on drvdata. In most cases the drvdata is the drm_device, so we
> can just make sure it is NULL when the device is not bound. In some
> drivers, this required minor code changes. To make things simpler,
> drm_atomic_helper_shutdown() has been modified to consider a NULL
> drm_device as a noop in the patch ("drm/atomic-helper:
> drm_atomic_helper_shutdown(NULL) should be a noop").
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
> 
> Note that checkpatch yells that "drivers/gpu/drm/tiny/cirrus.c" is
> marked as 'obsolete', but it seems silly not to include the fix if
> it's already been written. If someone wants me to take that out,
> though, I can.
> 
>  drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 9 +++++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 7 +++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 1 +
>  drivers/gpu/drm/arm/hdlcd_drv.c                 | 6 ++++++
>  drivers/gpu/drm/arm/malidp_drv.c                | 6 ++++++
>  drivers/gpu/drm/ast/ast_drv.c                   | 6 ++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 6 ++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c       | 8 ++++++++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++++++
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 6 ++++++
>  drivers/gpu/drm/logicvc/logicvc_drm.c           | 9 +++++++++
>  drivers/gpu/drm/loongson/lsdc_drv.c             | 6 ++++++
>  drivers/gpu/drm/mcde/mcde_drv.c                 | 9 +++++++++
>  drivers/gpu/drm/omapdrm/omap_drv.c              | 8 ++++++++
>  drivers/gpu/drm/qxl/qxl_drv.c                   | 7 +++++++
>  drivers/gpu/drm/sti/sti_drv.c                   | 7 +++++++
>  drivers/gpu/drm/sun4i/sun4i_drv.c               | 6 ++++++

For sun4i:
Tested-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

>  drivers/gpu/drm/tiny/bochs.c                    | 6 ++++++
>  drivers/gpu/drm/tiny/cirrus.c                   | 6 ++++++
>  19 files changed, 125 insertions(+)

Re: [RFT PATCH 1/6] drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop

[Doug Anderson]

Hi,

On Mon, Sep 4, 2023 at 12:55 AM Maxime Ripard <mripard@kernel.org> wrote:
>
> On Fri, 1 Sep 2023 16:39:52 -0700, Douglas Anderson wrote:
> > As with other places in the Linux kernel--kfree(NULL) being the most
> > famous example--it's convenient to treat being passed a NULL argument
> > as a noop in cleanup functions. Let's make
> > drm_atomic_helper_shutdown() work like this.
> >
> >
> > [ ... ]
>
> Acked-by: Maxime Ripard <mripard@kernel.org>

Thanks! If there are no objections, I'd tend to land this patch
sometime early next week just to get it out of the queue, even if
other patches in the series are still being discussed / need spinning.
If anyone objects to that idea, please shout.

-Doug

Re: [RFT PATCH 1/6] drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop

[Doug Anderson]

Hi,

On Tue, Sep 5, 2023 at 2:14 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Mon, Sep 4, 2023 at 12:55 AM Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Fri, 1 Sep 2023 16:39:52 -0700, Douglas Anderson wrote:
> > > As with other places in the Linux kernel--kfree(NULL) being the most
> > > famous example--it's convenient to treat being passed a NULL argument
> > > as a noop in cleanup functions. Let's make
> > > drm_atomic_helper_shutdown() work like this.
> > >
> > >
> > > [ ... ]
> >
> > Acked-by: Maxime Ripard <mripard@kernel.org>
>
> Thanks! If there are no objections, I'd tend to land this patch
> sometime early next week just to get it out of the queue, even if
> other patches in the series are still being discussed / need spinning.
> If anyone objects to that idea, please shout.

Landed to drm-misc-next.

2a073968289d drm/atomic-helper: drm_atomic_helper_shutdown(NULL)
should be a noop

Re: [RFT PATCH 0/6] drm: drm-misc drivers call drm_atomic_helper_shutdown() at the right times

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:40 PM Douglas Anderson <dianders@chromium.org> wrote:
>
>
> NOTE: in order to avoid email sending limits on the cover letter, I've
> split this patch series in two. Patches that target drm-misc and ones
> that don't. The cover letter of the two is identical other than this note.
>
> This patch series came about after a _long_ discussion between me and
> Maxime Ripard in response to a different patch I sent out [1]. 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!
>
> I've organized this series like this:
> 1. One patch for all simple cases of just needing a call at shutdown
>    time for drivers that go through drm-misc.
> 2. A separate patch for "drm/vc4", even though it goes through
>    drm-misc, since I wanted to leave an extra note for that one.
> 3. Patches for drivers that just needed a call at shutdown time for
>    drivers that _don't_ go through drm-misc.
> 4. Patches for the few drivers that had the call at shutdown time but
>    lacked it at remove time.
> 5. One patch for all simple cases of needing a call at shutdown and
>    remove (or unbind) time for drivers that go through drm-misc.
> 6. A separate patch for "drm/hisilicon/kirin", even though it goes
>    through drm-misc, since I wanted to leave an extra note for that
>    one.
> 7. Patches for drivers that needed a call at shutdown and remove (or
>    unbind) time for drivers that _don't_ go through drm-misc.
>
> I've labeled this patch series as RFT (request for testing) to help
> call attention to the fact that I didn't personally test any of these
> patches.
>
> If you're a maintainer of one of these drivers and you think that the
> patch for your driver looks fabulous, you've tested it, and you'd like
> to land it right away then please do. For non-drm-misc drivers there
> are no dependencies here. Some of the drm-misc drivers depend on the
> first patch, AKA ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL)
> should be a noop"). I've tried to call this out but it also should be
> obvious once you know to look for it.
>
> 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 driers 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/lkml/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid
>
>
> Douglas Anderson (6):
>   drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop
>   drm: Call drm_atomic_helper_shutdown() at shutdown time for misc
>     drivers
>   drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time
>   drm/ssd130x: Call drm_atomic_helper_shutdown() at remove time
>   drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for
>     misc drivers
>   drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at
>     shutdown/unbind time
>
>  .../gpu/drm/arm/display/komeda/komeda_drv.c   |  9 +++++
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   |  7 ++++
>  .../gpu/drm/arm/display/komeda/komeda_kms.h   |  1 +
>  drivers/gpu/drm/arm/hdlcd_drv.c               |  6 ++++
>  drivers/gpu/drm/arm/malidp_drv.c              |  6 ++++
>  drivers/gpu/drm/aspeed/aspeed_gfx_drv.c       |  7 ++++
>  drivers/gpu/drm/ast/ast_drv.c                 |  6 ++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  |  6 ++++
>  drivers/gpu/drm/drm_atomic_helper.c           |  3 ++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c     |  8 +++++
>  .../gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c   |  6 ++++
>  .../gpu/drm/hisilicon/kirin/kirin_drm_drv.c   |  9 +++++
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c       |  6 ++++
>  drivers/gpu/drm/logicvc/logicvc_drm.c         |  9 +++++
>  drivers/gpu/drm/loongson/lsdc_drv.c           |  6 ++++
>  drivers/gpu/drm/mcde/mcde_drv.c               |  9 +++++
>  drivers/gpu/drm/mgag200/mgag200_drv.c         |  8 +++++
>  drivers/gpu/drm/omapdrm/omap_drv.c            |  8 +++++
>  drivers/gpu/drm/pl111/pl111_drv.c             |  7 ++++
>  drivers/gpu/drm/qxl/qxl_drv.c                 |  7 ++++
>  drivers/gpu/drm/solomon/ssd130x.c             |  1 +
>  drivers/gpu/drm/sti/sti_drv.c                 |  7 ++++
>  drivers/gpu/drm/stm/drv.c                     |  7 ++++
>  drivers/gpu/drm/sun4i/sun4i_drv.c             |  6 ++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c           | 11 +++++-
>  drivers/gpu/drm/tiny/bochs.c                  |  6 ++++
>  drivers/gpu/drm/tiny/cirrus.c                 |  6 ++++
>  drivers/gpu/drm/tve200/tve200_drv.c           |  7 ++++
>  drivers/gpu/drm/vboxvideo/vbox_drv.c          | 10 ++++++
>  drivers/gpu/drm/vc4/vc4_drv.c                 | 36 ++++++++++++-------
>  30 files changed, 217 insertions(+), 14 deletions(-)

Just a heads up to keep folks in the loop: I've landed the first patch
in the drm-misc series, AKA ("drm/atomic-helper:
drm_atomic_helper_shutdown(NULL) should be a noop"), but I haven't
landed any of the others yet. I'm going to give them another ~week
just to be sure there are no objections.

-Doug

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[suijingfeng]

Hi,


On 2023/9/2 07:39, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear 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.
>
> All of the drivers in this patch were fairly straightforward to fix
> since they already had a call to drm_atomic_helper_shutdown() at
> remove/unbind time but were just lacking one at system shutdown. The
> only hitch is that some of these drivers use the component model to
> register/unregister their DRM devices. The shutdown callback is part
> of the original device. The typical solution here, based on how other
> DRM drivers do this, is to keep track of whether the device is bound
> based on drvdata. In most cases the drvdata is the drm_device, so we
> can just make sure it is NULL when the device is not bound. In some
> drivers, this required minor code changes. To make things simpler,
> drm_atomic_helper_shutdown() has been modified to consider a NULL
> drm_device as a noop in the patch ("drm/atomic-helper:
> drm_atomic_helper_shutdown(NULL) should be a noop").
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---


I have just tested the whole series, thanks for the patch. For drm/loongson only:


Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>



By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the lsdc_pci_shutdown() function,
And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the machine.
I did not witness something weird happen at present. As you have said, this is useful for
drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and drm/loongson etc)
drivers, you didn't mention what's the benefit for those drivers. Probably, you can
mention it with at least one sentence at the next version. I also prefer to alter the
lsdc_pci_shutdown() function as the following pattern:


static void lsdc_pci_shutdown(struct pci_dev *pdev)
{

     struct drm_device *ddev = pci_get_drvdata(pdev);

     drm_atomic_helper_shutdown(ddev);
}

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Doug Anderson]

Hi,

On Fri, Sep 15, 2023 at 2:11 AM suijingfeng <suijingfeng@loongson.cn> wrote:
>
> Hi,
>
>
> On 2023/9/2 07:39, Douglas Anderson wrote:
> > Based on grepping through the source code these drivers appear 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.
> >
> > All of the drivers in this patch were fairly straightforward to fix
> > since they already had a call to drm_atomic_helper_shutdown() at
> > remove/unbind time but were just lacking one at system shutdown. The
> > only hitch is that some of these drivers use the component model to
> > register/unregister their DRM devices. The shutdown callback is part
> > of the original device. The typical solution here, based on how other
> > DRM drivers do this, is to keep track of whether the device is bound
> > based on drvdata. In most cases the drvdata is the drm_device, so we
> > can just make sure it is NULL when the device is not bound. In some
> > drivers, this required minor code changes. To make things simpler,
> > drm_atomic_helper_shutdown() has been modified to consider a NULL
> > drm_device as a noop in the patch ("drm/atomic-helper:
> > drm_atomic_helper_shutdown(NULL) should be a noop").
> >
> > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
>
>
> I have just tested the whole series, thanks for the patch. For drm/loongson only:
>
>
> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

Thanks!


> By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the lsdc_pci_shutdown() function,
> And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the machine.
> I did not witness something weird happen at present. As you have said, this is useful for
> drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and drm/loongson etc)
> drivers, you didn't mention what's the benefit for those drivers.

I didn't mention it because I have no idea! I presume that for
non-drm_panel use cases it's not a huge deal, otherwise it wouldn't
have been missing from so many drivers. Thus, my "one sentence" reason
for the non-drm_panel case is just "we should do this because the
documentation of the API says we should", which is already in the
commit message. ;-)

If you have a specific other benefit you'd like me to list then I'm happy to.


> Probably, you can
> mention it with at least one sentence at the next version. I also prefer to alter the
> lsdc_pci_shutdown() function as the following pattern:
>
>
> static void lsdc_pci_shutdown(struct pci_dev *pdev)
> {
>
>      struct drm_device *ddev = pci_get_drvdata(pdev);
>
>      drm_atomic_helper_shutdown(ddev);
> }

I was hoping to land this patch without spinning it unless there's a
good reason. How strongly do you feel about needing to change the
above? I will note that I coded it the way I did specifically to try
to follow the style in the documentation in "drm_drv.c". In the
example "driver_shutdown()" function you can see that they combined it
into one line and so I followed that style. ;-) That being said, I
have no problem changing this if I spin the patch.

-Doug

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[suijingfeng]

Hi,


On 2023/9/15 21:44, Doug Anderson wrote:
> Hi,
>
> On Fri, Sep 15, 2023 at 2:11 AM suijingfeng <suijingfeng@loongson.cn> wrote:
>> Hi,
>>
>>
>> On 2023/9/2 07:39, Douglas Anderson wrote:
>>> Based on grepping through the source code these drivers appear 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.
>>>
>>> All of the drivers in this patch were fairly straightforward to fix
>>> since they already had a call to drm_atomic_helper_shutdown() at
>>> remove/unbind time but were just lacking one at system shutdown. The
>>> only hitch is that some of these drivers use the component model to
>>> register/unregister their DRM devices. The shutdown callback is part
>>> of the original device. The typical solution here, based on how other
>>> DRM drivers do this, is to keep track of whether the device is bound
>>> based on drvdata. In most cases the drvdata is the drm_device, so we
>>> can just make sure it is NULL when the device is not bound. In some
>>> drivers, this required minor code changes. To make things simpler,
>>> drm_atomic_helper_shutdown() has been modified to consider a NULL
>>> drm_device as a noop in the patch ("drm/atomic-helper:
>>> drm_atomic_helper_shutdown(NULL) should be a noop").
>>>
>>> Suggested-by: Maxime Ripard <mripard@kernel.org>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>
>> I have just tested the whole series, thanks for the patch. For drm/loongson only:
>>
>>
>> Reviewed-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Thanks!
>
>
>> By the way, I add 'pr_info("lsdc_pci_shutdown\n");' into the lsdc_pci_shutdown() function,
>> And seeing that lsdc_pci_shutdown() will be called when reboot and shutdown the machine.
>> I did not witness something weird happen at present. As you have said, this is useful for
>> drm panels drivers. But for the rest(drm/hibmc, drm/ast, drm/mgag200 and drm/loongson etc)
>> drivers, you didn't mention what's the benefit for those drivers.
> I didn't mention it because I have no idea! I presume that for
> non-drm_panel use cases it's not a huge deal, otherwise it wouldn't
> have been missing from so many drivers. Thus, my "one sentence" reason
> for the non-drm_panel case is just "we should do this because the
> documentation of the API says we should", which is already in the
> commit message. ;-)
>

OK, this sound fine.

> If you have a specific other benefit you'd like me to list then I'm happy to.

You should think about the answer of this question yourself.
But I will not object if you can't find one. OK. :-)

>
>> Probably, you can
>> mention it with at least one sentence at the next version. I also prefer to alter the
>> lsdc_pci_shutdown() function as the following pattern:
>>
>>
>> static void lsdc_pci_shutdown(struct pci_dev *pdev)
>> {
>>
>>       struct drm_device *ddev = pci_get_drvdata(pdev);
>>
>>       drm_atomic_helper_shutdown(ddev);
>> }
> I was hoping to land this patch without spinning it unless there's a
> good reason. How strongly do you feel about needing to change the
> above?


Not very strong, this version looks just fine.
I will not object if you keep it as is.
But I will also hear what the others reviewers say.
Thanks for the patch.

>
> -Doug

Re: [RFT PATCH 5/6] drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for misc drivers

[Tomi Valkeinen]

On 02/09/2023 02:39, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown time
> and at driver remove (or unbind) 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 and at driver remove (or unbind) time comes
> straight out of the kernel doc "driver instance overview" in
> drm_drv.c.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

and tested on Beagle Bone Black (tilcdc):

Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com> # tilcdc

  Tomi

> 
> A few notes about these fixes:
> - I confirmed that these drivers were all DRIVER_MODESET type drivers,
>    which I believe makes this relevant.
> - I confirmed that these drivers were all DRIVER_ATOMIC.
> - When adding drm_atomic_helper_shutdown() to the remove/unbind path,
>    I added it after drm_kms_helper_poll_fini() when the driver had
>    it. This seemed to be what other drivers did. If
>    drm_kms_helper_poll_fini() wasn't there I added it straight after
>    drm_dev_unregister().
> - This patch deals with drivers using the component model in similar
>    ways as the patch ("drm: Call drm_atomic_helper_shutdown() at
>    shutdown time for misc drivers")
> - These fixes rely on the patch ("drm/atomic-helper:
>    drm_atomic_helper_shutdown(NULL) should be a noop") to simplify
>    shutdown.
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
>   drivers/gpu/drm/aspeed/aspeed_gfx_drv.c |  7 +++++++
>   drivers/gpu/drm/mgag200/mgag200_drv.c   |  8 ++++++++
>   drivers/gpu/drm/pl111/pl111_drv.c       |  7 +++++++
>   drivers/gpu/drm/stm/drv.c               |  7 +++++++
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c     | 11 ++++++++++-
>   drivers/gpu/drm/tve200/tve200_drv.c     |  7 +++++++
>   drivers/gpu/drm/vboxvideo/vbox_drv.c    | 10 ++++++++++
>   7 files changed, 56 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> index d207b03f8357..78122b35a0cb 100644
> --- a/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> +++ b/drivers/gpu/drm/aspeed/aspeed_gfx_drv.c
> @@ -358,11 +358,18 @@ static void aspeed_gfx_remove(struct platform_device *pdev)
>   	sysfs_remove_group(&pdev->dev.kobj, &aspeed_sysfs_attr_group);
>   	drm_dev_unregister(drm);
>   	aspeed_gfx_unload(drm);
> +	drm_atomic_helper_shutdown(drm);
> +}
> +
> +static void aspeed_gfx_shutdown(struct platform_device *pdev)
> +{
> +	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
>   }
>   
>   static struct platform_driver aspeed_gfx_platform_driver = {
>   	.probe		= aspeed_gfx_probe,
>   	.remove_new	= aspeed_gfx_remove,
> +	.shutdown	= aspeed_gfx_shutdown,
>   	.driver = {
>   		.name = "aspeed_gfx",
>   		.of_match_table = aspeed_gfx_match,
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index abddf37f0ea1..2fb18b782b05 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -10,6 +10,7 @@
>   #include <linux/pci.h>
>   
>   #include <drm/drm_aperture.h>
> +#include <drm/drm_atomic_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fbdev_generic.h>
>   #include <drm/drm_file.h>
> @@ -278,6 +279,12 @@ static void mgag200_pci_remove(struct pci_dev *pdev)
>   	struct drm_device *dev = pci_get_drvdata(pdev);
>   
>   	drm_dev_unregister(dev);
> +	drm_atomic_helper_shutdown(dev);
> +}
> +
> +static void mgag200_pci_shutdown(struct pci_dev *pdev)
> +{
> +	drm_atomic_helper_shutdown(pci_get_drvdata(pdev));
>   }
>   
>   static struct pci_driver mgag200_pci_driver = {
> @@ -285,6 +292,7 @@ static struct pci_driver mgag200_pci_driver = {
>   	.id_table = mgag200_pciidlist,
>   	.probe = mgag200_pci_probe,
>   	.remove = mgag200_pci_remove,
> +	.shutdown = mgag200_pci_shutdown,
>   };
>   
>   drm_module_pci_driver_if_modeset(mgag200_pci_driver, mgag200_modeset);
> diff --git a/drivers/gpu/drm/pl111/pl111_drv.c b/drivers/gpu/drm/pl111/pl111_drv.c
> index ba3b5b5f0cdf..02e6b74d5016 100644
> --- a/drivers/gpu/drm/pl111/pl111_drv.c
> +++ b/drivers/gpu/drm/pl111/pl111_drv.c
> @@ -323,12 +323,18 @@ static void pl111_amba_remove(struct amba_device *amba_dev)
>   	struct pl111_drm_dev_private *priv = drm->dev_private;
>   
>   	drm_dev_unregister(drm);
> +	drm_atomic_helper_shutdown(drm);
>   	if (priv->panel)
>   		drm_panel_bridge_remove(priv->bridge);
>   	drm_dev_put(drm);
>   	of_reserved_mem_device_release(dev);
>   }
>   
> +static void pl111_amba_shutdown(struct amba_device *amba_dev)
> +{
> +	drm_atomic_helper_shutdown(amba_get_drvdata(amba_dev));
> +}
> +
>   /*
>    * This early variant lacks the 565 and 444 pixel formats.
>    */
> @@ -431,6 +437,7 @@ static struct amba_driver pl111_amba_driver __maybe_unused = {
>   	},
>   	.probe = pl111_amba_probe,
>   	.remove = pl111_amba_remove,
> +	.shutdown = pl111_amba_shutdown,
>   	.id_table = pl111_id_table,
>   };
>   
> diff --git a/drivers/gpu/drm/stm/drv.c b/drivers/gpu/drm/stm/drv.c
> index c68c831136c9..e8523abef27a 100644
> --- a/drivers/gpu/drm/stm/drv.c
> +++ b/drivers/gpu/drm/stm/drv.c
> @@ -114,6 +114,7 @@ static void drv_unload(struct drm_device *ddev)
>   	DRM_DEBUG("%s\n", __func__);
>   
>   	drm_kms_helper_poll_fini(ddev);
> +	drm_atomic_helper_shutdown(ddev);
>   	ltdc_unload(ddev);
>   }
>   
> @@ -225,6 +226,11 @@ static void stm_drm_platform_remove(struct platform_device *pdev)
>   	drm_dev_put(ddev);
>   }
>   
> +static void stm_drm_platform_shutdown(struct platform_device *pdev)
> +{
> +	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
> +}
> +
>   static const struct of_device_id drv_dt_ids[] = {
>   	{ .compatible = "st,stm32-ltdc"},
>   	{ /* end node */ },
> @@ -234,6 +240,7 @@ MODULE_DEVICE_TABLE(of, drv_dt_ids);
>   static struct platform_driver stm_drm_platform_driver = {
>   	.probe = stm_drm_platform_probe,
>   	.remove_new = stm_drm_platform_remove,
> +	.shutdown = stm_drm_platform_shutdown,
>   	.driver = {
>   		.name = "stm32-display",
>   		.of_match_table = drv_dt_ids,
> diff --git a/drivers/gpu/drm/tilcdc/tilcdc_drv.c b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> index fe56beea3e93..8ebd7134ee21 100644
> --- a/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> +++ b/drivers/gpu/drm/tilcdc/tilcdc_drv.c
> @@ -175,6 +175,7 @@ static void tilcdc_fini(struct drm_device *dev)
>   		drm_dev_unregister(dev);
>   
>   	drm_kms_helper_poll_fini(dev);
> +	drm_atomic_helper_shutdown(dev);
>   	tilcdc_irq_uninstall(dev);
>   	drm_mode_config_cleanup(dev);
>   
> @@ -389,6 +390,7 @@ static int tilcdc_init(const struct drm_driver *ddrv, struct device *dev)
>   
>   init_failed:
>   	tilcdc_fini(ddev);
> +	platform_set_drvdata(pdev, NULL);
>   
>   	return ret;
>   }
> @@ -537,7 +539,8 @@ static void tilcdc_unbind(struct device *dev)
>   	if (!ddev->dev_private)
>   		return;
>   
> -	tilcdc_fini(dev_get_drvdata(dev));
> +	tilcdc_fini(ddev);
> +	dev_set_drvdata(dev, NULL);
>   }
>   
>   static const struct component_master_ops tilcdc_comp_ops = {
> @@ -582,6 +585,11 @@ static int tilcdc_pdev_remove(struct platform_device *pdev)
>   	return 0;
>   }
>   
> +static void tilcdc_pdev_shutdown(struct platform_device *pdev)
> +{
> +	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
> +}
> +
>   static const struct of_device_id tilcdc_of_match[] = {
>   		{ .compatible = "ti,am33xx-tilcdc", },
>   		{ .compatible = "ti,da850-tilcdc", },
> @@ -592,6 +600,7 @@ MODULE_DEVICE_TABLE(of, tilcdc_of_match);
>   static struct platform_driver tilcdc_platform_driver = {
>   	.probe      = tilcdc_pdev_probe,
>   	.remove     = tilcdc_pdev_remove,
> +	.shutdown   = tilcdc_pdev_shutdown,
>   	.driver     = {
>   		.name   = "tilcdc",
>   		.pm     = pm_sleep_ptr(&tilcdc_pm_ops),
> diff --git a/drivers/gpu/drm/tve200/tve200_drv.c b/drivers/gpu/drm/tve200/tve200_drv.c
> index 0bb56d063536..acce210e2554 100644
> --- a/drivers/gpu/drm/tve200/tve200_drv.c
> +++ b/drivers/gpu/drm/tve200/tve200_drv.c
> @@ -242,6 +242,7 @@ static void tve200_remove(struct platform_device *pdev)
>   	struct tve200_drm_dev_private *priv = drm->dev_private;
>   
>   	drm_dev_unregister(drm);
> +	drm_atomic_helper_shutdown(drm);
>   	if (priv->panel)
>   		drm_panel_bridge_remove(priv->bridge);
>   	drm_mode_config_cleanup(drm);
> @@ -249,6 +250,11 @@ static void tve200_remove(struct platform_device *pdev)
>   	drm_dev_put(drm);
>   }
>   
> +static void tve200_shutdown(struct platform_device *pdev)
> +{
> +	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
> +}
> +
>   static const struct of_device_id tve200_of_match[] = {
>   	{
>   		.compatible = "faraday,tve200",
> @@ -263,6 +269,7 @@ static struct platform_driver tve200_driver = {
>   	},
>   	.probe = tve200_probe,
>   	.remove_new = tve200_remove,
> +	.shutdown = tve200_shutdown,
>   };
>   drm_module_platform_driver(tve200_driver);
>   
> diff --git a/drivers/gpu/drm/vboxvideo/vbox_drv.c b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> index 4fee15c97c34..047b95812334 100644
> --- a/drivers/gpu/drm/vboxvideo/vbox_drv.c
> +++ b/drivers/gpu/drm/vboxvideo/vbox_drv.c
> @@ -12,6 +12,7 @@
>   #include <linux/vt_kern.h>
>   
>   #include <drm/drm_aperture.h>
> +#include <drm/drm_atomic_helper.h>
>   #include <drm/drm_drv.h>
>   #include <drm/drm_fbdev_generic.h>
>   #include <drm/drm_file.h>
> @@ -97,11 +98,19 @@ static void vbox_pci_remove(struct pci_dev *pdev)
>   	struct vbox_private *vbox = pci_get_drvdata(pdev);
>   
>   	drm_dev_unregister(&vbox->ddev);
> +	drm_atomic_helper_shutdown(&vbox->ddev);
>   	vbox_irq_fini(vbox);
>   	vbox_mode_fini(vbox);
>   	vbox_hw_fini(vbox);
>   }
>   
> +static void vbox_pci_shutdown(struct pci_dev *pdev)
> +{
> +	struct vbox_private *vbox = pci_get_drvdata(pdev);
> +
> +	drm_atomic_helper_shutdown(&vbox->ddev);
> +}
> +
>   static int vbox_pm_suspend(struct device *dev)
>   {
>   	struct vbox_private *vbox = dev_get_drvdata(dev);
> @@ -165,6 +174,7 @@ static struct pci_driver vbox_pci_driver = {
>   	.id_table = pciidlist,
>   	.probe = vbox_pci_probe,
>   	.remove = vbox_pci_remove,
> +	.shutdown = vbox_pci_shutdown,
>   	.driver.pm = pm_sleep_ptr(&vbox_pm_ops),
>   };
>   

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Tomi Valkeinen]

On 02/09/2023 02:39, Douglas Anderson wrote:
> Based on grepping through the source code these drivers appear 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.
> 
> All of the drivers in this patch were fairly straightforward to fix
> since they already had a call to drm_atomic_helper_shutdown() at
> remove/unbind time but were just lacking one at system shutdown. The
> only hitch is that some of these drivers use the component model to
> register/unregister their DRM devices. The shutdown callback is part
> of the original device. The typical solution here, based on how other
> DRM drivers do this, is to keep track of whether the device is bound
> based on drvdata. In most cases the drvdata is the drm_device, so we
> can just make sure it is NULL when the device is not bound. In some
> drivers, this required minor code changes. To make things simpler,
> drm_atomic_helper_shutdown() has been modified to consider a NULL
> drm_device as a noop in the patch ("drm/atomic-helper:
> drm_atomic_helper_shutdown(NULL) should be a noop").
> 
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

For omapdrm:

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>
Tested-by: Tomi Valkeinen <tomi.valkeinen@ideasonboard.com>

  Tomi

Re: [RFT PATCH 2/6] drm: Call drm_atomic_helper_shutdown() at shutdown time for misc drivers

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:40 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Based on grepping through the source code these drivers appear 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.
>
> All of the drivers in this patch were fairly straightforward to fix
> since they already had a call to drm_atomic_helper_shutdown() at
> remove/unbind time but were just lacking one at system shutdown. The
> only hitch is that some of these drivers use the component model to
> register/unregister their DRM devices. The shutdown callback is part
> of the original device. The typical solution here, based on how other
> DRM drivers do this, is to keep track of whether the device is bound
> based on drvdata. In most cases the drvdata is the drm_device, so we
> can just make sure it is NULL when the device is not bound. In some
> drivers, this required minor code changes. To make things simpler,
> drm_atomic_helper_shutdown() has been modified to consider a NULL
> drm_device as a noop in the patch ("drm/atomic-helper:
> drm_atomic_helper_shutdown(NULL) should be a noop").
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> Note that checkpatch yells that "drivers/gpu/drm/tiny/cirrus.c" is
> marked as 'obsolete', but it seems silly not to include the fix if
> it's already been written. If someone wants me to take that out,
> though, I can.
>
>  drivers/gpu/drm/arm/display/komeda/komeda_drv.c | 9 +++++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 7 +++++++
>  drivers/gpu/drm/arm/display/komeda/komeda_kms.h | 1 +
>  drivers/gpu/drm/arm/hdlcd_drv.c                 | 6 ++++++
>  drivers/gpu/drm/arm/malidp_drv.c                | 6 ++++++
>  drivers/gpu/drm/ast/ast_drv.c                   | 6 ++++++
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c    | 6 ++++++
>  drivers/gpu/drm/fsl-dcu/fsl_dcu_drm_drv.c       | 8 ++++++++
>  drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++++++
>  drivers/gpu/drm/hyperv/hyperv_drm_drv.c         | 6 ++++++
>  drivers/gpu/drm/logicvc/logicvc_drm.c           | 9 +++++++++
>  drivers/gpu/drm/loongson/lsdc_drv.c             | 6 ++++++
>  drivers/gpu/drm/mcde/mcde_drv.c                 | 9 +++++++++
>  drivers/gpu/drm/omapdrm/omap_drv.c              | 8 ++++++++
>  drivers/gpu/drm/qxl/qxl_drv.c                   | 7 +++++++
>  drivers/gpu/drm/sti/sti_drv.c                   | 7 +++++++
>  drivers/gpu/drm/sun4i/sun4i_drv.c               | 6 ++++++
>  drivers/gpu/drm/tiny/bochs.c                    | 6 ++++++
>  drivers/gpu/drm/tiny/cirrus.c                   | 6 ++++++
>  19 files changed, 125 insertions(+)

This has been about 3 weeks now and it feels like that's enough bake
time and several people have managed to test it (thanks!). Landing in
drm-misc-next:

ce3d99c83495 drm: Call drm_atomic_helper_shutdown() at shutdown time
for misc drivers

Re: [RFT PATCH 3/6] drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:40 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Based on grepping through the source code these drivers appear 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>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> Though this patch could be squashed into the patch ("drm: Call
> drm_atomic_helper_shutdown() at shutdown time for misc drivers"), I
> kept it separate to call attention to this driver. While writing this
> patch, I noticed that the bind() function is using "devm" and thus
> assumes it doesn't need to do much explicit error handling. That's
> actually 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
>
>  drivers/gpu/drm/vc4/vc4_drv.c | 36 ++++++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 13 deletions(-)

Landed to drm-misc-next:

013d382d11a2 drm/vc4: Call drm_atomic_helper_shutdown() at shutdown time

Re: [RFT PATCH 4/6] drm/ssd130x: Call drm_atomic_helper_shutdown() at remove time

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:40 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 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.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> NOTE: I'm not 100% sure this is the correct thing to do, but I _think_
> so. Please shout if you know better.
>
>  drivers/gpu/drm/solomon/ssd130x.c | 1 +
>  1 file changed, 1 insertion(+)

Landed this to drm-misc-next. Since I wasn't 100% sure, if someone
finds that this is bad after-the-fact, we can certainly revert.

10c8204c8b17 drm/ssd130x: Call drm_atomic_helper_shutdown() at remove time

Re: [RFT PATCH 5/6] drm: Call drm_atomic_helper_shutdown() at shutdown/remove time for misc drivers

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:41 PM Douglas Anderson <dianders@chromium.org> wrote:
>
> Based on grepping through the source code these drivers appear to be
> missing a call to drm_atomic_helper_shutdown() at system shutdown time
> and at driver remove (or unbind) 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 and at driver remove (or unbind) time comes
> straight out of the kernel doc "driver instance overview" in
> drm_drv.c.
>
> A few notes about these fixes:
> - I confirmed that these drivers were all DRIVER_MODESET type drivers,
>   which I believe makes this relevant.
> - I confirmed that these drivers were all DRIVER_ATOMIC.
> - When adding drm_atomic_helper_shutdown() to the remove/unbind path,
>   I added it after drm_kms_helper_poll_fini() when the driver had
>   it. This seemed to be what other drivers did. If
>   drm_kms_helper_poll_fini() wasn't there I added it straight after
>   drm_dev_unregister().
> - This patch deals with drivers using the component model in similar
>   ways as the patch ("drm: Call drm_atomic_helper_shutdown() at
>   shutdown time for misc drivers")
> - These fixes rely on the patch ("drm/atomic-helper:
>   drm_atomic_helper_shutdown(NULL) should be a noop") to simplify
>   shutdown.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
>
>  drivers/gpu/drm/aspeed/aspeed_gfx_drv.c |  7 +++++++
>  drivers/gpu/drm/mgag200/mgag200_drv.c   |  8 ++++++++
>  drivers/gpu/drm/pl111/pl111_drv.c       |  7 +++++++
>  drivers/gpu/drm/stm/drv.c               |  7 +++++++
>  drivers/gpu/drm/tilcdc/tilcdc_drv.c     | 11 ++++++++++-
>  drivers/gpu/drm/tve200/tve200_drv.c     |  7 +++++++
>  drivers/gpu/drm/vboxvideo/vbox_drv.c    | 10 ++++++++++
>  7 files changed, 56 insertions(+), 1 deletion(-)

Landed on drm-misc-next:

3c4babae3c4a drm: Call drm_atomic_helper_shutdown() at shutdown/remove
time for misc drivers

Re: [RFT PATCH 6/6] drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:41 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
> and at driver unbind 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 and at driver remove (or unbind) time comes
> straight out of the kernel doc "driver instance overview" in
> drm_drv.c.
>
> I have attempted to put this in the right place at unbind time. In
> most other DRM drivers the call is made right after the call to
> drm_kms_helper_poll_fini(), so I've put it there. That means that this
> call will also be made in the case that we hit errors in bind, since
> kirin_drm_kms_cleanup() is called both in the bind error path and in
> unbind. I believe this is harmless even though it's not needed in the
> bind error path.
>
> For handling shutdown, we rely on the common technique of seeing if
> the drvdata is NULL to know whether we need to call
> drm_atomic_helper_shutdown(). This makes it important to make sure
> that the drvdata is NULL if bind failed or if unbind was called. We
> don't need the actual check for NULL and we'll rely on the patch
> ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
> noop").
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
>  drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c | 9 +++++++++
>  1 file changed, 9 insertions(+)

Landed in drm-misc-next:

918ce0906dcd drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at
shutdown/unbind time