Douglas Anderson
2023-Sep-01 23:41 UTC
[Nouveau] [RFT PATCH 00/15] drm: non-drm-misc drivers call drm_atomic_helper_shutdown() at the right times
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
at changeid
Douglas Anderson (15):
drm/armada: Call drm_atomic_helper_shutdown() at shutdown time
drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time
drm/ingenic: Call drm_atomic_helper_shutdown() at shutdown time
drm/kmb: Call drm_atomic_helper_shutdown() at shutdown time
drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time
drm/nouveau: Call drm_atomic_helper_shutdown() or equiv at shutdown
time
drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time
drm/arcpgu: Call drm_atomic_helper_shutdown() at shutdown time
drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time
drm/sprd: Call drm_atomic_helper_shutdown() at remove time
drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time
drm/gma500: Call drm_helper_force_disable_all() at shutdown/remove
time
drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind
time
drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove
time
drm/renesas/shmobile: Call drm_helper_force_disable_all() at
shutdown/remove time
drivers/gpu/drm/amd/amdgpu/amdgpu.h | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 +++
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 2 +
drivers/gpu/drm/armada/armada_drv.c | 8 +++
drivers/gpu/drm/exynos/exynos_drm_drv.c | 11 ++++
drivers/gpu/drm/gma500/psb_drv.c | 8 +++
drivers/gpu/drm/imx/dcss/dcss-drv.c | 8 +++
drivers/gpu/drm/imx/dcss/dcss-kms.c | 7 ++
drivers/gpu/drm/imx/dcss/dcss-kms.h | 1 +
drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 11 ++++
drivers/gpu/drm/ingenic/ingenic-drm-drv.c | 66 ++++++++++++-------
drivers/gpu/drm/kmb/kmb_drv.c | 6 ++
drivers/gpu/drm/mediatek/mtk_drm_drv.c | 9 +++
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++
drivers/gpu/drm/nouveau/nouveau_display.h | 1 +
drivers/gpu/drm/nouveau/nouveau_drm.c | 13 ++++
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
drivers/gpu/drm/nouveau/nouveau_platform.c | 6 ++
drivers/gpu/drm/radeon/radeon_drv.c | 7 +-
.../gpu/drm/renesas/shmobile/shmob_drm_drv.c | 10 +++
drivers/gpu/drm/sprd/sprd_drm.c | 4 +-
drivers/gpu/drm/tegra/drm.c | 6 ++
drivers/gpu/drm/tiny/arcpgu.c | 6 ++
23 files changed, 187 insertions(+), 24 deletions(-)
--
2.42.0.283.g2d96d420d3-goog
Douglas Anderson
2023-Sep-01 23:41 UTC
[Nouveau] [RFT PATCH 06/15] drm/nouveau: Call drm_atomic_helper_shutdown() or equiv at shutdown time
Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() (or
drm_helper_force_disable_all() if not using atomic) at system shutdown
time. Among other things, this means that if a panel is in use that it
won't be cleanly powered off at system shutdown time.
The fact that we should call drm_atomic_helper_shutdown() in the case
of OS shutdown/restart comes straight out of the kernel doc "driver
instance overview" in drm_drv.c.
Suggested-by: Maxime Ripard <mripard at kernel.org>
Signed-off-by: Douglas Anderson <dianders at chromium.org>
---
This commit is only compile-time tested. I made my best guess about
how to fit this into the existing code. If someone wishes a different
style, please yell.
drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++++++++
drivers/gpu/drm/nouveau/nouveau_display.h | 1 +
drivers/gpu/drm/nouveau/nouveau_drm.c | 13 +++++++++++++
drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
drivers/gpu/drm/nouveau/nouveau_platform.c | 6 ++++++
5 files changed, 30 insertions(+)
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 99977e5fe716..04b3a3c0b5a5 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -642,6 +642,15 @@ nouveau_display_fini(struct drm_device *dev, bool suspend,
bool runtime)
disp->fini(dev, runtime, suspend);
}
+void
+nouveau_display_shutdown(struct drm_device *dev)
+{
+ if (drm_drv_uses_atomic_modeset(dev))
+ drm_atomic_helper_shutdown(dev);
+ else
+ drm_helper_force_disable_all(dev);
+}
+
static void
nouveau_display_create_properties(struct drm_device *dev)
{
diff --git a/drivers/gpu/drm/nouveau/nouveau_display.h
b/drivers/gpu/drm/nouveau/nouveau_display.h
index 2ab2ddb1eadf..9df62e833cda 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.h
+++ b/drivers/gpu/drm/nouveau/nouveau_display.h
@@ -47,6 +47,7 @@ void nouveau_display_destroy(struct drm_device *dev);
int nouveau_display_init(struct drm_device *dev, bool resume, bool runtime);
void nouveau_display_hpd_resume(struct drm_device *dev);
void nouveau_display_fini(struct drm_device *dev, bool suspend, bool runtime);
+void nouveau_display_shutdown(struct drm_device *dev);
int nouveau_display_suspend(struct drm_device *dev, bool runtime);
void nouveau_display_resume(struct drm_device *dev, bool runtime);
int nouveau_display_vblank_enable(struct drm_crtc *crtc);
diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
b/drivers/gpu/drm/nouveau/nouveau_drm.c
index 4396f501b16a..d8e8160cdf35 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
@@ -879,6 +879,18 @@ nouveau_drm_remove(struct pci_dev *pdev)
pci_disable_device(pdev);
}
+void
+nouveau_drm_device_shutdown(struct drm_device *dev)
+{
+ nouveau_display_shutdown(dev);
+}
+
+static void
+nouveau_drm_shutdown(struct pci_dev *pdev)
+{
+ nouveau_drm_device_shutdown(pci_get_drvdata(pdev));
+}
+
static int
nouveau_do_suspend(struct drm_device *dev, bool runtime)
{
@@ -1343,6 +1355,7 @@ nouveau_drm_pci_driver = {
.id_table = nouveau_drm_pci_table,
.probe = nouveau_drm_probe,
.remove = nouveau_drm_remove,
+ .shutdown = nouveau_drm_shutdown,
.driver.pm = &nouveau_pm_ops,
};
diff --git a/drivers/gpu/drm/nouveau/nouveau_drv.h
b/drivers/gpu/drm/nouveau/nouveau_drv.h
index 3666a7403e47..aa936cabb6cf 100644
--- a/drivers/gpu/drm/nouveau/nouveau_drv.h
+++ b/drivers/gpu/drm/nouveau/nouveau_drv.h
@@ -327,6 +327,7 @@ struct drm_device *
nouveau_platform_device_create(const struct nvkm_device_tegra_func *,
struct platform_device *, struct nvkm_device **);
void nouveau_drm_device_remove(struct drm_device *dev);
+void nouveau_drm_device_shutdown(struct drm_device *dev);
#define NV_PRINTK(l,c,f,a...) do {
\
struct nouveau_cli *_cli = (c); \
diff --git a/drivers/gpu/drm/nouveau/nouveau_platform.c
b/drivers/gpu/drm/nouveau/nouveau_platform.c
index 23cd43a7fd19..b2e82a96411c 100644
--- a/drivers/gpu/drm/nouveau/nouveau_platform.c
+++ b/drivers/gpu/drm/nouveau/nouveau_platform.c
@@ -50,6 +50,11 @@ static int nouveau_platform_remove(struct platform_device
*pdev)
return 0;
}
+static void nouveau_platform_shutdown(struct platform_device *pdev)
+{
+ nouveau_drm_device_shutdown(platform_get_drvdata(pdev));
+}
+
#if IS_ENABLED(CONFIG_OF)
static const struct nvkm_device_tegra_func gk20a_platform_data = {
.iommu_bit = 34,
@@ -94,4 +99,5 @@ struct platform_driver nouveau_platform_driver = {
},
.probe = nouveau_platform_probe,
.remove = nouveau_platform_remove,
+ .shutdown = nouveau_platform_shutdown,
};
--
2.42.0.283.g2d96d420d3-goog