RE: [RFT PATCH v2 11/12] drm/radeon: Call drm_helper_force_disable_all() at shutdown/remove time

[Deucher, Alexander]

[Public]

> -----Original Message-----
> From: Douglas Anderson <dianders@xxxxxxxxxxxx>
> Sent: Thursday, September 21, 2023 3:27 PM
> To: dri-devel@xxxxxxxxxxxxxxxxxxxxx; Maxime Ripard <mripard@xxxxxxxxxx>
> Cc: Douglas Anderson <dianders@xxxxxxxxxxxx>; Pan, Xinhui
> <Xinhui.Pan@xxxxxxx>; airlied@xxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Koenig,
> Christian <Christian.Koenig@xxxxxxx>; daniel@xxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: [RFT PATCH v2 11/12] drm/radeon: Call
> drm_helper_force_disable_all() at shutdown/remove time
>
> Based on grepping through the source code, this driver appears to be missing
> a call to drm_atomic_helper_shutdown(), or in this case the non-atomic
> equivalent drm_helper_force_disable_all(), at system shutdown time and at
> driver remove time. This is important because
> drm_helper_force_disable_all() will cause panels to get disabled cleanly which
> may be important for their power sequencing. Future changes will remove any
> custom powering off in individual panel drivers so the DRM drivers need to
> start getting this right.
>
> The fact that we should call drm_atomic_helper_shutdown(), or in this case
> the non-atomic equivalent drm_helper_force_disable_all(), in the case of OS
> shutdown/restart comes straight out of the kernel doc "driver instance
> overview" in drm_drv.c.
>
> NOTE: in order to get things inserted in the right place, I had to replace the
> old/deprecated drm_put_dev() function with the equivalent new calls.
>
> Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Reviewed-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> I honestly have no idea if I got this patch right. The shutdown() function
> already had some special case logic for PPC, Loongson, and VMs and I don't
> 100% for sure know how this interacts with those. Everything here is just
> compile tested.

I think the reason for most of this funniness is to reduce shutdown time.  Lots of users complain if driver takes a while to shutdown and there is a point to be made that if the system is going into power down, there is not much reason to spend a lot of time messing with the hardware.

Alex

>
> (no changes since v1)
>
>  drivers/gpu/drm/radeon/radeon_drv.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon_drv.c
> b/drivers/gpu/drm/radeon/radeon_drv.c
> index 39cdede460b5..67995ea24852 100644
> --- a/drivers/gpu/drm/radeon/radeon_drv.c
> +++ b/drivers/gpu/drm/radeon/radeon_drv.c
> @@ -38,6 +38,7 @@
>  #include <linux/pci.h>
>
>  #include <drm/drm_aperture.h>
> +#include <drm/drm_crtc_helper.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_file.h>
>  #include <drm/drm_gem.h>
> @@ -357,7 +358,9 @@ radeon_pci_remove(struct pci_dev *pdev)  {
>       struct drm_device *dev = pci_get_drvdata(pdev);
>
> -     drm_put_dev(dev);
> +     drm_dev_unregister(dev);
> +     drm_helper_force_disable_all(dev);
> +     drm_dev_put(dev);
>  }
>
>  static void
> @@ -368,6 +371,8 @@ radeon_pci_shutdown(struct pci_dev *pdev)
>        */
>       if (radeon_device_is_virtual())
>               radeon_pci_remove(pdev);
> +     else
> +             drm_helper_force_disable_all(pci_get_drvdata(pdev));
>
>  #if defined(CONFIG_PPC64) || defined(CONFIG_MACH_LOONGSON64)
>       /*
> --
> 2.42.0.515.g380fc7ccd1-goog