Re: [PATCH v2 2/7] drm/atomic: Clean up drm_atomic_helper_async_check

[Maarten Lankhorst <maarten.lankhorst@linux.intel.com>]

Op 25-07-17 om 11:27 schreef Daniel Vetter:
> On Tue, Jul 25, 2017 at 11:11 AM, Maarten Lankhorst
> <maarten.lankhorst@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

The only thing we have atm excercising it is kms_cursor_legacy, but that doesn't check if flips can be overwritten.
Perhaps we should make IGT tests a requirement for features in the future?

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel