| Message ID | 20230901163944.RFT.6.I21e0916bbd276033f7d31979c0da171458dedd4d@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: drm-misc drivers call drm_atomic_helper_shutdown() at the right times | expand |
On Fri, 1 Sep 2023 16:39:57 -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. > > [ ... ] Acked-by: Maxime Ripard <mripard@kernel.org> Thanks! Maxime
Hi, On Fri, Sep 1, 2023 at 4:41 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. > > I have attempted to put this in the right place at unbind time. In > most other DRM drivers the call is made right after the call to > drm_kms_helper_poll_fini(), so I've put it there. That means that this > call will also be made in the case that we hit errors in bind, since > kirin_drm_kms_cleanup() is called both in the bind error path and in > unbind. I believe this is harmless even though it's not needed in the > bind error path. > > For handling shutdown, we rely on the common technique of seeing if > the drvdata is NULL to know whether we need to call > drm_atomic_helper_shutdown(). This makes it important to make sure > that the drvdata is NULL if bind failed or if unbind was called. We > don't need the actual check for NULL and we'll rely on the patch > ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a > noop"). > > 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/hisilicon/kirin/kirin_drm_drv.c | 9 +++++++++ > 1 file changed, 9 insertions(+) Landed in drm-misc-next: 918ce0906dcd drm/hisilicon/kirin: Call drm_atomic_helper_shutdown() at shutdown/unbind time
diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c index e8c77bcc6dae..75292a2f4644 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_drv.c @@ -206,6 +206,7 @@ static int kirin_drm_kms_init(struct drm_device *dev, static int kirin_drm_kms_cleanup(struct drm_device *dev) { drm_kms_helper_poll_fini(dev); + drm_atomic_helper_shutdown(dev); kirin_drm_private_cleanup(dev); drm_mode_config_cleanup(dev); @@ -244,6 +245,7 @@ static int kirin_drm_bind(struct device *dev) kirin_drm_kms_cleanup(drm_dev); err_drm_dev_put: drm_dev_put(drm_dev); + dev_set_drvdata(dev, NULL); return ret; } @@ -255,6 +257,7 @@ static void kirin_drm_unbind(struct device *dev) drm_dev_unregister(drm_dev); kirin_drm_kms_cleanup(drm_dev); drm_dev_put(drm_dev); + dev_set_drvdata(dev, NULL); } static const struct component_master_ops kirin_drm_ops = { @@ -284,6 +287,11 @@ static void kirin_drm_platform_remove(struct platform_device *pdev) component_master_del(&pdev->dev, &kirin_drm_ops); } +static void kirin_drm_platform_shutdown(struct platform_device *pdev) +{ + drm_atomic_helper_shutdown(platform_get_drvdata(pdev)); +} + static const struct of_device_id kirin_drm_dt_ids[] = { { .compatible = "hisilicon,hi6220-ade", .data = &ade_driver_data, @@ -295,6 +303,7 @@ MODULE_DEVICE_TABLE(of, kirin_drm_dt_ids); static struct platform_driver kirin_drm_platform_driver = { .probe = kirin_drm_platform_probe, .remove_new = kirin_drm_platform_remove, + .shutdown = kirin_drm_platform_shutdown, .driver = { .name = "kirin-drm", .of_match_table = kirin_drm_dt_ids,
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. I have attempted to put this in the right place at unbind time. In most other DRM drivers the call is made right after the call to drm_kms_helper_poll_fini(), so I've put it there. That means that this call will also be made in the case that we hit errors in bind, since kirin_drm_kms_cleanup() is called both in the bind error path and in unbind. I believe this is harmless even though it's not needed in the bind error path. For handling shutdown, we rely on the common technique of seeing if the drvdata is NULL to know whether we need to call drm_atomic_helper_shutdown(). This makes it important to make sure that the drvdata is NULL if bind failed or if unbind was called. We don't need the actual check for NULL and we'll rely on the patch ("drm/atomic-helper: drm_atomic_helper_shutdown(NULL) should be a noop"). 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/hisilicon/kirin/kirin_drm_drv.c | 9 +++++++++ 1 file changed, 9 insertions(+)