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@xxxxxxxxxxxxx>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@xxxxxxxxxxxxxxx>
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
