On Wed, May 21, 2014 at 11:26:55AM +0300, Ville Syrj?l? wrote: > For everything but the reset_vblank_counter() thing: > Reviewed-by: Ville Syrj?l? <ville.syrjala at linux.intel.com> > > And another caveat: I only glanced at the crtc_helper stuff. It looks > sane but I didn't go reading through the drivers to figure out if they > would fall over or work.
Oh, the rfc was really just that. I don't have any plans to burn my hands trying to merge those patches ;-) Especially since interest from non-i915 hackers seems to be low. > About the reset_vblank_counter(), I think we might still need it to keep > the counter sane when the power well goes off. But I think we have > other problems on that front esp. with suspend to disk. There the counter > should definitely get reset on all platforms, so we migth apply any kind > of diff to the user visible value. The fix would likely be to skip the > diff adjustment when resuming. > > I tried to take a quick look at that yesterday but there was something > really fishy happening as the code didn't seem to observe the wraparound > at all, even though I confirmed w/ intel_reg_read that it definitely > did appear to wrap. I'll take another look at it today. > > Another idea might be to rip out the diff adjustment altogether. That > should just mean that the user visible counter wouldn't advance at all > between drm_vblank_off() and drm_vblank_on(). But that might actually > be the sane thing to do. Maybe we should just do a +1 there to make > sure we don't report the same value before and after modeset. It should > fix both the suspend problems and the power well problem. Hm, like I've mentioned yesterday on irc the tests I have actually pass, at least if I throw your sanitize_crtc patch on top. vblank frame counter values monotonically increase across suspend/resume, runtime pm and anything else I manged to throw at it. And the limit in the test is 100 frames later, but I've only observed a few tens at most. So I think the code as-is actually works. Whether intentional or not is still under dispute though ;-) The real problem I have with the hsw counter reset is that the same issue should affect _any_ platform where we support runtime pm. Like snb or byt. But the code isn't there. Also if we have such a bug then it will also affect hibernate and suspend to disk. Which means that this should be done in drm_crtc_vblank_off/on, not in the guts of some random platforms runtime pm code. So in my opinion the hsw vblank_count reset code needs to go anyway because: - Either it isn't need any more (and we have the tests for this now) and it's just cargo-culted duct-tape. - Or we need, but then it's in the wrong spot. Given that can you reconsider that patch please? Thanks, Daniel > > On Wed, May 14, 2014 at 08:51:02PM +0200, Daniel Vetter wrote: > > Hi all, > > > > This is Ville's vblank rework series, slightly pimped. I've added kerneldoc, > > some polish and also some additional nasty igt testcases for these crtc > > on/off > > vs vblank issues. Seems all to hold together nicely. > > > > Now one thing I wanted to do is roll this out across all drm drivers, but > > that > > looks ugly: Many drivers don't support vblanks really, and I couldn't fully > > audit whether they'd e.g. blow up when userspace tries to use the vblank > > wait > > ioctl. But I think we want to have more sanity since otherwise userspace is > > doomed to carry countless ugly hacks around forever. > > > > The last two patches are my attempt at that. By doing the drm_vblank_on/off > > dance in the crtc helpers after we know that the crtc is on/off we should > > have > > at least a somewhat sane baseline behaviour. Note that since > > drm_vblank_on/off > > are idempotent drivers are free to call these functions in their callback > > at an > > earlier time, where it makes more sense. But at least it's guaranteed to > > happen. > > > > Otoh drivers still need to manually complete pageflips, so this doesn't > > solve > > all the issues. But until we have a solid cross-driver testsuite for these > > corner-cases (all the interesting stuff happens when you race vblank waits > > against concurrent modesets, dpms, or system/runtime suspend/resume) we're > > pretty much guaranteed to have some that works at best occasionally and is > > guaranteed to have different behaviour in corner cases. Note that the > > patches > > won't degrade behaviour for existing drivers, the drm core changes simply > > allow > > us to finally fix things up correctly. > > > > I guess in a way this is a more generic discussion for the drm subsystem at > > large. > > > > Coments and review highly welcome. > > > > Cheers, Daniel > > > > Daniel Vetter (8): > > drm/i915: Remove drm_vblank_pre/post_modeset calls > > drm/doc: Discourage usage of MODESET_CTL ioctl > > drm/irq: kerneldoc polish > > drm/irq: Add kms-native crtc interface functions > > drm/i915: rip our vblank reset hacks for runtime PM > > drm/i915: Accurately initialize fifo underrun state on gmch platforms > > [RFC] drm/irq: More robustness in drm_vblank_on|off > > [RFC] drm/crtc-helper: Enforce sane vblank counter semantics > > > > Peter Hurley (1): > > drm: Use correct spinlock flavor in drm_vblank_get() > > > > Ville Syrj?l? (3): > > drm: Make the vblank disable timer per-crtc > > drm: Make blocking vblank wait return when the vblank interrupts get > > disabled > > drm: Add drm_vblank_on() > > > > Documentation/DocBook/drm.tmpl | 16 +- > > drivers/gpu/drm/drm_crtc_helper.c | 12 ++ > > drivers/gpu/drm/drm_irq.c | 377 > > ++++++++++++++++++++++++++--------- > > drivers/gpu/drm/i915/i915_irq.c | 4 +- > > drivers/gpu/drm/i915/intel_display.c | 36 ++-- > > drivers/gpu/drm/i915/intel_drv.h | 2 - > > drivers/gpu/drm/i915/intel_pm.c | 40 ---- > > include/drm/drmP.h | 10 +- > > 8 files changed, 341 insertions(+), 156 deletions(-) > > > > -- > > 1.8.3.1 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > > -- > Ville Syrj?l? > Intel OTC -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
