Hi Daniel,
Thank you for the patch.
On Wednesday 08 Jun 2016 17:15:36 Daniel Vetter wrote:
> To facilitate easier reviewing this is split out from the overall
> nonblocking commit rework. It just rolls out the helper functions
> and uses them in the main drm_atomic_helper_commit() function
> to make it clear where in the flow they're used.
>
> The next patch will actually split drm_atomic_helper_commit() into
> 2 pieces, with the tail being run asynchronously from a worker.
>
> v2: Improve kerneldocs (Maarten).
>
> v3: Don't convert ERESTARTSYS to EINTR (Maarten). Also don't fail if
> the wait succeed in stall_check - we need to convert that case (it
> returns the remaining jiffies) to 0 for success.
>
> v4: Switch to long for wait_for_completion_timeout return value
> everywhere (Maarten).
>
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Tested-by: Tomeu Vizoso <tomeu.vizoso at collabora.com>
> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> Cc: Tomeu Vizoso <tomeu.vizoso at gmail.com>
> Cc: Daniel Stone <daniels at collabora.com>
> Tested-by: Liviu Dudau <Liviu.Dudau at arm.com>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 347 +++++++++++++++++++++++++++++++++
> include/drm/drm_atomic_helper.h | 7 +
> 2 files changed, 354 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c index 326ee34cdba4..fa2f89253b17
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1155,6 +1155,10 @@ int drm_atomic_helper_commit(struct drm_device *dev,
> if (nonblock)
> return -EBUSY;
>
> + ret = drm_atomic_helper_setup_commit(state, nonblock);
> + if (ret)
> + return ret;
> +
> ret = drm_atomic_helper_prepare_planes(dev, state);
> if (ret)
> return ret;
> @@ -1185,16 +1189,22 @@ int drm_atomic_helper_commit(struct drm_device *dev,
>
> drm_atomic_helper_wait_for_fences(dev, state);
>
> + drm_atomic_helper_wait_for_dependencies(state);
> +
> drm_atomic_helper_commit_modeset_disables(dev, state);
>
> drm_atomic_helper_commit_planes(dev, state, false);
>
> drm_atomic_helper_commit_modeset_enables(dev, state);
>
> + drm_atomic_helper_commit_hw_done(state);
> +
> drm_atomic_helper_wait_for_vblanks(dev, state);
>
> drm_atomic_helper_cleanup_planes(dev, state);
>
> + drm_atomic_helper_commit_cleanup_done(state);
> +
> drm_atomic_state_free(state);
>
> return 0;
> @@ -1239,6 +1249,306 @@ EXPORT_SYMBOL(drm_atomic_helper_commit);
> * being displayed.
> */
>
> +static int stall_checks(struct drm_crtc *crtc, bool nonblock)
> +{
> + struct drm_crtc_commit *commit, *stall_commit = NULL;
> + bool completed = true;
> + int i;
> + long ret = 0;
> +
> + spin_lock(&crtc->commit_lock);
> + i = 0;
> + list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> + if (i == 0) {
> + completed = try_wait_for_completion(&commit-
>flip_done);
> + /* Userspace is not allowed to get ahead of the
previous
> + * commit with nonblocking ones. */
> + if (!completed && nonblock) {
> + spin_unlock(&crtc->commit_lock);
> + return -EBUSY;
> + }
> + } else if (i == 1) {
> + stall_commit = commit;
> + drm_crtc_commit_get(stall_commit);
> + } else
> + break;
> +
> + i++;
> + }
> + spin_unlock(&crtc->commit_lock);
> +
> + if (!stall_commit)
> + return 0;
> +
> + /* We don't want to let commits get ahead of cleanup work too much,
> + * stalling on 2nd previous commit means triple-buffer won't ever
stall.
> + */
> + ret = wait_for_completion_interruptible_timeout(&commit->cleanup_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CRTC:%d:%s] cleanup_done timed out\n",
> + crtc->base.id, crtc->name);
> +
> + drm_crtc_commit_put(stall_commit);
> +
> + return ret < 0 ? ret : 0;
> +}
> +
> +/**
> + * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
> + * @state: new modeset state to be committed
> + * @nonblock: whether nonblocking behavior is requested.
> + *
> + * This function prepares @state to be used by the atomic helper's support
> for + * nonblocking commits. Drivers using the nonblocking commit
> infrastructure + * should always call this function from their
> ->atomic_commit hook. + *
> + * To be able to use this support drivers need to use a few more helper
> + * functions. drm_atomic_helper_wait_for_dependencies() must be called
> before + * actually committing the hardware state, and for nonblocking
> commits this call + * must be placed in the async worker. See also
> drm_atomic_helper_swap_state() + * and it's stall parameter, for when a
> driver's commit hooks look at the + * ->state pointers of struct &drm_crtc,
> &drm_plane or &drm_connector directly. + *
> + * Completion of the hardware commit step must be signalled using
> + * drm_atomic_helper_commit_hw_done(). After this step the driver is not
> allowed + * to read or change any permanent software or hardware modeset
> state. The only + * exception is state protected by other means than
> &drm_modeset_lock locks. + * Only the free standing @state with pointers to
> the old state structures can + * be inspected, e.g. to clean up old buffers
> using
> + * drm_atomic_helper_cleanup_planes().
> + *
> + * At the very end, before cleaning up @state drivers must call
> + * drm_atomic_helper_commit_cleanup_done().
> + *
> + * This is all implemented by in drm_atomic_helper_commit(), giving drivers
> a + * complete and esay-to-use default implementation of the
> atomic_commit() hook. + *
> + * The tracking of asynchronously executed and still pending commits is
> done + * using the core structure &drm_crtc_commit.
> + *
> + * By default there's no need to clean up resources allocated by this
> function + * explicitly: drm_atomic_state_default_clear() will take care of
> that + * automatically.
> + *
> + * Returns:
> + *
> + * 0 on success. -EBUSY when userspace schedules nonblocking commits too
> fast, + * -ENOMEM on allocation failures and -EINTR when a signal is
> pending. + */
> +int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> + bool nonblock)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc_commit *commit;
> + int i, ret;
> +
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + commit = kzalloc(sizeof(*commit), GFP_KERNEL);
> + if (!commit)
> + return -ENOMEM;
> +
> + init_completion(&commit->flip_done);
> + init_completion(&commit->hw_done);
> + init_completion(&commit->cleanup_done);
> + INIT_LIST_HEAD(&commit->commit_entry);
> + kref_init(&commit->ref);
> + commit->crtc = crtc;
> +
> + state->crtcs[i].commit = commit;
> +
> + ret = stall_checks(crtc, nonblock);
> + if (ret)
> + return ret;
> +
> + /* Drivers only send out events when at least either current
or
> + * new CRTC state is active. Complete right away if everything
> + * stays off. */
> + if (!crtc->state->active && !crtc_state->active) {
> + complete_all(&commit->flip_done);
> + continue;
> + }
> +
> + /* Legacy cursor updates are fully unsynced. */
> + if (state->legacy_cursor_update) {
> + complete_all(&commit->flip_done);
> + continue;
> + }
> +
> + if (!crtc_state->event) {
> + commit->event = kzalloc(sizeof(*commit->event),
> + GFP_KERNEL);
> + if (!commit->event)
> + return -ENOMEM;
> +
> + crtc_state->event = commit->event;
> + }
> +
> + crtc_state->event->base.completion = &commit->flip_done;
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_setup_commit);
> +
> +
> +static struct drm_crtc_commit *preceeding_commit(struct drm_crtc *crtc)
> +{
> + struct drm_crtc_commit *commit;
> + int i = 0;
> +
> + list_for_each_entry(commit, &crtc->commit_list, commit_entry) {
> + /* skip the first entry, that's the current commit */
> + if (i == 1)
> + return commit;
> + i++;
> + }
> +
> + return NULL;
> +}
> +
> +/**
> + * drm_atomic_helper_wait_for_dependencies - wait for required preceeding
> commits + * @state: new modeset state to be committed
> + *
> + * This function waits for all preceeding commits that touch the same CRTC
> as + * @state to both be committed to the hardware (as signalled by
> + * drm_atomic_Helper_commit_hw_done) and executed by the hardware (as
> signalled + * by calling drm_crtc_vblank_send_event on the event member of
> + * &drm_crtc_state).
> + *
> + * This is part of the atomic helper support for nonblocking commits, see
> + * drm_atomic_helper_setup_commit() for an overview.
> + */
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state
> *state) +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc_commit *commit;
> + int i;
> + long ret;
> +
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + spin_lock(&crtc->commit_lock);
> + commit = preceeding_commit(crtc);
> + if (commit)
> + drm_crtc_commit_get(commit);
> + spin_unlock(&crtc->commit_lock);
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
> + crtc->base.id, crtc->name);
> +
> + /* Currently no support for overwriting flips, hence
> + * stall for previous one to execute completely. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> + crtc->base.id, crtc->name);
> +
> + drm_crtc_commit_put(commit);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_wait_for_dependencies);
> +
> +/**
> + * drm_atomic_helper_commit_hw_done - setup possible nonblocking commit
> + * @state: new modeset state to be committed
> + *
> + * This function is used to signal completion of the hardware commit step.
> After + * this step the driver is not allowed to read or change any
> permanent software + * or hardware modeset state. The only exception is
> state protected by other + * means than &drm_modeset_lock locks.
> + *
> + * Drivers should try to postpone any expensive or delayed cleanup work
> after + * this function is called.
> + *
> + * This is part of the atomic helper support for nonblocking commits, see
> + * drm_atomic_helper_setup_commit() for an overview.
> + */
> +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc_commit *commit;
> + int i;
> +
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + commit = state->crtcs[i].commit;
> + if (!commit)
> + continue;
> +
> + /* backend must have consumed any event by now */
> + WARN_ON(crtc->state->event);
> + spin_lock(&crtc->commit_lock);
> + complete_all(&commit->hw_done);
> + spin_unlock(&crtc->commit_lock);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_hw_done);
> +
> +/**
> + * drm_atomic_helper_commit_cleanup_done - signal completion of commit
> + * @state: new modeset state to be committed
> + *
> + * This signals completion of the atomic update @state, including any
> cleanup + * work. If used, it must be called right before calling
> + * drm_atomic_state_free().
> + *
> + * This is part of the atomic helper support for nonblocking commits, see
> + * drm_atomic_helper_setup_commit() for an overview.
> + */
> +void drm_atomic_helper_commit_cleanup_done(struct drm_atomic_state *state)
> +{
> + struct drm_crtc *crtc;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc_commit *commit;
> + int i;
> + long ret;
> +
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + commit = state->crtcs[i].commit;
> + if (WARN_ON(!commit))
> + continue;
> +
> + spin_lock(&crtc->commit_lock);
> + complete_all(&commit->cleanup_done);
> + WARN_ON(!try_wait_for_completion(&commit->hw_done));
> +
> + /* commit_list borrows our reference, need to remove before we
> + * clean up our drm_atomic_state. But only after it actually
> + * completed, otherwise subsequent commits won't stall
properly. */
> + if (try_wait_for_completion(&commit->flip_done)) {
> + list_del(&commit->commit_entry);
> + spin_unlock(&crtc->commit_lock);
> + continue;
> + }
> +
> + spin_unlock(&crtc->commit_lock);
> +
> + /* We must wait for the vblank event to signal our completion
> + * before releasing our reference, since the vblank work does
> + * not hold a reference of its own. */
> + ret = wait_for_completion_timeout(&commit->flip_done,
> + 10*HZ);
Why is this needed ? drm_atomic_helper_commit_cleanup_done() is called in
commit_tail() after the call to drm_atomic_helper_commit_tail() or to the
driver's .atomic_commit_tail() handler. If I'm not mistaken both already wait
for the page flip to complete, either with a call to
drm_atomic_helper_wait_for_vblanks() in drm_atomic_helper_commit_tail() or
with a custom method in the driver's .atomic_commit_tail() handler.
> + if (ret == 0)
> + DRM_ERROR("[CRTC:%d:%s] flip_done timed out\n",
> + crtc->base.id, crtc->name);
> +
> + spin_lock(&crtc->commit_lock);
> + list_del(&commit->commit_entry);
> + spin_unlock(&crtc->commit_lock);
> + }
> +}
> +EXPORT_SYMBOL(drm_atomic_helper_commit_cleanup_done);
> +
> /**
> * drm_atomic_helper_prepare_planes - prepare plane resources before commit
> * @dev: DRM device
> @@ -1558,17 +1868,45 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes);
> *
> * 5. Call drm_atomic_helper_cleanup_planes() with @state, which since step
> 3 * contains the old state. Also do any other cleanup required with that
> state. + *
> + * @stall must be set when nonblocking commits for this driver directly
> access + * the ->state pointer of &drm_plane, &drm_crtc or &drm_connector.
> With the + * current atomic helpers this is almost always the case, since
> the helpers + * don't pass the right state structures to the callbacks.
> */
> void drm_atomic_helper_swap_state(struct drm_atomic_state *state,
> bool stall)
> {
> int i;
> + long ret;
> struct drm_connector *connector;
> struct drm_connector_state *conn_state;
> struct drm_crtc *crtc;
> struct drm_crtc_state *crtc_state;
> struct drm_plane *plane;
> struct drm_plane_state *plane_state;
> + struct drm_crtc_commit *commit;
> +
> + if (stall) {
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + spin_lock(&crtc->commit_lock);
> + commit = list_first_entry_or_null(&crtc->commit_list,
> + struct drm_crtc_commit, commit_entry);
> + if (commit)
> + drm_crtc_commit_get(commit);
> + spin_unlock(&crtc->commit_lock);
> +
> + if (!commit)
> + continue;
> +
> + ret = wait_for_completion_timeout(&commit->hw_done,
> + 10*HZ);
> + if (ret == 0)
> + DRM_ERROR("[CRTC:%d:%s] hw_done timed out\n",
> + crtc->base.id, crtc->name);
> + drm_crtc_commit_put(commit);
> + }
> + }
>
> for_each_connector_in_state(state, connector, conn_state, i) {
> connector->state->state = state;
> @@ -1580,6 +1918,15 @@ void drm_atomic_helper_swap_state(struct
> drm_atomic_state *state, crtc->state->state = state;
> swap(state->crtcs[i].state, crtc->state);
> crtc->state->state = NULL;
> +
> + if (state->crtcs[i].commit) {
> + spin_lock(&crtc->commit_lock);
> + list_add(&state->crtcs[i].commit->commit_entry,
> + &crtc->commit_list);
> + spin_unlock(&crtc->commit_lock);
> +
> + state->crtcs[i].commit->event = NULL;
> + }
> }
>
> for_each_plane_in_state(state, plane, plane_state, i) {
> diff --git a/include/drm/drm_atomic_helper.h
> b/include/drm/drm_atomic_helper.h index 07ede3a82d54..368cbffc54ac 100644
> --- a/include/drm/drm_atomic_helper.h
> +++ b/include/drm/drm_atomic_helper.h
> @@ -74,6 +74,13 @@ void drm_atomic_helper_disable_planes_on_crtc(struct
> drm_crtc *crtc, void drm_atomic_helper_swap_state(struct drm_atomic_state
> *state, bool stall);
>
> +/* nonblocking commit helpers */
> +int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
> + bool nonblock);
> +void drm_atomic_helper_wait_for_dependencies(struct drm_atomic_state
> *state); +void drm_atomic_helper_commit_hw_done(struct drm_atomic_state
> *state); +void drm_atomic_helper_commit_cleanup_done(struct
> drm_atomic_state *state); +
> /* implementations for legacy interfaces */
> int drm_atomic_helper_update_plane(struct drm_plane *plane,
> struct drm_crtc *crtc,
--
Regards,
Laurent Pinchart
