| Message ID | 2c28c0a137854d39b6bc997a21bd6d2db1f7a0a5.1687423204.git.geert+renesas@glider.be (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: renesas: shmobile: Atomic conversion + DT support | expand |
Hi Geert, Thank you for the patch. On Thu, Jun 22, 2023 at 11:21:44AM +0200, Geert Uytterhoeven wrote: > When the device is unbound from the driver, the display may be active. > Make sure it gets shut down. > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index a29c0c1093725b6e..636f1888b815579b 100644 > --- 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); I assume this will be turned into drm_atomic_helper_shutdown() later. Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > drm_kms_helper_poll_fini(ddev); > return 0; > }
Hi 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. > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > --- > drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c > index a29c0c1093725b6e..636f1888b815579b 100644 > --- 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() ? > drm_kms_helper_poll_fini(ddev); > return 0; > }
Hi Sui, On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng <suijingfeng@loongson.cn> 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? > > Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> > > Reviewed-by: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com> > > --- 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
Hi, On 2023/7/5 18:29, Geert Uytterhoeven wrote: > Hi Sui, > > On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng<suijingfeng@loongson.cn> 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@glider.be> >>> Reviewed-by: Laurent Pinchart<laurent.pinchart+renesas@ideasonboard.com> >>> --- 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 >
diff --git a/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c b/drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c index a29c0c1093725b6e..636f1888b815579b 100644 --- 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); drm_kms_helper_poll_fini(ddev); return 0; }
When the device is unbound from the driver, the display may be active. Make sure it gets shut down. Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be> --- drivers/gpu/drm/renesas/shmobile/shmob_drm_drv.c | 2 ++ 1 file changed, 2 insertions(+)