[RFC PATCH 5/5] drm/atomic: Make async plane update checks work as intended.

[Daniel Vetter]

On Wed, Aug 30, 2017 at 02:53:43PM +0200, Maarten Lankhorst wrote:
> Op 30-08-17 om 14:46 schreef Daniel Vetter:
> > On Wed, Aug 30, 2017 at 02:17:52PM +0200, Maarten Lankhorst wrote:
> >> By always keeping track of the last commit in plane_state, we know
> >> whether there is an active update on the plane or not. With that
> >> information we can reject the fast update, and force the slowpath
> >> to be used as was originally intended.
> >>
> >> Cc: Gustavo Padovan <gustavo.padovan at collabora.com>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > Makes sense, but I think like patch 1 it would be better to do this in a
> > separate series. Which would then include a patch to move i915 over to the
> > async plane support.
> >
> > One more comment below.
> >
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c  | 25 +++++++++++--------------
> >>  drivers/gpu/drm/i915/intel_display.c |  8 ++++++++
> >>  include/drm/drm_plane.h              |  5 +++--
> >>  3 files changed, 22 insertions(+), 16 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 034f563fb130..384d99347bb3 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -1388,8 +1388,8 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >>  {
> >>  	struct drm_crtc *crtc;
> >>  	struct drm_crtc_state *crtc_state;
> >> -	struct drm_plane *__plane, *plane = NULL;
> >> -	struct drm_plane_state *__plane_state, *plane_state = NULL;
> >> +	struct drm_plane *plane;
> >> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> >>  	const struct drm_plane_helper_funcs *funcs;
> >>  	int i, n_planes = 0;
> >>  
> >> @@ -1398,33 +1398,33 @@ int drm_atomic_helper_async_check(struct drm_device *dev,
> >>  			return -EINVAL;
> >>  	}
> >>  
> >> -	for_each_new_plane_in_state(state, __plane, __plane_state, i) {
> >> +	for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i)
> >>  		n_planes++;
> >> -		plane = __plane;
> >> -		plane_state = __plane_state;
> >> -	}
> >>  
> >>  	/* FIXME: we support only single plane updates for now */
> >> -	if (!plane || n_planes != 1)
> >> +	if (n_planes != 1)
> >>  		return -EINVAL;
> >>  
> >> -	if (!plane_state->crtc)
> >> +	if (!new_plane_state->crtc)
> >>  		return -EINVAL;
> >>  
> >>  	funcs = plane->helper_private;
> >>  	if (!funcs->atomic_async_update)
> >>  		return -EINVAL;
> >>  
> >> -	if (plane_state->fence)
> >> +	if (new_plane_state->fence)
> >>  		return -EINVAL;
> >>  
> >>  	/*
> >> -	 * TODO: Don't do an async update if there is an outstanding commit modifying
> >> +	 * 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.
> >>  	 */
> >> +	if (old_plane_state->commit &&
> >> +	    !try_wait_for_completion(&old_plane_state->commit->hw_done))
> >> +		return -EBUSY;
> > Instead of forcing us to always set the plane_state->commit pointer (bunch
> > of pointles refcounting), perhaps just check
> > plane_state->crtc->state->commit? We do hold the necessary locks to at
> > least look at that.
> Then we'd always take the slowpath?
> 
> The point here was to check whether the current plane was part of the
> most recent commit, to know this we must either add a flip_planes mask
> member to drm_crtc_commit, or add a pointer in plane_state to the most
> recent commit it was part of.

Hm right, that would block us against ongoing page_flips ... I guess
tracking commits at the plane level makes sense. If you can add that to
the commit message please, since otherwise I'll forget again?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch