Op 08-06-16 om 14:18 schreef Daniel Vetter:

atomic_flush seems to be the right place, but I'm not entirely sure whether this will catch them all. It could be that when disabling the crtc we'll miss the vblank.

While at it nuke the dummy functions.

v2: Be more robust and either arm, when the CRTC is on, or just send the event out right away.

Cc: Xinliang Liu xinliang.liu@linaro.org Cc: Xinwei Kong kong.kongxinwei@hisilicon.com Cc: Archit Taneja architt@codeaurora.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c index fba6372d060e..ed76baad525f 100644 --- a/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c +++ b/drivers/gpu/drm/hisilicon/kirin/kirin_drm_ade.c @@ -502,13 +502,6 @@ static void ade_crtc_disable(struct drm_crtc *crtc) acrtc->enable = false; }

-static int ade_crtc_atomic_check(struct drm_crtc *crtc,

• 		 struct drm_crtc_state *state)
  

-{

• /* do nothing */
• return 0;

-}

static void ade_crtc_mode_set_nofb(struct drm_crtc *crtc) { struct ade_crtc *acrtc = to_ade_crtc(crtc); @@ -537,6 +530,7 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, { struct ade_crtc *acrtc = to_ade_crtc(crtc); struct ade_hw_ctx *ctx = acrtc->ctx;

• struct drm_pending_vblank_event *event = crtc->state->event; void __iomem *base = ctx->base;

  /* only crtc is enabled regs take effect */

@@ -545,12 +539,22 @@ static void ade_crtc_atomic_flush(struct drm_crtc *crtc, /* flush ade registers */ writel(ADE_ENABLE, base + ADE_EN); }

• if (event) {

• crtc->state->event = NULL;
  

^I keep wondering why we set this to NULL every time. Nothing should depend on this right? duplicate_state sets this member to NULL.. I'd rather have crtc_state be constified after commit. Other than that..

Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

~Maarten

Hi Daniel,

On Wednesday 08 Jun 2016 14:18:58 Daniel Vetter wrote:

atomic_flush seems to be the right place, right after we commit the plane updates. Again use the fullproof version, since the pipe might be off.

Cc: Boris Brezillon boris.brezillon@free-electrons.com Cc: Maxime Ripard maxime.ripard@free-electrons.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/sun4i/sun4i_crtc.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/sun4i/sun4i_crtc.c b/drivers/gpu/drm/sun4i/sun4i_crtc.c index 4182a21f5923..f628b6d8f23f 100644 --- a/drivers/gpu/drm/sun4i/sun4i_crtc.c +++ b/drivers/gpu/drm/sun4i/sun4i_crtc.c @@ -51,10 +51,22 @@ static void sun4i_crtc_atomic_flush(struct drm_crtc *crtc, { struct sun4i_crtc *scrtc = drm_crtc_to_sun4i_crtc(crtc); struct sun4i_drv *drv = scrtc->drv;

• struct drm_pending_vblank_event *event = crtc->state->event;

  DRM_DEBUG_DRIVER("Committing plane changes\n");

  sun4i_backend_commit(drv->backend);

• if (event) {

• crtc->state->event = NULL;
  

• spin_lock_irq(&crtc->dev->event_lock);
  

• if (drm_crtc_vblank_get(crtc) == 0)
  

• 	drm_crtc_arm_vblank_event(crtc, event);
  

• else
  

• 	drm_crtc_send_vblank_event(crtc, event);
  

• spin_unlock_irq(&crtc->dev->event_lock);
  

• }

Is it me, or is it entirely useless given the code in sun4i_crtc_atomic_flush() that sets crtc->state->event to NULL ?

AuthorDate: Wed Jun 8 14:18:58 2016 +0200 CommitDate: Fri Jun 10 16:55:48 2016 +0200

It would have been worth it waiting for the maintainers to review the patch...

You've added similar code to a bunch of drivers. How do you protect against the race conditions documented in the drm_crtc_arm_vblank_event() function ?

}

static void sun4i_crtc_disable(struct drm_crtc *crtc)

On Wed, Jun 08, 2016 at 02:18:59PM +0200, Daniel Vetter wrote:

Just a bit of drive-by ocd.

v2: Improve per Liviu's feedback.

Cc: Liviu Dudau Liviu.Dudau@arm.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Acked-by: Liviu Dudau Liviu.Dudau@arm.com

---

include/drm/drm_atomic.h | 10 ++++++++++ 1 file changed, 10 insertions(+)

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d12cfb9c6062..a16861c882aa 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -198,6 +198,16 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); (plane_state) = (__state)->planes[__i].state, 1); \ (__i)++) \ for_each_if (plane_state)

+/**

• • drm_atomic_crtc_needs_modeset - compute combined modeset need
• • @state: &drm_crtc_state for the CRTC
• • To give drivers flexibility struct &drm_crtc_state has 3 booleans to track
• • whether the state CRTC changed enough to need a full modeset cycle:
• • connectors_changed, mode_changed and active_change. This helper simply
• • combines these three to compute the overall need for a modeset for @state.
• */

static inline bool drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state) { -- 2.8.1

On Wed, Jun 08, 2016 at 02:19:00PM +0200, Daniel Vetter wrote:

Split out from my big nonblocking atomic commit helper code as prep work. While add it, also add some neat asciiart to document how it's supposed to be used.

v2: Resurrect misplaced hunk in the kerneldoc.

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.c | 22 +++++++ drivers/gpu/drm/drm_crtc.c | 3 + drivers/gpu/drm/drm_fops.c | 6 ++ include/drm/drmP.h | 1 + include/drm/drm_atomic.h | 6 ++ include/drm/drm_crtc.h | 149 +++++++++++++++++++++++++++++++++++++++++-- 6 files changed, 181 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 5e4b820a977c..d99ab2f6663f 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -33,6 +33,20 @@

#include "drm_crtc_internal.h"

+static void crtc_commit_free(struct kref *kref) +{

• struct drm_crtc_commit *commit =

• container_of(kref, struct drm_crtc_commit, ref);
  

• kfree(commit);

+}

+void drm_crtc_commit_put(struct drm_crtc_commit *commit) +{

• kref_put(&commit->ref, crtc_commit_free);

+} +EXPORT_SYMBOL(drm_crtc_commit_put);

/**

• drm_atomic_state_default_release -
• release memory initialized by drm_atomic_state_init

@@ -148,6 +162,14 @@ void drm_atomic_state_default_clear(struct drm_atomic_state *state)

crtc->funcs->atomic_destroy_state(crtc,
				  state->crtcs[i].state);

• if (state->crtcs[i].commit) {
  

• 	kfree(state->crtcs[i].commit->event);
  

• 	state->crtcs[i].commit->event = NULL;
  

• 	drm_crtc_commit_put(state->crtcs[i].commit);
  

• }
  

• state->crtcs[i].commit = NULL;
  

  state->crtcs[i].ptr = NULL; state->crtcs[i].state = NULL; }

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index d3d0b4162e76..ce0569c3f942 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -669,6 +669,9 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, crtc->dev = dev; crtc->funcs = funcs;

• INIT_LIST_HEAD(&crtc->commit_list);
• spin_lock_init(&crtc->commit_lock);
• drm_modeset_lock_init(&crtc->mutex); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 44b3ecdeca63..323c238fcac7 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -686,6 +686,12 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e) { assert_spin_locked(&dev->event_lock);

• if (e->completion) {

• /* ->completion might disappear as soon as it signalled. */
  

• complete_all(e->completion);
  

• e->completion = NULL;
  

• }
• if (e->fence) { fence_signal(e->fence); fence_put(e->fence);

diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 15fe4a21a9da..dce655abd23d 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -283,6 +283,7 @@ struct drm_ioctl_desc {

/* Event queued up for userspace to read */ struct drm_pending_event {

• struct completion *completion; struct drm_event *event; struct fence *fence; struct list_head link;

diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index a16861c882aa..856a9c85a838 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -30,6 +30,12 @@

#include <drm/drm_crtc.h>

+void drm_crtc_commit_put(struct drm_crtc_commit *commit); +static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit) +{

• kref_get(&commit->ref);

+}

struct drm_atomic_state * __must_check drm_atomic_state_alloc(struct drm_device *dev); void drm_atomic_state_clear(struct drm_atomic_state *state); diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 520abafc9d00..14aa8212e30f 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -727,9 +727,6 @@ struct drm_crtc_funcs {

• @gamma_store: gamma ramp values
• @helper_private: mid-layer private data
• @properties: property tracking for this CRTC

• • @state: current atomic state for this CRTC
• • @acquire_ctx: per-CRTC implicit acquire context used by atomic drivers for
• • legacy IOCTLs
  • Each CRTC may have one or more connectors associated with it. This structure
  • allows the CRTC to be controlled.

@@ -786,11 +783,37 @@ struct drm_crtc {

struct drm_object_properties properties;

• /**

• * @state:
  

• *
  

• * Current atomic state for this CRTC.
  

• */
  

  struct drm_crtc_state *state;

• /*

• * For legacy crtc IOCTLs so that atomic drivers can get at the locking
  

• * acquire context.
  

• /**

• * @commit_list:
  

• *
  

• * List of &drm_crtc_commit structures tracking pending commits.
  

• * Protected by @commit_lock. This list doesn't hold its own full
  

• * reference, but burrows it from the ongoing commit. Commit entries
  

• * must be removed from this list once the commit is fully completed,
  

• * but before it's correspoding &drm_atomic_state gets destroyed.
  

• */
  

• struct list_head commit_list;
• /**

• * @commit_lock:
  

• *
  

• * Spinlock to protect @commit_list.
  

• */
  

• spinlock_t commit_lock;
• /**

• * @acquire_ctx:
  

• *
  

• * Per-CRTC implicit acquire context used by atomic drivers for legacy
  

• * IOCTLs, so that atomic drivers can get at the locking acquire
  

• * context.
  

  */ struct drm_modeset_acquire_ctx *acquire_ctx;

}; @@ -1732,6 +1755,111 @@ struct drm_bridge { void *driver_private; };

+/**

• • struct drm_crtc_commit - track modeset commits on a CRTC
• • This structure is used to track pending modeset changes and atomic commit on
• • a per-CRTC basis. Since updating the list should never block this structure
• • is reference counted to allow waiters to safely wait on an event to complete,
• • without holding any locks.
• • It has 3 different events in total to allow a fine-grained synchronoization

spelling police: synchronization

• • between outstanding updates::
• • atomic commit thread hardware
• • write new state into hardware ----> ...
• • signal hw_done

• • 				switch to new state on next
    

• • ... v/hblank
• • wait for buffers to show up ...
• • ... send completion irq

• • 				irq handler signals flip_done
    

• • cleanup old buffers
• • signal cleanup_done
• • wait for flip_done <----
• • clean up atomic state
• • The important bit to know is that cleanup_done is the terminal event, but the
• • ordering between flip_done and hw_done is entirely up to the specific driver
• • and modeset state change.
• • For an implementation of how to use this look at
• • drm_atomic_helper_setup_commit() from the atomic helper library.
• */

+struct drm_crtc_commit {

• /**

• * @crtc:
  

• *
  

• * DRM CRTC for this commit.
  

• */
  

• struct drm_crtc *crtc;
• /**

• * @ref:
  

• *
  

• * Reference count for this structure. Needed to allow blocking on
  

• * completions without the risk of the completion disappearing
  

• * meanwhile.
  

• */
  

• struct kref ref;
• /**

• * @flip_done:
  

• *
  

• * Will be signaled when the hardware has flipped to the new set of
  

• * buffers. Signals at the same time as when the drm event for this
  

• * commit is sent to userspace, or when an out-fence is singalled. Note
  

• * that for most hardware, in most cases this happens after @hw_done is
  

• * signalled.
  

• */
  

• struct completion flip_done;
• /**

• * @hw_done:
  

• *
  

• * Will be signalled when all hw register changes for this commit have
  

• * been written out. Especially when disabling a pipe this can be much
  

• * later than than @flip_done, since that can signal already when the
  

• * screen goes black, whereas to fully shut down a pipe more register
  

• * I/O is required.
  

• *
  

• * Note that this does not need to include separately reference-counted
  

• * resources like backing storage buffer pinning, or runtime pm
  

• * management.
  

• */
  

• struct completion hw_done;
• /**

• * @cleanup_done:
  

• *
  

• * Will be signalled after old buffers have been cleaned up again by
  

what do you mean by "cleaned up *again*" ? If you are trying to emphasize the following sentence, I think you can drop "again" from it, but again, I'm only a software developer :)

Best regards, Liviu

• * calling drm_atomic_helper_cleanup_planes(). Since this can only
  

• * happen after a vblank wait completed it might be a bit later. This
  

• * completion is useful to throttle updates and avoid hardware updates
  

• * getting ahead of the buffer cleanup too much.
  

• */
  

• struct completion cleanup_done;
• /**

• * @commit_entry:
  

• *
  

• * Entry on the per-CRTC commit_list. Protected by crtc->commit_lock.
  

• */
  

• struct list_head commit_entry;
• /**

• * @event:
  

• *
  

• * &drm_pending_vblank_event pointer to clean up private events.
  

• */
  

• struct drm_pending_vblank_event *event;

+};

struct __drm_planes_state { struct drm_plane *ptr; struct drm_plane_state *state; @@ -1740,6 +1868,7 @@ struct __drm_planes_state { struct __drm_crtcs_state { struct drm_crtc *ptr; struct drm_crtc_state *state;

• struct drm_crtc_commit *commit;

};

struct __drm_connnectors_state { @@ -1770,6 +1899,14 @@ struct drm_atomic_state { struct __drm_connnectors_state *connectors;

struct drm_modeset_acquire_ctx *acquire_ctx;

• /**

• * @commit_work:
  

• *
  

• * Work item which can be used by the driver or helpers to execute the
  

• * commit without blocking.
  

• */
  

• struct work_struct commit_work;

};

-- 2.8.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

Op 08-06-16 om 14:19 schreef Daniel Vetter:

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.

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 | 346 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 + 2 files changed, 353 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 326ee34cdba4..63e46827b303 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,305 @@ 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, ret = 0;

^Should probably be a long ret, or truncation gets done too early if we decide to bump the timeout to infinity.

Must admit I missed the error of ret > 0 in v2.

Looking good though..

Reviewed-by: Maarten Lankhorst maarten.lankhorst@linux.intel.com

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,

Op 08-06-16 om 14:19 schreef Daniel Vetter:

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.

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 | 346 ++++++++++++++++++++++++++++++++++++ include/drm/drm_atomic_helper.h | 7 + 2 files changed, 353 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 326ee34cdba4..63e46827b303 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,305 @@ 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, 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

^extra cap here

On Fri, Jun 10, 2016 at 12:07:53AM +0200, Daniel Vetter wrote:

Now that the core helpers support nonblocking atomic commits there's no need to invent that wheel separately (instead of fixing the bug in the atomic implementation of virtio, as it should have been done!).

v2: Rebased on top of

commit e7cf0963f816fa44190caaf51aeffaa614c340c6 Author: Gerd Hoffmann kraxel@redhat.com Date: Tue May 31 08:50:47 2016 +0200

virtio-gpu: add atomic_commit function

Cc: Gerd Hoffmann kraxel@redhat.com Tested-by: Gerd Hoffmann kraxel@redhat.com (v1) Reviewed-by: Gerd Hoffmann kraxel@redhat.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Gerd, can you pls retest? I think due to your change in the above referenced commit to switch to active_only=true in commit_planes() this is now broken. -Daniel

---

drivers/gpu/drm/virtio/virtgpu_display.c | 71 ++++++-------------------------- 1 file changed, 13 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 3d0fa049b34c..a09dc57fea9c 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -38,56 +38,11 @@ #define XRES_MAX 8192 #define YRES_MAX 8192

-static int virtio_gpu_page_flip(struct drm_crtc *crtc,

• 		struct drm_framebuffer *fb,
  

• 		struct drm_pending_vblank_event *event,
  

• 		uint32_t flags)
  

-{

• struct virtio_gpu_device *vgdev = crtc->dev->dev_private;
• struct virtio_gpu_output *output =

• container_of(crtc, struct virtio_gpu_output, crtc);
  

• struct drm_plane *plane = crtc->primary;
• struct virtio_gpu_framebuffer *vgfb;
• struct virtio_gpu_object *bo;
• unsigned long irqflags;
• uint32_t handle;
• plane->fb = fb;
• vgfb = to_virtio_gpu_framebuffer(plane->fb);
• bo = gem_to_virtio_gpu_obj(vgfb->obj);
• handle = bo->hw_res_handle;
• DRM_DEBUG("handle 0x%x%s, crtc %dx%d\n", handle,

•   bo->dumb ? ", dumb" : "",
  

•   crtc->mode.hdisplay, crtc->mode.vdisplay);
  

• if (bo->dumb) {

• virtio_gpu_cmd_transfer_to_host_2d
  

• 	(vgdev, handle, 0,
  

• 	 cpu_to_le32(crtc->mode.hdisplay),
  

• 	 cpu_to_le32(crtc->mode.vdisplay),
  

• 	 0, 0, NULL);
  

• }
• virtio_gpu_cmd_set_scanout(vgdev, output->index, handle,

• 		   crtc->mode.hdisplay,
  

• 		   crtc->mode.vdisplay, 0, 0);
  

• virtio_gpu_cmd_resource_flush(vgdev, handle, 0, 0,

• 		      crtc->mode.hdisplay,
  

• 		      crtc->mode.vdisplay);
  

• if (event) {

• spin_lock_irqsave(&crtc->dev->event_lock, irqflags);
  

• drm_crtc_send_vblank_event(crtc, event);
  

• spin_unlock_irqrestore(&crtc->dev->event_lock, irqflags);
  

• }
• return 0;

-}

static const struct drm_crtc_funcs virtio_gpu_crtc_funcs = { .set_config = drm_atomic_helper_set_config, .destroy = drm_crtc_cleanup,

• .page_flip = virtio_gpu_page_flip,

• .page_flip = drm_atomic_helper_page_flip, .reset = drm_atomic_helper_crtc_reset, .atomic_duplicate_state = drm_atomic_helper_crtc_duplicate_state, .atomic_destroy_state = drm_atomic_helper_crtc_destroy_state,

@@ -185,6 +140,7 @@ static void virtio_gpu_crtc_atomic_flush(struct drm_crtc *crtc, spin_lock_irqsave(&crtc->dev->event_lock, flags); if (crtc->state->event) drm_crtc_send_vblank_event(crtc, crtc->state->event);

• crtc->state->event = NULL; spin_unlock_irqrestore(&crtc->dev->event_lock, flags);

}

@@ -388,30 +344,28 @@ virtio_gpu_user_framebuffer_create(struct drm_device *dev, return &virtio_gpu_fb->base; }

-static int vgdev_atomic_commit(struct drm_device *dev,

• 	       struct drm_atomic_state *state,
  

• 	       bool nonblock)
  

+static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) {

• if (nonblock)

• return -EBUSY;
  

• drm_atomic_helper_swap_state(state, true);
• drm_atomic_helper_wait_for_fences(dev, state);

• struct drm_device *dev = state->dev;

  drm_atomic_helper_commit_modeset_disables(dev, state); drm_atomic_helper_commit_modeset_enables(dev, state); drm_atomic_helper_commit_planes(dev, state, true);

• drm_atomic_helper_commit_hw_done(state);

• drm_atomic_helper_wait_for_vblanks(dev, state); drm_atomic_helper_cleanup_planes(dev, state);

• drm_atomic_state_free(state);
• return 0;

}

+struct drm_mode_config_helper_funcs virtio_mode_config_helpers = {

• .atomic_commit_tail = vgdev_atomic_commit_tail,

+};

static const struct drm_mode_config_funcs virtio_gpu_mode_funcs = { .fb_create = virtio_gpu_user_framebuffer_create, .atomic_check = drm_atomic_helper_check,

• .atomic_commit = vgdev_atomic_commit,

• .atomic_commit = drm_atomic_helper_commit,

};

int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) @@ -419,7 +373,8 @@ int virtio_gpu_modeset_init(struct virtio_gpu_device *vgdev) int i;

drm_mode_config_init(vgdev->ddev);

• vgdev->ddev->mode_config.funcs = (void *)&virtio_gpu_mode_funcs;

• vgdev->ddev->mode_config.funcs = &virtio_gpu_mode_funcs;

• vgdev->ddev->mode_config.helper_private = &virtio_mode_config_helpers;

  /* modes will be validated against the framebuffer size */ vgdev->ddev->mode_config.min_width = XRES_MIN;

-- 2.8.1

On Wed, 8 Jun 2016 14:19:15 +0200 Daniel Vetter daniel.vetter@ffwll.ch wrote:

Drivers transitioning to atomic might not yet want to enable full DRIVER_ATOMIC support when it's not entirely working. But using atomic internally makes a lot more sense earlier.

Instead of spreading such flags to more places I figured it's simpler to just check for mode_config->funcs->atomic_commit, and use atomic paths if that is set. For the only driver currently transitioning (i915) this does the right thing.

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Boris Brezillon boris.brezillon@free-electrons.com

---

drivers/gpu/drm/drm_fb_helper.c | 6 ++---- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- include/drm/drm_fb_helper.h | 11 ----------- 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c0e0a2e78d75..ba2fcb2a68ad 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)

drm_warn_on_modeset_not_all_locked(dev);

• if (fb_helper->atomic)

• if (dev->mode_config.funcs->atomic_commit) return restore_fbdev_mode_atomic(fb_helper);

  drm_for_each_plane(plane, dev) {

@@ -716,8 +716,6 @@ int drm_fb_helper_init(struct drm_device *dev, i++; }

• fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);
• return 0;

out_free: drm_fb_helper_crtc_free(fb_helper); @@ -1344,7 +1342,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }

• if (fb_helper->atomic) {

• if (dev->mode_config.funcs->atomic_commit) { ret = pan_display_atomic(var, info); goto unlock; }

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e67690f3d..4c725ad6fb54 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -724,8 +724,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }

• ifbdev->helper.atomic = true;
• dev_priv->fbdev = ifbdev; INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5b4aa35026a3..db8d4780eaa2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -212,17 +212,6 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug;

• /**

• * @atomic:
  

• *
  

• * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
  

• * true if driver has DRIVER_ATOMIC feature flag, but drivers can
  

• * override it to true after drm_fb_helper_init() if they support atomic
  

• * modeset but do not yet advertise DRIVER_ATOMIC (note that fb-helper
  

• * does not require ASYNC commits).
  

• */
  

• bool atomic;

};

#ifdef CONFIG_DRM_FBDEV_EMULATION

On Sun, Jun 12, 2016 at 05:01:27PM +0800, Ying Liu wrote:

On Wed, Jun 8, 2016 at 8:19 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Drivers transitioning to atomic might not yet want to enable full DRIVER_ATOMIC support when it's not entirely working. But using atomic internally makes a lot more sense earlier.

Instead of spreading such flags to more places I figured it's simpler to just check for mode_config->funcs->atomic_commit, and use atomic paths if that is set. For the only driver currently transitioning (i915) this does the right thing.

Well, imx-drm is transitioning. Unfortunately, after applying this patch, legacy fbdev cannot enable displays when the imx-drm transitional patch set reaches phase 3 step 1[1].

For those transitioning drivers with fine-grained steps, this patch is likely to expose the atomic function __drm_atomic_helper_set_config to legacy fbdev support too early. They could still be using drm_crtc_helper_set_config when adding ->atomic_commit.

BTW, this patch and those for generic nonblocking commit are making the imx-drm transition a bit harder. Not good timing probably. But, I'll go on with the task anyway.

Hm, making transition harder wasn't really the intention. What exactly is blowing up for you? At least how I planned the transition the first thing you should be able to do is basic modesets (once you fill out ->atomic_commit), so I hoped that this wouldn't cause trouble.

Wrt nonblocking commit helpers breaking transitioning drivers: The most likely cause is your driver isn't handling crtc_state->event yet. Before you start using ->atomic_commit or one of the helpers to map legacy ioctl to atomic, you need to fill out some code to handle that in ->atomic_begin or ->atomic_flush. Then the transition should still work as before.

I'll be happy to help you out debug this, and then update my blog post with the transition plan with the latest findings.

Thanks, Daniel

[1] https://patchwork.kernel.org/patch/9144035/

Regards, Liu Ying

...

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_fb_helper.c | 6 ++---- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- include/drm/drm_fb_helper.h | 11 ----------- 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c0e0a2e78d75..ba2fcb2a68ad 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)

    drm_warn_on_modeset_not_all_locked(dev);

•   if (fb_helper->atomic)
  

•   if (dev->mode_config.funcs->atomic_commit)
            return restore_fbdev_mode_atomic(fb_helper);
  
    drm_for_each_plane(plane, dev) {
  

@@ -716,8 +716,6 @@ int drm_fb_helper_init(struct drm_device *dev, i++; }

•   fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);
  

•   return 0;
  

out_free: drm_fb_helper_crtc_free(fb_helper); @@ -1344,7 +1342,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }

•   if (fb_helper->atomic) {
  

•   if (dev->mode_config.funcs->atomic_commit) {
            ret = pan_display_atomic(var, info);
            goto unlock;
    }
  

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e67690f3d..4c725ad6fb54 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -724,8 +724,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }

•   ifbdev->helper.atomic = true;
  

•   dev_priv->fbdev = ifbdev;
    INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
  
  

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5b4aa35026a3..db8d4780eaa2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -212,17 +212,6 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug;

•   /**
  

•    * @atomic:
  

•    *
  

•    * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
  

•    * true if driver has DRIVER_ATOMIC feature flag, but drivers can
  

•    * override it to true after drm_fb_helper_init() if they support atomic
  

•    * modeset but do not yet advertise DRIVER_ATOMIC (note that fb-helper
  

•    * does not require ASYNC commits).
  

•    */
  

•   bool atomic;
  

};

#ifdef CONFIG_DRM_FBDEV_EMULATION

2.8.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

On Mon, Jun 13, 2016 at 05:26:28PM +0800, Ying Liu wrote:

On Mon, Jun 13, 2016 at 3:58 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Sun, Jun 12, 2016 at 05:01:27PM +0800, Ying Liu wrote:

...

On Wed, Jun 8, 2016 at 8:19 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Drivers transitioning to atomic might not yet want to enable full DRIVER_ATOMIC support when it's not entirely working. But using atomic internally makes a lot more sense earlier.

Instead of spreading such flags to more places I figured it's simpler to just check for mode_config->funcs->atomic_commit, and use atomic paths if that is set. For the only driver currently transitioning (i915) this does the right thing.

Well, imx-drm is transitioning. Unfortunately, after applying this patch, legacy fbdev cannot enable displays when the imx-drm transitional patch set reaches phase 3 step 1[1].

For those transitioning drivers with fine-grained steps, this patch is likely to expose the atomic function __drm_atomic_helper_set_config to legacy fbdev support too early. They could still be using drm_crtc_helper_set_config when adding ->atomic_commit.

BTW, this patch and those for generic nonblocking commit are making the imx-drm transition a bit harder. Not good timing probably. But, I'll go on with the task anyway.

Hm, making transition harder wasn't really the intention. What exactly is blowing up for you? At least how I planned the transition the first thing you should be able to do is basic modesets (once you fill out ->atomic_commit), so I hoped that this wouldn't cause trouble.

At the imx-drm transition phase 1, ipu_plane_atomic_check checks crtc->enabled and hopes it to be true. Till phase 3 step 1, this function is not changed. This patch exposes restore_fbdev_mode_atomic and pan_display_atomic to legacy fbdev too early. Both of them call drm_atomic_commit which does plane check at atomic check stage. Then, the plane check fails for the legacy fbdev, because drm_atomic_commit sets crtc->enabled later than the legacy path drm_crtc_helper_set_mode does. Actually, drm_atomic_commit doesn't set crtc->enabled until the commit stage if you use the atomic helper.

Hm, my idea was that in phase 2 (when you roll out state structures and checks), you'd change your atomic_check functions from looking at legacy stuff like crtc->enabled, to instead look at new state like crtc_state->enabled.

OTOH, we fill out ->atomic_commit with drm_atomic_helper_commit at phase 3 step 1, then exposing drm_atomic_commit means that we need to handle crtc_state->event no later than phase 3 step 1... I haven't decided the right order/process to add the handling.

Yes, this is a change: Before you can start with phase 3 you need to handle crtc_state->event in atomic_flush (or a similar place).

So, would it be an option to revert this patch...

I'd really like to avoid that - the old hack of adding a knob for every place we should use atomic was getting complicated :( And right now there's many more atomic drivers than legacy ones, and only 2 drivers in transition (i915 and imx). Given that I think we should optimize more for atomic drivers.

But like I said, of course transition shouldn't be too painful.

...

Wrt nonblocking commit helpers breaking transitioning drivers: The most likely cause is your driver isn't handling crtc_state->event yet. Before you start using ->atomic_commit or one of the helpers to map legacy ioctl to atomic, you need to fill out some code to handle that in ->atomic_begin or ->atomic_flush. Then the transition should still work as before.

I do have confusion on the event handling in some drivers' ->atomic_flush. At least sun4i, kirin and fsl-dcu have the below snip: ========================================== if (event) { crtc->state->event = NULL;

            spin_lock_irq(&crtc->dev->event_lock);
            if (drm_crtc_vblank_get(crtc) == 0)
                    drm_crtc_arm_vblank_event(crtc, event);
            else
                    drm_crtc_send_vblank_event(crtc, event);
            spin_unlock_irq(&crtc->dev->event_lock);
    }

========================================== It looks drm_crtc_vblank_get seldom fails?

If it never fails for you, then you haven't put drm_crtc_vblank_on/off() into your crtc enable/disable callbacks. Ofc this assumes you have real vblank support.

And why do we send vblank event when it fails?

This is just the most failsafe way to make sure the event gets out. It's rather inaccurate though, so would be much better if you can make that decision by instead looking at crtc->state->active, like i915 does. drm_crtc_vblank_get always works in-betwen drm_crtc_vblank_on/off() calls, i.e. when the crtc is supposed to be on. And when you disable the crtc and it stays disable then you should send out the vblank event directly, after the pipe is off. But that's really the only case where you should call drm_crtc_send_vblank_event directly.

The kerneldoc for these functions try to explain this a bit ...

Anyway, pls don't look at these drivers - I just hacked them up because they didn't implement event handling at all. Which is breaking atomic. For a good example of how it should be done, look at rockchip (already merged in drm-misc) or i915 (patches on the m-l). -Daniel

Regards, Liu Ying

...

I'll be happy to help you out debug this, and then update my blog post with the transition plan with the latest findings.

Thanks, Daniel

...

[1] https://patchwork.kernel.org/patch/9144035/

Regards, Liu Ying

...

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_fb_helper.c | 6 ++---- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- include/drm/drm_fb_helper.h | 11 ----------- 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c0e0a2e78d75..ba2fcb2a68ad 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)

    drm_warn_on_modeset_not_all_locked(dev);

•   if (fb_helper->atomic)
  

•   if (dev->mode_config.funcs->atomic_commit)
            return restore_fbdev_mode_atomic(fb_helper);
  
    drm_for_each_plane(plane, dev) {
  

@@ -716,8 +716,6 @@ int drm_fb_helper_init(struct drm_device *dev, i++; }

•   fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);
  

•   return 0;
  

out_free: drm_fb_helper_crtc_free(fb_helper); @@ -1344,7 +1342,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }

•   if (fb_helper->atomic) {
  

•   if (dev->mode_config.funcs->atomic_commit) {
            ret = pan_display_atomic(var, info);
            goto unlock;
    }
  

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e67690f3d..4c725ad6fb54 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -724,8 +724,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }

•   ifbdev->helper.atomic = true;
  

•   dev_priv->fbdev = ifbdev;
    INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
  
  

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5b4aa35026a3..db8d4780eaa2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -212,17 +212,6 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug;

•   /**
  

•    * @atomic:
  

•    *
  

•    * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
  

•    * true if driver has DRIVER_ATOMIC feature flag, but drivers can
  

•    * override it to true after drm_fb_helper_init() if they support atomic
  

•    * modeset but do not yet advertise DRIVER_ATOMIC (note that fb-helper
  

•    * does not require ASYNC commits).
  

•    */
  

•   bool atomic;
  

};

#ifdef CONFIG_DRM_FBDEV_EMULATION

2.8.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Mon, Jun 20, 2016 at 01:55:57PM +0800, Ying Liu wrote:

On Mon, Jun 13, 2016 at 10:01 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Mon, Jun 13, 2016 at 05:26:28PM +0800, Ying Liu wrote:

...

On Mon, Jun 13, 2016 at 3:58 PM, Daniel Vetter daniel@ffwll.ch wrote:

...

On Sun, Jun 12, 2016 at 05:01:27PM +0800, Ying Liu wrote:

...

On Wed, Jun 8, 2016 at 8:19 PM, Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Drivers transitioning to atomic might not yet want to enable full DRIVER_ATOMIC support when it's not entirely working. But using atomic internally makes a lot more sense earlier.

Instead of spreading such flags to more places I figured it's simpler to just check for mode_config->funcs->atomic_commit, and use atomic paths if that is set. For the only driver currently transitioning (i915) this does the right thing.

Well, imx-drm is transitioning. Unfortunately, after applying this patch, legacy fbdev cannot enable displays when the imx-drm transitional patch set reaches phase 3 step 1[1].

For those transitioning drivers with fine-grained steps, this patch is likely to expose the atomic function __drm_atomic_helper_set_config to legacy fbdev support too early. They could still be using drm_crtc_helper_set_config when adding ->atomic_commit.

BTW, this patch and those for generic nonblocking commit are making the imx-drm transition a bit harder. Not good timing probably. But, I'll go on with the task anyway.

Hm, making transition harder wasn't really the intention. What exactly is blowing up for you? At least how I planned the transition the first thing you should be able to do is basic modesets (once you fill out ->atomic_commit), so I hoped that this wouldn't cause trouble.

At the imx-drm transition phase 1, ipu_plane_atomic_check checks crtc->enabled and hopes it to be true. Till phase 3 step 1, this function is not changed. This patch exposes restore_fbdev_mode_atomic and pan_display_atomic to legacy fbdev too early. Both of them call drm_atomic_commit which does plane check at atomic check stage. Then, the plane check fails for the legacy fbdev, because drm_atomic_commit sets crtc->enabled later than the legacy path drm_crtc_helper_set_mode does. Actually, drm_atomic_commit doesn't set crtc->enabled until the commit stage if you use the atomic helper.

Hm, my idea was that in phase 2 (when you roll out state structures and checks), you'd change your atomic_check functions from looking at legacy stuff like crtc->enabled, to instead look at new state like crtc_state->enabled.

...

OTOH, we fill out ->atomic_commit with drm_atomic_helper_commit at phase 3 step 1, then exposing drm_atomic_commit means that we need to handle crtc_state->event no later than phase 3 step 1... I haven't decided the right order/process to add the handling.

Yes, this is a change: Before you can start with phase 3 you need to handle crtc_state->event in atomic_flush (or a similar place).

...

So, would it be an option to revert this patch...

I'd really like to avoid that - the old hack of adding a knob for every place we should use atomic was getting complicated :( And right now there's many more atomic drivers than legacy ones, and only 2 drivers in transition (i915 and imx). Given that I think we should optimize more for atomic drivers.

But like I said, of course transition shouldn't be too painful.

...

...

Wrt nonblocking commit helpers breaking transitioning drivers: The most likely cause is your driver isn't handling crtc_state->event yet. Before you start using ->atomic_commit or one of the helpers to map legacy ioctl to atomic, you need to fill out some code to handle that in ->atomic_begin or ->atomic_flush. Then the transition should still work as before.

I do have confusion on the event handling in some drivers' ->atomic_flush. At least sun4i, kirin and fsl-dcu have the below snip: ========================================== if (event) { crtc->state->event = NULL;

            spin_lock_irq(&crtc->dev->event_lock);
            if (drm_crtc_vblank_get(crtc) == 0)
                    drm_crtc_arm_vblank_event(crtc, event);
            else
                    drm_crtc_send_vblank_event(crtc, event);
            spin_unlock_irq(&crtc->dev->event_lock);
    }

========================================== It looks drm_crtc_vblank_get seldom fails?

If it never fails for you, then you haven't put drm_crtc_vblank_on/off() into your crtc enable/disable callbacks. Ofc this assumes you have real vblank support.

...

And why do we send vblank event when it fails?

This is just the most failsafe way to make sure the event gets out. It's rather inaccurate though, so would be much better if you can make that decision by instead looking at crtc->state->active, like i915 does. drm_crtc_vblank_get always works in-betwen drm_crtc_vblank_on/off() calls, i.e. when the crtc is supposed to be on. And when you disable the crtc and it stays disable then you should send out the vblank event directly, after the pipe is off. But that's really the only case where you should call drm_crtc_send_vblank_event directly.

The kerneldoc for these functions try to explain this a bit ...

Anyway, pls don't look at these drivers - I just hacked them up because they didn't implement event handling at all. Which is breaking atomic. For a good example of how it should be done, look at rockchip (already merged in drm-misc) or i915 (patches on the m-l).

It looks calling drm_vblank_off in crtc_helper->disable will set vblank->enabled to false and increase vblank->refcount by one. The next enable operation could call crtc_helper->atomic_begin or atomic_flush and then crtc_helper->enable. In ->atomic_begin or atomic_flush, it looks drm_crtc_vblank_get could be called. The problem is that drm_crtc_vblank_get finds vblank->refcount is not 1 after an add-1-operation and vblank->enabled is false, which finally makes the function return -EINVAL. How to handle this problem?

That's how it's supposed to work: When the vblank is off, then you can't send out vblank events. If the pipe gets (re)enabled, then you need to move the vblank generation to where it's fully on. If it stays disabled, then you can directly call drm_crtc_send_vblank_event.

btw much easier to discuss this on irc. -Daniel

Regards, Liu Ying

...

-Daniel

...

Regards, Liu Ying

...

I'll be happy to help you out debug this, and then update my blog post with the transition plan with the latest findings.

Thanks, Daniel

...

[1] https://patchwork.kernel.org/patch/9144035/

Regards, Liu Ying

...

Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/drm_fb_helper.c | 6 ++---- drivers/gpu/drm/i915/intel_fbdev.c | 2 -- include/drm/drm_fb_helper.h | 11 ----------- 3 files changed, 2 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c index c0e0a2e78d75..ba2fcb2a68ad 100644 --- a/drivers/gpu/drm/drm_fb_helper.c +++ b/drivers/gpu/drm/drm_fb_helper.c @@ -385,7 +385,7 @@ static int restore_fbdev_mode(struct drm_fb_helper *fb_helper)

    drm_warn_on_modeset_not_all_locked(dev);

•   if (fb_helper->atomic)
  

•   if (dev->mode_config.funcs->atomic_commit)
            return restore_fbdev_mode_atomic(fb_helper);
  
    drm_for_each_plane(plane, dev) {
  

@@ -716,8 +716,6 @@ int drm_fb_helper_init(struct drm_device *dev, i++; }

•   fb_helper->atomic = !!drm_core_check_feature(dev, DRIVER_ATOMIC);
  

•   return 0;
  

out_free: drm_fb_helper_crtc_free(fb_helper); @@ -1344,7 +1342,7 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var, return -EBUSY; }

•   if (fb_helper->atomic) {
  

•   if (dev->mode_config.funcs->atomic_commit) {
            ret = pan_display_atomic(var, info);
            goto unlock;
    }
  

diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c index ef8e67690f3d..4c725ad6fb54 100644 --- a/drivers/gpu/drm/i915/intel_fbdev.c +++ b/drivers/gpu/drm/i915/intel_fbdev.c @@ -724,8 +724,6 @@ int intel_fbdev_init(struct drm_device *dev) return ret; }

•   ifbdev->helper.atomic = true;
  

•   dev_priv->fbdev = ifbdev;
    INIT_WORK(&dev_priv->fbdev_suspend_work, intel_fbdev_suspend_worker);
  
  

diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h index 5b4aa35026a3..db8d4780eaa2 100644 --- a/include/drm/drm_fb_helper.h +++ b/include/drm/drm_fb_helper.h @@ -212,17 +212,6 @@ struct drm_fb_helper { * needs to be reprobe when fbdev is in control again. */ bool delayed_hotplug;

•   /**
  

•    * @atomic:
  

•    *
  

•    * Use atomic updates for restore_fbdev_mode(), etc.  This defaults to
  

•    * true if driver has DRIVER_ATOMIC feature flag, but drivers can
  

•    * override it to true after drm_fb_helper_init() if they support atomic
  

•    * modeset but do not yet advertise DRIVER_ATOMIC (note that fb-helper
  

•    * does not require ASYNC commits).
  

•    */
  

•   bool atomic;
  

};

#ifdef CONFIG_DRM_FBDEV_EMULATION

2.8.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

Hi Daniel,

Thank you for the patch.

On Wednesday 08 Jun 2016 14:19:18 Daniel Vetter wrote:

Rockchip just blew up here on testing, because I removed some "is this crtc already disabled/enabled" state tracking from callbacks (not needed with atomic). Turns out that was needed to work around rockchip still calling legacy helper code.

Since me explaining on irc/mailing-list plus kerneldoc isn't enough, be more verbose and add dmesg output. Not that anyone actually reads that, either.

How about also removing it from atomic drivers then ? At least omapdrm calls this function.

Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_crtc_helper.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index a6e42433ef0e..b47ec24939a0 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -232,6 +232,9 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev) */ void drm_helper_disable_unused_functions(struct drm_device *dev) {

• if (drm_core_check_feature(dev, DRIVER_ATOMIC))

• DRM_ERROR("Called for atomic driver, this is not what you want.\n");
  

• drm_modeset_lock_all(dev); __drm_helper_disable_unused_functions(dev); drm_modeset_unlock_all(dev);

Hi Daniel,

On Thursday 09 Jun 2016 10:26:44 Daniel Vetter wrote:

On Thu, Jun 09, 2016 at 01:36:30AM +0300, Laurent Pinchart wrote:

...

On Wednesday 08 Jun 2016 14:19:18 Daniel Vetter wrote:

...

Rockchip just blew up here on testing, because I removed some "is this crtc already disabled/enabled" state tracking from callbacks (not needed with atomic). Turns out that was needed to work around rockchip still calling legacy helper code.

Since me explaining on irc/mailing-list plus kerneldoc isn't enough, be more verbose and add dmesg output. Not that anyone actually reads that, either.

How about also removing it from atomic drivers then ? At least omapdrm calls this function.

I tried, and thought I caught them all. See preceeding sti patch. Not sure why I missed it, will remedy.

With the call removed from drivers,

Acked-by: Laurent Pinchart laurent.pinchart@ideasonboard.com

...

...

Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Signed-off-by: Daniel Vetter daniel.vetter@ffwll.ch

---

drivers/gpu/drm/drm_crtc_helper.c | 3 +++ 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index a6e42433ef0e..b47ec24939a0 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -232,6 +232,9 @@ static void __drm_helper_disable_unused_functions(struct drm_device *dev) */

void drm_helper_disable_unused_functions(struct drm_device *dev) {

• if (drm_core_check_feature(dev, DRIVER_ATOMIC))

• DRM_ERROR("Called for atomic driver, this is not what you
  

want.\n");

• drm_modeset_lock_all(dev); __drm_helper_disable_unused_functions(dev); drm_modeset_unlock_all(dev);

It's a legacy helper function which won't do good with atomic helpers.

Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 --- 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 31dfa0893416..adb10fbe918d 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -279,9 +279,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) if (ret) goto fini;

- /* disable all the possible outputs/crtcs before entering KMS mode */ - drm_helper_disable_unused_functions(dev); - ret = drm_fb_helper_initial_config(helper, 32); if (ret) goto fini;

On 10/06/16 01:14, Daniel Vetter wrote:

It's a legacy helper function which won't do good with atomic helpers.

Cc: Tomi Valkeinen tomi.valkeinen@ti.com Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/omapdrm/omap_fbdev.c | 3 --- 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_fbdev.c b/drivers/gpu/drm/omapdrm/omap_fbdev.c index 31dfa0893416..adb10fbe918d 100644 --- a/drivers/gpu/drm/omapdrm/omap_fbdev.c +++ b/drivers/gpu/drm/omapdrm/omap_fbdev.c @@ -279,9 +279,6 @@ struct drm_fb_helper *omap_fbdev_init(struct drm_device *dev) if (ret) goto fini;

• /* disable all the possible outputs/crtcs before entering KMS mode */
• drm_helper_disable_unused_functions(dev);
• ret = drm_fb_helper_initial_config(helper, 32); if (ret) goto fini;

Acked-by: Tomi Valkeinen tomi.valkeinen@ti.com