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.pado...@collabora.com>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankho...@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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Reply via email to