On Thu, 2019-04-04 at 09:17 -0500, Bjorn Helgaas wrote:
> [+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.
>
Hi, sorry it took me so long to get back to this - I've been busy with some
other responsibilities at work that came up last moment.
So I did try making it so that we cancel hpd_work from pci_driver->shutdown
and wait for it to complete, however that didn't really mseem to make any
difference. I did however try adding a workaround in the past that would shut
down the GPU whenever the kernel was shutting down (basically calling
nouveau_drm_remove() on shutdown) and that did actually fix the issue though,
but I didn't go with it as a final solution because of the problems it would
cause if we tried shutting down the card when it's in a bad state we could end
up hanging the whole system.
Do we want to have this discussion on the bz btw, or is this email thread
fine?
> 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 work_struct *work)
>
> pm_runtime_get_sync(drm->dev->dev);
>
> + msleep(2000);
> drm_helper_hpd_irq_event(drm->dev);
>
> pm_runtime_mark_last_busy(drm->dev->dev);
>
>
>
> diff --git a/drivers/gpu/drm/nouveau/nouveau_drm.c
> b/drivers/gpu/drm/nouveau/nouveau_drm.c
> index 5020265bfbd9..48da72caa017 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_drm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_drm.c
> @@ -946,9 +946,6 @@ nouveau_pmops_runtime_resume(struct device *dev)
> nvif_mask(&device->object, 0x088488, (1 << 25), (1 << 25));
> drm_dev->switch_power_state = DRM_SWITCH_POWER_ON;
>
> - /* Monitors may have been connected / disconnected during suspend */
> - schedule_work(&nouveau_drm(drm_dev)->hpd_work);
> -
> return ret;
> }
>
--
Cheers,
Lyude Paul
