[Intel-gfx] [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Daniel Vetter]

On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> Op 25-07-17 om 10:23 schreef Daniel Vetter:
>> On Wed, Jul 19, 2017 at 04:39:15PM +0200, Maarten Lankhorst wrote:
>>>      /*
>>> -     * Don't do an async update if there is an outstanding commit modifying
>>> -     * the plane.  This prevents our async update's changes from getting
>>> -     * overridden by a previous synchronous update's state.
>>> +     * FIXME: We should prevent an async update if there is an outstanding
>>> +     * commit modifying the plane.  This prevents our async update's
>>> +     * changes from getting overwritten by a previous synchronous update's
>>> +     * state.
>>>       */
>>> -    for_each_new_crtc_in_state(state, crtc, crtc_state, i) {
>>> -            if (plane->crtc != crtc)
>>> -                    continue;
>>> -
>>> -            spin_lock(&crtc->commit_lock);
>>> -            commit = list_first_entry_or_null(&crtc->commit_list,
>>> -                                              struct drm_crtc_commit,
>>> -                                              commit_entry);
>>> -            if (!commit) {
>>> -                    spin_unlock(&crtc->commit_lock);
>>> -                    continue;
>>> -            }
>>> -            spin_unlock(&crtc->commit_lock);
>>> -
>>> -            if (!crtc->state->state)
>>> -                    continue;
>>> -
>>> -            for_each_plane_in_state(crtc->state->state, __plane,
>>> -                                    __plane_state, j) {
>> I'm pretty sure this oopses, because crtc->state->state is NULL after
>> commit. I think Gustavo needs to review this first (and write a nasty igt
>> testcase to catch it) before we remove this. I think the correct check is
>> to simply bail out if our current crtc has a pending commit (i.e.
>> !list_empty(&crtc->commit_list) should be all we need to check.
> It didn't oops. Right above it was a null check. It was never executed. :)
>
> obj->state->state is always NULL. Excluding a brief moment during swap_state where this may oops,  this code willl never do a thing.

Oh right. It's still completely buggy, and I'd like to fix that first,
testcase included. Can you pls poke Gustavo a bit (or maybe he's on
vacation)?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch