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(+)

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),
-- 
2.42.0.283.g2d96d420d3-goog

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

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

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