Re: [PATCH 3/6] drm/amdgpu: drop explicit pci D3/D0 setting for ATPX power control

[Lukas Wunner <lukas@wunner.de>]

On Thu, Jun 02, 2016 at 03:26:18PM -0400, Alex Deucher wrote:
> On Thu, Jun 2, 2016 at 3:23 PM, Lukas Wunner <lukas@wunner.de> wrote:
> > On Thu, Jun 02, 2016 at 09:33:34AM -0400, Alex Deucher wrote:
> >> The ATPX power control method does this for you.
> >>
> >> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 8 ++++----
> >>  1 file changed, 4 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> index 6c38901..e2bf4ef 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> >> @@ -419,9 +419,7 @@ static int amdgpu_pmops_runtime_suspend(struct device *dev)
> >>       pci_ignore_hotplug(pdev);
> >>       if (amdgpu_is_atpx_hybrid())
> >>               pci_set_power_state(pdev, PCI_D3cold);
> >> -     else if (amdgpu_has_atpx_dgpu_power_cntl())
> >> -             pci_set_power_state(pdev, PCI_D3cold);
> >> -     else
> >> +     else if (!amdgpu_has_atpx_dgpu_power_cntl())
> >
> > You're removing code that you added yesterday:
> > https://lists.freedesktop.org/archives/dri-devel/2016-June/109245.html
> >
> > Why are you doing this?
> 
> To avoid changing behavior in intermediate steps to avoid breaking
> multiple platforms in one patch.  The original behavior before this
> patch set was set d3cold unconditionally.  The previous change set
> d3hot for systems without ATPX power control, and finally this patch
> removes setting d3cold for systems with ATPX power control.

Thank you for the explanation, I was trying to make sense of this
but kept scratching my head. It would be good to have the explanation
in the commit message(s), they're fairly terse. (In this patch it's
just a one-liner.)

Best regards,

Lukas

> >>               pci_set_power_state(pdev, PCI_D3hot);
> >>       drm_dev->switch_power_state = DRM_SWITCH_POWER_DYNAMIC_OFF;
> >>
> >> @@ -439,7 +437,9 @@ static int amdgpu_pmops_runtime_resume(struct device *dev)
> >>
> >>       drm_dev->switch_power_state = DRM_SWITCH_POWER_CHANGING;
> >>
> >> -     pci_set_power_state(pdev, PCI_D0);
> >> +     if (amdgpu_is_atpx_hybrid() ||
> >> +         !amdgpu_has_atpx_dgpu_power_cntl())
> >> +             pci_set_power_state(pdev, PCI_D0);
> >>       pci_restore_state(pdev);
> >>       ret = pci_enable_device(pdev);
> >>       if (ret)
> >> --
> >> 2.5.5
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel