Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
Hi,
On 2023/7/5 18:29, Geert Uytterhoeven wrote:
Hi Sui,
On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng 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
Reviewed-by: Laurent Pinchart
--- 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
#include
+#include
#include
#include
#include
@@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev)
struct drm_device *ddev = >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
Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
Hi Sui,
On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng 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
> > Reviewed-by: Laurent Pinchart
> > --- 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
> > #include
> >
> > +#include
> > #include
> > #include
> > #include
> > @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device
> > *pdev)
> > struct drm_device *ddev = >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
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Re: [32/39] drm: renesas: shmobile: Shutdown the display on remove
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 Reviewed-by: Laurent Pinchart --- 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 #include +#include #include #include #include @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev) struct drm_device *ddev = >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; } -- Jingfeng
Re: [PATCH 32/39] drm: renesas: shmobile: Shutdown the display on remove
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 > --- > 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 > #include > > +#include > #include > #include > #include > @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev) > struct drm_device *ddev = >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 > drm_kms_helper_poll_fini(ddev); > return 0; > } -- Regards, Laurent Pinchart
[PATCH 32/39] drm: renesas: shmobile: Shutdown the display on remove
When the device is unbound from the driver, the display may be active. Make sure it gets shut down. Signed-off-by: Geert Uytterhoeven --- 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 #include +#include #include #include #include @@ -145,6 +146,7 @@ static int shmob_drm_remove(struct platform_device *pdev) struct drm_device *ddev = >ddev; drm_dev_unregister(ddev); + drm_helper_force_disable_all(ddev); drm_kms_helper_poll_fini(ddev); return 0; } -- 2.34.1
