Re: [PATCH] drm/atomic-helper: roll out commit synchronization

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@linux.intel.com Tested-by: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Tomeu Vizoso tomeu.vizoso@gmail.com Cc: Daniel Stone daniels@collabora.com Tested-by: Liviu Dudau Liviu.Dudau@arm.com Signed-off-by: Daniel Vetter daniel.vetter@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,