Hi Mario,
Can you please take a look at the patches I've submitted and review
them (at least the first 2)? Dave will close the 3.18 drm-next merge
window at the end of this week and I'd like to really get this in.
Thanks, Daniel
On Wed, Sep 10, 2014 at 5:45 PM, Mario Kleiner
<mario.kleiner.de@xxxxxxxxx> wrote:
> On Wed, Sep 10, 2014 at 5:29 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
>> 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.
>>
>
> 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@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
--
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
