On Sun, Feb 17, 2013 at 5:31 PM, Hugh Dickins <hughd at google.com> wrote: > On Sun, 17 Feb 2013, Daniel Vetter wrote: >> On Sun, Feb 17, 2013 at 3:21 AM, Hugh Dickins <hughd at google.com> wrote: >> > On Sat, 16 Feb 2013, Hugh Dickins wrote: >> >> On Sat, 16 Feb 2013, Linus Torvalds wrote: >> >> > >> >> > I think it's worth it to give them a heads-up already. So I've cc'd >> >> > the main suspects here.. >> >> >> >> Okay, thanks. >> >> >> >> > >> >> > Daniel, Dave - any comments about a NULL fb in >> >> > intel_choose_pipe_bpp_dither() during either suspend or resume? Some >> >> > googling shows this: >> >> > >> >> > https://bugzilla.redhat.com/show_bug.cgi?id=895123 >> >> >> >> Great, yes, I'm sure that's the same (though it says "suspend" >> >> and I say "resume"). >> >> >> >> > >> >> > which sounds remarkably similar, and is also during a suspend attempt >> >> > (but apparently Satish got a full oops out).. Some timing race with a >> >> > worker entry? >> > >> > Comparing Satish's backtrace and drivers/gpu/drm history, it's clear that >> > the oops comes from Daniel's 3.8-rc2 45e2b5f640b3 "drm/i915: force restore >> > on lid open", whose force_restore case now passes down crtc->base.fb. But >> > I wouldn't have a clue why that's usually non-NULL but occasionally NULL: >> > your timing race with a worker entry, perhaps. >> > >> > And 45e2b5f640b3 contains a fine history of going back and forth, so I >> > wouldn't want to play further with it out of ignorance - though tempted >> > to replace the "if (force_restore) {" by an interim safe-seeming >> > compromise of "if (force_restore && crtc->base.fb) {". > > My suggestion there in the line above was stupidly wrong :( > >> >> Two things to try while I try to grow a clue on what exactly is going on: > > Thank you. > > By the way, I hope you've looked back through this thread, and realize > that Dave and I both had ThinkPad T4?0s suspend/resume display problems, > but they've gone off in different directions: so a lot of the discussion > comes from Dave having CONFIG_PROVE_RCU_DELAY, and has nothing to do with > what we now know to be my oops in i915/intel_display.c.
Oh, I haven't read the earlier parts of the thread, but agree that it's a completely different bug: Different chipset (this matters massively for gpus usually), Dave's issue happens on -rc1 (which doesn't contain the offending lid_notifier patch yet) and seems to die someplace completely else than your box. >> 1. Related to new ACPI sleep states we've recently made the lid_notifier >> locking more sound, maybe that helps: >> >> http://cgit.freedesktop.org/~danvet/drm-intel/commit/?h=drm-intel-next-queued&id=b8efb17b3d687695b81485f606fc4e6c35a50f9a > > Looks like it may be relevant, but I'll ignore it for now: > preferring first to test the more minimal safety you suggest below. > >> >> 2. The new i915 force restore code is indeed missing a safety check >> compared to the old crtc helper based one: >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 6eb3882..095094c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9153,7 +9153,13 @@ void intel_modeset_setup_hw_state(struct drm_device >> *dev, >> >> if (force_restore) { >> for_each_pipe(pipe) { >> - >> intel_crtc_restore_mode(dev_priv->pipe_to_crtc_mapping[pipe]); >> + struct drm_crtc * crtc = >> + dev_priv->pipe_to_crtc_mapping[pipe]; >> + >> + if (!crtc->enabled) >> + continue; >> + >> + intel_crtc_restore_mode(crtc); >> } >> >> i915_redisable_vga(dev); > > I see your followup, where you observe that intel_modeset_affected_pipes() > should already have made this check; but you do say it would still be good > to prove one way or the other, so I'll run from now with the patch below. > > Note that we haven't got to worrying about what will be in 3.9 yet > (linux-next tells me to expect hangcheck timer problems): it's Linus's > current git for 3.8 final that we're working on at present. Right, patch was on top of -next, but there shouldn't be any (functional) differences in this area compared to 3.8. The first part of the big rework landed in 3.7 and contained the crtc->enabled check from day one. For the hangcheck issue in -next, that might be a new one. If you catch it again, can you please grab the i915_error_state from debugfs and throw it over to me? That should be enough for basic analysis. > And since quick resumes have always worked for me, it's only occasionally > that a long suspend (e.g. overnight) fails for me in this way, so I won't > be able to report success for several days - but failure may come sooner. > > And, it being very tiresome to debug when it does fail, I have inserted > WARN_ONs and more safety: here's what I'm running with now. Yep, that should be interesting once it catches something. For more debug noise can you set drm.debug=0xe (should work even when setting it in /sys/modules/drm/parameters at runtime). That spills tons of stuff into dmesg which will come handy in reconstructing how things fell apart. Presuming your machines survives a bad resume and you can grab dmesg, ofc. Thanks, Daniel > --- 3.8-rc7/drivers/gpu/drm/i915/intel_display.c 2013-01-17 > 20:06:11.384002362 -0800 > +++ linux/drivers/gpu/drm/i915/intel_display.c 2013-02-17 07:50:28.012075150 > -0800 > @@ -4156,7 +4156,9 @@ static bool intel_choose_pipe_bpp_dither > * also stays within the max display bpc discovered above. > */ > > - switch (fb->depth) { > + if (WARN_ON(!fb)) > + bpc = 8; > + else switch (fb->depth) { > case 8: > bpc = 8; /* since we go through a colormap */ > break; > @@ -9302,6 +9304,10 @@ void intel_modeset_setup_hw_state(struct > if (force_restore) { > for_each_pipe(pipe) { > crtc = > to_intel_crtc(dev_priv->pipe_to_crtc_mapping[pipe]); > + if (WARN_ON(!crtc->base.enabled)) > + continue; > + if (WARN_ON(!crtc->base.fb)) > + continue; > intel_set_mode(&crtc->base, &crtc->base.mode, > crtc->base.x, crtc->base.y, > crtc->base.fb); > } -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
