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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

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.

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