| Message ID | 20230901164111.RFT.15.Iaf638a1d4c8b3c307a6192efabb4cbb06b195f15@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | drm: non-drm-misc drivers call drm_atomic_helper_shutdown() at the right times | expand |
Hi Douglas, On Sat, Sep 2, 2023 at 1:42 AM Douglas Anderson <dianders@chromium.org> wrote: > Based on grepping through the source code, this driver appears to be > missing a call to drm_atomic_helper_shutdown(), or in this case the > non-atomic equivalent drm_helper_force_disable_all(), at system > shutdown time and at driver remove time. This is important because > drm_helper_force_disable_all() will cause panels to get disabled > cleanly which may be important for their power sequencing. Future > changes will remove any custom powering off in individual panel > drivers so the DRM drivers need to start getting this right. > > The fact that we should call drm_atomic_helper_shutdown(), or in this > case the non-atomic equivalent drm_helper_force_disable_all(), 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> > Signed-off-by: Douglas Anderson <dianders@chromium.org> Thanks for your patch! > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > @@ -179,12 +180,20 @@ static int shmob_drm_remove(struct platform_device *pdev) > > drm_dev_unregister(ddev); > drm_kms_helper_poll_fini(ddev); > + drm_helper_force_disable_all(ddev); After "[PATCH v3 36/41] drm: renesas: shmobile: Atomic conversion part 1"[1], this function will already call drm_atomic_helper_shutdown()... > free_irq(sdev->irq, ddev); > drm_dev_put(ddev); > > return 0; > } > > +static void shmob_drm_shutdown(struct platform_device *pdev) > +{ > + struct shmob_drm_device *sdev = platform_get_drvdata(pdev); > + > + drm_helper_force_disable_all(sdev->ddev); ... and this should be replaced by a call to drm_atomic_helper_shutdown(). [1] https://lore.kernel.org/dri-devel/fd7a6702490bd431f314d6591551bb39e77e3304.1692178020.git.geert+renesas@glider.be Gr{oetje,eeting}s, Geert
Hi, On Mon, Sep 4, 2023 at 12:28 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Douglas, > > On Sat, Sep 2, 2023 at 1:42 AM Douglas Anderson <dianders@chromium.org> wrote: > > Based on grepping through the source code, this driver appears to be > > missing a call to drm_atomic_helper_shutdown(), or in this case the > > non-atomic equivalent drm_helper_force_disable_all(), at system > > shutdown time and at driver remove time. This is important because > > drm_helper_force_disable_all() will cause panels to get disabled > > cleanly which may be important for their power sequencing. Future > > changes will remove any custom powering off in individual panel > > drivers so the DRM drivers need to start getting this right. > > > > The fact that we should call drm_atomic_helper_shutdown(), or in this > > case the non-atomic equivalent drm_helper_force_disable_all(), 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> > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Thanks for your patch! > > > --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > > @@ -179,12 +180,20 @@ static int shmob_drm_remove(struct platform_device *pdev) > > > > drm_dev_unregister(ddev); > > drm_kms_helper_poll_fini(ddev); > > + drm_helper_force_disable_all(ddev); > > After "[PATCH v3 36/41] drm: renesas: shmobile: Atomic conversion part > 1"[1], this function will already call drm_atomic_helper_shutdown()... > > > free_irq(sdev->irq, ddev); > > drm_dev_put(ddev); > > > > return 0; > > } > > > > +static void shmob_drm_shutdown(struct platform_device *pdev) > > +{ > > + struct shmob_drm_device *sdev = platform_get_drvdata(pdev); > > + > > + drm_helper_force_disable_all(sdev->ddev); > > ... and this should be replaced by a call to drm_atomic_helper_shutdown(). > > [1] https://lore.kernel.org/dri-devel/fd7a6702490bd431f314d6591551bb39e77e3304.1692178020.git.geert+renesas@glider.be Ah, thanks! I will put this patch on hold and check back in a few weeks to see how things are looking. If you wanted to fold it into your series I certainly wouldn't object to it, but if not that's fine too. ;-) -Doug
diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c index 30493ce87419..d6dd46c925c5 100644 --- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c @@ -15,6 +15,7 @@ #include <linux/pm.h> #include <linux/slab.h> +#include <drm/drm_crtc_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fbdev_generic.h> #include <drm/drm_gem_dma_helper.h> @@ -179,12 +180,20 @@ static int shmob_drm_remove(struct platform_device *pdev) drm_dev_unregister(ddev); drm_kms_helper_poll_fini(ddev); + drm_helper_force_disable_all(ddev); free_irq(sdev->irq, ddev); drm_dev_put(ddev); return 0; } +static void shmob_drm_shutdown(struct platform_device *pdev) +{ + struct shmob_drm_device *sdev = platform_get_drvdata(pdev); + + drm_helper_force_disable_all(sdev->ddev); +} + static int shmob_drm_probe(struct platform_device *pdev) { struct shmob_drm_platform_data *pdata = pdev->dev.platform_data; @@ -289,6 +298,7 @@ static int shmob_drm_probe(struct platform_device *pdev) static struct platform_driver shmob_drm_platform_driver = { .probe = shmob_drm_probe, .remove = shmob_drm_remove, + .shutdown = shmob_drm_shutdown, .driver = { .name = "shmob-drm", .pm = pm_sleep_ptr(&shmob_drm_pm_ops),
Based on grepping through the source code, this driver appears to be missing a call to drm_atomic_helper_shutdown(), or in this case the non-atomic equivalent drm_helper_force_disable_all(), at system shutdown time and at driver remove time. This is important because drm_helper_force_disable_all() will cause panels to get disabled cleanly which may be important for their power sequencing. Future changes will remove any custom powering off in individual panel drivers so the DRM drivers need to start getting this right. The fact that we should call drm_atomic_helper_shutdown(), or in this case the non-atomic equivalent drm_helper_force_disable_all(), 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> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- This commit is only compile-time tested. drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 10 ++++++++++ 1 file changed, 10 insertions(+)