Hi,
Hi Sui, On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng <suijingfeng@xxxxxxxxxxx> wrote:On 2023/6/22 17:21, Geert Uytterhoeven wrote:When the device is unbound from the driver, the display may be active. Make sure it gets shut down.would you mind to give a short description why this is necessary.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?
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. :-)
Signed-off-by: Geert Uytterhoeven <geert+renesas@xxxxxxxxx> Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>--- 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);Is it that the DRM core recommend us to use drm_atomic_helper_disable_all() ?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.drm_kms_helper_poll_fini(ddev); return 0; }Gr{oetje,eeting}s, Geert
