Re: [Nouveau] [PATCH] pci/quirks: Add quirk to reset nvgpu at boot for the Lenovo ThinkPad P50

2019-04-04 Thread Bjorn Helgaas
[+cc Hans, author of 0b2fe6594fa2 ("drm/nouveau: Queue hpd_work on (runtime) 
resume")]

On Fri, Mar 22, 2019 at 06:30:15AM -0500, Bjorn Helgaas wrote:
> On Thu, Mar 21, 2019 at 05:48:19PM -0500, Bjorn Helgaas wrote:
> > On Wed, Mar 13, 2019 at 06:25:02PM -0400, Lyude Paul wrote:
> > > On Fri, 2019-02-15 at 16:17 -0500, Lyude Paul wrote:
> > > > On Thu, 2019-02-14 at 18:43 -0600, Bjorn Helgaas wrote:
> > > > > On Tue, Feb 12, 2019 at 05:02:30PM -0500, Lyude Paul wrote:
> > > > > > On a very specific subset of ThinkPad P50 SKUs, particularly
> > > > > > ones that come with a Quadro M1000M chip instead of the M2000M
> > > > > > variant, the BIOS seems to have a very nasty habit of not
> > > > > > always resetting the secondary Nvidia GPU between full reboots
> > > > > > if the laptop is configured in Hybrid Graphics mode. The
> > > > > > reason for this happening is unknown, but the following steps
> > > > > > and possibly a good bit of patience will reproduce the issue:
> > > > > > 
> > > > > > 1. Boot up the laptop normally in Hybrid graphics mode
> > > > > > 2. Make sure nouveau is loaded and that the GPU is awake
> > > > > > 2. Allow the nvidia GPU to runtime suspend itself after being idle
> > > > > > 3. Reboot the machine, the more sudden the better (e.g sysrq-b may 
> > > > > > help)
> > > > > > 4. If nouveau loads up properly, reboot the machine again and go 
> > > > > > back to
> > > > > > step 2 until you reproduce the issue
> > > > > > 
> > > > > > This results in some very strange behavior: the GPU will quite
> > > > > > literally be left in exactly the same state it was in when the
> > > > > > previously booted kernel started the reboot. This has all
> > > > > > sorts of bad sideaffects: for starters, this completely breaks
> > > > > > nouveau starting with a mysterious EVO channel failure that
> > > > > > happens well before we've actually used the EVO channel for
> > > > > > anything:
> > 
> > Thanks for the hybrid tutorial (snipped from this response).  IIUC,
> > what you said was that in hybrid mode, the Intel GPU drives the
> > built-in display and the Nvidia GPU drives any external displays and
> > may be used for DRI PRIME rendering (whatever that is).  But since you
> > say the Nvidia device gets runtime suspended, I assume there's no
> > external display here and you're not using DRI PRIME.
> > 
> > I wonder if it's related to the fact that the Nvidia GPU has been
> > runtime suspended before you do the reboot.  Can you try turning of
> > runtime power management for the GPU by setting the runpm module
> > parameter to 0?  I *think* this would be booting with
> > "nouveau.runpm=0".
> 
> Sorry, I wasn't really thinking here.  You already *said* this is
> related to runtime suspend.  It only happens when the Nvidia GPU has
> been suspended.
> 
> I don't know that much about suspend, but ISTR seeing comments about
> resuming devices before we shutdown.  If we do that, maybe there's
> some kind of race between that resume and the reboot?

I think we do in fact resume PCI devices before shutdown.  Here's the
path I'm looking at:

  device_shutdown
pm_runtime_get_noresume
pm_runtime_barrier
dev->bus->shutdown
  pci_device_shutdown
pm_runtime_resume
  __pm_runtime_resume(dev, 0)
rpm_resume(dev, 0)
  __update_runtime_status(dev, RPM_RESUMING)
  callback = RPM_GET_CALLBACK(dev, runtime_resume)
  rpm_callback(callback, dev)
__rpm_callback
  pci_pm_runtime_resume
drv->pm->runtime_resume
  nouveau_pmops_runtime_resume
nouveau_do_resume
schedule_work(hpd_work)   # <---
...
nouveau_display_hpd_work
  pm_runtime_get_sync
  drm_helper_hpd_irq_event
  pm_runtime_mark_last_busy
  pm_runtime_put_sync

I'm curious about that "schedule_work(hpd_work)" near the end because
no other drivers seem to use schedule_work() in the runtime_resume
path, and I don't know how that synchronizes with the shutdown
process.  I don't see anything that waits for
nouveau_display_hpd_work() to complete, so it seems like something
that could be a race.

I wonder this problem would be easier to reproduce if you added a
sleep in nouveau_display_hpd_work() as in the first hunk below, and I
wonder if the problem would then go away if you stopped scheduling
hpd_work as in the second hunk?  Obviously the second hunk isn't a
solution, it's just an attempt to figure out if I'm looking in the
right area.

Bjorn


diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 55c0fa451163..e50806012d41 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -350,6 +350,7 @@ nouveau_display_hpd_work(struct 

Re: [Nouveau] [PATCH] drm/nouveau: Remove duplicate ACPI_VIDEO_NOTIFY_PROBE definition

2019-04-04 Thread Hans de Goede

Hi,

On 04-04-19 15:49, Bjorn Helgaas wrote:

From: Bjorn Helgaas 

Commit 3a6536c51d5d ("drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE")
added a definition of ACPI_VIDEO_NOTIFY_PROBE because  didn't
supply one.  Later, commit eff4a751cce5 ("ACPI / video: Move
ACPI_VIDEO_NOTIFY_* defines to acpi/video.h") moved ACPI_VIDEO_NOTIFY_PROBE
and other definitions to , so the copy in nouveau_display.c
is now unnecessary.

Remove the unnecessary definition from nouveau_display.c.

Signed-off-by: Bjorn Helgaas 
CC: Hans de Goede 


Thank you for cleaning this up:

Acked-by: Hans de Goede 

Regards,

Hans




---
  drivers/gpu/drm/nouveau/nouveau_display.c | 9 -
  1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 55c0fa451163..832da8e0020d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -358,15 +358,6 @@ nouveau_display_hpd_work(struct work_struct *work)
  
  #ifdef CONFIG_ACPI
  
-/*

- * Hans de Goede: This define belongs in acpi/video.h, I've submitted a patch
- * to the acpi subsys to move it there from drivers/acpi/acpi_video.c .
- * This should be dropped once that is merged.
- */
-#ifndef ACPI_VIDEO_NOTIFY_PROBE
-#define ACPI_VIDEO_NOTIFY_PROBE0x81
-#endif
-
  static int
  nouveau_display_acpi_ntfy(struct notifier_block *nb, unsigned long val,
  void *data)


___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau

[Nouveau] [PATCH] drm/nouveau: Remove duplicate ACPI_VIDEO_NOTIFY_PROBE definition

2019-04-04 Thread Bjorn Helgaas
From: Bjorn Helgaas 

Commit 3a6536c51d5d ("drm/nouveau: Intercept ACPI_VIDEO_NOTIFY_PROBE")
added a definition of ACPI_VIDEO_NOTIFY_PROBE because  didn't
supply one.  Later, commit eff4a751cce5 ("ACPI / video: Move
ACPI_VIDEO_NOTIFY_* defines to acpi/video.h") moved ACPI_VIDEO_NOTIFY_PROBE
and other definitions to , so the copy in nouveau_display.c
is now unnecessary.

Remove the unnecessary definition from nouveau_display.c.

Signed-off-by: Bjorn Helgaas 
CC: Hans de Goede 
---
 drivers/gpu/drm/nouveau/nouveau_display.c | 9 -
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c 
b/drivers/gpu/drm/nouveau/nouveau_display.c
index 55c0fa451163..832da8e0020d 100644
--- a/drivers/gpu/drm/nouveau/nouveau_display.c
+++ b/drivers/gpu/drm/nouveau/nouveau_display.c
@@ -358,15 +358,6 @@ nouveau_display_hpd_work(struct work_struct *work)
 
 #ifdef CONFIG_ACPI
 
-/*
- * Hans de Goede: This define belongs in acpi/video.h, I've submitted a patch
- * to the acpi subsys to move it there from drivers/acpi/acpi_video.c .
- * This should be dropped once that is merged.
- */
-#ifndef ACPI_VIDEO_NOTIFY_PROBE
-#define ACPI_VIDEO_NOTIFY_PROBE0x81
-#endif
-
 static int
 nouveau_display_acpi_ntfy(struct notifier_block *nb, unsigned long val,
  void *data)
-- 
2.21.0.392.gf8f6787159e-goog

___
Nouveau mailing list
Nouveau@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/nouveau