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@xxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
