On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
<mario.kleiner.de@xxxxxxxxx> wrote:
> 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.
Sorry about the confusion, I've somehow thought that you've retracted
those comments in Message-ID:
<CAEsyxygK4Foqhky1WceRAk_hYbeX2OgPFTjYHu_ZFHLBX46dwA@xxxxxxxxxxxxxx>
But I've missed that that was about just one of the issues.
> 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));
Yeah, I guess that makes sense. I'm not really a fan of giving users
too powerful module options to hack around driver bugs since often
that means they'll never report the bug :( But we have the support now
to mark certain module options as debug-only and they'll taint the
kernel if set, so this is fixable.
I'll follow up with the patch you've suggested.
> ...
>
> 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 &&
> ) {
Yeah, makes sense (well the follow-up one ofc). I'll do a patch which
adds this and adds a comment. Aside I think it would be useful to add
a #define for the 0 return value, since the magic checks all over are
imo fairly hard to understand.
I'll also float a patch for rfc about that.
Thanks for your comments and again my apologies for missing that
there's still outstanding work left to do on this.
Cheers, Daniel
>
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
