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