| Message ID | 20230921192749.1542462-1-dianders@chromium.org |
|---|---|
| Headers | show |
| Series | drm: call drm_atomic_helper_shutdown() at the right times | expand |
On 21.09.2023 21:26, 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. > > 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 this fix: > - When adding drm_atomic_helper_shutdown() to the unbind path, I added > it after drm_kms_helper_poll_fini() since that's when other drivers > seemed to have it. > - Technically with a previous patch, ("drm/atomic-helper: > drm_atomic_helper_shutdown(NULL) should be a noop"), we don't > actually need to check to see if our "drm" pointer is NULL before > calling drm_atomic_helper_shutdown(). We'll leave the "if" test in, > though, so that this patch can land without any dependencies. It > could potentially be removed later. > - This patch also makes sure to set the drvdata to NULL in the case of > bind errors to make sure that shutdown can't access freed data. > > Suggested-by: Maxime Ripard <mripard@kernel.org> > Reviewed-by: Maxime Ripard <mripard@kernel.org> > Signed-off-by: Douglas Anderson <dianders@chromium.org> Seems to be working fine on all my test Exynos-based boards with display. Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com> > --- > This commit is only compile-time tested. > > (no changes since v1) > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > index 8399256cb5c9..5380fb6c55ae 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > @@ -300,6 +300,7 @@ static int exynos_drm_bind(struct device *dev) > drm_mode_config_cleanup(drm); > exynos_drm_cleanup_dma(drm); > kfree(private); > + dev_set_drvdata(dev, NULL); > err_free_drm: > drm_dev_put(drm); > > @@ -313,6 +314,7 @@ static void exynos_drm_unbind(struct device *dev) > drm_dev_unregister(drm); > > drm_kms_helper_poll_fini(drm); > + drm_atomic_helper_shutdown(drm); > > component_unbind_all(drm->dev, drm); > drm_mode_config_cleanup(drm); > @@ -350,9 +352,18 @@ static int exynos_drm_platform_remove(struct platform_device *pdev) > return 0; > } > > +static void exynos_drm_platform_shutdown(struct platform_device *pdev) > +{ > + struct drm_device *drm = platform_get_drvdata(pdev); > + > + if (drm) > + drm_atomic_helper_shutdown(drm); > +} > + > static struct platform_driver exynos_drm_platform_driver = { > .probe = exynos_drm_platform_probe, > .remove = exynos_drm_platform_remove, > + .shutdown = exynos_drm_platform_shutdown, > .driver = { > .name = "exynos-drm", > .pm = &exynos_drm_pm_ops, Best regards
Thanks for testing. :) Acked-by : Inki Dae <inki.dae@samsung.com> 2023년 9월 22일 (금) 오후 3:00, Marek Szyprowski <m.szyprowski@samsung.com>님이 작성: > > > On 21.09.2023 21:26, 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. > > > > 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 this fix: > > - When adding drm_atomic_helper_shutdown() to the unbind path, I added > > it after drm_kms_helper_poll_fini() since that's when other drivers > > seemed to have it. > > - Technically with a previous patch, ("drm/atomic-helper: > > drm_atomic_helper_shutdown(NULL) should be a noop"), we don't > > actually need to check to see if our "drm" pointer is NULL before > > calling drm_atomic_helper_shutdown(). We'll leave the "if" test in, > > though, so that this patch can land without any dependencies. It > > could potentially be removed later. > > - This patch also makes sure to set the drvdata to NULL in the case of > > bind errors to make sure that shutdown can't access freed data. > > > > Suggested-by: Maxime Ripard <mripard@kernel.org> > > Reviewed-by: Maxime Ripard <mripard@kernel.org> > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Seems to be working fine on all my test Exynos-based boards with display. > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > --- > > This commit is only compile-time tested. > > > > (no changes since v1) > > > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > index 8399256cb5c9..5380fb6c55ae 100644 > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > @@ -300,6 +300,7 @@ static int exynos_drm_bind(struct device *dev) > > drm_mode_config_cleanup(drm); > > exynos_drm_cleanup_dma(drm); > > kfree(private); > > + dev_set_drvdata(dev, NULL); > > err_free_drm: > > drm_dev_put(drm); > > > > @@ -313,6 +314,7 @@ static void exynos_drm_unbind(struct device *dev) > > drm_dev_unregister(drm); > > > > drm_kms_helper_poll_fini(drm); > > + drm_atomic_helper_shutdown(drm); > > > > component_unbind_all(drm->dev, drm); > > drm_mode_config_cleanup(drm); > > @@ -350,9 +352,18 @@ static int exynos_drm_platform_remove(struct platform_device *pdev) > > return 0; > > } > > > > +static void exynos_drm_platform_shutdown(struct platform_device *pdev) > > +{ > > + struct drm_device *drm = platform_get_drvdata(pdev); > > + > > + if (drm) > > + drm_atomic_helper_shutdown(drm); > > +} > > + > > static struct platform_driver exynos_drm_platform_driver = { > > .probe = exynos_drm_platform_probe, > > .remove = exynos_drm_platform_remove, > > + .shutdown = exynos_drm_platform_shutdown, > > .driver = { > > .name = "exynos-drm", > > .pm = &exynos_drm_pm_ops, > > Best regards > -- > Marek Szyprowski, PhD > Samsung R&D Institute Poland >
Hi, On Thu, Oct 5, 2023 at 7:20 PM Inki Dae <daeinki@gmail.com> wrote: > > Thanks for testing. :) > > Acked-by : Inki Dae <inki.dae@samsung.com> Inki: does that mean you'd like this to go through drm-misc? I'm happy to do that, but there are no dependencies here so this could easily go through your tree. > 2023년 9월 22일 (금) 오후 3:00, Marek Szyprowski <m.szyprowski@samsung.com>님이 작성: > > > > > > On 21.09.2023 21:26, 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. > > > > > > 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 this fix: > > > - When adding drm_atomic_helper_shutdown() to the unbind path, I added > > > it after drm_kms_helper_poll_fini() since that's when other drivers > > > seemed to have it. > > > - Technically with a previous patch, ("drm/atomic-helper: > > > drm_atomic_helper_shutdown(NULL) should be a noop"), we don't > > > actually need to check to see if our "drm" pointer is NULL before > > > calling drm_atomic_helper_shutdown(). We'll leave the "if" test in, > > > though, so that this patch can land without any dependencies. It > > > could potentially be removed later. > > > - This patch also makes sure to set the drvdata to NULL in the case of > > > bind errors to make sure that shutdown can't access freed data. > > > > > > Suggested-by: Maxime Ripard <mripard@kernel.org> > > > Reviewed-by: Maxime Ripard <mripard@kernel.org> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > > > Seems to be working fine on all my test Exynos-based boards with display. > > > > Tested-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > > Reviewed-by: Marek Szyprowski <m.szyprowski@samsung.com> > > > > > --- > > > This commit is only compile-time tested. > > > > > > (no changes since v1) > > > > > > drivers/gpu/drm/exynos/exynos_drm_drv.c | 11 +++++++++++ > > > 1 file changed, 11 insertions(+) > > > > > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.c b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > index 8399256cb5c9..5380fb6c55ae 100644 > > > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.c > > > @@ -300,6 +300,7 @@ static int exynos_drm_bind(struct device *dev) > > > drm_mode_config_cleanup(drm); > > > exynos_drm_cleanup_dma(drm); > > > kfree(private); > > > + dev_set_drvdata(dev, NULL); > > > err_free_drm: > > > drm_dev_put(drm); > > > > > > @@ -313,6 +314,7 @@ static void exynos_drm_unbind(struct device *dev) > > > drm_dev_unregister(drm); > > > > > > drm_kms_helper_poll_fini(drm); > > > + drm_atomic_helper_shutdown(drm); > > > > > > component_unbind_all(drm->dev, drm); > > > drm_mode_config_cleanup(drm); > > > @@ -350,9 +352,18 @@ static int exynos_drm_platform_remove(struct platform_device *pdev) > > > return 0; > > > } > > > > > > +static void exynos_drm_platform_shutdown(struct platform_device *pdev) > > > +{ > > > + struct drm_device *drm = platform_get_drvdata(pdev); > > > + > > > + if (drm) > > > + drm_atomic_helper_shutdown(drm); > > > +} > > > + > > > static struct platform_driver exynos_drm_platform_driver = { > > > .probe = exynos_drm_platform_probe, > > > .remove = exynos_drm_platform_remove, > > > + .shutdown = exynos_drm_platform_shutdown, > > > .driver = { > > > .name = "exynos-drm", > > > .pm = &exynos_drm_pm_ops, > > > > Best regards > > -- > > Marek Szyprowski, PhD > > Samsung R&D Institute Poland > >
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 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. 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 drivers 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. NOTE: I landed everything I could from v1 of the patch series [2] [3] to drm-misc. This v2 is everyone that is still left. If you'd like me to land one of the patches here to drm-misc for you, please say so. Otherwise I will assume maintainers will pick patches for their particular driver and land them. There are no dependencies. [1] https://lore.kernel.org/lkml/20230804140605.RFC.4.I930069a32baab6faf46d6b234f89613b5cec0f14@changeid [2] https://lore.kernel.org/r/20230901234015.566018-1-dianders@chromium.org [3] https://lore.kernel.org/r/20230901234202.566951-1-dianders@chromium.org Changes in v2: - Rebased and resolved conflicts. Douglas Anderson (12): drm/imx/dcss: 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/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/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/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 ++++++- drivers/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 ++++++ 20 files changed, 124 insertions(+), 2 deletions(-)