Re: [PATCH 0/9] drm/nouveau: Cleanup event/handler design

[Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org>]

On 08/28/2013 03:15 AM, Ben Skeggs wrote:
> On Wed, Aug 28, 2013 at 6:12 AM, Peter Hurley <peter-WaGBZJeGNqdsbIuE7sb01tBPR1lH4CV8@public.gmane.org> wrote:
>> This series was originally motivated by a deadlock, introduced in
>> commit 1d7c71a3e2f77336df536855b0efd2dc5bdeb41b
>> 'drm/nouveau/disp: port vblank handling to event interface',
>> due to inverted lock order between nouveau_drm_vblank_enable()
>> and nouveau_drm_vblank_handler() (the complete
>> lockdep report is included in the patch 4/5 changelog).
> Hey Peter,
>
> Thanks for the patch series.  I've only taken a cursory glance through
> the patches thus far, as they're definitely too intrusive for -fixes
> at this point.  I will take a proper look through the series within
> the next week or so, I have some work pending which may possibly make
> some of these changes unnecessary, though from what I can tell,
> there's other good bits here we'll want.
>
> In a previous mail on the locking issue, you mentioned the drm core
> doing the "wrong thing" here too, did you have any further thoughts on
> correcting that issue too?

I'm working on the premise that drm_handle_vblank() can be lockless;
that would minimize the api disruption. I still have a fair bit of
analysis to do, but some early observations:

1) The vbl_time_lock spinlock is unnecessary -- drm_handle_vblank()
    could be protected with vbl_lock, since everywhere else
    vbl_time_lock is held, vbl_lock is already held.

2) The _vblank_count[crtc] doesn't need to be atomic. All the code
    paths that update _vblank_count[] are already spinlocked. Even
    if the code weren't spinlocked, the atomic_inc/_adds aren't
    necessary; only the memory barriers and read/write order of
    the vblank count/timestamp tuple is relevant.

3) Careful enabling of drm_handle_vblank() is sufficient to
    solve the concurrency between drm_handle_vblank() and
    drm_vblank_get() without needing a spinlock for exclusion.
    drm_handle_vblank() would need to account for the possibility of
    having missed a vblank interrupt between the reading of the
    hw vblank counter and the enabling drm_handle_vblank().

4) I'm still thinking about how/whether to exclude
    drm_handle_vblank() and vblank_disable_and_save(). One thought
    is to replace the timeout timer with delayed work instead so
    that vblank_disable_and_save() could wait for
    drm_handle_vblank() to finish after disabling it.
    [I'd also need to verify that the drm drivers other than
    intel which use drm_vblank_off() do so from non-atomic context.]

I realize that I'm mostly referring to the hw counter/timestamp
flavor of vblank handling; that's primarily because it requires a
more rigorous handling and has race conditions that don't apply
to the software-only counter.

Regards,
Peter Hurley