Nouveau - Nov 2017 - [PATCH v2] drm: don't continue with anything after the GPU couldn't be woken up

On Tue, Nov 21, 2017 at 6:46 PM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Tue, Nov 21, 2017 at 04:01:16PM +0100, Karol Herbst wrote:
>> This should make systems more stable where resuming the GPU fails. This
>> can happen due to bad firmware or due to a bug within the kernel. The
>> last thing which should happen in either case is an unusable system.
>>
>> v2: do the same in nouveau_pmops_resume
>>
>> Tested-by: Karl Hastings <kazen at redhat.com>
>> Signed-off-by: Karol Herbst <kherbst at redhat.com>
>> ---
>>  drm/nouveau/nouveau_drm.c | 31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
>> index 8d4a5be3..6e4cb4f7 100644
>> --- a/drm/nouveau/nouveau_drm.c
>> +++ b/drm/nouveau/nouveau_drm.c
>> @@ -792,6 +792,27 @@ nouveau_pmops_suspend(struct device *dev)
>>       return 0;
>>  }
>>
>> +static int
>> +nouveau_set_power_state_D0(struct pci_dev *pdev)
>> +{
>> +     struct nouveau_drm *drm = nouveau_drm(pci_get_drvdata(pdev));
>> +     int ret;
>> +
>> +     pci_set_power_state(pdev, PCI_D0);
>> +     /* abort if anything went wrong */
>> +     if (pdev->current_state != PCI_D0) {
>> +             NV_ERROR(drm, "couldn't wake up GPU!\n");
>> +             return -EBUSY;
>> +     }
>
> Looks to me like the more idiomatic way to do this is:
>
>         ret = pci_set_power_state(pdev, PCI_D0);
>         if (ret < 0 && ret != -EIO)
>                 return ret;
>
I thought so too, but it ends up returning 0 even if setting the power
state fails. Or maybe I did something wrong when installing the
kernel. I could take another shot at it, but what I came up with seems
to work. Adding airlied in CC, because he saw my patch and didn't
complain about it. Hopefully he knows more.
>> +     pci_restore_state(pdev);
>> +     ret = pci_enable_device(pdev);
>> +     if (ret)
>> +             return ret;
>> +
>> +     pci_set_master(pdev);
>
> Looking closer it also seems like pci_enable_device() will already set
> the power state to D0 (via do_pci_enable_device()). Is the sequence
> above really necessary because the hardware is quirky, or was it
> cargo-culted?
>
> Thierry
No clue. And because it was already there in the original code I
didn't really felt like doing anything with it.

On Tue, Nov 21, 2017 at 08:03:20PM +0100, Karol Herbst
wrote:
> On Tue, Nov 21, 2017 at 6:46 PM, Thierry Reding
> <thierry.reding at gmail.com> wrote:
> > On Tue, Nov 21, 2017 at 04:01:16PM +0100, Karol Herbst wrote:
> >> This should make systems more stable where resuming the GPU fails.
This
> >> can happen due to bad firmware or due to a bug within the kernel.
The
> >> last thing which should happen in either case is an unusable
system.
> >>
> >> v2: do the same in nouveau_pmops_resume
> >>
> >> Tested-by: Karl Hastings <kazen at redhat.com>
> >> Signed-off-by: Karol Herbst <kherbst at redhat.com>
> >> ---
> >>  drm/nouveau/nouveau_drm.c | 31 +++++++++++++++++++++++--------
> >>  1 file changed, 23 insertions(+), 8 deletions(-)
> >>
> >> diff --git a/drm/nouveau/nouveau_drm.c b/drm/nouveau/nouveau_drm.c
> >> index 8d4a5be3..6e4cb4f7 100644
> >> --- a/drm/nouveau/nouveau_drm.c
> >> +++ b/drm/nouveau/nouveau_drm.c
> >> @@ -792,6 +792,27 @@ nouveau_pmops_suspend(struct device *dev)
> >>       return 0;
> >>  }
> >>
> >> +static int
> >> +nouveau_set_power_state_D0(struct pci_dev *pdev)
> >> +{
> >> +     struct nouveau_drm *drm =
nouveau_drm(pci_get_drvdata(pdev));
> >> +     int ret;
> >> +
> >> +     pci_set_power_state(pdev, PCI_D0);
> >> +     /* abort if anything went wrong */
> >> +     if (pdev->current_state != PCI_D0) {
> >> +             NV_ERROR(drm, "couldn't wake up
GPU!\n");
> >> +             return -EBUSY;
> >> +     }
> >
> > Looks to me like the more idiomatic way to do this is:
> >
> >         ret = pci_set_power_state(pdev, PCI_D0);
> >         if (ret < 0 && ret != -EIO)
> >                 return ret;
> >
> 
> I thought so too, but it ends up returning 0 even if setting the power
> state fails. Or maybe I did something wrong when installing the
> kernel. I could take another shot at it, but what I came up with seems
> to work. Adding airlied in CC, because he saw my patch and didn't
> complain about it. Hopefully he knows more.
pci_raw_set_power_state(), called by pci_set_power_state(), contains
this, which looks to me like it would be the only case where the problem
you're describing could be coming from:

	dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
	if (dev->current_state != state && printk_ratelimit())
		dev_info(&dev->dev, "Refused to change power state, currently in
D%d\n",
			 dev->current_state);

Do you happen to see this in the kernel logs? Perhaps this should be
considered an error rather than just an KERN_INFO level message?

Adding Bjorn and linux-pci for visibility.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20171122/292b313d/attachment.sig>

On Wed, Nov 22, 2017 at 11:31 AM, Thierry Reding
<thierry.reding at gmail.com> wrote:
> On Tue, Nov 21, 2017 at 08:03:20PM +0100, Karol Herbst wrote:
>> On Tue, Nov 21, 2017 at 6:46 PM, Thierry Reding
>> <thierry.reding at gmail.com> wrote:
>> > On Tue, Nov 21, 2017 at 04:01:16PM +0100, Karol Herbst wrote:
>> >> This should make systems more stable where resuming the GPU
fails. This
>> >> can happen due to bad firmware or due to a bug within the
kernel. The
>> >> last thing which should happen in either case is an unusable
system.
>> >>
>> >> v2: do the same in nouveau_pmops_resume
>> >>
>> >> Tested-by: Karl Hastings <kazen at redhat.com>
>> >> Signed-off-by: Karol Herbst <kherbst at redhat.com>
>> >> ---
>> >>  drm/nouveau/nouveau_drm.c | 31
+++++++++++++++++++++++--------
>> >>  1 file changed, 23 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drm/nouveau/nouveau_drm.c
b/drm/nouveau/nouveau_drm.c
>> >> index 8d4a5be3..6e4cb4f7 100644
>> >> --- a/drm/nouveau/nouveau_drm.c
>> >> +++ b/drm/nouveau/nouveau_drm.c
>> >> @@ -792,6 +792,27 @@ nouveau_pmops_suspend(struct device *dev)
>> >>       return 0;
>> >>  }
>> >>
>> >> +static int
>> >> +nouveau_set_power_state_D0(struct pci_dev *pdev)
>> >> +{
>> >> +     struct nouveau_drm *drm =
nouveau_drm(pci_get_drvdata(pdev));
>> >> +     int ret;
>> >> +
>> >> +     pci_set_power_state(pdev, PCI_D0);
>> >> +     /* abort if anything went wrong */
>> >> +     if (pdev->current_state != PCI_D0) {
>> >> +             NV_ERROR(drm, "couldn't wake up
GPU!\n");
>> >> +             return -EBUSY;
>> >> +     }
>> >
>> > Looks to me like the more idiomatic way to do this is:
>> >
>> >         ret = pci_set_power_state(pdev, PCI_D0);
>> >         if (ret < 0 && ret != -EIO)
>> >                 return ret;
>> >
>>
>> I thought so too, but it ends up returning 0 even if setting the power
>> state fails. Or maybe I did something wrong when installing the
>> kernel. I could take another shot at it, but what I came up with seems
>> to work. Adding airlied in CC, because he saw my patch and didn't
>> complain about it. Hopefully he knows more.
>
> pci_raw_set_power_state(), called by pci_set_power_state(), contains
> this, which looks to me like it would be the only case where the problem
> you're describing could be coming from:
>
>         dev->current_state = (pmcsr & PCI_PM_CTRL_STATE_MASK);
>         if (dev->current_state != state && printk_ratelimit())
>                 dev_info(&dev->dev, "Refused to change power
state, currently in D%d\n",
>                          dev->current_state);
>
> Do you happen to see this in the kernel logs? Perhaps this should be
> considered an error rather than just an KERN_INFO level message?
>
> Adding Bjorn and linux-pci for visibility.
>
> Thierry
yeah, that is the error we have in dmesg.