diff mbox series

[v2] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

Message ID 20240611102744.v2.1.I2b014f90afc4729b6ecc7b5ddd1f6dedcea4625b@changeid (mailing list archive)
State New, archived
Headers show
Series [v2] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time | expand

Commit Message

Doug Anderson June 11, 2024, 5:27 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.

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 as of commit 2a073968289d
("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
noop") we don't need to confirm that "drm" is non-NULL.

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>
---
v1 of this patch was part of a series [1]. It got tested and reviewed
but never landed. Reposting separately in the hopes that Mediatek DRM
folks will land it. If, instead, Mediatek DRM folks want to Ack it I'm
happy to land through drm-misc.

I noticed that this was missing when I failed to add "mediatek" to my
patch series IDing which DRM modeset drivers did this properly [2].
Assuming my patch lands, that means that Mediatek devices will start
getting more warnings printed.

[1] https://lore.kernel.org/r/20230901164111.RFT.5.I2b014f90afc4729b6ecc7b5ddd1f6dedcea4625b@changeid
[2] https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid

Changes in v2:
- Removed NULL check since it's not needed since 6.7
- Rebased on ToT.

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

Comments

Maxime Ripard June 12, 2024, 7:56 a.m. UTC | #1
On Tue, 11 Jun 2024 10:27:44 -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.
> 
> [...]

Applied to misc/kernel.git (drm-misc-fixes).

Thanks!
Maxime
Linus Walleij June 17, 2024, 9:04 a.m. UTC | #2
On Tue, Jun 11, 2024 at 7:28 PM Douglas Anderson <dianders@chromium.org> wrote:

> Based on grepping through the source code this driver appears to be
> missing a call to drm_atomic_helper_shutdown() 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 as of commit 2a073968289d
> ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
> noop") we don't need to confirm that "drm" is non-NULL.
>
> 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>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Chun-Kuang Hu June 27, 2024, 2:02 p.m. UTC | #3
Hi, Douglas

Douglas Anderson <dianders@chromium.org> 於 2024年6月12日 週三 上午1:28寫道:
>
> 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 as of commit 2a073968289d
> ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
> noop") we don't need to confirm that "drm" is non-NULL.

Applied to mediatek-drm-next [1], thanks.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/log/?h=mediatek-drm-next

>
> 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>
> ---
> v1 of this patch was part of a series [1]. It got tested and reviewed
> but never landed. Reposting separately in the hopes that Mediatek DRM
> folks will land it. If, instead, Mediatek DRM folks want to Ack it I'm
> happy to land through drm-misc.
>
> I noticed that this was missing when I failed to add "mediatek" to my
> patch series IDing which DRM modeset drivers did this properly [2].
> Assuming my patch lands, that means that Mediatek devices will start
> getting more warnings printed.
>
> [1] https://lore.kernel.org/r/20230901164111.RFT.5.I2b014f90afc4729b6ecc7b5ddd1f6dedcea4625b@changeid
> [2] https://lore.kernel.org/r/20240611074846.1.Ieb287c2c3ee3f6d3b0d5f49b29f746b93621749c@changeid
>
> Changes in v2:
> - Removed NULL check since it's not needed since 6.7
> - Rebased on ToT.
>
>  drivers/gpu/drm/mediatek/mtk_drm_drv.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> index b5f605751b0a..de811e2265da 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
> @@ -952,6 +952,13 @@ 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);
> +
> +       drm_atomic_helper_shutdown(private->drm);
> +}
> +
>  static int mtk_drm_sys_prepare(struct device *dev)
>  {
>         struct mtk_drm_private *private = dev_get_drvdata(dev);
> @@ -983,6 +990,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.45.2.505.gda0bf45e8d-goog
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_drv.c b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
index b5f605751b0a..de811e2265da 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -952,6 +952,13 @@  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);
+
+	drm_atomic_helper_shutdown(private->drm);
+}
+
 static int mtk_drm_sys_prepare(struct device *dev)
 {
 	struct mtk_drm_private *private = dev_get_drvdata(dev);
@@ -983,6 +990,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,