RE: [RFT PATCH v2 07/12] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown 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>; Zhang, Bokun
> <Bokun.Zhang@xxxxxxx>; Zhang, Hawking <Hawking.Zhang@xxxxxxx>;
> Zhu, James <James.Zhu@xxxxxxx>; Zhao, Victor <Victor.Zhao@xxxxxxx>;
> Pan, Xinhui <Xinhui.Pan@xxxxxxx>; airlied@xxxxxxxxx; Deucher, Alexander
> <Alexander.Deucher@xxxxxxx>; amd-gfx@xxxxxxxxxxxxxxxxxxxxx; Koenig,
> Christian <Christian.Koenig@xxxxxxx>; daniel@xxxxxxxx; Kuehling, Felix
> <Felix.Kuehling@xxxxxxx>; jim.cromie@xxxxxxxxx; Ma, Le
> <Le.Ma@xxxxxxx>; Lazar, Lijo <Lijo.Lazar@xxxxxxx>; linux-
> kernel@xxxxxxxxxxxxxxx; maarten.lankhorst@xxxxxxxxxxxxxxx; Limonciello,
> Mario <Mario.Limonciello@xxxxxxx>; mdaenzer@xxxxxxxxxx; Zhang,
> Morris <Shiwu.Zhang@xxxxxxx>; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@xxxxxxx>; tzimmermann@xxxxxxx
> Subject: [RFT PATCH v2 07/12] drm/amdgpu: Call
> drm_atomic_helper_shutdown() at shutdown time
>
> Based on grepping through the source code this driver appears to be missing a
> call to drm_atomic_helper_shutdown() at system shutdown time. Among
> other things, this means that if a panel is in use that it won't be cleanly
> powered off at system shutdown time.
>
> The fact that we should call drm_atomic_helper_shutdown() in the case of OS
> shutdown/restart comes straight out of the kernel doc "driver instance
> overview" in drm_drv.c.
>
> Suggested-by: Maxime Ripard <mripard@xxxxxxxxxx>
> Signed-off-by: Douglas Anderson <dianders@xxxxxxxxxxxx>
> ---
> This commit is only compile-time tested.
>
> ...and further, I'd say that this patch is more of a plea for help than a patch I
> think is actually right. I'm _fairly_ certain that drm/amdgpu needs this call at
> shutdown time but the logic is a bit hard for me to follow. I'd appreciate if
> anyone who actually knows what this should look like could illuminate me, or
> perhaps even just post a patch themselves!
>
> (no changes since v1)
>
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  1 +
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 10 ++++++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    |  2 ++
>  3 files changed, 13 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 8f2255b3a38a..cfcff0b37466 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1104,6 +1104,7 @@ static inline struct amdgpu_device
> *amdgpu_ttm_adev(struct ttm_device *bdev)  int amdgpu_device_init(struct
> amdgpu_device *adev,
>                      uint32_t flags);
>  void amdgpu_device_fini_hw(struct amdgpu_device *adev);
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev);
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev);
>
>  int amdgpu_gpu_wait_for_idle(struct amdgpu_device *adev); diff --git
> a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index a2cdde0ca0a7..fa5925c2092d 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -4247,6 +4247,16 @@ void amdgpu_device_fini_hw(struct
> amdgpu_device *adev)
>
>  }
>
> +void amdgpu_device_shutdown_hw(struct amdgpu_device *adev) {

This needs a better name since its only for displays.  Also maybe move it into amdgpu_display.c since it's really about turning off the displays.  That said is this really even needed?  The driver already calls its suspend functionality to turn off all of the hardware and put it into a quiescent state before shutdown.  Basically shares the same code we use for suspend.


> +     if (adev->mode_info.mode_config_initialized) {
> +             if (!drm_drv_uses_atomic_modeset(adev_to_drm(adev)))
> +                     drm_helper_force_disable_all(adev_to_drm(adev));
> +             else
> +                     drm_atomic_helper_shutdown(adev_to_drm(adev));
> +     }
> +}
> +
>  void amdgpu_device_fini_sw(struct amdgpu_device *adev)  {
>       int idx;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index e90f730eb715..3a7cbff111d1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -2333,6 +2333,8 @@ amdgpu_pci_shutdown(struct pci_dev *pdev)
>       struct drm_device *dev = pci_get_drvdata(pdev);
>       struct amdgpu_device *adev = drm_to_adev(dev);
>
> +     amdgpu_device_shutdown_hw(adev);

I would move this after the ras_intr check below.

Alex

> +
>       if (amdgpu_ras_intr_triggered())
>               return;
>
> --
> 2.42.0.515.g380fc7ccd1-goog