[Intel-gfx] [PULL] topic/vblank-rework

[Mario Kleiner]

Hmm, not quite an ack from my side for the pull in its current form. I
said if the two remaining issues i mentioned are addressed, then i'm
happy with it and can have my reviewed/acked-by. Looking at the code
they haven't been adressed.

However, this is easily fixable on top of the current patches:

1. A vblank_disable_timeout module parameter of zero should always
leave vblank irq's enabled and also override the drivers choice,
otherwise a user can't override the driver on a broken driver/gpu
combo, which is the only use case for having that module parameter.
Currenty the disable_immediately flag overrides the users override ->
Ouch.

So in drm_vblank_put():

...

/* Last user schedules interrupt disable */
if (atomic_dec_and_test(&vblank->refcount)) {
>>> Insert zero -> opt-out check <<<
   if (drm_vblank_offdelay == 0)
       return;
>>> Remaining code continues <<<
   if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
       vblank_disable_fn((unsigned long)vblank);
   else if (drm_vblank_offdelay > 0)
       mod_timer(&vblank->disable_timer, jiffies +
((drm_vblank_offdelay * HZ)/1000));

...

2. For the "drm: Have the vblank counter account for the time ... "
patch, we must opt-out of that last timestamp/counter update/bump if
the driver doesn't support high-precision vblank timestamping,
otherwise the vblank count and timestamp will be inconsistent with
each other - or outright wrong in case of the timestamp. Rather
deliver a slightly outdated, but correct count+timestamp pair to
userspace, which is still useable for practical purposes, than a pair
that's outright wrong and will definitely confuse clients.

A simple fix in static void vblank_disable_and_save() would be to
replace the new...

if (!vblank->enabled) {

... check by ...

if (!vblank->enabled &&
) {


On Wed, Sep 10, 2014 at 2:05 PM, Daniel Vetter <daniel.vetter at ffwll.ch> wrote:
> Hi Dave,
>
> So here's the final bits of Ville's vblank rework with a bit of cleanup
> from Mario on top.
>
> The neat thing this finally allows is to immediately disable the vblank
> interrupt on the last drm_vblank_put if the hardware has perfectly
> accurate vblank counter and timestamp readout support. On i915 that
> required piles of small adjustements from Ville since depending upon the
> platform and port the vblank happens at different scanout lines.
>
> Of course this is fully opt-in and per-device (we need that since gen2
> doesn't have a hw vblank counter).
>
> Mario reviewed the entire pile too and after some initial hesitation
> (about drivers without accurate timestampt support) acked it.
>
> Cheers, Daniel
>
>
> The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3:
>
>   drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000)
>
> are available in the git repository at:
>
>   git://anongit.freedesktop.org/drm-intel tags/topic/vblank-rework-2014-09-10
>
> for you to fetch changes up to 2368ffb18b1d2b04eb80478d225676caa7a3c4c8:
>
>   drm: Use vblank_disable_and_save in drm_vblank_cleanup() (2014-09-10 09:41:29 +0200)
>
> ----------------------------------------------------------------
> Mario Kleiner (2):
>       drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
>       drm: Use vblank_disable_and_save in drm_vblank_cleanup()
>
> Ville Syrjälä (16):
>       drm: Always reject drm_vblank_get() after drm_vblank_off()
>       drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
>       drm: Don't clear vblank timestamps when vblank interrupt is disabled
>       drm: Move drm_update_vblank_count()
>       drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off()
>       drm: Avoid random vblank counter jumps if the hardware counter has been reset
>       drm: Reduce the amount of dev->vblank[crtc] in the code
>       drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
>       drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
>       drm: Disable vblank interrupt immediately when drm_vblank_offdelay<0
>       drm: Add dev->vblank_disable_immediate flag
>       drm/i915: Opt out of vblank disable timer on >gen2
>       drm: Kick start vblank interrupts at drm_vblank_on()
>       drm/i915: Update scanline_offset only for active crtcs
>       drm: Fix confusing debug message in drm_update_vblank_count()
>       drm: Store the vblank timestamp when adjusting the counter during disable
>
>  Documentation/DocBook/drm.tmpl       |   7 +
>  drivers/gpu/drm/drm_drv.c            |   4 +-
>  drivers/gpu/drm/drm_irq.c            | 345 ++++++++++++++++++++++-------------
>  drivers/gpu/drm/i915/i915_irq.c      |   8 +
>  drivers/gpu/drm/i915/intel_display.c |  17 +-
>  include/drm/drmP.h                   |  12 +-
>  6 files changed, 256 insertions(+), 137 deletions(-)
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx