[RFT PATCH v2 07/12] drm/amdgpu: Call drm_atomic_helper_shutdown() at shutdown time

[Doug Anderson]

Hi,

On Mon, Sep 25, 2023 at 8:57 AM Deucher, Alexander
<Alexander.Deucher at amd.com> wrote:
>
> [Public]
>
> > -----Original Message-----
> > From: Douglas Anderson <dianders at chromium.org>
> > Sent: Thursday, September 21, 2023 3:27 PM
> > To: dri-devel at lists.freedesktop.org; Maxime Ripard <mripard at kernel.org>
> > Cc: Douglas Anderson <dianders at chromium.org>; Zhang, Bokun
> > <Bokun.Zhang at amd.com>; Zhang, Hawking <Hawking.Zhang at amd.com>;
> > Zhu, James <James.Zhu at amd.com>; Zhao, Victor <Victor.Zhao at amd.com>;
> > Pan, Xinhui <Xinhui.Pan at amd.com>; airlied at gmail.com; Deucher, Alexander
> > <Alexander.Deucher at amd.com>; amd-gfx at lists.freedesktop.org; Koenig,
> > Christian <Christian.Koenig at amd.com>; daniel at ffwll.ch; Kuehling, Felix
> > <Felix.Kuehling at amd.com>; jim.cromie at gmail.com; Ma, Le
> > <Le.Ma at amd.com>; Lazar, Lijo <Lijo.Lazar at amd.com>; linux-
> > kernel at vger.kernel.org; maarten.lankhorst at linux.intel.com; Limonciello,
> > Mario <Mario.Limonciello at amd.com>; mdaenzer at redhat.com; Zhang,
> > Morris <Shiwu.Zhang at amd.com>; SHANMUGAM, SRINIVASAN
> > <SRINIVASAN.SHANMUGAM at amd.com>; tzimmermann at suse.de
> > 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 at kernel.org>
> > Signed-off-by: Douglas Anderson <dianders at chromium.org>
> > ---
> > 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.

As per my comment above, for this driver, my patch was a "plea for
help". I have no idea if it's really needed or if suspend handles it.

My main concerns are:

a) If it's possible that someone out there is using this DRM driver
with a "drm_panel" then we need to make sure the panel gets disabled /
unprepared properly at shutdown time. The goal is to remove the
special logic in some panel drivers that disables the panel at
shutdown time. The guidance I got from Maxime is that we should be
relying on the DRM driver to disable panels at shutdown time and not
have extra per-panel code for it.

b) It is documented that DRM driers call drm_atomic_helper_shutdown()
at shutdown time. Even if things are working today, it's always
possible that something will change later and break for drivers that
aren't doing this.


If you're confident that everything is great for the "amdgpu" driver
then I'm happy to drop this patch and not consider it a blocker for
the eventual removal of the code in the individual panels drivers.

If, after reading this, you conclude that some sort of patch is
needed, I'd love it if you could test/post a patch yourself and then
I'll drop this patch from my series.


-Doug