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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Aug 27, 2015 at 02:43:35PM +0200, Maarten Lankhorst wrote:
> Op 27-08-15 om 14:19 schreef Daniel Stone:
> > Hi,
> >
> > On 4 August 2015 at 12:34, Maarten Lankhorst
> > <maarten.lankhorst@linux.intel.com> wrote:
> >> Commit ec9f932ed41622d120de52a5b525e4d77b9ef17e
> >> "drm/atomic: Cleanup on error properly in the atomic ioctl."
> >> cleaned up some error paths, but didn't fix the TEST_ONLY path.
> >> In the check only case plane->fb shouldn't be updated, and
> >> the vblank events should be cleared as on failure.
> > Bikeshedding a bit ...
> >
> > 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.

Why would commit fail when the we're in DPMS off? I would suggest it
should be allowed. The operation would just a be a nop from a HW point
of view, all the calculation/checks would still be performed.

> 
> Though I guess you're right and it's currently not allowed.
> >> @@ -1532,7 +1533,7 @@ retry:
> >>                 ret = drm_atomic_check_only(state);
> >>                 /* _check_only() does not free state, unlike _commit() */
> >>                 if (!ret)
> >> -                       drm_atomic_state_free(state);
> >> +                       goto free;
> >>         } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> >>                 ret = drm_atomic_async_commit(state);
> >>         } else {
> >> @@ -1566,6 +1567,7 @@ out:
> >>         }
> >>
> >>         if (ret) {
> >> +free:
> > 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.
> 
> 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@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);
> +		}
> +	}
> +
>  	if (ret == -EDEADLK) {
>  		drm_atomic_state_clear(state);
>  		drm_modeset_backoff(&ctx);
>  		goto retry;
>  	}
>  
> -	if (ret) {
> -		if (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);
> -			}
> -		}
> -
> +	if (ret || arg->flags & DRM_MODE_ATOMIC_TEST_ONLY)
>  		drm_atomic_state_free(state);
> -	}
>  
>  	drm_modeset_drop_locks(&ctx);
>  	drm_modeset_acquire_fini(&ctx);
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx