[PATCH v2] drm/atomic: Fix bookkeeping with TEST_ONLY, v2.

[Daniel Stone]

Hi,

On 27 August 2015 at 13:43, Maarten Lankhorst
<maarten.lankhorst at linux.intel.com> wrote:
> Op 27-08-15 om 14:19 schreef Daniel Stone:
>> On 4 August 2015 at 12:34, Maarten Lankhorst
>> <maarten.lankhorst at linux.intel.com> wrote:
>> An early test precludes TEST_ONLY | PAGE_FLIP_EVENT, so you don't need
>> to mention this in the commit message; in this case, the main change
>> is about plane->{,old_}fb.
> Even testing with PAGE_FLIP_EVENT would be useful because
> event && !crtc_state->active should not be allowed. In that case test
> could succeed but commit could fail.

Will reply to that in the (very old) thread.

>> This is a bit nasty. Can we please move the label above the
>> conditional and change the condition to (ret || flags & TEST_ONLY)?
>> Doing that, you could also move the label above the (ret == -EDEADLK)
>> check, which would cover ->atomic_check needing to grab other states
>> (global resources?) and failing.
> If our bookkeeping is correct then it won't be harmful to fixup old_fb.
> Cleaning up more old_fb for more planes than initially had fb updates can always
> happen anyway, because a modeset will add all affected planes.

It won't happen: plane_mask will always be 0, because it's only set in
the branch which is now !TEST_ONLY. So yeah, it's safe to just skip
the label completely.

> How about the below patch? Apply with git am --scissors
>
> -------->8------
> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> cleaned up some error paths, but allowed -EDEADLK to
> leak vblank events.
>
> Additionally check_only was updating plane->fb, which should
> not be done when checking a new configuration only.
>
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2448f42480f..78ffb4965548 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1542,7 +1542,8 @@ retry:
>                         copied_props++;
>                 }
>
> -               if (obj->type == DRM_MODE_OBJECT_PLANE && count_props) {
> +               if (obj->type == DRM_MODE_OBJECT_PLANE && count_props &&
> +                   !(arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)) {
>                         plane = obj_to_plane(obj);
>                         plane_mask |= (1 << drm_plane_index(plane));
>                         plane->old_fb = plane->fb;
> @@ -1564,10 +1565,11 @@ retry:
>         }
>
>         if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> +               /*
> +                * Unlike commit, check_only does not clean up state.
> +                * Below we call drm_atomic_state_free for it.
> +                */
>                 ret = drm_atomic_check_only(state);
> -               /* _check_only() does not free state, unlike _commit() */
> -               if (!ret)
> -                       drm_atomic_state_free(state);
>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
>                 ret = drm_atomic_async_commit(state);
>         } else {
> @@ -1594,25 +1596,24 @@ out:
>                 plane->old_fb = NULL;
>         }
>
> +       if (ret && arg->flags & DRM_MODE_PAGE_FLIP_EVENT) {
> +               for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +                       if (!crtc_state->event)
> +                               continue;
> +
> +                       destroy_vblank_event(dev, file_priv,
> +                                            crtc_state->event);
> +               }
> +       }

Yeah, moving this looks good for fixing the -EDEADLK leak. So, great.
Can you please add a comment above though noting that we don't need to
call this branch on (ret == 0 && flags & DRM_MODE_ATOMIC_TEST_ONLY),
as we do for freeing the state, because TEST_ONLY and PAGE_FLIP_EVENT
are mutually exclusive? Otherwise the next person to come along and
have the same idea of allowing them is probably going to break this.
:P

With that though, feel free to send it directly with:
Reviewed-by: Daniel Stone <daniels at collabora.com>

Cheers,
Daniel