diff mbox series

[RFT,v2,04/12] drm/nouveau: Call drm_atomic_helper_shutdown() or equiv at shutdown time

Message ID 20230921122641.RFT.v2.4.Ie7588ec6e0f93e8bc700e76b265ad1a7ad6b15ad@changeid (mailing list archive)
State New, archived
Headers show
Series drm: call drm_atomic_helper_shutdown() at the right times | expand

Commit Message

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

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

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

(no changes since v1)

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

Comments

Lyude Paul Sept. 22, 2023, 9:06 p.m. UTC | #1
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@redhat.com>
Tested-by: Lyude Paul <lyude@redhat.com>

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@kernel.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested. I made my best guess about
> how to fit this into the existing code. If someone wishes a different
> style, please yell.
> 
> (no changes since v1)
> 
>  drivers/gpu/drm/nouveau/nouveau_display.c  |  9 +++++++++
>  drivers/gpu/drm/nouveau/nouveau_display.h  |  1 +
>  drivers/gpu/drm/nouveau/nouveau_drm.c      | 13 +++++++++++++
>  drivers/gpu/drm/nouveau/nouveau_drv.h      |  1 +
>  drivers/gpu/drm/nouveau/nouveau_platform.c |  6 ++++++
>  5 files changed, 30 insertions(+)
> 
> diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c
> index 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,
>  };
Doug Anderson Nov. 17, 2023, 11 p.m. UTC | #2
Hi,

On Fri, Sep 22, 2023 at 2:06 PM Lyude Paul <lyude@redhat.com> wrote:
>
> 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@redhat.com>
> Tested-by: Lyude Paul <lyude@redhat.com>

Any idea of where / how this patch should land. Would you expect me to
land it through drm-misc, or would you expect it to go through someone
else's tree?
Doug Anderson Dec. 5, 2023, 8:45 p.m. UTC | #3
Hi,

On Fri, Nov 17, 2023 at 3:00 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Sep 22, 2023 at 2:06 PM Lyude Paul <lyude@redhat.com> wrote:
> >
> > 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@redhat.com>
> > Tested-by: Lyude Paul <lyude@redhat.com>
>
> Any idea of where / how this patch should land. Would you expect me to
> land it through drm-misc, or would you expect it to go through someone
> else's tree?

Still hoping to find a way to land this patch, since it's been
reviewed and tested. Would anyone object if I landed it via drm-misc?

-Doug
Maxime Ripard Dec. 6, 2023, 8:14 a.m. UTC | #4
On Tue, Dec 05, 2023 at 12:45:07PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Fri, Nov 17, 2023 at 3:00 PM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Fri, Sep 22, 2023 at 2:06 PM Lyude Paul <lyude@redhat.com> wrote:
> > >
> > > 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@redhat.com>
> > > Tested-by: Lyude Paul <lyude@redhat.com>
> >
> > Any idea of where / how this patch should land. Would you expect me to
> > land it through drm-misc, or would you expect it to go through someone
> > else's tree?
> 
> Still hoping to find a way to land this patch, since it's been
> reviewed and tested. Would anyone object if I landed it via drm-misc?

Nouveau isn't maintained in drm-misc, so I would expect it to go through
the usual nouveau tree. Lyude, Karol, Danilo?

Maxime
diff mbox series

Patch

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,
 };