On Thu, Nov 21, 2019 at 5:13 PM Dave Airlie <airlied@xxxxxxxxx> wrote:
>
> On Fri, 22 Nov 2019 at 06:05, Alex Deucher <alexdeucher@xxxxxxxxx> wrote:
> >
> > On Thu, Nov 21, 2019 at 2:53 PM Dave Airlie <airlied@xxxxxxxxx> wrote:
> > >
> > > On Fri, 22 Nov 2019 at 02:55, Alex Deucher <alexdeucher@xxxxxxxxx> 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@xxxxxxx>
> > > > ---
> > > > 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).
>
> It matters to the OS, as it has to bring the device out of cold for
> certain operations that work in hot (like conifg space access).
Well, in that case, d3cold better matches what actually happens.
Nothing can access the device until it's powered back up via ATPX.
>
> But sorry I misread the patch I thought you were dropping the calls in
> favour of the pci core doing them.
No problem.
Alex
>
> Dave.
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
