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

Commit Message

Doug Anderson Sept. 21, 2023, 7:26 p.m. UTC

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 <[e-mail]>
Reviewed-by: Maxime Ripard <[e-mail]>
Signed-off-by: Douglas Anderson <[e-mail]>
---
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(+)

Comments

Laurentiu Palcu Sept. 22, 2023, 7:56 a.m. UTC | #1

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 <[e-mail]>
> Reviewed-by: Maxime Ripard <[e-mail]>
> Signed-off-by: Douglas Anderson <[e-mail]>

No issues found on i.MX8MQ.

Tested-by: Laurentiu Palcu <[e-mail]>
Reviewed-by: Laurentiu Palcu <[e-mail]>

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
>

Doug Anderson Sept. 22, 2023, 3:44 p.m. UTC | #2

Hi,

On Fri, Sep 22, 2023 at 12:56 AM Laurentiu Palcu
<[e-mail]> 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 <[e-mail]>
> > Reviewed-by: Maxime Ripard <[e-mail]>
> > Signed-off-by: Douglas Anderson <[e-mail]>
>
> No issues found on i.MX8MQ.
>
> Tested-by: Laurentiu Palcu <[e-mail]>
> Reviewed-by: Laurentiu Palcu <[e-mail]>

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

Laurentiu Palcu Sept. 25, 2023, 5:47 a.m. UTC | #3

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
> <[e-mail]> 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 <[e-mail]>
> > > Reviewed-by: Maxime Ripard <[e-mail]>
> > > Signed-off-by: Douglas Anderson <[e-mail]>
> >
> > No issues found on i.MX8MQ.
> >
> > Tested-by: Laurentiu Palcu <[e-mail]>
> > Reviewed-by: Laurentiu Palcu <[e-mail]>
> 
> 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

Doug Anderson Sept. 25, 2023, 10:53 p.m. UTC | #4

Hi,

On Sun, Sep 24, 2023 at 10:47 PM Laurentiu Palcu
<[e-mail]> 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
> > <[e-mail]> 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 <[e-mail]>
> > > > Reviewed-by: Maxime Ripard <[e-mail]>
> > > > Signed-off-by: Douglas Anderson <[e-mail]>
> > >
> > > No issues found on i.MX8MQ.
> > >
> > > Tested-by: Laurentiu Palcu <[e-mail]>
> > > Reviewed-by: Laurentiu Palcu <[e-mail]>
> >
> > 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

Patch

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,