[32/39] drm: renesas: shmobile: Shutdown the display on remove

[suijingfeng]

Hi,

On 2023/7/5 18:29, Geert Uytterhoeven wrote:
> Hi Sui,
>
> On Tue, Jun 27, 2023 at 4:57 PM Sui Jingfeng<suijingfeng at 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 at glider.be>
>>> Reviewed-by: Laurent Pinchart<laurent.pinchart+renesas at 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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20230706/df64eefb/attachment.htm>