On Wed, Sep 10, 2014 at 5:29 PM, Daniel Vetter <daniel.vetter at ffwll.ch>
wrote:
> On Wed, Sep 10, 2014 at 4:19 PM, Mario Kleiner
> <mario.kleiner.de at gmail.com> 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 at mail.gmail.com>
>
> But I've missed that that was about just one of the issues.
>
Thought so. That one patch turns out to be crucial. My own software
immediately complained loudly about broken vblank irqs and switched to
lower performance fallbacks when that patch was missing.
I'll test the patches on a few more cards in the next days - but so
far things look good at least as far as my special test cases go.
>> 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.
>
Thanks. I think the modules parameters i usually care about will get
proper testing and reporting, because while my software and users are
good at detecting such problems, they wouldn't know how to fix them
themselves, and at the same time they crucially depend on this stuff
working, so this gets reported to me quickly and i can give them the
module param workaround in private e-mail and take it from there with
proper bug reports or patches.
>> ...
>>
>> 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.
>
Good!
thanks,
-mario
> 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 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
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
