actually very glad to see this because I think I've seen one bug in the wild
as a result of things not getting shut down :)
Reviewed-by: Lyude Paul <lyude@xxxxxxxxxx>
Tested-by: Lyude Paul <lyude@xxxxxxxxxx>
On Thu, 2023-09-21 at 12:26 -0700, Douglas Anderson wrote:
> 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@xxxxxxxxxx>
> Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> This commit is only compile-time tested. I made my best guess about
> how to fit this into the existing code. If someone wishes a different
> style, please yell.
>
> (no changes since v1)
>
> drivers/gpu/drm/nouveau/nouveau_display.c | 9 +++++++++
> drivers/gpu/drm/nouveau/nouveau_display.h | 1 +
> drivers/gpu/drm/nouveau/nouveau_drm.c | 13 +++++++++++++
> drivers/gpu/drm/nouveau/nouveau_drv.h | 1 +
> drivers/gpu/drm/nouveau/nouveau_platform.c | 6 ++++++
> 5 files changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index d8c92521226d..05c3688ccb76 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 50589f982d1a..8ecfd66b7aab 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)
> {
> @@ -1346,6 +1358,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,
> };
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
