<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> </head> <body> Hi, <div class="moz-cite-prefix">On 2023/7/5 18:29, Geert Uytterhoeven wrote: </div> <blockquote type="cite" cite="mid:CAMuHMdXoZiDupub2zEFjOwTJFswEsJq62zVa_K-g6TWg+zS7-g@mail.gmail.com"> <pre class="moz-quote-pre" wrap="">Hi Sui, On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng <a class="moz-txt-link-rfc2396E" href="mailto:suijingfeng@loongson.cn"><suijingfeng@loongson.cn></a> wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">On 2023/6/22 17:21, Geert Uytterhoeven wrote: </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">When the device is unbound from the driver, the display may be active. Make sure it gets shut down. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> would you mind to give a short description why this is necessary. </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> That's a good comment. It turned out that this is not really necessary here, but to avoid a regression with "[PATCH 34/39] drm: renesas: shmobile: Atomic conversion part 1", where it is needed to call drm_atomic_helper_shutdown(). As the comments for drm_atomic_helper_shutdown() says it is the atomic version of drm_helper_force_disable_all(), I figured I had to introduce a call to the latter first, before doing the atomic conversion. Does that make sense? </pre> </blockquote> I'm just noticed that I'm actually ask a duplicated question. I didn't notice Laurent's remark about this patch. I'm actually agree with Laurent that this function should be turned into drm_atomic_helper_shutdown() finally. Yes, I do noticed that you replace this function with in [PATCH 34/39], Originally, I thought this patch could possibly merged with the [PATCH 34/39]. then, the net result of the merged two patch is equivalent to adding drm_atomic_helper_shutdown() function only in the shmob_drm_remove() function. I also realized that it is necessary that disable the CRTC when the driver going to leave. Otherwise there are some plane resource still being referenced. OK, I'm satisfy about you answer (if no other reviewers complains). Thanks for you answer. :-) <blockquote type="cite" cite="mid:CAMuHMdXoZiDupub2zEFjOwTJFswEsJq62zVa_K-g6TWg+zS7-g@mail.gmail.com"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">Signed-off-by: Geert Uytterhoeven <a class="moz-txt-link-rfc2396E" href="mailto:geert+renesas@glider.be"><geert+renesas@glider.be></a> Reviewed-by: Laurent Pinchart <a class="moz-txt-link-rfc2396E" href="mailto:laurent.pinchart+renesas@ideasonboard.com"><laurent.pinchart+renesas@ideasonboard.com></a> </pre> </blockquote> </blockquote> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <blockquote type="cite"> <pre class="moz-quote-pre" wrap="">--- a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c +++ b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c @@ -16,6 +16,7 @@ #include <linux/pm_runtime.h> #include <linux/slab.h> +#include <drm/drm_crtc_helper.h> #include <drm/drm_drv.h> #include <drm/drm_fbdev_generic.h> #include <drm/drm_gem_dma_helper.h> @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev) struct drm_device *ddev = &sdev->ddev; drm_dev_unregister(ddev); + drm_helper_force_disable_all(ddev); </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Is it that the DRM core recommend us to use drm_atomic_helper_disable_all() ? </pre> </blockquote> <pre class="moz-quote-pre" wrap=""> Well, drm_atomic_helper_shutdown() is a convenience wrapper around drm_atomic_helper_disable_all()... But we can't call any atomic helpers yet, before the conversion to atomic modesetting. </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> </pre> <blockquote type="cite"> <pre class="moz-quote-pre" wrap=""> drm_kms_helper_poll_fini(ddev); return 0; } </pre> </blockquote> </blockquote> <pre class="moz-quote-pre" wrap=""> Gr{oetje,eeting}s, Geert </pre> </blockquote> </body> </html>