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@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
-- Ville Syrjälä Intel OTC