RE: [PATCH] drm/amdgpu: move pci handling out of pm ops
> -Original Message-
> From: amd-gfx On Behalf Of Alex
> Deucher
> Sent: 2019/November/26, Tuesday 9:51 AM
> To: amd-gfx list
> Cc: Deucher, Alexander
> Subject: Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
>
> Ping? I've tested this on all the cards I have access to.
>
> Alex
>
> On Thu, Nov 21, 2019 at 11:55 AM Alex Deucher
> 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
Reviewed-by: Zhan Liu
> > ---
> > 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);
> > dev->switch_power_state = DRM_SWITCH_POWER_OFF;
> > }
> > }
> > @@ -3198,7 +3208,7 @@ void amdgpu_device_fini(struct amdgpu_device
> *adev)
> > * Returns 0 for success or an error on failure.
> > * Called at driver suspend.
> > */
> > -int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool
> > fbcon)
> > +int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
> >
Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
Ping? I've tested this on all the cards I have access to.
Alex
On Thu, Nov 21, 2019 at 11:55 AM Alex Deucher 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
> ---
> 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);
> dev->switch_power_state = DRM_SWITCH_POWER_OFF;
> }
> }
> @@ -3198,7 +3208,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
> * Returns 0 for success or an error on failure.
> * Called at driver suspend.
> */
> -int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
> +int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
> {
> struct amdgpu_device *adev;
> struct drm_crtc *crtc;
> @@ -3281,13 +3291,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
> suspend, bool fbcon)
> */
> amdgpu_bo_evict_vram(adev);
>
> - if (suspend) {
> - pci_save_state(dev->pdev);
> - /* Shut down the device */
> - pci_disable_device(dev->pdev);
> - pci_set_power_state(dev->pdev, PCI_D3hot);
> - }
> -
> return 0;
> }
>
> @@ -3302,7 +3305,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
> suspend, bool fbcon)
> * Returns 0 for success or an error on failure.
> * Called at driver resume.
> */
> -int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
> +int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
> {
> struct drm_connector *connector;
> struct drm_connector_list_iter iter;
> @@ -3313,14 +3316,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool
> resume, bool fbcon)
>
Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
On Thu, Nov 21, 2019 at 5:13 PM Dave Airlie wrote:
>
> On Fri, 22 Nov 2019 at 06:05, Alex Deucher wrote:
> >
> > On Thu, Nov 21, 2019 at 2:53 PM Dave Airlie wrote:
> > >
> > > On Fri, 22 Nov 2019 at 02:55, Alex Deucher 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
> > > > ---
> > > > 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 unt
Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
On Fri, 22 Nov 2019 at 06:05, Alex Deucher wrote:
>
> On Thu, Nov 21, 2019 at 2:53 PM Dave Airlie wrote:
> >
> > On Fri, 22 Nov 2019 at 02:55, Alex Deucher 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
> > > ---
> > > 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).
But sorry I misread the patch I thought you were dropping the calls in
favour of the pci core doing them.
Dave.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
On Thu, Nov 21, 2019 at 2:53 PM Dave Airlie wrote:
>
> On Fri, 22 Nov 2019 at 02:55, Alex Deucher 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
> > ---
> > 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.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/amdgpu: move pci handling out of pm ops
On Fri, 22 Nov 2019 at 02:55, Alex Deucher 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
> ---
> 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.
Dave.
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/amdgpu: move pci handling out of pm ops
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
---
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);
dev->switch_power_state = DRM_SWITCH_POWER_OFF;
}
}
@@ -3198,7 +3208,7 @@ void amdgpu_device_fini(struct amdgpu_device *adev)
* Returns 0 for success or an error on failure.
* Called at driver suspend.
*/
-int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
+int amdgpu_device_suspend(struct drm_device *dev, bool fbcon)
{
struct amdgpu_device *adev;
struct drm_crtc *crtc;
@@ -3281,13 +3291,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
suspend, bool fbcon)
*/
amdgpu_bo_evict_vram(adev);
- if (suspend) {
- pci_save_state(dev->pdev);
- /* Shut down the device */
- pci_disable_device(dev->pdev);
- pci_set_power_state(dev->pdev, PCI_D3hot);
- }
-
return 0;
}
@@ -3302,7 +3305,7 @@ int amdgpu_device_suspend(struct drm_device *dev, bool
suspend, bool fbcon)
* Returns 0 for success or an error on failure.
* Called at driver resume.
*/
-int amdgpu_device_resume(struct drm_device *dev, bool resume, bool fbcon)
+int amdgpu_device_resume(struct drm_device *dev, bool fbcon)
{
struct drm_connector *connector;
struct drm_connector_list_iter iter;
@@ -3313,14 +3316,6 @@ int amdgpu_device_resume(struct drm_device *dev, bool
resume, bool fbcon)
if (dev->switch_power_state == DRM_SWITCH_POWER_OFF)
return 0;
- if (resume) {
- pci_set_power_state(dev->pdev, PCI_D0);
- pci_restore_state(dev->pdev);
- r = pci_enable_device(dev->pdev);
- if (r)
- return r;
- }
-
