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.
-Daniel
>
> - return funcs->atomic_async_check(plane, plane_state);
> + return funcs->atomic_async_check(plane, new_plane_state);
> }
> EXPORT_SYMBOL(drm_atomic_helper_async_check);
>
> @@ -1790,9 +1790,6 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> }
>
> for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) {
> - if (new_plane_state->crtc)
> - continue;
> -
> if (nonblock && old_plane_state->commit &&
> !try_wait_for_completion(&old_plane_state->commit->flip_done))
> return -EBUSY;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 70ce02753177..cf21ec4ce920 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13043,6 +13043,14 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> goto slow;
>
> old_plane_state = plane->state;
> + /*
> + * 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))
> + goto slow;
>
> /*
> * If any parameters change that may affect watermarks,
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 7d96116fd4c4..feb9941d0cdb 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -124,9 +124,10 @@ struct drm_plane_state {
> bool visible;
>
> /**
> - * @commit: Tracks the pending commit to prevent use-after-free conditions.
> + * @commit: Tracks the pending commit to prevent use-after-free conditions,
> + * and for async plane updates.
> *
> - * Is only set when @crtc is NULL.
> + * May be NULL.
> */
> struct drm_crtc_commit *commit;
>
> --
> 2.11.0
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
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
