[RFT PATCH 02/15] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[RFT PATCH 02/15] 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

 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.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 02/15] drm/imx/dcss: Call drm_atomic_helper_shutdown() at shutdown time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:41:13 -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.
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFT PATCH 05/15] 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

 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 961715dd5b11..8b1c9c992ca8 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_drv.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_drv.c
@@ -921,6 +921,14 @@ static int mtk_drm_remove(struct platform_device *pdev)
 	return 0;
 }
 
+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);
@@ -952,6 +960,7 @@ static const struct dev_pm_ops mtk_drm_pm_ops = {
 static struct platform_driver mtk_drm_platform_driver = {
 	.probe	= mtk_drm_probe,
 	.remove	= mtk_drm_remove,
+	.shutdown = mtk_drm_shutdown,
 	.driver	= {
 		.name	= "mediatek-drm",
 		.pm     = &mtk_drm_pm_ops,
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 05/15] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:41:16 -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.
> 
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 05/15] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

[Fei Shao]

Hi,

On Sat, Sep 2, 2023 at 7:42 AM Douglas Anderson <dianders@chromium.org> wrote:
...<snip>
> @@ -952,6 +960,7 @@ static const struct dev_pm_ops mtk_drm_pm_ops = {
>  static struct platform_driver mtk_drm_platform_driver = {
>         .probe  = mtk_drm_probe,
>         .remove = mtk_drm_remove,

I think this patch, and perhaps some others in this series, will have
a trivial conflict to Uwe's work about the remove callback conversion
e.g. [1], so you might want to rebase the series onto the latest
linux-next.

On the other hand, I tested this patch on MT8195 and MT8188
Chromebooks and I don't see issues during boot / reboot, so

Reviewed-by: Fei Shao <fshao@chromium.org>
Tested-by: Fei Shao <fshao@chromium.org>

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



> +       .shutdown = mtk_drm_shutdown,
>         .driver = {
>                 .name   = "mediatek-drm",
>                 .pm     = &mtk_drm_pm_ops,
> --
> 2.42.0.283.g2d96d420d3-goog
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 05/15] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Fri, Sep 8, 2023 at 4:51 AM Fei Shao <fshao@chromium.org> wrote:
>
> Hi,
>
> On Sat, Sep 2, 2023 at 7:42 AM Douglas Anderson <dianders@chromium.org> wrote:
> ...<snip>
> > @@ -952,6 +960,7 @@ static const struct dev_pm_ops mtk_drm_pm_ops = {
> >  static struct platform_driver mtk_drm_platform_driver = {
> >         .probe  = mtk_drm_probe,
> >         .remove = mtk_drm_remove,
>
> I think this patch, and perhaps some others in this series, will have
> a trivial conflict to Uwe's work about the remove callback conversion
> e.g. [1], so you might want to rebase the series onto the latest
> linux-next.
>
> On the other hand, I tested this patch on MT8195 and MT8188
> Chromebooks and I don't see issues during boot / reboot, so
>
> Reviewed-by: Fei Shao <fshao@chromium.org>
> Tested-by: Fei Shao <fshao@chromium.org>
>
> [1]: https://git.kernel.org/pub/scm/linux/kernel/git/chunkuang.hu/linux.git/commit/?h=mediatek-drm-next&id=b3af12a0b46888340e024ba8b231605bcf2d0ab3

That makes sense. I had based this series on drm-misc-next which
didn't have those, but now that a new -rc1 is out it then
drm-misc-next should rebase shortly. I'll make sure that the next
version includes Uwe's changes as much as possible.

That being said, I also wouldn't object if the maintainer of this DRM
driver wanted to resolve conflicts themselves and land the patch
without me needing to resend. The conflict is trivial, there are no
dependencies and no reason to land the series all at once, so landing
this patch early would mean less spam for the maintainer since they
would no longer get CCed on future versions. :-P Just sayin...

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 05/15] drm/mediatek: Call drm_atomic_helper_shutdown() at shutdown time

[Fei Shao]

On Tue, Sep 12, 2023 at 12:11 AM Doug Anderson <dianders@chromium.org> wrote:
>
[...]
>
> That makes sense. I had based this series on drm-misc-next which
> didn't have those, but now that a new -rc1 is out it then
> drm-misc-next should rebase shortly. I'll make sure that the next
> version includes Uwe's changes as much as possible.
>
> That being said, I also wouldn't object if the maintainer of this DRM
> driver wanted to resolve conflicts themselves and land the patch
> without me needing to resend. The conflict is trivial, there are no
> dependencies and no reason to land the series all at once, so landing
> this patch early would mean less spam for the maintainer since they
> would no longer get CCed on future versions. :-P Just sayin...

Oh then feel free to ignore that if the changes weren't in the tree at
that time... It was just a gentle reminder. Thanks for clarifying.  :)

Regards,
Fei

>
> -Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFT PATCH 11/15] 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

 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.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 11/15] drm/exynos: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:41:22 -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
> 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.
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFT PATCH 13/15] drm/imx/ipuv3: 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>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This commit is only compile-time tested.

 drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
index 4a866ac60fff..4c8bc49758a7 100644
--- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
@@ -257,6 +257,7 @@ static int imx_drm_bind(struct device *dev)
 	drm_kms_helper_poll_fini(drm);
 	component_unbind_all(drm->dev, drm);
 err_kms:
+	dev_set_drvdata(dev, NULL);
 	drm_dev_put(drm);
 
 	return ret;
@@ -269,6 +270,7 @@ static void imx_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);
 
@@ -298,6 +300,14 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static void imx_drm_platform_shutdown(struct platform_device *pdev)
+{
+	struct drm_device *drm = platform_get_drvdata(pdev);
+
+	if (drm)
+		drm_atomic_helper_shutdown(platform_get_drvdata(pdev));
+}
+
 #ifdef CONFIG_PM_SLEEP
 static int imx_drm_suspend(struct device *dev)
 {
@@ -325,6 +335,7 @@ MODULE_DEVICE_TABLE(of, imx_drm_dt_ids);
 static struct platform_driver imx_drm_pdrv = {
 	.probe		= imx_drm_platform_probe,
 	.remove		= imx_drm_platform_remove,
+	.shutdown	= imx_drm_platform_shutdown,
 	.driver		= {
 		.name	= "imx-drm",
 		.pm	= &imx_drm_pm_ops,
-- 
2.42.0.283.g2d96d420d3-goog


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Maxime Ripard]

On Fri, 1 Sep 2023 16:41:24 -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
> 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.
> 
> [ ... ]

Reviewed-by: Maxime Ripard <mripard@kernel.org>

Thanks!
Maxime

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Philipp Zabel]

On Fr, 2023-09-01 at 16:41 -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
> 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>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>

Thank you,
Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Doug Anderson]

Hi,

On Mon, Sep 4, 2023 at 1:30 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Fr, 2023-09-01 at 16:41 -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
> > 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>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
>
> Thank you,
> Tested-by: Philipp Zabel <p.zabel@pengutronix.de>

Thanks! I notice that:

./scripts/get_maintainer.pl --scm -f drivers/gpu/drm/imx/ipuv3/imx-drm-core.c

Doesn't say drm-misc but also when I look at the MAINTAINERS file and
find the section for "DRM DRIVERS FOR FREESCALE IMX" it doesn't
explicitly list a different git tree. I guess the "shawnguo" git tree
listed by get_maintainer.pl is just from regex matching?

Would you expect this to go through drm-misc? If so, I'll probably
land it sooner rather than later. I can also post up a patch making it
obvious that "DRM DRIVERS FOR FREESCALE IMX" goes through drm-misc if
you don't object.

Thanks!

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Philipp Zabel]

Hi,

On Di, 2023-09-05 at 13:29 -0700, Doug Anderson wrote:
> Hi,
> 
> On Mon, Sep 4, 2023 at 1:30 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > 
> > On Fr, 2023-09-01 at 16:41 -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
> > > 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>
> > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > 
> > Thank you,
> > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> 
> Thanks! I notice that:
> 
> ./scripts/get_maintainer.pl --scm -f drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> 
> Doesn't say drm-misc but also when I look at the MAINTAINERS file and
> find the section for "DRM DRIVERS FOR FREESCALE IMX"

That should probably say "IMX5/6" nowadays. There are a lot more i.MX
that do not use IPUv3 than those that do.

> it doesn't explicitly list a different git tree.

I used to send pull requests from git.pengutronix.de/git/pza/linux,
same as for the reset controller framework. I might still have to do
that for changes in drivers/gpu/ipu-v3 that need coordination between
drm and v4l2, but usually pure drm/imx/ipuv3 changes are pushed to drm-
misc.

> I guess the "shawnguo" git tree listed by get_maintainer.pl is just
> from regex matching?

The "N: imx" pattern in "ARM/FREESCALE IMX / MXC ARM ARCHITECTURE", I
think.

> Would you expect this to go through drm-misc? If so, I'll probably
> land it sooner rather than later. I can also post up a patch making it
> obvious that "DRM DRIVERS FOR FREESCALE IMX" goes through drm-misc if
> you don't object.

Yes, both would be great.

regards
Philipp

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Doug Anderson]

Hi,

On Tue, Sep 5, 2023 at 10:47 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> Hi,
>
> On Di, 2023-09-05 at 13:29 -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, Sep 4, 2023 at 1:30 AM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > On Fr, 2023-09-01 at 16:41 -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
> > > > 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>
> > > > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > >
> > > Thank you,
> > > Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
> >
> > Thanks! I notice that:
> >
> > ./scripts/get_maintainer.pl --scm -f drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> >
> > Doesn't say drm-misc but also when I look at the MAINTAINERS file and
> > find the section for "DRM DRIVERS FOR FREESCALE IMX"
>
> That should probably say "IMX5/6" nowadays. There are a lot more i.MX
> that do not use IPUv3 than those that do.
>
> > it doesn't explicitly list a different git tree.
>
> I used to send pull requests from git.pengutronix.de/git/pza/linux,
> same as for the reset controller framework. I might still have to do
> that for changes in drivers/gpu/ipu-v3 that need coordination between
> drm and v4l2, but usually pure drm/imx/ipuv3 changes are pushed to drm-
> misc.
>
> > I guess the "shawnguo" git tree listed by get_maintainer.pl is just
> > from regex matching?
>
> The "N: imx" pattern in "ARM/FREESCALE IMX / MXC ARM ARCHITECTURE", I
> think.
>
> > Would you expect this to go through drm-misc? If so, I'll probably
> > land it sooner rather than later. I can also post up a patch making it
> > obvious that "DRM DRIVERS FOR FREESCALE IMX" goes through drm-misc if
> > you don't object.
>
> Yes, both would be great.

Maintainers update posted at:

https://lore.kernel.org/r/20230906072803.1.Idef7e77e8961cbeb8625183eec9db0356b2eccd0@changeid

I'll aim to land ${SUBJECT} patch early next week unless there are any
objections.

-Doug

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [RFT PATCH 13/15] drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at shutdown/unbind time

[Doug Anderson]

Hi,

On Fri, Sep 1, 2023 at 4:42 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
> 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>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> This commit is only compile-time tested.
>
>  drivers/gpu/drm/imx/ipuv3/imx-drm-core.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> index 4a866ac60fff..4c8bc49758a7 100644
> --- a/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> +++ b/drivers/gpu/drm/imx/ipuv3/imx-drm-core.c
> @@ -257,6 +257,7 @@ static int imx_drm_bind(struct device *dev)
>         drm_kms_helper_poll_fini(drm);
>         component_unbind_all(drm->dev, drm);
>  err_kms:
> +       dev_set_drvdata(dev, NULL);
>         drm_dev_put(drm);
>
>         return ret;
> @@ -269,6 +270,7 @@ static void imx_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);
>
> @@ -298,6 +300,14 @@ static int imx_drm_platform_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static void imx_drm_platform_shutdown(struct platform_device *pdev)
> +{
> +       struct drm_device *drm = platform_get_drvdata(pdev);
> +
> +       if (drm)
> +               drm_atomic_helper_shutdown(platform_get_drvdata(pdev));

Since this is now landing through the drm-misc-next tree, I got rid of
the check for NULL when applying and landed this after the patch
("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a
noop").


> @@ -325,6 +335,7 @@ MODULE_DEVICE_TABLE(of, imx_drm_dt_ids);
>  static struct platform_driver imx_drm_pdrv = {
>         .probe          = imx_drm_platform_probe,
>         .remove         = imx_drm_platform_remove,
> +       .shutdown       = imx_drm_platform_shutdown,

There was a trivial context conflict with commit 3095f1122203
("drm/imx/ipuv3: Convert to platform remove callback returning void")
that I resolved while applying.

I've now pushed to drm-misc-next:

02680d71dea8 drm/imx/ipuv3: Call drm_atomic_helper_shutdown() at
shutdown/unbind time

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel