Re: [PATCH 00/12] irq vblank handling rework

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch