[PATCH] drm/amdgpu: move pci handling out of pm ops

[Alex Deucher]

On Thu, Nov 21, 2019 at 2:53 PM Dave Airlie <airlied at gmail.com> wrote:
>
> On Fri, 22 Nov 2019 at 02:55, Alex Deucher <alexdeucher at gmail.com> wrote:
> >
> > The documentation says the that PCI core handles this
> > for you unless you choose to implement it.  Just rely
> > on the PCI core to handle the pci specific bits.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 +--
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 33 +++++++++-------------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c    | 16 +++++------
> >  3 files changed, 24 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > index 97843462c2fb..2e9d0be05f2f 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> > @@ -1191,8 +1191,8 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv);
> >  void amdgpu_driver_postclose_kms(struct drm_device *dev,
> >                                  struct drm_file *file_priv);
> >  int amdgpu_device_ip_suspend(struct amdgpu_device *adev);
> > -int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon);
> > -int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon);
> > +int amdgpu_device_suspend(struct drm_device *dev, bool fbcon);
> > +int amdgpu_device_resume(struct drm_device *dev, bool fbcon);
> >  u32 amdgpu_get_vblank_counter_kms(struct drm_device *dev, unsigned int pipe);
> >  int amdgpu_enable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> >  void amdgpu_disable_vblank_kms(struct drm_device *dev, unsigned int pipe);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index b1408c5e4640..d832bd22ba9d 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1090,6 +1090,7 @@ static int amdgpu_device_check_arguments(struct amdgpu_device *adev)
> >  static void amdgpu_switcheroo_set_state(struct pci_dev *pdev, enum vga_switcheroo_state state)
> >  {
> >         struct drm_device *dev = pci_get_drvdata(pdev);
> > +       int r;
> >
> >         if (amdgpu_device_supports_boco(dev) && state == VGA_SWITCHEROO_OFF)
> >                 return;
> > @@ -1099,7 +1100,12 @@ static void amdgpu_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
> >                 /* don't suspend or resume card normally */
> >                 dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> >
> > -               amdgpu_device_resume(dev, true, true);
> > +               pci_set_power_state(dev->pdev, PCI_D0);
> > +               pci_restore_state(dev->pdev);
> > +               r = pci_enable_device(dev->pdev);
> > +               if (r)
> > +                       DRM_WARN("pci_enable_device failed (%d)\n", r);
> > +               amdgpu_device_resume(dev, true);
> >
> >                 dev->switch_power_state = DRM_SWITCH_POWER_ON;
> >                 drm_kms_helper_poll_enable(dev);
> > @@ -1107,7 +1113,11 @@ static void amdgpu_switcheroo_set_state(struct pci_dev *pdev, enum vga_switchero
> >                 pr_info("amdgpu: switched off\n");
> >                 drm_kms_helper_poll_disable(dev);
> >                 dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> > -               amdgpu_device_suspend(dev, true, true);
> > +               amdgpu_device_suspend(dev, true);
> > +               pci_save_state(dev->pdev);
> > +               /* Shut down the device */
> > +               pci_disable_device(dev->pdev);
> > +               pci_set_power_state(dev->pdev, PCI_D3cold);
>
>
> I think this will break D3 cold on ATPX systems (i.e. before PCIE d3
> cold support).
>
> I'm not 100% sure but I'd really like to test it, as I don't think the
> core will put the device into D3cold for us here.

Should be the same as before, I just moved the pci funcs up from the
callee into the caller, or are you talking about the switch from d3hot
to d3cold?  I can make the hot/cold change a separate commit however.
That said, it doesn't really matter what d3 state we choose here.  The
APTX call cuts the power rail anyway (effectively d3cold).

Alex

>
> Dave.