[RFT PATCH v2 00/12] drm: call drm_atomic_helper_shutdown() at the right times

[RFT PATCH v2 00/12] drm: call drm_atomic_helper_shutdown() at the right times

[Douglas Anderson]

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

-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 01/12] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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.

(no changes since v1)

 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 +
 3 files changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/imx/dcss/dcss-drv.c b/drivers/gpu/drm/imx/dcss/dcss-drv.c
index c68b0d93ae9e..b61cec0cc79d 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-drv.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-drv.c
@@ -92,6 +92,13 @@ static int dcss_drv_platform_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void dcss_drv_platform_shutdown(struct platform_device *pdev)
+{
+	struct dcss_drv *mdrv = dev_get_drvdata(&pdev->dev);
+
+	dcss_kms_shutdown(mdrv->kms);
+}
+
 static struct dcss_type_data dcss_types[] = {
 	[DCSS_IMX8MQ] = {
 		.name = "DCSS_IMX8MQ",
@@ -114,6 +121,7 @@ MODULE_DEVICE_TABLE(of, dcss_of_match);
 static struct platform_driver dcss_platform_driver = {
 	.probe	= dcss_drv_platform_probe,
 	.remove	= dcss_drv_platform_remove,
+	.shutdown = dcss_drv_platform_shutdown,
 	.driver	= {
 		.name = "imx-dcss",
 		.of_match_table	= dcss_of_match,
diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
index 896de946f8df..d0ea4e97cded 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
+++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
@@ -172,3 +172,10 @@ void dcss_kms_detach(struct dcss_kms_dev *kms)
 	dcss_crtc_deinit(&kms->crtc, drm);
 	drm->dev_private = NULL;
 }
+
+void dcss_kms_shutdown(struct dcss_kms_dev *kms)
+{
+	struct drm_device *drm = &kms->base;
+
+	drm_atomic_helper_shutdown(drm);
+}
diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.h b/drivers/gpu/drm/imx/dcss/dcss-kms.h
index dfe5dd99eea3..62521c1fd6d2 100644
--- a/drivers/gpu/drm/imx/dcss/dcss-kms.h
+++ b/drivers/gpu/drm/imx/dcss/dcss-kms.h
@@ -34,6 +34,7 @@ struct dcss_kms_dev {
 
 struct dcss_kms_dev *dcss_kms_attach(struct dcss_dev *dcss);
 void dcss_kms_detach(struct dcss_kms_dev *kms);
+void dcss_kms_shutdown(struct dcss_kms_dev *kms);
 int dcss_crtc_init(struct dcss_crtc *crtc, struct drm_device *drm);
 void dcss_crtc_deinit(struct dcss_crtc *crtc, struct drm_device *drm);
 struct dcss_plane *dcss_plane_init(struct drm_device *drm,
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [RFT PATCH v2 01/12] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Laurentiu Palcu]

Hi,

On Thu, Sep 21, 2023 at 12:26:44PM -0700, 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. 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>

No issues found on i.MX8MQ.

Tested-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

Thanks,
Laurentiu

> ---
> This commit is only compile-time tested.
> 
> (no changes since v1)
> 
>  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 +
>  3 files changed, 16 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-drv.c b/drivers/gpu/drm/imx/dcss/dcss-drv.c
> index c68b0d93ae9e..b61cec0cc79d 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-drv.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-drv.c
> @@ -92,6 +92,13 @@ static int dcss_drv_platform_remove(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static void dcss_drv_platform_shutdown(struct platform_device *pdev)
> +{
> +	struct dcss_drv *mdrv = dev_get_drvdata(&pdev->dev);
> +
> +	dcss_kms_shutdown(mdrv->kms);
> +}
> +
>  static struct dcss_type_data dcss_types[] = {
>  	[DCSS_IMX8MQ] = {
>  		.name = "DCSS_IMX8MQ",
> @@ -114,6 +121,7 @@ MODULE_DEVICE_TABLE(of, dcss_of_match);
>  static struct platform_driver dcss_platform_driver = {
>  	.probe	= dcss_drv_platform_probe,
>  	.remove	= dcss_drv_platform_remove,
> +	.shutdown = dcss_drv_platform_shutdown,
>  	.driver	= {
>  		.name = "imx-dcss",
>  		.of_match_table	= dcss_of_match,
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.c b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> index 896de946f8df..d0ea4e97cded 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-kms.c
> +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.c
> @@ -172,3 +172,10 @@ void dcss_kms_detach(struct dcss_kms_dev *kms)
>  	dcss_crtc_deinit(&kms->crtc, drm);
>  	drm->dev_private = NULL;
>  }
> +
> +void dcss_kms_shutdown(struct dcss_kms_dev *kms)
> +{
> +	struct drm_device *drm = &kms->base;
> +
> +	drm_atomic_helper_shutdown(drm);
> +}
> diff --git a/drivers/gpu/drm/imx/dcss/dcss-kms.h b/drivers/gpu/drm/imx/dcss/dcss-kms.h
> index dfe5dd99eea3..62521c1fd6d2 100644
> --- a/drivers/gpu/drm/imx/dcss/dcss-kms.h
> +++ b/drivers/gpu/drm/imx/dcss/dcss-kms.h
> @@ -34,6 +34,7 @@ struct dcss_kms_dev {
>  
>  struct dcss_kms_dev *dcss_kms_attach(struct dcss_dev *dcss);
>  void dcss_kms_detach(struct dcss_kms_dev *kms);
> +void dcss_kms_shutdown(struct dcss_kms_dev *kms);
>  int dcss_crtc_init(struct dcss_crtc *crtc, struct drm_device *drm);
>  void dcss_crtc_deinit(struct dcss_crtc *crtc, struct drm_device *drm);
>  struct dcss_plane *dcss_plane_init(struct drm_device *drm,
> -- 
> 2.42.0.515.g380fc7ccd1-goog
> 

Re: [RFT PATCH v2 01/12] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Fri, Sep 22, 2023 at 12:56 AM Laurentiu Palcu
<laurentiu.palcu@oss.nxp.com> wrote:
>
> Hi,
>
> On Thu, Sep 21, 2023 at 12:26:44PM -0700, 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. 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>
>
> No issues found on i.MX8MQ.
>
> Tested-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>

Thanks! Would you expect this patch to land through drm-misc? If so,
I'm happy to commit it there with your tags. If patches to this driver
normally flow through drm-misc, I'm also happy to post a patch to
MAINTAINERS (or review a patch you post) adding this to the entry for
"NXP i.MX 8MQ DCSS DRIVER":

T:     git git://anongit.freedesktop.org/drm/drm-misc

...which would make it obvious in the future that things should land
through drm-misc. This is similar to what I did for commit
92e62478b62c ("MAINTAINERS: Update DRM DRIVERS FOR FREESCALE IMX
entry"). :-)

-Doug

Re: [RFT PATCH v2 01/12] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Laurentiu Palcu]

Hi Doug,

On Fri, Sep 22, 2023 at 08:44:16AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Fri, Sep 22, 2023 at 12:56 AM Laurentiu Palcu
> <laurentiu.palcu@oss.nxp.com> wrote:
> >
> > Hi,
> >
> > On Thu, Sep 21, 2023 at 12:26:44PM -0700, 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. 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>
> >
> > No issues found on i.MX8MQ.
> >
> > Tested-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> 
> Thanks! Would you expect this patch to land through drm-misc? If so,
> I'm happy to commit it there with your tags.

Yes, please do. The i.MX8MQ DCSS patches go through drm-misc.

> If patches to this driver normally flow through drm-misc, I'm also
> happy to post a patch to MAINTAINERS (or review a patch you post)
> adding this to the entry for "NXP i.MX 8MQ DCSS DRIVER":
> 
> T:     git git://anongit.freedesktop.org/drm/drm-misc
> 
> ...which would make it obvious in the future that things should land
> through drm-misc.

Thanks, that sounds good.

Laurentiu

Re: [RFT PATCH v2 01/12] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Sun, Sep 24, 2023 at 10:47 PM Laurentiu Palcu
<laurentiu.palcu@oss.nxp.com> wrote:
>
> Hi Doug,
>
> On Fri, Sep 22, 2023 at 08:44:16AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, Sep 22, 2023 at 12:56 AM Laurentiu Palcu
> > <laurentiu.palcu@oss.nxp.com> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Sep 21, 2023 at 12:26:44PM -0700, 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. 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>
> > >
> > > No issues found on i.MX8MQ.
> > >
> > > Tested-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> > > Reviewed-by: Laurentiu Palcu <laurentiu.palcu@oss.nxp.com>
> >
> > Thanks! Would you expect this patch to land through drm-misc? If so,
> > I'm happy to commit it there with your tags.
>
> Yes, please do. The i.MX8MQ DCSS patches go through drm-misc.

OK, landed in drm-misc-next:

89755ee1d593 drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time


> > If patches to this driver normally flow through drm-misc, I'm also
> > happy to post a patch to MAINTAINERS (or review a patch you post)
> > adding this to the entry for "NXP i.MX 8MQ DCSS DRIVER":
> >
> > T:     git git://anongit.freedesktop.org/drm/drm-misc
> >
> > ...which would make it obvious in the future that things should land
> > through drm-misc.
>
> Thanks, that sounds good.

https://lore.kernel.org/r/20230925154929.1.I3287e895ce8e68d41b458494a49a1b5ec5c71013@changeid

[RFT PATCH v2 02/12] drm/kmb: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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.

(no changes since v1)

 drivers/gpu/drm/kmb/kmb_drv.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/kmb/kmb_drv.c b/drivers/gpu/drm/kmb/kmb_drv.c
index 24035b53441c..af9bd34fefc0 100644
--- a/drivers/gpu/drm/kmb/kmb_drv.c
+++ b/drivers/gpu/drm/kmb/kmb_drv.c
@@ -476,6 +476,11 @@ static int kmb_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void kmb_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static int kmb_probe(struct platform_device *pdev)
 {
 	struct device *dev = get_device(&pdev->dev);
@@ -622,6 +627,7 @@ static SIMPLE_DEV_PM_OPS(kmb_pm_ops, kmb_pm_suspend, kmb_pm_resume);
 static struct platform_driver kmb_platform_driver = {
 	.probe = kmb_probe,
 	.remove = kmb_remove,
+	.shutdown = kmb_shutdown,
 	.driver = {
 		.name = "kmb-drm",
 		.pm = &kmb_pm_ops,
-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 03/12] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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.

This driver users the component model and shutdown happens in the base
driver. The "drvdata" for this driver will always be valid if
shutdown() is called and we know that if the "drm" pointer in our
private data is non-NULL then we need to call
drm_atomic_helper_shutdown(). 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.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Fei Shao <fshao@chromium.org>
Tested-by: Fei Shao <fshao@chromium.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v2:
- Rebased and resolved conflicts.

 drivers/gpu/drm/mediatek/mtk_drm_drv.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index d16cc8219105..6bab360c0c1a 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -919,6 +919,14 @@ static void mtk_drm_remove(struct platform_device *pdev)
 		of_node_put(private->comp_node[i]);
 }
 
+static void mtk_drm_shutdown(struct platform_device *pdev)
+{
+	struct mtk_drm_private *private = platform_get_drvdata(pdev);
+
+	if (private->drm)
+		drm_atomic_helper_shutdown(private->drm);
+}
+
 static int mtk_drm_sys_prepare(struct device *dev)
 {
 	struct mtk_drm_private *private = dev_get_drvdata(dev);
@@ -950,6 +958,7 @@ static const struct dev_pm_ops mtk_drm_pm_ops = {
 static struct platform_driver mtk_drm_platform_driver = {
 	.probe	= mtk_drm_probe,
 	.remove_new = mtk_drm_remove,
+	.shutdown = mtk_drm_shutdown,
 	.driver	= {
 		.name	= "mediatek-drm",
 		.pm     = &mtk_drm_pm_ops,
-- 
2.42.0.515.g380fc7ccd1-goog

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

[Douglas Anderson]

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,
 };
-- 
2.42.0.515.g380fc7ccd1-goog

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

[Lyude Paul]

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

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

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

[Doug Anderson]

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?

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

[Doug Anderson]

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[RFT PATCH v2 05/12] drm/tegra: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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.

(no changes since v1)

 drivers/gpu/drm/tegra/drm.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index ff36171c8fb7..ce2d4153f7bd 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -1312,6 +1312,11 @@ static int host1x_drm_remove(struct host1x_device *dev)
 	return 0;
 }
 
+static void host1x_drm_shutdown(struct host1x_device *dev)
+{
+	drm_atomic_helper_shutdown(dev_get_drvdata(&dev->dev));
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int host1x_drm_suspend(struct device *dev)
 {
@@ -1380,6 +1385,7 @@ static struct host1x_driver host1x_drm_driver = {
 	},
 	.probe = host1x_drm_probe,
 	.remove = host1x_drm_remove,
+	.shutdown = host1x_drm_shutdown,
 	.subdevs = host1x_drm_subdevs,
 };
 
-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 06/12] drm/arcpgu: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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.

(no changes since v1)

 drivers/gpu/drm/tiny/arcpgu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/tiny/arcpgu.c b/drivers/gpu/drm/tiny/arcpgu.c
index e5b10e41554a..c1e851c982e4 100644
--- a/drivers/gpu/drm/tiny/arcpgu.c
+++ b/drivers/gpu/drm/tiny/arcpgu.c
@@ -414,6 +414,11 @@ static int arcpgu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void arcpgu_shutdown(struct platform_device *pdev)
+{
+	drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 static const struct of_device_id arcpgu_of_table[] = {
 	{.compatible = "snps,arcpgu"},
 	{}
@@ -424,6 +429,7 @@ MODULE_DEVICE_TABLE(of, arcpgu_of_table);
 static struct platform_driver arcpgu_platform_driver = {
 	.probe = arcpgu_probe,
 	.remove = arcpgu_remove,
+	.shutdown = arcpgu_shutdown,
 	.driver = {
 		   .name = "arcpgu",
 		   .of_match_table = arcpgu_of_table,
-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 07/12] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Douglas Anderson]

Based on grepping through the source code this driver appears to be
missing a call to drm_atomic_helper_shutdown() 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

...and further, I'd say that this patch is more of a plea for help
than a patch I think is actually right. I'm _fairly_ certain that
drm/amdgpu needs this call at shutdown time but the logic is a bit
hard for me to follow. I'd appreciate if anyone who actually knows
what this should look like could illuminate me, or perhaps even just
post a patch themselves!

(no changes since v1)

 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 ++
 3 files changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 8f2255b3a38a..cfcff0b37466 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -1104,6 +1104,7 @@ static inline struct amdgpu_device *amdgpu_ttm_adev(struct ttm_device *bdev)
 int amdgpu_device_init(struct amdgpu_device *adev,
 		       uint32_t flags);
 void amdgpu_device_fini_hw(struct amdgpu_device *adev);
+void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
 void amdgpu_device_fini_sw(struct amdgpu_device *adev);
 
 int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a2cdde0ca0a7..fa5925c2092d 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -4247,6 +4247,16 @@ void amdgpu_device_fini_hw(struct amdgpu_device *adev)
 
 }
 
+void amdgpu_device_shutdown_hw(struct amdgpu_device *adev)
+{
+	if (adev->mode_info.mode_config_initialized) {
+		if (!drm_drv_uses_atomic_modeset(adev_to_drm(adev)))
+			drm_helper_force_disable_all(adev_to_drm(adev));
+		else
+			drm_atomic_helper_shutdown(adev_to_drm(adev));
+	}
+}
+
 void amdgpu_device_fini_sw(struct amdgpu_device *adev)
 {
 	int idx;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index e90f730eb715..3a7cbff111d1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -2333,6 +2333,8 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 	struct amdgpu_device *adev = drm_to_adev(dev);
 
+	amdgpu_device_shutdown_hw(adev);
+
 	if (amdgpu_ras_intr_triggered())
 		return;
 
-- 
2.42.0.515.g380fc7ccd1-goog

RE: [RFT PATCH v2 07/12] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Douglas Anderson <dianders@chromium.org>
> Sent: Thursday, September 21, 2023 3:27 PM
> To: dri-devel@lists.freedesktop.org; Maxime Ripard <mripard@kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>; Zhang, Bokun
> <Bokun.Zhang@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> Zhu, James <James.Zhu@amd.com>; Zhao, Victor <Victor.Zhao@amd.com>;
> Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Koenig,
> Christian <Christian.Koenig@amd.com>; daniel@ffwll.ch; Kuehling, Felix
> <Felix.Kuehling@amd.com>; jim.cromie@gmail.com; Ma, Le
> <Le.Ma@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; linux-
> kernel@vger.kernel.org; maarten.lankhorst@linux.intel.com; Limonciello,
> Mario <Mario.Limonciello@amd.com>; mdaenzer@redhat.com; Zhang,
> Morris <Shiwu.Zhang@amd.com>; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@amd.com>; tzimmermann@suse.de
> Subject: [RFT PATCH v2 07/12] drm/amdgpu: Call
> drm_atomic_helper_shutdown() at shutdown time
>
> Based on grepping through the source code this driver appears to be missing a
> call to drm_atomic_helper_shutdown() 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>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
> ...and further, I'd say that this patch is more of a plea for help than a patch I
> think is actually right. I'm _fairly_ certain that drm/amdgpu needs this call at
> shutdown time but the logic is a bit hard for me to follow. I'd appreciate if
> anyone who actually knows what this should look like could illuminate me, or
> perhaps even just post a patch themselves!
>
> (no changes since v1)
>
>  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 ++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 8f2255b3a38a..cfcff0b37466 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1104,6 +1104,7 @@ static inline struct amdgpu_device
> *amdgpu_ttm_adev(struct ttm_device *bdev)  int amdgpu_device_init(struct
> amdgpu_device *adev,
>                      uint32_t flags);
>  void amdgpu_device_fini_hw(struct amdgpu_device *adev);
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev);
>
>  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev); diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a2cdde0ca0a7..fa5925c2092d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4247,6 +4247,16 @@ void amdgpu_device_fini_hw(struct
> amdgpu_device *adev)
>
>  }
>
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev) {

This needs a better name since its only for displays.  Also maybe move it into amdgpu_display.c since it's really about turning off the displays.  That said is this really even needed?  The driver already calls its suspend functionality to turn off all of the hardware and put it into a quiescent state before shutdown.  Basically shares the same code we use for suspend.


> +     if (adev->mode_info.mode_config_initialized) {
> +             if (!drm_drv_uses_atomic_modeset(adev_to_drm(adev)))
> +                     drm_helper_force_disable_all(adev_to_drm(adev));
> +             else
> +                     drm_atomic_helper_shutdown(adev_to_drm(adev));
> +     }
> +}
> +
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev)  {
>       int idx;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index e90f730eb715..3a7cbff111d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2333,6 +2333,8 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
>       struct drm_device *dev = pci_get_drvdata(pdev);
>       struct amdgpu_device *adev = drm_to_adev(dev);
>
> +     amdgpu_device_shutdown_hw(adev);

I would move this after the ras_intr check below.

Alex

> +
>       if (amdgpu_ras_intr_triggered())
>               return;
>
> --
> 2.42.0.515.g380fc7ccd1-goog

Re: [RFT PATCH v2 07/12] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Mon, Sep 25, 2023 at 8:57 AM Deucher, Alexander
<Alexander.Deucher@amd.com> wrote:
>
> [Public]
>
> > -----Original Message-----
> > From: Douglas Anderson <dianders@chromium.org>
> > Sent: Thursday, September 21, 2023 3:27 PM
> > To: dri-devel@lists.freedesktop.org; Maxime Ripard <mripard@kernel.org>
> > Cc: Douglas Anderson <dianders@chromium.org>; Zhang, Bokun
> > <Bokun.Zhang@amd.com>; Zhang, Hawking <Hawking.Zhang@amd.com>;
> > Zhu, James <James.Zhu@amd.com>; Zhao, Victor <Victor.Zhao@amd.com>;
> > Pan, Xinhui <Xinhui.Pan@amd.com>; airlied@gmail.com; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Koenig,
> > Christian <Christian.Koenig@amd.com>; daniel@ffwll.ch; Kuehling, Felix
> > <Felix.Kuehling@amd.com>; jim.cromie@gmail.com; Ma, Le
> > <Le.Ma@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; linux-
> > kernel@vger.kernel.org; maarten.lankhorst@linux.intel.com; Limonciello,
> > Mario <Mario.Limonciello@amd.com>; mdaenzer@redhat.com; Zhang,
> > Morris <Shiwu.Zhang@amd.com>; SHANMUGAM, SRINIVASAN
> > <SRINIVASAN.SHANMUGAM@amd.com>; tzimmermann@suse.de
> > Subject: [RFT PATCH v2 07/12] drm/amdgpu: Call
> > drm_atomic_helper_shutdown() at shutdown time
> >
> > Based on grepping through the source code this driver appears to be missing a
> > call to drm_atomic_helper_shutdown() 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>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > This commit is only compile-time tested.
> >
> > ...and further, I'd say that this patch is more of a plea for help than a patch I
> > think is actually right. I'm _fairly_ certain that drm/amdgpu needs this call at
> > shutdown time but the logic is a bit hard for me to follow. I'd appreciate if
> > anyone who actually knows what this should look like could illuminate me, or
> > perhaps even just post a patch themselves!
> >
> > (no changes since v1)
> >
> >  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 ++
> >  3 files changed, 13 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 8f2255b3a38a..cfcff0b37466 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1104,6 +1104,7 @@ static inline struct amdgpu_device
> > *amdgpu_ttm_adev(struct ttm_device *bdev)  int amdgpu_device_init(struct
> > amdgpu_device *adev,
> >                      uint32_t flags);
> >  void amdgpu_device_fini_hw(struct amdgpu_device *adev);
> > +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
> >  void amdgpu_device_fini_sw(struct amdgpu_device *adev);
> >
> >  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev); diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index a2cdde0ca0a7..fa5925c2092d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -4247,6 +4247,16 @@ void amdgpu_device_fini_hw(struct
> > amdgpu_device *adev)
> >
> >  }
> >
> > +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev) {
>
> This needs a better name since its only for displays.  Also maybe move it into amdgpu_display.c since it's really about turning off the displays.  That said is this really even needed?  The driver already calls its suspend functionality to turn off all of the hardware and put it into a quiescent state before shutdown.  Basically shares the same code we use for suspend.

As per my comment above, for this driver, my patch was a "plea for
help". I have no idea if it's really needed or if suspend handles it.

My main concerns are:

a) If it's possible that someone out there is using this DRM driver
with a "drm_panel" then we need to make sure the panel gets disabled /
unprepared properly at shutdown time. The goal is to remove the
special logic in some panel drivers that disables the panel at
shutdown time. The guidance I got from Maxime is that we should be
relying on the DRM driver to disable panels at shutdown time and not
have extra per-panel code for it.

b) It is documented that DRM driers call drm_atomic_helper_shutdown()
at shutdown time. Even if things are working today, it's always
possible that something will change later and break for drivers that
aren't doing this.


If you're confident that everything is great for the "amdgpu" driver
then I'm happy to drop this patch and not consider it a blocker for
the eventual removal of the code in the individual panels drivers.

If, after reading this, you conclude that some sort of patch is
needed, I'd love it if you could test/post a patch yourself and then
I'll drop this patch from my series.


-Doug

[RFT PATCH v2 08/12] drm/sprd: Call drm_atomic_helper_shutdown() at remove time

[Douglas Anderson]

Based on grepping through the source code, this driver appears to be
missing a call to drm_atomic_helper_shutdown() at remove time. Let's
add it.

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

While at it, let's also fix it so that if the driver's bind fails or
if a driver gets unbound that the drvdata gets set to NULL. This will
make sure we can't get confused during a later shutdown().

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.

(no changes since v1)

 drivers/gpu/drm/sprd/sprd_drm.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/sprd/sprd_drm.c b/drivers/gpu/drm/sprd/sprd_drm.c
index 0aa39156f2fa..86a175116140 100644
--- a/drivers/gpu/drm/sprd/sprd_drm.c
+++ b/drivers/gpu/drm/sprd/sprd_drm.c
@@ -114,6 +114,7 @@ static int sprd_drm_bind(struct device *dev)
 	drm_kms_helper_poll_fini(drm);
 err_unbind_all:
 	component_unbind_all(drm->dev, drm);
+	platform_set_drvdata(pdev, NULL);
 	return ret;
 }
 
@@ -122,10 +123,11 @@ static void sprd_drm_unbind(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 
 	drm_dev_unregister(drm);
-
 	drm_kms_helper_poll_fini(drm);
+	drm_atomic_helper_shutdown(drm);
 
 	component_unbind_all(drm->dev, drm);
+	dev_set_drvdata(dev, NULL);
 }
 
 static const struct component_master_ops drm_component_ops = {
-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 09/12] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Douglas Anderson]

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>
---
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,
-- 
2.42.0.515.g380fc7ccd1-goog

Re: [RFT PATCH v2 09/12] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Marek Szyprowski]

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

Re: [RFT PATCH v2 09/12] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Inki Dae]

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
>

Re: [RFT PATCH v2 09/12] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Doug Anderson]

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
> >

Re: [RFT PATCH v2 09/12] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Inki Dae]

Hi,

2023년 10월 6일 (금) 오후 10:51, Doug Anderson <dianders@chromium.org>님이 작성:
>
> 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.

Ah, you are right. No dependency here. I will pick it up.

Thanks,
Inki Dae

>
>
> > 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
> > >

[RFT PATCH v2 10/12] drm/gma500: Call drm_helper_force_disable_all() at shutdown/remove time

[Douglas Anderson]

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>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

(no changes since v1)

 drivers/gpu/drm/gma500/psb_drv.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 8b64f61ffaf9..a5a399bbe8f5 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -20,6 +20,7 @@
 #include <acpi/video.h>
 
 #include <drm/drm.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_ioctl.h>
@@ -485,6 +486,12 @@ static void psb_pci_remove(struct pci_dev *pdev)
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
 	drm_dev_unregister(dev);
+	drm_helper_force_disable_all(dev);
+}
+
+static void psb_pci_shutdown(struct pci_dev *pdev)
+{
+	drm_helper_force_disable_all(pci_get_drvdata(pdev));
 }
 
 static DEFINE_RUNTIME_DEV_PM_OPS(psb_pm_ops, gma_power_suspend, gma_power_resume, NULL);
@@ -521,6 +528,7 @@ static struct pci_driver psb_pci_driver = {
 	.id_table = pciidlist,
 	.probe = psb_pci_probe,
 	.remove = psb_pci_remove,
+	.shutdown = psb_pci_shutdown,
 	.driver.pm = &psb_pm_ops,
 };
 
-- 
2.42.0.515.g380fc7ccd1-goog

[RFT PATCH v2 11/12] drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove time

[Douglas Anderson]

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.

NOTE: in order to get things inserted in the right place, I had to
replace the old/deprecated drm_put_dev() function with the equivalent
new calls.

Suggested-by: Maxime Ripard <mripard@kernel.org>
Reviewed-by: Maxime Ripard <mripard@kernel.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I honestly have no idea if I got this patch right. The shutdown()
function already had some special case logic for PPC, Loongson, and
VMs and I don't 100% for sure know how this interacts with
those. Everything here is just compile tested.

(no changes since v1)

 drivers/gpu/drm/radeon/radeon_drv.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/radeon/radeon_drv.c b/drivers/gpu/drm/radeon/radeon_drv.c
index 39cdede460b5..67995ea24852 100644
--- a/drivers/gpu/drm/radeon/radeon_drv.c
+++ b/drivers/gpu/drm/radeon/radeon_drv.c
@@ -38,6 +38,7 @@
 #include <linux/pci.h>
 
 #include <drm/drm_aperture.h>
+#include <drm/drm_crtc_helper.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_file.h>
 #include <drm/drm_gem.h>
@@ -357,7 +358,9 @@ radeon_pci_remove(struct pci_dev *pdev)
 {
 	struct drm_device *dev = pci_get_drvdata(pdev);
 
-	drm_put_dev(dev);
+	drm_dev_unregister(dev);
+	drm_helper_force_disable_all(dev);
+	drm_dev_put(dev);
 }
 
 static void
@@ -368,6 +371,8 @@ radeon_pci_shutdown(struct pci_dev *pdev)
 	 */
 	if (radeon_device_is_virtual())
 		radeon_pci_remove(pdev);
+	else
+		drm_helper_force_disable_all(pci_get_drvdata(pdev));
 
 #if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
 	/*
-- 
2.42.0.515.g380fc7ccd1-goog

RE: [RFT PATCH v2 11/12] drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove time

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Douglas Anderson <dianders@chromium.org>
> Sent: Thursday, September 21, 2023 3:27 PM
> To: dri-devel@lists.freedesktop.org; Maxime Ripard <mripard@kernel.org>
> Cc: Douglas Anderson <dianders@chromium.org>; Pan, Xinhui
> <Xinhui.Pan@amd.com>; airlied@gmail.com; Deucher, Alexander
> <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Koenig,
> Christian <Christian.Koenig@amd.com>; daniel@ffwll.ch; linux-
> kernel@vger.kernel.org
> Subject: [RFT PATCH v2 11/12] drm/radeon: Call
> drm_helper_force_disable_all() at shutdown/remove time
>
> 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.
>
> NOTE: in order to get things inserted in the right place, I had to replace the
> old/deprecated drm_put_dev() function with the equivalent new calls.
>
> Suggested-by: Maxime Ripard <mripard@kernel.org>
> Reviewed-by: Maxime Ripard <mripard@kernel.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> I honestly have no idea if I got this patch right. The shutdown() function
> already had some special case logic for PPC, Loongson, and VMs and I don't
> 100% for sure know how this interacts with those. Everything here is just
> compile tested.

I think the reason for most of this funniness is to reduce shutdown time.  Lots of users complain if driver takes a while to shutdown and there is a point to be made that if the system is going into power down, there is not much reason to spend a lot of time messing with the hardware.

Alex

>
> (no changes since v1)
>
>  drivers/gpu/drm/radeon/radeon_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
> b/drivers/gpu/drm/radeon/radeon_drv.c
> index 39cdede460b5..67995ea24852 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -38,6 +38,7 @@
>  #include <linux/pci.h>
>
>  #include <drm/drm_aperture.h>
> +#include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -357,7 +358,9 @@ radeon_pci_remove(struct pci_dev *pdev)  {
>       struct drm_device *dev = pci_get_drvdata(pdev);
>
> -     drm_put_dev(dev);
> +     drm_dev_unregister(dev);
> +     drm_helper_force_disable_all(dev);
> +     drm_dev_put(dev);
>  }
>
>  static void
> @@ -368,6 +371,8 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>        */
>       if (radeon_device_is_virtual())
>               radeon_pci_remove(pdev);
> +     else
> +             drm_helper_force_disable_all(pci_get_drvdata(pdev));
>
>  #if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
>       /*
> --
> 2.42.0.515.g380fc7ccd1-goog

Re: [RFT PATCH v2 11/12] drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove time

[Doug Anderson]

Hi,

On Mon, Sep 25, 2023 at 8:49 AM Deucher, Alexander
<Alexander.Deucher@amd.com> wrote:
>
> [Public]
>
> > -----Original Message-----
> > From: Douglas Anderson <dianders@chromium.org>
> > Sent: Thursday, September 21, 2023 3:27 PM
> > To: dri-devel@lists.freedesktop.org; Maxime Ripard <mripard@kernel.org>
> > Cc: Douglas Anderson <dianders@chromium.org>; Pan, Xinhui
> > <Xinhui.Pan@amd.com>; airlied@gmail.com; Deucher, Alexander
> > <Alexander.Deucher@amd.com>; amd-gfx@lists.freedesktop.org; Koenig,
> > Christian <Christian.Koenig@amd.com>; daniel@ffwll.ch; linux-
> > kernel@vger.kernel.org
> > Subject: [RFT PATCH v2 11/12] drm/radeon: Call
> > drm_helper_force_disable_all() at shutdown/remove time
> >
> > 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.
> >
> > NOTE: in order to get things inserted in the right place, I had to replace the
> > old/deprecated drm_put_dev() function with the equivalent new calls.
> >
> > Suggested-by: Maxime Ripard <mripard@kernel.org>
> > Reviewed-by: Maxime Ripard <mripard@kernel.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > I honestly have no idea if I got this patch right. The shutdown() function
> > already had some special case logic for PPC, Loongson, and VMs and I don't
> > 100% for sure know how this interacts with those. Everything here is just
> > compile tested.
>
> I think the reason for most of this funniness is to reduce shutdown time.  Lots of users complain if driver takes a while to shutdown and there is a point to be made that if the system is going into power down, there is not much reason to spend a lot of time messing with the hardware.

Sure, you don't want to do too much at shutdown time. That's the whole
reason that "shutdown" doesn't do a full remove / uninitialization of
all drivers. ...but drm_atomic_helper_shutdown() is documented to do
the things that are important for shutdown. Specifically, it cleanly
disables all of the displays. Depending on the display, this could
avoid temporary garbage on the display at reboot time or it could even
be important for the long term health of the panel.

-Doug

[RFT PATCH v2 12/12] drm/renesas/shmobile: Call drm_helper_force_disable_all() at shutdown/remove time

[Douglas Anderson]

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>
Cc: Geert Uytterhoeven <geert+renesas@glider.be>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

As Geert pointed out in response to v1 [1], this patch conflicts with
the patches doing atomic conversion [2]. Since those patches don't
appear to be landed yet, I'm simply reposting v1. If those patches
land, I'm more than happy to re-post this one. I'm also more than
happy if someone wants to incorporate these changes into a different
patch.

[1] https://lore.kernel.org/r/CAMuHMdWOB7d-KE3F7aeZvVimNuy_U30uk=PND7=tWmPzCd7_eg@mail.gmail.com
[2] https://lore.kernel.org/dri-devel/fd7a6702490bd431f314d6591551bb39e77e3304.1692178020.git.geert+renesas@glider.be/

Changes in v2:
- Rebased and resolved conflicts.

 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 e5db4e0095ba..8c4c9d17a79e 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,10 +180,18 @@ static void 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);
 }
 
+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;
@@ -287,6 +296,7 @@ static int shmob_drm_probe(struct platform_device *pdev)
 static struct platform_driver shmob_drm_platform_driver = {
 	.probe		= shmob_drm_probe,
 	.remove_new	= shmob_drm_remove,
+	.shutdown	= shmob_drm_shutdown,
 	.driver		= {
 		.name	= "shmob-drm",
 		.pm	= pm_sleep_ptr(&shmob_drm_pm_ops),
-- 
2.42.0.515.g380fc7ccd1-goog