| Message ID | 20230901164111.RFT.5.I2b014f90afc4729b6ecc7b5ddd1f6dedcea4625b@changeid (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | None | expand |
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
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 > >
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
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
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,
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(+)