[PATCH 0/5] drm/amdgpu: Page flip improvement and related cleanups

[PATCH 0/5] drm/amdgpu: Page flip improvement and related cleanups

[Michel Dänzer]

The main purpose of this series is patch 5, which can avoid delaying
flips for a frame in some cases. It depends on patch 4 to prevent flips
from taking effect too early, which in turn depends on the core DRM
patch 2.

Patches 1 & 3 are cleanups I noticed while working on the other patches.

Mario, it would be great if you could run these through your rigorous
vblank testing.

[PATCH 1/5] drm: Only handle _DRM_VBLANK_NEXTONMISS once
[PATCH 2/5] drm: Keep track of last vblank sequence waited for
[PATCH 3/5] drm/amdgpu: Unpin BO if we can't get fences in
[PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before
[PATCH 5/5] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/5] drm: Only handle _DRM_VBLANK_NEXTONMISS once

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

Consolidate the _DRM_VBLANK_NEXTONMISS handling between drm_wait_vblank
and drm_queue_vblank_event.

This is a cleanup spotted while working on the next change.

(The way it was previously handled could also theoretically result in
drm_queue_vblank_event unnecessarily bumping vblwait->request.sequence,
if the vblank counter happened to increment between the
drm_vblank_count(_and_time) calls in each function, but that's unlikely)

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_irq.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 3c1a6f1..39ea4fc 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1685,12 +1685,6 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 
 	seq = drm_vblank_count_and_time(dev, pipe, &now);
 
-	if ((vblwait->request.type & _DRM_VBLANK_NEXTONMISS) &&
-	    (seq - vblwait->request.sequence) <= (1 << 23)) {
-		vblwait->request.sequence = seq + 1;
-		vblwait->reply.sequence = vblwait->request.sequence;
-	}
-
 	DRM_DEBUG("event on vblank count %d, current %d, crtc %u\n",
 		  vblwait->request.sequence, seq, pipe);
 
@@ -1787,6 +1781,11 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 		goto done;
 	}
 
+	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
+	    (seq - vblwait->request.sequence) <= (1 << 23)) {
+		vblwait->request.sequence = seq + 1;
+	}
+
 	if (flags & _DRM_VBLANK_EVENT) {
 		/* must hold on to the vblank ref until the event fires
 		 * drm_vblank_put will be called asynchronously
@@ -1794,11 +1793,6 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 		return drm_queue_vblank_event(dev, pipe, vblwait, file_priv);
 	}
 
-	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
-	    (seq - vblwait->request.sequence) <= (1<<23)) {
-		vblwait->request.sequence = seq + 1;
-	}
-
 	DRM_DEBUG("waiting on vblank count %d, crtc %u\n",
 		  vblwait->request.sequence, pipe);
 	vblank->last_wait = vblwait->request.sequence;
-- 
2.8.1

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

[PATCH 2/5] drm: Keep track of last vblank sequence waited for per-file-descriptor

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

The per-device tracking hasn't been used since commit dafffda0
("drm/info: Remove unused code").

The per-file-descriptor tracking will be used by a following amdgpu
driver change.

Note that last_vblank_wait is only updated when the corresponding wait
completes, and is set to the vblank seqno waited for, not the current
one when the wait completes.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_fops.c |  8 ++++++++
 drivers/gpu/drm/drm_irq.c  | 18 +++++++++++++++++-
 include/drm/drmP.h         |  4 +++-
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 7af7f8b..1f52f02 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -256,6 +256,13 @@ static int drm_open_helper(struct file *filp, struct drm_minor *minor)
 	priv->pid = get_pid(task_pid(current));
 	priv->minor = minor;
 
+	if (drm_is_primary_client(priv)) {
+		priv->num_crtcs = dev->num_crtcs;
+		priv->last_vblank_wait = kcalloc(priv->num_crtcs,
+						 sizeof(*priv->last_vblank_wait),
+						 GFP_KERNEL);
+	}
+
 	/* for compatibility root is always authenticated */
 	priv->authenticated = capable(CAP_SYS_ADMIN);
 	priv->lock_count = 0;
@@ -530,6 +537,7 @@ int drm_release(struct inode *inode, struct file *filp)
 	WARN_ON(!list_empty(&file_priv->event_list));
 
 	put_pid(file_priv->pid);
+	kfree(file_priv->last_vblank_wait);
 	kfree(file_priv);
 
 	/* ========================================================
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 39ea4fc..62845f4 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -77,6 +77,15 @@ MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] (0
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
 MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");
 
+/* Keep track of the latest seqno waited for by this file descriptor */
+static void update_last_vblank_wait(struct drm_file *file_priv, unsigned pipe,
+				    u32 seq)
+{
+	if (pipe < file_priv->num_crtcs &&
+	    (int)(seq - file_priv->last_vblank_wait[pipe]) > 0)
+		file_priv->last_vblank_wait[pipe] = seq;
+}
+
 static void store_vblank(struct drm_device *dev, unsigned int pipe,
 			 u32 vblank_count_inc,
 			 struct timeval *t_vblank, u32 last)
@@ -1694,6 +1703,9 @@ static int drm_queue_vblank_event(struct drm_device *dev, unsigned int pipe,
 	e->event.sequence = vblwait->request.sequence;
 	if ((seq - vblwait->request.sequence) <= (1 << 23)) {
 		drm_vblank_put(dev, pipe);
+
+		update_last_vblank_wait(file_priv, pipe,
+					vblwait->request.sequence);
 		send_vblank_event(dev, e, seq, &now);
 		vblwait->reply.sequence = seq;
 	} else {
@@ -1795,7 +1807,6 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 
 	DRM_DEBUG("waiting on vblank count %d, crtc %u\n",
 		  vblwait->request.sequence, pipe);
-	vblank->last_wait = vblwait->request.sequence;
 	DRM_WAIT_ON(ret, vblank->queue, 3 * HZ,
 		    (((drm_vblank_count(dev, pipe) -
 		       vblwait->request.sequence) <= (1 << 23)) ||
@@ -1805,6 +1816,9 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	if (ret != -EINTR) {
 		struct timeval now;
 
+		update_last_vblank_wait(file_priv, pipe,
+					vblwait->request.sequence);
+
 		vblwait->reply.sequence = drm_vblank_count_and_time(dev, pipe, &now);
 		vblwait->reply.tval_sec = now.tv_sec;
 		vblwait->reply.tval_usec = now.tv_usec;
@@ -1841,6 +1855,8 @@ static void drm_handle_vblank_events(struct drm_device *dev, unsigned int pipe)
 
 		list_del(&e->base.link);
 		drm_vblank_put(dev, pipe);
+		update_last_vblank_wait(e->base.file_priv, pipe,
+					e->event.sequence);
 		send_vblank_event(dev, e, seq, &now);
 	}
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 360b2a7..d7c95b6 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -354,6 +354,9 @@ struct drm_file {
 	struct mutex event_read_lock;
 
 	struct drm_prime_file_private prime;
+
+	unsigned num_crtcs;
+	u32 *last_vblank_wait;		/* Last vblank seqno waited per CRTC */
 };
 
 /**
@@ -733,7 +736,6 @@ struct drm_vblank_crtc {
 	atomic_t refcount;		/* number of users of vblank interruptsper crtc */
 	u32 last;			/* protected by dev->vbl_lock, used */
 					/* for wraparound handling */
-	u32 last_wait;			/* Last vblank seqno waited per CRTC */
 	unsigned int inmodeset;		/* Display driver is setting mode */
 	unsigned int pipe;		/* crtc index */
 	int framedur_ns;		/* frame/field duration in ns */
-- 
2.8.1

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

[PATCH 3/5] drm/amdgpu: Unpin BO if we can't get fences in amdgpu_crtc_page_flip

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

If reservation_object_get_fences_rcu failed, we'd previously go directly
to the cleanup label, so we'd leave the BO pinned.

While we're at it, remove two amdgpu_bo_unreserve calls in favour of two
new labels.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index 7268030..de95ea7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -220,19 +220,17 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 
 	r = amdgpu_bo_pin_restricted(new_rbo, AMDGPU_GEM_DOMAIN_VRAM, 0, 0, &base);
 	if (unlikely(r != 0)) {
-		amdgpu_bo_unreserve(new_rbo);
 		r = -EINVAL;
 		DRM_ERROR("failed to pin new rbo buffer before flip\n");
-		goto cleanup;
+		goto unreserve;
 	}
 
 	r = reservation_object_get_fences_rcu(new_rbo->tbo.resv, &work->excl,
 					      &work->shared_count,
 					      &work->shared);
 	if (unlikely(r != 0)) {
-		amdgpu_bo_unreserve(new_rbo);
 		DRM_ERROR("failed to get fences for buffer\n");
-		goto cleanup;
+		goto unpin;
 	}
 
 	amdgpu_bo_get_tiling_flags(new_rbo, &tiling_flags);
@@ -275,9 +273,11 @@ pflip_cleanup:
 		DRM_ERROR("failed to reserve new rbo in error path\n");
 		goto cleanup;
 	}
+unpin:
 	if (unlikely(amdgpu_bo_unpin(new_rbo) != 0)) {
 		DRM_ERROR("failed to unpin new rbo in error path\n");
 	}
+unreserve:
 	amdgpu_bo_unreserve(new_rbo);
 
 cleanup:
-- 
2.8.1

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

[PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

If userspace wants a page flip to take effect during vblank sequence n,
it has to wait for vblank seqno n-1 before calling the
DRM_IOCTL_MODE_PAGE_FLIP ioctl.

This change makes sure that we do not program the flip to the hardware
before the end of vblank seqno n-1 in this case, to prevent the flip
from taking effect too early.

On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
during vblank, but userspace didn't wait for the current vblank seqno
before, this change would still allow the flip to be programmed during
the current vblank seqno.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  3 ++-
 drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 34 +++++++++++++++++++++++++----
 2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 992f00b..7b53967 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -712,10 +712,11 @@ void amdgpu_doorbell_get_kfd_info(struct amdgpu_device *adev,
  */
 
 struct amdgpu_flip_work {
-	struct work_struct		flip_work;
+	struct delayed_work		flip_work;
 	struct work_struct		unpin_work;
 	struct amdgpu_device		*adev;
 	int				crtc_id;
+	u32				avoid_vblank;
 	uint64_t			base;
 	struct drm_pending_vblank_event *event;
 	struct amdgpu_bo		*old_rbo;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
index de95ea7..a9f7851 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
@@ -41,7 +41,7 @@ static void amdgpu_flip_callback(struct fence *f, struct fence_cb *cb)
 		container_of(cb, struct amdgpu_flip_work, cb);
 
 	fence_put(f);
-	schedule_work(&work->flip_work);
+	schedule_work(&work->flip_work.work);
 }
 
 static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
@@ -63,8 +63,10 @@ static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
 
 static void amdgpu_flip_work_func(struct work_struct *__work)
 {
+	struct delayed_work *delayed_work =
+		container_of(__work, struct delayed_work, work);
 	struct amdgpu_flip_work *work =
-		container_of(__work, struct amdgpu_flip_work, flip_work);
+		container_of(delayed_work, struct amdgpu_flip_work, flip_work);
 	struct amdgpu_device *adev = work->adev;
 	struct amdgpu_crtc *amdgpuCrtc = adev->mode_info.crtcs[work->crtc_id];
 
@@ -81,6 +83,19 @@ static void amdgpu_flip_work_func(struct work_struct *__work)
 		if (amdgpu_flip_handle_fence(work, &work->shared[i]))
 			return;
 
+	/* Wait until we're out of the last waited-for vertical blank
+	 * period
+	 */
+	if (amdgpuCrtc->enabled &&
+	    drm_crtc_vblank_count(crtc) == work->avoid_vblank &&
+	    (amdgpu_get_crtc_scanoutpos(adev->ddev, work->crtc_id, 0,
+					&vpos, &hpos, NULL, NULL,
+					&crtc->hwmode)
+	     & DRM_SCANOUTPOS_IN_VBLANK)) {
+		schedule_delayed_work(&work->flip_work, usecs_to_jiffies(1000));
+		return;
+	}
+
 	/* We borrow the event spin lock for protecting flip_status */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 
@@ -175,6 +190,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 			  uint32_t page_flip_flags)
 {
 	struct drm_device *dev = crtc->dev;
+	struct drm_file *file_priv = event->base.file_priv;
 	struct amdgpu_device *adev = dev->dev_private;
 	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
 	struct amdgpu_framebuffer *old_amdgpu_fb;
@@ -191,7 +207,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
-	INIT_WORK(&work->flip_work, amdgpu_flip_work_func);
+	INIT_DELAYED_WORK(&work->flip_work, amdgpu_flip_work_func);
 	INIT_WORK(&work->unpin_work, amdgpu_unpin_work_func);
 
 	work->event = event;
@@ -244,6 +260,16 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 		goto pflip_cleanup;
 	}
 
+	/* If this file descriptor has waited for the current vblank period,
+	 * do not program the flip during this vblank period
+	 */
+	if (amdgpu_crtc->crtc_id < file_priv->num_crtcs)
+		work->avoid_vblank =
+			file_priv->last_vblank_wait[amdgpu_crtc->crtc_id];
+
+	if (!work->avoid_vblank)
+		work->avoid_vblank = drm_crtc_vblank_count(crtc) - 1;
+
 	/* we borrow the event spin lock for protecting flip_wrok */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 	if (amdgpu_crtc->pflip_status != AMDGPU_FLIP_NONE) {
@@ -262,7 +288,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 	/* update crtc fb */
 	crtc->primary->fb = fb;
 	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-	amdgpu_flip_work_func(&work->flip_work);
+	amdgpu_flip_work_func(&work->flip_work.work);
 	return 0;
 
 vblank_cleanup:
-- 
2.8.1

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

[PATCH 5/5] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again

[Michel Dänzer]

From: Michel Dänzer <michel.daenzer@amd.com>

With the previous change, it's safe to let page flips take effect
anytime during a vertical blank period.

This can avoid delaying a flip by a frame in some cases where we only
get to amdgpu_flip_work_func -> adev->mode_info.funcs->page_flip during
the target vertical blank period.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 8 ++++----
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 4 ++--
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c  | 8 ++++----
 3 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index be1cf38..99a4638 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -646,8 +646,8 @@ static void dce_v10_0_resume_mc_access(struct amdgpu_device *adev,
 
 		if (save->crtc_enabled[i]) {
 			tmp = RREG32(mmMASTER_UPDATE_MODE + crtc_offsets[i]);
-			if (REG_GET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE) != 3) {
-				tmp = REG_SET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE, 3);
+			if (REG_GET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE) != 0) {
+				tmp = REG_SET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE, 0);
 				WREG32(mmMASTER_UPDATE_MODE + crtc_offsets[i], tmp);
 			}
 			tmp = RREG32(mmGRPH_UPDATE + crtc_offsets[i]);
@@ -2275,8 +2275,8 @@ static int dce_v10_0_crtc_do_set_base(struct drm_crtc *crtc,
 	WREG32(mmVIEWPORT_SIZE + amdgpu_crtc->crtc_offset,
 	       (viewport_w << 16) | viewport_h);
 
-	/* set pageflip to happen only at start of vblank interval (front porch) */
-	WREG32(mmMASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 3);
+	/* set pageflip to happen anywhere in vblank interval */
+	WREG32(mmMASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 0);
 
 	if (!atomic && fb && fb != crtc->primary->fb) {
 		amdgpu_fb = to_amdgpu_framebuffer(fb);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index cd2ec81..44f0d22 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2251,8 +2251,8 @@ static int dce_v11_0_crtc_do_set_base(struct drm_crtc *crtc,
 	WREG32(mmVIEWPORT_SIZE + amdgpu_crtc->crtc_offset,
 	       (viewport_w << 16) | viewport_h);
 
-	/* set pageflip to happen only at start of vblank interval (front porch) */
-	WREG32(mmCRTC_MASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 3);
+	/* set pageflip to happen anywhere in vblank interval */
+	WREG32(mmCRTC_MASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 0);
 
 	if (!atomic && fb && fb != crtc->primary->fb) {
 		amdgpu_fb = to_amdgpu_framebuffer(fb);
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index c087f93..86864dd 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -591,8 +591,8 @@ static void dce_v8_0_resume_mc_access(struct amdgpu_device *adev,
 
 		if (save->crtc_enabled[i]) {
 			tmp = RREG32(mmMASTER_UPDATE_MODE + crtc_offsets[i]);
-			if (REG_GET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE) != 3) {
-				tmp = REG_SET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE, 3);
+			if (REG_GET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE) != 0) {
+				tmp = REG_SET_FIELD(tmp, MASTER_UPDATE_MODE, MASTER_UPDATE_MODE, 0);
 				WREG32(mmMASTER_UPDATE_MODE + crtc_offsets[i], tmp);
 			}
 			tmp = RREG32(mmGRPH_UPDATE + crtc_offsets[i]);
@@ -2190,8 +2190,8 @@ static int dce_v8_0_crtc_do_set_base(struct drm_crtc *crtc,
 	WREG32(mmVIEWPORT_SIZE + amdgpu_crtc->crtc_offset,
 	       (viewport_w << 16) | viewport_h);
 
-	/* set pageflip to happen only at start of vblank interval (front porch) */
-	WREG32(mmMASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 3);
+	/* set pageflip to happen anywhere in vblank interval */
+	WREG32(mmMASTER_UPDATE_MODE + amdgpu_crtc->crtc_offset, 0);
 
 	if (!atomic && fb && fb != crtc->primary->fb) {
 		amdgpu_fb = to_amdgpu_framebuffer(fb);
-- 
2.8.1

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

Re: [PATCH 0/5] drm/amdgpu: Page flip improvement and related cleanups

[Christian König]

Am 10.06.2016 um 10:57 schrieb Michel Dänzer:
> The main purpose of this series is patch 5, which can avoid delaying
> flips for a frame in some cases. It depends on patch 4 to prevent flips
> from taking effect too early, which in turn depends on the core DRM
> patch 2.
>
> Patches 1 & 3 are cleanups I noticed while working on the other patches.
>
> Mario, it would be great if you could run these through your rigorous
> vblank testing.
>
> [PATCH 1/5] drm: Only handle _DRM_VBLANK_NEXTONMISS once
> [PATCH 2/5] drm: Keep track of last vblank sequence waited for

Acked-by: Christian König <christian.koenig@amd.com> for those two.

> [PATCH 3/5] drm/amdgpu: Unpin BO if we can't get fences in
> [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before
> [PATCH 5/5] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again

Reviewed-by: Christian König <christian.koenig@amd.com> for those three.

Cheers,
Christian.

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

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

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> If userspace wants a page flip to take effect during vblank sequence n,
> it has to wait for vblank seqno n-1 before calling the
> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> 
> This change makes sure that we do not program the flip to the hardware
> before the end of vblank seqno n-1 in this case, to prevent the flip
> from taking effect too early.
> 
> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> during vblank, but userspace didn't wait for the current vblank seqno
> before, this change would still allow the flip to be programmed during
> the current vblank seqno.

This just sounds like you're sending vblank events out a bit too early.
And watching vblank waits that userspace does works, but it's fragile,
add-hoc and I don't really jump in joy about adding that to the vblank
core. Is there no way you can adjust sending out the vblank events
similarly, to make sure userspace can never sneak in a pageflip too early?

That way it would be all nicely contained to amdgpu, and not leaking into
the core.
-Daniel

> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu.h         |  3 ++-
>  drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 34 +++++++++++++++++++++++++----
>  2 files changed, 32 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index 992f00b..7b53967 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -712,10 +712,11 @@ void amdgpu_doorbell_get_kfd_info(struct amdgpu_device *adev,
>   */
>  
>  struct amdgpu_flip_work {
> -	struct work_struct		flip_work;
> +	struct delayed_work		flip_work;
>  	struct work_struct		unpin_work;
>  	struct amdgpu_device		*adev;
>  	int				crtc_id;
> +	u32				avoid_vblank;
>  	uint64_t			base;
>  	struct drm_pending_vblank_event *event;
>  	struct amdgpu_bo		*old_rbo;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> index de95ea7..a9f7851 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c
> @@ -41,7 +41,7 @@ static void amdgpu_flip_callback(struct fence *f, struct fence_cb *cb)
>  		container_of(cb, struct amdgpu_flip_work, cb);
>  
>  	fence_put(f);
> -	schedule_work(&work->flip_work);
> +	schedule_work(&work->flip_work.work);
>  }
>  
>  static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
> @@ -63,8 +63,10 @@ static bool amdgpu_flip_handle_fence(struct amdgpu_flip_work *work,
>  
>  static void amdgpu_flip_work_func(struct work_struct *__work)
>  {
> +	struct delayed_work *delayed_work =
> +		container_of(__work, struct delayed_work, work);
>  	struct amdgpu_flip_work *work =
> -		container_of(__work, struct amdgpu_flip_work, flip_work);
> +		container_of(delayed_work, struct amdgpu_flip_work, flip_work);
>  	struct amdgpu_device *adev = work->adev;
>  	struct amdgpu_crtc *amdgpuCrtc = adev->mode_info.crtcs[work->crtc_id];
>  
> @@ -81,6 +83,19 @@ static void amdgpu_flip_work_func(struct work_struct *__work)
>  		if (amdgpu_flip_handle_fence(work, &work->shared[i]))
>  			return;
>  
> +	/* Wait until we're out of the last waited-for vertical blank
> +	 * period
> +	 */
> +	if (amdgpuCrtc->enabled &&
> +	    drm_crtc_vblank_count(crtc) == work->avoid_vblank &&
> +	    (amdgpu_get_crtc_scanoutpos(adev->ddev, work->crtc_id, 0,
> +					&vpos, &hpos, NULL, NULL,
> +					&crtc->hwmode)
> +	     & DRM_SCANOUTPOS_IN_VBLANK)) {
> +		schedule_delayed_work(&work->flip_work, usecs_to_jiffies(1000));
> +		return;
> +	}
> +
>  	/* We borrow the event spin lock for protecting flip_status */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>  
> @@ -175,6 +190,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  			  uint32_t page_flip_flags)
>  {
>  	struct drm_device *dev = crtc->dev;
> +	struct drm_file *file_priv = event->base.file_priv;
>  	struct amdgpu_device *adev = dev->dev_private;
>  	struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>  	struct amdgpu_framebuffer *old_amdgpu_fb;
> @@ -191,7 +207,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> -	INIT_WORK(&work->flip_work, amdgpu_flip_work_func);
> +	INIT_DELAYED_WORK(&work->flip_work, amdgpu_flip_work_func);
>  	INIT_WORK(&work->unpin_work, amdgpu_unpin_work_func);
>  
>  	work->event = event;
> @@ -244,6 +260,16 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  		goto pflip_cleanup;
>  	}
>  
> +	/* If this file descriptor has waited for the current vblank period,
> +	 * do not program the flip during this vblank period
> +	 */
> +	if (amdgpu_crtc->crtc_id < file_priv->num_crtcs)
> +		work->avoid_vblank =
> +			file_priv->last_vblank_wait[amdgpu_crtc->crtc_id];
> +
> +	if (!work->avoid_vblank)
> +		work->avoid_vblank = drm_crtc_vblank_count(crtc) - 1;
> +
>  	/* we borrow the event spin lock for protecting flip_wrok */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>  	if (amdgpu_crtc->pflip_status != AMDGPU_FLIP_NONE) {
> @@ -262,7 +288,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
>  	/* update crtc fb */
>  	crtc->primary->fb = fb;
>  	spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -	amdgpu_flip_work_func(&work->flip_work);
> +	amdgpu_flip_work_func(&work->flip_work.work);
>  	return 0;
>  
>  vblank_cleanup:
> -- 
> 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 10.06.2016 23:43, Daniel Vetter wrote:
> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> If userspace wants a page flip to take effect during vblank sequence n,
>> it has to wait for vblank seqno n-1 before calling the
>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
>>
>> This change makes sure that we do not program the flip to the hardware
>> before the end of vblank seqno n-1 in this case, to prevent the flip
>> from taking effect too early.
>>
>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
>> during vblank, but userspace didn't wait for the current vblank seqno
>> before, this change would still allow the flip to be programmed during
>> the current vblank seqno.
> 
> This just sounds like you're sending vblank events out a bit too early.
> And watching vblank waits that userspace does works, but it's fragile,
> add-hoc and I don't really jump in joy about adding that to the vblank
> core. Is there no way you can adjust sending out the vblank events
> similarly, to make sure userspace can never sneak in a pageflip too early?

What you call "too early" is actually "during the vertical blank period
waited for". IMHO only notifying userspace of a vertical blank period
when it's already over would defeat the purpose.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
> On 10.06.2016 23:43, Daniel Vetter wrote:
> > On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> >> From: Michel Dänzer <michel.daenzer@amd.com>
> >>
> >> If userspace wants a page flip to take effect during vblank sequence n,
> >> it has to wait for vblank seqno n-1 before calling the
> >> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> >>
> >> This change makes sure that we do not program the flip to the hardware
> >> before the end of vblank seqno n-1 in this case, to prevent the flip
> >> from taking effect too early.
> >>
> >> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> >> during vblank, but userspace didn't wait for the current vblank seqno
> >> before, this change would still allow the flip to be programmed during
> >> the current vblank seqno.
> > 
> > This just sounds like you're sending vblank events out a bit too early.
> > And watching vblank waits that userspace does works, but it's fragile,
> > add-hoc and I don't really jump in joy about adding that to the vblank
> > core. Is there no way you can adjust sending out the vblank events
> > similarly, to make sure userspace can never sneak in a pageflip too early?
> 
> What you call "too early" is actually "during the vertical blank period
> waited for". IMHO only notifying userspace of a vertical blank period
> when it's already over would defeat the purpose.

Afaiui the rules are:
- The timestamp for vblank event needs to agree with whatever oml_sync
  requries.
- The event delivery itself needs to be consistent with what page_flip
  takes, i.e. if userspace sees an event and immediately issues a
  page_flip then it should not be able to hit the same vblank with that
  pageflip.
- The event needs to be after the old buffers are not longer used and can
  be reused for rendering.
- There's no requirement at all that the event gets delivered at a
  specific point in the vblank, hardware is too different for that to work
  - that kind of precision is why we have a separate timestamp.

I assume you're goal is to not delay page_flips unecessarily, without
breaking requirement 2 here. Imo a simpler fix would be to delay the
vblank handling to end of vblank. Fixes everything without hacks, no
single-use driver oddity in the core, and you still can page_flip as late
as you want without forcing a delay to the next vblank if your already
past the start of vblank. And since you have high-precision timestamp
support the vblank core will fudge the timestamp as needed.

Ofc this assumes that amd hw has a vblank_end irq, but most desktop hw
seems to have that (didn't check tbh whether amd has it). So all that's
needed I think is to enable the vblank_end irq (in lockstep with the start
one) and move drm_handle_vblank() from its current place to that new irq.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 06/13/16 17:06, Daniel Vetter wrote:
> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
>> On 10.06.2016 23:43, Daniel Vetter wrote:
>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>
>>>> If userspace wants a page flip to take effect during vblank sequence n,
>>>> it has to wait for vblank seqno n-1 before calling the
>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
>>>>
>>>> This change makes sure that we do not program the flip to the hardware
>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
>>>> from taking effect too early.
>>>>
>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
>>>> during vblank, but userspace didn't wait for the current vblank seqno
>>>> before, this change would still allow the flip to be programmed during
>>>> the current vblank seqno.
>>>
>>> This just sounds like you're sending vblank events out a bit too early.
>>> And watching vblank waits that userspace does works, but it's fragile,
>>> add-hoc and I don't really jump in joy about adding that to the vblank
>>> core. Is there no way you can adjust sending out the vblank events
>>> similarly, to make sure userspace can never sneak in a pageflip too early?
>>
>> What you call "too early" is actually "during the vertical blank period
>> waited for". IMHO only notifying userspace of a vertical blank period
>> when it's already over would defeat the purpose.
> 
> Afaiui the rules are:
> - The timestamp for vblank event needs to agree with whatever oml_sync
>   requries.
> - The event delivery itself needs to be consistent with what page_flip
>   takes, i.e. if userspace sees an event and immediately issues a
>   page_flip then it should not be able to hit the same vblank with that
>   pageflip.
> - The event needs to be after the old buffers are not longer used and can
>   be reused for rendering.

That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
DRM_IOCTL_WAIT_VBLANK.

> - There's no requirement at all that the event gets delivered at a
>   specific point in the vblank, hardware is too different for that to work

As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
a vertical blank period. If that doesn't work as intended with some
hardware, that's tough luck but not really my problem. :)

>   - that kind of precision is why we have a separate timestamp.

I'm afraid this last item gives away that you're relatively new to this
code. ;) The timestamp was originally literally just the current
gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
ioctl didn't have any asynchronous notification functionality). It was
relatively recently that Mario changed the timestamp to correspond to
the end of the vertical blank period / start of scanout of the next
frame, presumably due to your first rule above.


> I assume you're goal is to not delay page_flips unecessarily, without
> breaking requirement 2 here. Imo a simpler fix would be to delay the
> vblank handling to end of vblank. Fixes everything without hacks, [...]

Except it breaks the original purpose of the wait for vblank
functionality, which is to wait for the beginning of a vertical blank
period. [0] You're focusing too much on page flips and suggesting to
throw out the vblank baby with the bathwater. I really don't see the big
issue which would justify that.


[0] As an analogy, how useful would e.g. calendar notifications be if
they arrived at the end of the events they're about? "Hey, that meeting
you were supposed to attend? It just finished!"


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
> On 06/13/16 17:06, Daniel Vetter wrote:
> > On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
> >> On 10.06.2016 23:43, Daniel Vetter wrote:
> >>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> >>>> From: Michel Dänzer <michel.daenzer@amd.com>
> >>>>
> >>>> If userspace wants a page flip to take effect during vblank sequence n,
> >>>> it has to wait for vblank seqno n-1 before calling the
> >>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> >>>>
> >>>> This change makes sure that we do not program the flip to the hardware
> >>>> before the end of vblank seqno n-1 in this case, to prevent the flip
> >>>> from taking effect too early.
> >>>>
> >>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> >>>> during vblank, but userspace didn't wait for the current vblank seqno
> >>>> before, this change would still allow the flip to be programmed during
> >>>> the current vblank seqno.
> >>>
> >>> This just sounds like you're sending vblank events out a bit too early.
> >>> And watching vblank waits that userspace does works, but it's fragile,
> >>> add-hoc and I don't really jump in joy about adding that to the vblank
> >>> core. Is there no way you can adjust sending out the vblank events
> >>> similarly, to make sure userspace can never sneak in a pageflip too early?
> >>
> >> What you call "too early" is actually "during the vertical blank period
> >> waited for". IMHO only notifying userspace of a vertical blank period
> >> when it's already over would defeat the purpose.
> > 
> > Afaiui the rules are:
> > - The timestamp for vblank event needs to agree with whatever oml_sync
> >   requries.
> > - The event delivery itself needs to be consistent with what page_flip
> >   takes, i.e. if userspace sees an event and immediately issues a
> >   page_flip then it should not be able to hit the same vblank with that
> >   pageflip.
> > - The event needs to be after the old buffers are not longer used and can
> >   be reused for rendering.
> 
> That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
> DRM_IOCTL_WAIT_VBLANK.

Yup, mixed that up.

> > - There's no requirement at all that the event gets delivered at a
> >   specific point in the vblank, hardware is too different for that to work
> 
> As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
> a vertical blank period. If that doesn't work as intended with some
> hardware, that's tough luck but not really my problem. :)
> 
> >   - that kind of precision is why we have a separate timestamp.
> 
> I'm afraid this last item gives away that you're relatively new to this
> code. ;) The timestamp was originally literally just the current
> gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
> ioctl didn't have any asynchronous notification functionality). It was
> relatively recently that Mario changed the timestamp to correspond to
> the end of the vertical blank period / start of scanout of the next
> frame, presumably due to your first rule above.

Most hw just seems to give you a vblank interrupt somewhere in the vblank
are, or sometimes even slightly before that. Also there's scheduling
jitter.

> > I assume you're goal is to not delay page_flips unecessarily, without
> > breaking requirement 2 here. Imo a simpler fix would be to delay the
> > vblank handling to end of vblank. Fixes everything without hacks, [...]
> 
> Except it breaks the original purpose of the wait for vblank
> functionality, which is to wait for the beginning of a vertical blank
> period. [0] You're focusing too much on page flips and suggesting to
> throw out the vblank baby with the bathwater. I really don't see the big
> issue which would justify that.
> 
> 
> [0] As an analogy, how useful would e.g. calendar notifications be if
> they arrived at the end of the events they're about? "Hey, that meeting
> you were supposed to attend? It just finished!"

Ok, what exactly is the use-case for waiting for vblanks _without_
scheduling a flip afterwards? At least in drm the rule is that ABI is what
userspace observes and actually cares about. The above few rules are what
generic userspace (afaik at least) cares about, and what therefore drivers
should implement. Note that at least to my knowledge absolutely nothing
cares when exactly in the vblank the interrupt fires, and the event gets
delivered. We even moved that around in the i915 driver iirc, exactly
because you could squeeze in a page-flip like you can here on radeon.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 06/13/16 23:06, Daniel Vetter wrote:
> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
>> On 06/13/16 17:06, Daniel Vetter wrote:
>>> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
>>>> On 10.06.2016 23:43, Daniel Vetter wrote:
>>>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>
>>>>>> If userspace wants a page flip to take effect during vblank sequence n,
>>>>>> it has to wait for vblank seqno n-1 before calling the
>>>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
>>>>>>
>>>>>> This change makes sure that we do not program the flip to the hardware
>>>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
>>>>>> from taking effect too early.
>>>>>>
>>>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
>>>>>> during vblank, but userspace didn't wait for the current vblank seqno
>>>>>> before, this change would still allow the flip to be programmed during
>>>>>> the current vblank seqno.
>>>>>
>>>>> This just sounds like you're sending vblank events out a bit too early.
>>>>> And watching vblank waits that userspace does works, but it's fragile,
>>>>> add-hoc and I don't really jump in joy about adding that to the vblank
>>>>> core. Is there no way you can adjust sending out the vblank events
>>>>> similarly, to make sure userspace can never sneak in a pageflip too early?
>>>>
>>>> What you call "too early" is actually "during the vertical blank period
>>>> waited for". IMHO only notifying userspace of a vertical blank period
>>>> when it's already over would defeat the purpose.
>>>
>>> Afaiui the rules are:
>>> - The timestamp for vblank event needs to agree with whatever oml_sync
>>>   requries.
>>> - The event delivery itself needs to be consistent with what page_flip
>>>   takes, i.e. if userspace sees an event and immediately issues a
>>>   page_flip then it should not be able to hit the same vblank with that
>>>   pageflip.
>>> - The event needs to be after the old buffers are not longer used and can
>>>   be reused for rendering.
>>
>> That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
>> DRM_IOCTL_WAIT_VBLANK.
> 
> Yup, mixed that up.
> 
>>> - There's no requirement at all that the event gets delivered at a
>>>   specific point in the vblank, hardware is too different for that to work
>>
>> As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
>> a vertical blank period. If that doesn't work as intended with some
>> hardware, that's tough luck but not really my problem. :)
>>
>>>   - that kind of precision is why we have a separate timestamp.
>>
>> I'm afraid this last item gives away that you're relatively new to this
>> code. ;) The timestamp was originally literally just the current
>> gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
>> ioctl didn't have any asynchronous notification functionality). It was
>> relatively recently that Mario changed the timestamp to correspond to
>> the end of the vertical blank period / start of scanout of the next
>> frame, presumably due to your first rule above.
> 
> Most hw just seems to give you a vblank interrupt somewhere in the vblank
> are, or sometimes even slightly before that.

Our hardware tends to trigger the vblank interrupt early, but it's still
useful in that drawing operations submitted after it cannot affect the
previously scanned out frame.

> Also there's scheduling jitter.

Sure, but there's a big difference between "no guarantee that we're
still in vblank" vs "guarantee that we're no longer in vblank".


>>> I assume you're goal is to not delay page_flips unecessarily, without
>>> breaking requirement 2 here. Imo a simpler fix would be to delay the
>>> vblank handling to end of vblank. Fixes everything without hacks, [...]
>>
>> Except it breaks the original purpose of the wait for vblank
>> functionality, which is to wait for the beginning of a vertical blank
>> period. [0] You're focusing too much on page flips and suggesting to
>> throw out the vblank baby with the bathwater. I really don't see the big
>> issue which would justify that.
>>
>>
>> [0] As an analogy, how useful would e.g. calendar notifications be if
>> they arrived at the end of the events they're about? "Hey, that meeting
>> you were supposed to attend? It just finished!"
> 
> Ok, what exactly is the use-case for waiting for vblanks _without_
> scheduling a flip afterwards? At least in drm the rule is that ABI is what
> userspace observes and actually cares about.

E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
waits for the target vertical blank period before emitting the drawing
commands for a buffer swap operation. If the vblank notification only
arrives when the vertical blank period is already over, this is very
likely to result in tearing.

Some X compositors and AFAIK even applications such as media players can
use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
used directly like that, but nonetheless it is.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
> On 06/13/16 23:06, Daniel Vetter wrote:
> > On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
> >> On 06/13/16 17:06, Daniel Vetter wrote:
> >>> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
> >>>> On 10.06.2016 23:43, Daniel Vetter wrote:
> >>>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> >>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
> >>>>>>
> >>>>>> If userspace wants a page flip to take effect during vblank sequence n,
> >>>>>> it has to wait for vblank seqno n-1 before calling the
> >>>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> >>>>>>
> >>>>>> This change makes sure that we do not program the flip to the hardware
> >>>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
> >>>>>> from taking effect too early.
> >>>>>>
> >>>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> >>>>>> during vblank, but userspace didn't wait for the current vblank seqno
> >>>>>> before, this change would still allow the flip to be programmed during
> >>>>>> the current vblank seqno.
> >>>>>
> >>>>> This just sounds like you're sending vblank events out a bit too early.
> >>>>> And watching vblank waits that userspace does works, but it's fragile,
> >>>>> add-hoc and I don't really jump in joy about adding that to the vblank
> >>>>> core. Is there no way you can adjust sending out the vblank events
> >>>>> similarly, to make sure userspace can never sneak in a pageflip too early?
> >>>>
> >>>> What you call "too early" is actually "during the vertical blank period
> >>>> waited for". IMHO only notifying userspace of a vertical blank period
> >>>> when it's already over would defeat the purpose.
> >>>
> >>> Afaiui the rules are:
> >>> - The timestamp for vblank event needs to agree with whatever oml_sync
> >>>   requries.
> >>> - The event delivery itself needs to be consistent with what page_flip
> >>>   takes, i.e. if userspace sees an event and immediately issues a
> >>>   page_flip then it should not be able to hit the same vblank with that
> >>>   pageflip.
> >>> - The event needs to be after the old buffers are not longer used and can
> >>>   be reused for rendering.
> >>
> >> That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
> >> DRM_IOCTL_WAIT_VBLANK.
> > 
> > Yup, mixed that up.
> > 
> >>> - There's no requirement at all that the event gets delivered at a
> >>>   specific point in the vblank, hardware is too different for that to work
> >>
> >> As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
> >> a vertical blank period. If that doesn't work as intended with some
> >> hardware, that's tough luck but not really my problem. :)
> >>
> >>>   - that kind of precision is why we have a separate timestamp.
> >>
> >> I'm afraid this last item gives away that you're relatively new to this
> >> code. ;) The timestamp was originally literally just the current
> >> gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
> >> ioctl didn't have any asynchronous notification functionality). It was
> >> relatively recently that Mario changed the timestamp to correspond to
> >> the end of the vertical blank period / start of scanout of the next
> >> frame, presumably due to your first rule above.
> > 
> > Most hw just seems to give you a vblank interrupt somewhere in the vblank
> > are, or sometimes even slightly before that.
> 
> Our hardware tends to trigger the vblank interrupt early, but it's still
> useful in that drawing operations submitted after it cannot affect the
> previously scanned out frame.
> 
> > Also there's scheduling jitter.
> 
> Sure, but there's a big difference between "no guarantee that we're
> still in vblank" vs "guarantee that we're no longer in vblank".
> 
> 
> >>> I assume you're goal is to not delay page_flips unecessarily, without
> >>> breaking requirement 2 here. Imo a simpler fix would be to delay the
> >>> vblank handling to end of vblank. Fixes everything without hacks, [...]
> >>
> >> Except it breaks the original purpose of the wait for vblank
> >> functionality, which is to wait for the beginning of a vertical blank
> >> period. [0] You're focusing too much on page flips and suggesting to
> >> throw out the vblank baby with the bathwater. I really don't see the big
> >> issue which would justify that.
> >>
> >>
> >> [0] As an analogy, how useful would e.g. calendar notifications be if
> >> they arrived at the end of the events they're about? "Hey, that meeting
> >> you were supposed to attend? It just finished!"
> > 
> > Ok, what exactly is the use-case for waiting for vblanks _without_
> > scheduling a flip afterwards? At least in drm the rule is that ABI is what
> > userspace observes and actually cares about.
> 
> E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
> waits for the target vertical blank period before emitting the drawing
> commands for a buffer swap operation. If the vblank notification only
> arrives when the vertical blank period is already over, this is very
> likely to result in tearing.
> 
> Some X compositors and AFAIK even applications such as media players can
> use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
> used directly like that, but nonetheless it is.

Is there really anything using it like that outside of -ati? I didn't know
that we pass vblank waits to X clients. Either way annoying, since it
means you need to keep things working like this for amd drivers forever.
Afaik others don't use it like that, at least not on intel. Weston has
some hacks to use vblank waits for plane flips, but that's all disabled
code because it just doesn't work - you need full atomic.

Anyway, I still don't like adding hacks to drm core like this for
single-use in just one driver. drm_irq.c is already really complex and
suffering badly from this, and we're pretty close to always accidentally
breaking something when touching it. How bad would it really be to just
always delay the page_flip past vblank? Userspace can still use async
flips for lower latency.

If that's not good enough I'd say we should add a
faster-than-vblank-but-still-synced page_flip flag. Then userspace could
tell you exactly whether you should always wait (no flags), or never wait
(with this new flag). It would also neatly fit into atomic plans, since we
want to implement that (so that everyone has a benchmarking/low-latency
mode, without hw support for async flips on all planes). And it wouldn't
need special tracking code in drm_irq.c, making me happy. Thoughts?
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 14.06.2016 14:53, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
>> On 06/13/16 23:06, Daniel Vetter wrote:
>>> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
>>>> On 06/13/16 17:06, Daniel Vetter wrote:
>>>>> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
>>>>>> On 10.06.2016 23:43, Daniel Vetter wrote:
>>>>>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
>>>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
>>>>>>>>
>>>>>>>> If userspace wants a page flip to take effect during vblank sequence n,
>>>>>>>> it has to wait for vblank seqno n-1 before calling the
>>>>>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
>>>>>>>>
>>>>>>>> This change makes sure that we do not program the flip to the hardware
>>>>>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
>>>>>>>> from taking effect too early.
>>>>>>>>
>>>>>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
>>>>>>>> during vblank, but userspace didn't wait for the current vblank seqno
>>>>>>>> before, this change would still allow the flip to be programmed during
>>>>>>>> the current vblank seqno.
>>>>>>>
>>>>>>> This just sounds like you're sending vblank events out a bit too early.
>>>>>>> And watching vblank waits that userspace does works, but it's fragile,
>>>>>>> add-hoc and I don't really jump in joy about adding that to the vblank
>>>>>>> core. Is there no way you can adjust sending out the vblank events
>>>>>>> similarly, to make sure userspace can never sneak in a pageflip too early?
>>>>>>
>>>>>> What you call "too early" is actually "during the vertical blank period
>>>>>> waited for". IMHO only notifying userspace of a vertical blank period
>>>>>> when it's already over would defeat the purpose.
>>>>>
>>>>> Afaiui the rules are:
>>>>> - The timestamp for vblank event needs to agree with whatever oml_sync
>>>>>   requries.
>>>>> - The event delivery itself needs to be consistent with what page_flip
>>>>>   takes, i.e. if userspace sees an event and immediately issues a
>>>>>   page_flip then it should not be able to hit the same vblank with that
>>>>>   pageflip.
>>>>> - The event needs to be after the old buffers are not longer used and can
>>>>>   be reused for rendering.
>>>>
>>>> That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
>>>> DRM_IOCTL_WAIT_VBLANK.
>>>
>>> Yup, mixed that up.
>>>
>>>>> - There's no requirement at all that the event gets delivered at a
>>>>>   specific point in the vblank, hardware is too different for that to work
>>>>
>>>> As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
>>>> a vertical blank period. If that doesn't work as intended with some
>>>> hardware, that's tough luck but not really my problem. :)
>>>>
>>>>>   - that kind of precision is why we have a separate timestamp.
>>>>
>>>> I'm afraid this last item gives away that you're relatively new to this
>>>> code. ;) The timestamp was originally literally just the current
>>>> gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
>>>> ioctl didn't have any asynchronous notification functionality). It was
>>>> relatively recently that Mario changed the timestamp to correspond to
>>>> the end of the vertical blank period / start of scanout of the next
>>>> frame, presumably due to your first rule above.
>>>
>>> Most hw just seems to give you a vblank interrupt somewhere in the vblank
>>> are, or sometimes even slightly before that.
>>
>> Our hardware tends to trigger the vblank interrupt early, but it's still
>> useful in that drawing operations submitted after it cannot affect the
>> previously scanned out frame.
>>
>>> Also there's scheduling jitter.
>>
>> Sure, but there's a big difference between "no guarantee that we're
>> still in vblank" vs "guarantee that we're no longer in vblank".
>>
>>
>>>>> I assume you're goal is to not delay page_flips unecessarily, without
>>>>> breaking requirement 2 here. Imo a simpler fix would be to delay the
>>>>> vblank handling to end of vblank. Fixes everything without hacks, [...]
>>>>
>>>> Except it breaks the original purpose of the wait for vblank
>>>> functionality, which is to wait for the beginning of a vertical blank
>>>> period. [0] You're focusing too much on page flips and suggesting to
>>>> throw out the vblank baby with the bathwater. I really don't see the big
>>>> issue which would justify that.
>>>>
>>>>
>>>> [0] As an analogy, how useful would e.g. calendar notifications be if
>>>> they arrived at the end of the events they're about? "Hey, that meeting
>>>> you were supposed to attend? It just finished!"
>>>
>>> Ok, what exactly is the use-case for waiting for vblanks _without_
>>> scheduling a flip afterwards? At least in drm the rule is that ABI is what
>>> userspace observes and actually cares about.
>>
>> E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
>> waits for the target vertical blank period before emitting the drawing
>> commands for a buffer swap operation. If the vblank notification only
>> arrives when the vertical blank period is already over, this is very
>> likely to result in tearing.
>>
>> Some X compositors and AFAIK even applications such as media players can
>> use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
>> used directly like that, but nonetheless it is.
> 
> Is there really anything using it like that outside of -ati?

Yes. With DRI3/Present, it's driver independent code in
xserver/present/. With DRI2, it's theoretically up to the DDX driver,
but all drivers seem to have basically the same logic; the modesetting
driver certainly does.


> I didn't know that we pass vblank waits to X clients. Either way annoying,
> since it means you need to keep things working like this for amd drivers
> forever.

Please don't try to single out our drivers, it seems like rather your
driver is the odd man out in this case.


> Afaik others don't use it like that, at least not on intel.

UXA has the same DRI2 logic, not sure about SNA.


> Anyway, I still don't like adding hacks to drm core like this for
> single-use in just one driver.

The driver changes will be ported to radeon of course, and why couldn't
it be used by other drivers for the same purpose?


> drm_irq.c is already really complex and suffering badly from this, and
> we're pretty close to always accidentally breaking something when touching it.

I just don't see how my change makes this any worse.


> How bad would it really be to just always delay the page_flip past vblank?

In the variable refresh rate case it could be pretty bad, basically
dropping to the minimum refresh rate.


> If that's not good enough I'd say we should add a
> faster-than-vblank-but-still-synced page_flip flag. Then userspace could
> tell you exactly whether you should always wait (no flags), or never wait
> (with this new flag).

That would be an inferior solution compared to my series, e.g.: If
userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
is reached, it cannot use the new flag, otherwise the flip might take
effect too early. However, if we are then already in the target vblank
period when the fences have signalled and we are ready to program the
flip, we have to wait for the end of vblank first, and the flip will be
delayed by one frame.

If we're going to change the userspace interface, it would be better to
re-purpose the reserved field of struct drm_mode_crtc_page_flip for
explicitly specifying the target vblank seqno (via a new cap and flag).
Then the kernel and userspace would no longer need to second-guess each
other.

But even if we take that route, this series would be desirable for
getting us most of the way there for existing userspace.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer

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

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Tue, Jun 14, 2016 at 04:25:28PM +0900, Michel Dänzer wrote:
> On 14.06.2016 14:53, Daniel Vetter wrote:
> > On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
> >> On 06/13/16 23:06, Daniel Vetter wrote:
> >>> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
> >>>> On 06/13/16 17:06, Daniel Vetter wrote:
> >>>>> On Mon, Jun 13, 2016 at 10:54:37AM +0900, Michel Dänzer wrote:
> >>>>>> On 10.06.2016 23:43, Daniel Vetter wrote:
> >>>>>>> On Fri, Jun 10, 2016 at 05:57:12PM +0900, Michel Dänzer wrote:
> >>>>>>>> From: Michel Dänzer <michel.daenzer@amd.com>
> >>>>>>>>
> >>>>>>>> If userspace wants a page flip to take effect during vblank sequence n,
> >>>>>>>> it has to wait for vblank seqno n-1 before calling the
> >>>>>>>> DRM_IOCTL_MODE_PAGE_FLIP ioctl.
> >>>>>>>>
> >>>>>>>> This change makes sure that we do not program the flip to the hardware
> >>>>>>>> before the end of vblank seqno n-1 in this case, to prevent the flip
> >>>>>>>> from taking effect too early.
> >>>>>>>>
> >>>>>>>> On the other hand, if the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called
> >>>>>>>> during vblank, but userspace didn't wait for the current vblank seqno
> >>>>>>>> before, this change would still allow the flip to be programmed during
> >>>>>>>> the current vblank seqno.
> >>>>>>>
> >>>>>>> This just sounds like you're sending vblank events out a bit too early.
> >>>>>>> And watching vblank waits that userspace does works, but it's fragile,
> >>>>>>> add-hoc and I don't really jump in joy about adding that to the vblank
> >>>>>>> core. Is there no way you can adjust sending out the vblank events
> >>>>>>> similarly, to make sure userspace can never sneak in a pageflip too early?
> >>>>>>
> >>>>>> What you call "too early" is actually "during the vertical blank period
> >>>>>> waited for". IMHO only notifying userspace of a vertical blank period
> >>>>>> when it's already over would defeat the purpose.
> >>>>>
> >>>>> Afaiui the rules are:
> >>>>> - The timestamp for vblank event needs to agree with whatever oml_sync
> >>>>>   requries.
> >>>>> - The event delivery itself needs to be consistent with what page_flip
> >>>>>   takes, i.e. if userspace sees an event and immediately issues a
> >>>>>   page_flip then it should not be able to hit the same vblank with that
> >>>>>   pageflip.
> >>>>> - The event needs to be after the old buffers are not longer used and can
> >>>>>   be reused for rendering.
> >>>>
> >>>> That's only relevant for DRM_IOCTL_MODE_PAGE_FLIP, not
> >>>> DRM_IOCTL_WAIT_VBLANK.
> >>>
> >>> Yup, mixed that up.
> >>>
> >>>>> - There's no requirement at all that the event gets delivered at a
> >>>>>   specific point in the vblank, hardware is too different for that to work
> >>>>
> >>>> As the name implies, the purpose of DRM_IOCTL_WAIT_VBLANK is to wait for
> >>>> a vertical blank period. If that doesn't work as intended with some
> >>>> hardware, that's tough luck but not really my problem. :)
> >>>>
> >>>>>   - that kind of precision is why we have a separate timestamp.
> >>>>
> >>>> I'm afraid this last item gives away that you're relatively new to this
> >>>> code. ;) The timestamp was originally literally just the current
> >>>> gettimeofday when the wait finished (the original DRM_IOCTL_WAIT_VBLANK
> >>>> ioctl didn't have any asynchronous notification functionality). It was
> >>>> relatively recently that Mario changed the timestamp to correspond to
> >>>> the end of the vertical blank period / start of scanout of the next
> >>>> frame, presumably due to your first rule above.
> >>>
> >>> Most hw just seems to give you a vblank interrupt somewhere in the vblank
> >>> are, or sometimes even slightly before that.
> >>
> >> Our hardware tends to trigger the vblank interrupt early, but it's still
> >> useful in that drawing operations submitted after it cannot affect the
> >> previously scanned out frame.
> >>
> >>> Also there's scheduling jitter.
> >>
> >> Sure, but there's a big difference between "no guarantee that we're
> >> still in vblank" vs "guarantee that we're no longer in vblank".
> >>
> >>
> >>>>> I assume you're goal is to not delay page_flips unecessarily, without
> >>>>> breaking requirement 2 here. Imo a simpler fix would be to delay the
> >>>>> vblank handling to end of vblank. Fixes everything without hacks, [...]
> >>>>
> >>>> Except it breaks the original purpose of the wait for vblank
> >>>> functionality, which is to wait for the beginning of a vertical blank
> >>>> period. [0] You're focusing too much on page flips and suggesting to
> >>>> throw out the vblank baby with the bathwater. I really don't see the big
> >>>> issue which would justify that.
> >>>>
> >>>>
> >>>> [0] As an analogy, how useful would e.g. calendar notifications be if
> >>>> they arrived at the end of the events they're about? "Hey, that meeting
> >>>> you were supposed to attend? It just finished!"
> >>>
> >>> Ok, what exactly is the use-case for waiting for vblanks _without_
> >>> scheduling a flip afterwards? At least in drm the rule is that ABI is what
> >>> userspace observes and actually cares about.
> >>
> >> E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
> >> waits for the target vertical blank period before emitting the drawing
> >> commands for a buffer swap operation. If the vblank notification only
> >> arrives when the vertical blank period is already over, this is very
> >> likely to result in tearing.
> >>
> >> Some X compositors and AFAIK even applications such as media players can
> >> use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
> >> used directly like that, but nonetheless it is.
> > 
> > Is there really anything using it like that outside of -ati?
> 
> Yes. With DRI3/Present, it's driver independent code in
> xserver/present/. With DRI2, it's theoretically up to the DDX driver,
> but all drivers seem to have basically the same logic; the modesetting
> driver certainly does.

Hm, didn't know that everyone does that. Seemed to silly an idea to waste
all that gpu bandwidth by waiting for vblank ...

> > I didn't know that we pass vblank waits to X clients. Either way annoying,
> > since it means you need to keep things working like this for amd drivers
> > forever.
> 
> Please don't try to single out our drivers, it seems like rather your
> driver is the odd man out in this case.

Hm, why/where is i915 special? It sounds like it's very much not, it's
just that normal page_flips (i.e. neither async nor variable refresh rate)
get committed atomically by the hw at the exact same time the vblank irq
fires. And if you look at the metric pile of atomic drivers we now have,
they at least seem to all work like that. Afaik amd hw seems to be the
exception with being able to make a flip still happen after the vblank
fired. I didn't check nouveau, maybe that one is similar.

> > Afaik others don't use it like that, at least not on intel.
> 
> UXA has the same DRI2 logic, not sure about SNA.
> 
> 
> > Anyway, I still don't like adding hacks to drm core like this for
> > single-use in just one driver.
> 
> The driver changes will be ported to radeon of course, and why couldn't
> it be used by other drivers for the same purpose?

Because other drivers work already, see above. At least all the atomic
drivers will work like that, or at least seem to be written to work like
that. So afaics it's amd drivers only.

> > drm_irq.c is already really complex and suffering badly from this, and
> > we're pretty close to always accidentally breaking something when touching it.
> 
> I just don't see how my change makes this any worse.

It's death by a thousand cuts. Of course none of the individual ones are
bad alone.

> > How bad would it really be to just always delay the page_flip past vblank?
> 
> In the variable refresh rate case it could be pretty bad, basically
> dropping to the minimum refresh rate.

Variable refresh rate is entirely undefined right now in kms. We probably
need an entirely new set of flags to make that work smoothly. And I think
even for variable rate refresh you want the current vblank stuff to tick
as regularly as possible. And then your page_flip (without special flags
would only need to make sure it's not faster than that). Variable refresh
rate page_flips probably need a special flag, maybe even the same
dont-tear-but-flip-asap flag.

> > If that's not good enough I'd say we should add a
> > faster-than-vblank-but-still-synced page_flip flag. Then userspace could
> > tell you exactly whether you should always wait (no flags), or never wait
> > (with this new flag).
> 
> That would be an inferior solution compared to my series, e.g.: If
> userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
> is reached, it cannot use the new flag, otherwise the flip might take
> effect too early. However, if we are then already in the target vblank
> period when the fences have signalled and we are ready to program the
> flip, we have to wait for the end of vblank first, and the flip will be
> delayed by one frame.
> 
> If we're going to change the userspace interface, it would be better to
> re-purpose the reserved field of struct drm_mode_crtc_page_flip for
> explicitly specifying the target vblank seqno (via a new cap and flag).
> Then the kernel and userspace would no longer need to second-guess each
> other.
> 
> But even if we take that route, this series would be desirable for
> getting us most of the way there for existing userspace.

Well that's what I mean. You'r patches here shoehorn what you want into
existing api, trying to second-guess userspaces intentions inferred from
what it does. Ime that tends to end in trouble. It would be much better to
have a clear uabi for this. And my flag was just a suggestion.

I just had a discussion on irc with Dave about another topic were Dave
complained about some of the inferred abi rules SNA uses (entirely
differently, in probe code). And I thought it was a nice idea, since hey
it works and its easier. But really it's not, since in the end kms is
supposed to be somewhat generic. And usually your nice idea for inferring
behaviour then tends to break down somewhere. At least I think the generic
kms rules are:

- vblank events fire at lockstep (not everyone gets this right, but you
  end up with either lagging desktop or 100% cpu usuage on weston if you
  don't). Which also means for variable refresh rate you need to keep this
  vblank running at full refresh rate (which is annoying, but just means
  we need a flag for vblank waits).
- pageflip immediately after a vblank wait needs to hit the next vblank.
- pageflip in a loop needs to result in at most 1 flip per vblank, and if
  you're too fast then the kernel should return -EBUSY. Latest atomic
  heleprs even enforce this, to standardized the uabi more.

Imo everything else (in this case: make the flip complete on the same
frame as the vblank, if you hit the vblank window) needs special flags,
with clear meaning of what they do. The specific flip target sounds like a
good idea, except that current userspace can't be fixed, so we need to
make it work without any flag. And the new flag would be for flip-asap, or
flip-variable-refresh or something like that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Chris Wilson]

On Tue, Jun 14, 2016 at 07:53:41AM +0200, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
> > E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
> > waits for the target vertical blank period before emitting the drawing
> > commands for a buffer swap operation. If the vblank notification only
> > arrives when the vertical blank period is already over, this is very
> > likely to result in tearing.
> > 
> > Some X compositors and AFAIK even applications such as media players can
> > use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
> > used directly like that, but nonetheless it is.
> 
> Is there really anything using it like that outside of -ati? I didn't know
> that we pass vblank waits to X clients.

No, because EGL doesn't offer an equivalent to OML_sync_control
applications like kodi have implemented their own vblank scheduling via
DRM_IOCTL_WAIT_VBLANK directly.

> Either way annoying, since it
> means you need to keep things working like this for amd drivers forever.
> Afaik others don't use it like that, at least not on intel. Weston has
> some hacks to use vblank waits for plane flips, but that's all disabled
> code because it just doesn't work - you need full atomic.

Inside Xorg/Present, as Michel said, the vblank notification is used to
drive onscreen copies *within* the vblank period (i.e. to try and avoid
visible tearing).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 14.06.2016 17:06, Daniel Vetter wrote:
> On Tue, Jun 14, 2016 at 04:25:28PM +0900, Michel Dänzer wrote:
>> On 14.06.2016 14:53, Daniel Vetter wrote:
>>> On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
>>>> On 06/13/16 23:06, Daniel Vetter wrote:
>>>>> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
>>>>>> On 06/13/16 17:06, Daniel Vetter wrote:
>>>>>>>
>>>>>>> Afaiui the rules are:
>>>>>>> - The timestamp for vblank event needs to agree with whatever oml_sync
>>>>>>>   requries.
>>>>>>> - The event delivery itself needs to be consistent with what page_flip
>>>>>>>   takes, i.e. if userspace sees an event and immediately issues a
>>>>>>>   page_flip then it should not be able to hit the same vblank with that
>>>>>>>   pageflip.
>>>>>>> [...]
>>>>>>
>>>>>>> I assume you're goal is to not delay page_flips unecessarily, without
>>>>>>> breaking requirement 2 here. Imo a simpler fix would be to delay the
>>>>>>> vblank handling to end of vblank. Fixes everything without hacks, [...]
>>>>>>
>>>>>> Except it breaks the original purpose of the wait for vblank
>>>>>> functionality, which is to wait for the beginning of a vertical blank
>>>>>> period. [0] You're focusing too much on page flips and suggesting to
>>>>>> throw out the vblank baby with the bathwater. I really don't see the big
>>>>>> issue which would justify that.
>>>>>>
>>>>>>
>>>>>> [0] As an analogy, how useful would e.g. calendar notifications be if
>>>>>> they arrived at the end of the events they're about? "Hey, that meeting
>>>>>> you were supposed to attend? It just finished!"
>>>>>
>>>>> Ok, what exactly is the use-case for waiting for vblanks _without_
>>>>> scheduling a flip afterwards? At least in drm the rule is that ABI is what
>>>>> userspace observes and actually cares about.
>>>>
>>>> E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
>>>> waits for the target vertical blank period before emitting the drawing
>>>> commands for a buffer swap operation. If the vblank notification only
>>>> arrives when the vertical blank period is already over, this is very
>>>> likely to result in tearing.
>>>>
>>>> Some X compositors and AFAIK even applications such as media players can
>>>> use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
>>>> used directly like that, but nonetheless it is.
>>>
>>> Is there really anything using it like that outside of -ati?
>>
>> Yes. With DRI3/Present, it's driver independent code in
>> xserver/present/. With DRI2, it's theoretically up to the DDX driver,
>> but all drivers seem to have basically the same logic; the modesetting
>> driver certainly does.
> 
> Hm, didn't know that everyone does that. Seemed to silly an idea to waste
> all that gpu bandwidth by waiting for vblank ...

Actually it's kind of the other way around: One of the reasons for using
sync-to-vblank is to save power for rendering stuff which cannot be seen
anyway.

Also note that only the specific client using DRI2 or Present for
presentation is blocked, nothing else.


>>> I didn't know that we pass vblank waits to X clients. Either way annoying,
>>> since it means you need to keep things working like this for amd drivers
>>> forever.
>>
>> Please don't try to single out our drivers, it seems like rather your
>> driver is the odd man out in this case.
> 
> Hm, why/where is i915 special?

By only triggering the vblank interrupt at the end of a vertical blank
period, breaking the original DRM_IOCTL_WAIT_VBLANK functionality.


>>> How bad would it really be to just always delay the page_flip past vblank?
>>
>> In the variable refresh rate case it could be pretty bad, basically
>> dropping to the minimum refresh rate.
> 
> Variable refresh rate is entirely undefined right now in kms.

I don't think that really matters for the issue I'm thinking of.

My understanding is that variable refresh rate basically works by
transmitting the frame contents using the maximum refresh rate timing,
and then dynamically extending the vertical blank period until either
the next flip arrives, or the time since the last frame was transmitted
corresponds to the minimum refresh rate. So if we always wait for the
end of vertical blank before programming a flip, we'll probably end up
degrading to the minimum refresh rate whenever we're already in vertical
blank by the time we're ready to program the flip. Which will happen
with any app which cannot sustain a framerate > the maximum refresh
rate. So we'll end up running at either the minimum or maximum refresh
rate (or possibly even worse, flip-flopping between the two), defeating
the purpose of variable refresh rate.


Also, as I explained before, even without variable refresh rate, always
delaying flip programming past vblank can cause flips to be
unnecessarily delayed by one frame.


>>> If that's not good enough I'd say we should add a
>>> faster-than-vblank-but-still-synced page_flip flag. Then userspace could
>>> tell you exactly whether you should always wait (no flags), or never wait
>>> (with this new flag).
>>
>> That would be an inferior solution compared to my series, e.g.: If
>> userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
>> is reached, it cannot use the new flag, otherwise the flip might take
>> effect too early. However, if we are then already in the target vblank
>> period when the fences have signalled and we are ready to program the
>> flip, we have to wait for the end of vblank first, and the flip will be
>> delayed by one frame.
>>
>> If we're going to change the userspace interface, it would be better to
>> re-purpose the reserved field of struct drm_mode_crtc_page_flip for
>> explicitly specifying the target vblank seqno (via a new cap and flag).
>> Then the kernel and userspace would no longer need to second-guess each
>> other.
>>
>> But even if we take that route, this series would be desirable for
>> getting us most of the way there for existing userspace.
> 
> Well that's what I mean. You'r patches here shoehorn what you want into
> existing api, trying to second-guess userspaces intentions inferred from
> what it does. Ime that tends to end in trouble. It would be much better to
> have a clear uabi for this. And my flag was just a suggestion.
> 
> I just had a discussion on irc with Dave about another topic were Dave
> complained about some of the inferred abi rules SNA uses (entirely
> differently, in probe code). And I thought it was a nice idea, since hey
> it works and its easier. But really it's not, since in the end kms is
> supposed to be somewhat generic. And usually your nice idea for inferring
> behaviour then tends to break down somewhere. At least I think the generic
> kms rules are:
> 
> - vblank events fire at lockstep [...].
> - pageflip immediately after a vblank wait needs to hit the next vblank.
> - pageflip in a loop needs to result in at most 1 flip per vblank, and if
>   you're too fast then the kernel should return -EBUSY. [...]

My series doesn't break any of these rules (or any others I'm aware of).
It avoids unnecessarily delaying flips in some cases within the confines
of these rules.


> Imo everything else (in this case: make the flip complete on the same
> frame as the vblank, if you hit the vblank window) needs special flags,
> with clear meaning of what they do. The specific flip target sounds like a
> good idea, except that current userspace can't be fixed, so we need to
> make it work without any flag.

Hey, that was my point above! So you agree that (something like) this
series will be needed anyway, even if new flags are added to make it
more explicit? :)


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Wed, Jun 15, 2016 at 05:03:41PM +0900, Michel Dänzer wrote:
> On 14.06.2016 17:06, Daniel Vetter wrote:
> > On Tue, Jun 14, 2016 at 04:25:28PM +0900, Michel Dänzer wrote:
> >> On 14.06.2016 14:53, Daniel Vetter wrote:
> >>> On Tue, Jun 14, 2016 at 11:09:10AM +0900, Michel Dänzer wrote:
> >>>> On 06/13/16 23:06, Daniel Vetter wrote:
> >>>>> On Mon, Jun 13, 2016 at 05:58:29PM +0900, Michel Dänzer wrote:
> >>>>>> On 06/13/16 17:06, Daniel Vetter wrote:
> >>>>>>>
> >>>>>>> Afaiui the rules are:
> >>>>>>> - The timestamp for vblank event needs to agree with whatever oml_sync
> >>>>>>>   requries.
> >>>>>>> - The event delivery itself needs to be consistent with what page_flip
> >>>>>>>   takes, i.e. if userspace sees an event and immediately issues a
> >>>>>>>   page_flip then it should not be able to hit the same vblank with that
> >>>>>>>   pageflip.
> >>>>>>> [...]
> >>>>>>
> >>>>>>> I assume you're goal is to not delay page_flips unecessarily, without
> >>>>>>> breaking requirement 2 here. Imo a simpler fix would be to delay the
> >>>>>>> vblank handling to end of vblank. Fixes everything without hacks, [...]
> >>>>>>
> >>>>>> Except it breaks the original purpose of the wait for vblank
> >>>>>> functionality, which is to wait for the beginning of a vertical blank
> >>>>>> period. [0] You're focusing too much on page flips and suggesting to
> >>>>>> throw out the vblank baby with the bathwater. I really don't see the big
> >>>>>> issue which would justify that.
> >>>>>>
> >>>>>>
> >>>>>> [0] As an analogy, how useful would e.g. calendar notifications be if
> >>>>>> they arrived at the end of the events they're about? "Hey, that meeting
> >>>>>> you were supposed to attend? It just finished!"
> >>>>>
> >>>>> Ok, what exactly is the use-case for waiting for vblanks _without_
> >>>>> scheduling a flip afterwards? At least in drm the rule is that ABI is what
> >>>>> userspace observes and actually cares about.
> >>>>
> >>>> E.g.: In cases where page flipping cannot be used, Xorg / the DDX driver
> >>>> waits for the target vertical blank period before emitting the drawing
> >>>> commands for a buffer swap operation. If the vblank notification only
> >>>> arrives when the vertical blank period is already over, this is very
> >>>> likely to result in tearing.
> >>>>
> >>>> Some X compositors and AFAIK even applications such as media players can
> >>>> use DRM_IOCTL_WAIT_VBLANK similarly. Obviously it's not intended to be
> >>>> used directly like that, but nonetheless it is.
> >>>
> >>> Is there really anything using it like that outside of -ati?
> >>
> >> Yes. With DRI3/Present, it's driver independent code in
> >> xserver/present/. With DRI2, it's theoretically up to the DDX driver,
> >> but all drivers seem to have basically the same logic; the modesetting
> >> driver certainly does.
> > 
> > Hm, didn't know that everyone does that. Seemed to silly an idea to waste
> > all that gpu bandwidth by waiting for vblank ...
> 
> Actually it's kind of the other way around: One of the reasons for using
> sync-to-vblank is to save power for rendering stuff which cannot be seen
> anyway.
> 
> Also note that only the specific client using DRI2 or Present for
> presentation is blocked, nothing else.
> 
> 
> >>> I didn't know that we pass vblank waits to X clients. Either way annoying,
> >>> since it means you need to keep things working like this for amd drivers
> >>> forever.
> >>
> >> Please don't try to single out our drivers, it seems like rather your
> >> driver is the odd man out in this case.
> > 
> > Hm, why/where is i915 special?
> 
> By only triggering the vblank interrupt at the end of a vertical blank
> period, breaking the original DRM_IOCTL_WAIT_VBLANK functionality.

Ok, turns out we never did that, but we did move the drm event for page
flip completion around. So indeed vblank wait drm event should fire at
start of vblank if possible. I still maintain that (by default) you
shouldn't allow a page flip to overtake a drm event.

> >>> How bad would it really be to just always delay the page_flip past vblank?
> >>
> >> In the variable refresh rate case it could be pretty bad, basically
> >> dropping to the minimum refresh rate.
> > 
> > Variable refresh rate is entirely undefined right now in kms.
> 
> I don't think that really matters for the issue I'm thinking of.
> 
> My understanding is that variable refresh rate basically works by
> transmitting the frame contents using the maximum refresh rate timing,
> and then dynamically extending the vertical blank period until either
> the next flip arrives, or the time since the last frame was transmitted
> corresponds to the minimum refresh rate. So if we always wait for the
> end of vertical blank before programming a flip, we'll probably end up
> degrading to the minimum refresh rate whenever we're already in vertical
> blank by the time we're ready to program the flip. Which will happen
> with any app which cannot sustain a framerate > the maximum refresh
> rate. So we'll end up running at either the minimum or maximum refresh
> rate (or possibly even worse, flip-flopping between the two), defeating
> the purpose of variable refresh rate.
> 
> 
> Also, as I explained before, even without variable refresh rate, always
> delaying flip programming past vblank can cause flips to be
> unnecessarily delayed by one frame.

Yes, but right now you show them too early, breaking existing stuff. My
stance is that you need an explicit flag to tell the kernel which one you
want (userspace tells you already by either asking for a specific frame,
or for "as soon as possible"). I don't like second-guessing userspace.
And yes that means that for variable vrefresh you probably need to run it
at 60Hz by default, except when userspace set a special mode flag which
allows variable vrefresh, telling the kernel that it knows things can get
slow if it doesn't ask for asap.

> >>> If that's not good enough I'd say we should add a
> >>> faster-than-vblank-but-still-synced page_flip flag. Then userspace could
> >>> tell you exactly whether you should always wait (no flags), or never wait
> >>> (with this new flag).
> >>
> >> That would be an inferior solution compared to my series, e.g.: If
> >> userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
> >> is reached, it cannot use the new flag, otherwise the flip might take
> >> effect too early. However, if we are then already in the target vblank
> >> period when the fences have signalled and we are ready to program the
> >> flip, we have to wait for the end of vblank first, and the flip will be
> >> delayed by one frame.
> >>
> >> If we're going to change the userspace interface, it would be better to
> >> re-purpose the reserved field of struct drm_mode_crtc_page_flip for
> >> explicitly specifying the target vblank seqno (via a new cap and flag).
> >> Then the kernel and userspace would no longer need to second-guess each
> >> other.
> >>
> >> But even if we take that route, this series would be desirable for
> >> getting us most of the way there for existing userspace.
> > 
> > Well that's what I mean. You'r patches here shoehorn what you want into
> > existing api, trying to second-guess userspaces intentions inferred from
> > what it does. Ime that tends to end in trouble. It would be much better to
> > have a clear uabi for this. And my flag was just a suggestion.
> > 
> > I just had a discussion on irc with Dave about another topic were Dave
> > complained about some of the inferred abi rules SNA uses (entirely
> > differently, in probe code). And I thought it was a nice idea, since hey
> > it works and its easier. But really it's not, since in the end kms is
> > supposed to be somewhat generic. And usually your nice idea for inferring
> > behaviour then tends to break down somewhere. At least I think the generic
> > kms rules are:
> > 
> > - vblank events fire at lockstep [...].
> > - pageflip immediately after a vblank wait needs to hit the next vblank.
> > - pageflip in a loop needs to result in at most 1 flip per vblank, and if
> >   you're too fast then the kernel should return -EBUSY. [...]
> 
> My series doesn't break any of these rules (or any others I'm aware of).
> It avoids unnecessarily delaying flips in some cases within the confines
> of these rules.
> 
> 
> > Imo everything else (in this case: make the flip complete on the same
> > frame as the vblank, if you hit the vblank window) needs special flags,
> > with clear meaning of what they do. The specific flip target sounds like a
> > good idea, except that current userspace can't be fixed, so we need to
> > make it work without any flag.
> 
> Hey, that was my point above! So you agree that (something like) this
> series will be needed anyway, even if new flags are added to make it
> more explicit? :)

Yeah I think there's a bit of confusion going on here ;-) Of course I'm
not against fixing this, and I agree that fixing it by delaying the vblank
drm event (like I proposed at first) is not good. What I think would be
best to fix this:

- For all current userspace (i.e. no flags or anything) force vrefresh to
  to be fixed, and delay page flips which hit the vblank window to be
  after that. This way amd drivers are consistent with every other kms
  drivers, and work like current userspace seems to expect: Wait for
  vblank, then assume any flips will only hit after the next vblank.

- For clients which don't care about which frame to be displayed on, add a
  flag to page_flip (and atomic_commit) that asks for asap, but still
  without tearing. With atomic we even want to be able to overwrite older
  flips, to reduce latency as much as possible for clients who can render
  more frames than vrefresh.

- For variable vrefresh I think we need yet another flag (crtc property,
  or mode flag) so that userspace can tell the kernel when it's ok to have
  vblank times with massive jitter. Again default should be current mode.
  Userspace can then enable variable vrefresh when it only has clients
  which don't care when exactly they show up. As soon as something shows
  up which asks for precise timestamps and displaying of the flip (using
  OML_sync_control or Present), userspace can disable the variable
  vrefresh mode again to get back to a lockstep 60Hz (or whatever it is).

- Anyone using WAIT_VBLANK behind the compositors back might be in even
  more trouble, but imo we shouldn't give too much concern to that case.
  Instead apply more pressure for everyone to switch over to Present or
  OML_sync_control (and we need that for EGL, too).

That way there's no second-guessing of userspace intentions by watching
vblank waits, it should be possible to extend for variable refresh and
benchmark/low-latency mode, and we don't need special driver hacks in core
code either. Thoughts on whether this plan would fit you too?

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Michel Dänzer]

On 15.06.2016 18:23, Daniel Vetter wrote:
> On Wed, Jun 15, 2016 at 05:03:41PM +0900, Michel Dänzer wrote:
>> On 14.06.2016 17:06, Daniel Vetter wrote:
>>> On Tue, Jun 14, 2016 at 04:25:28PM +0900, Michel Dänzer wrote:
>>>> On 14.06.2016 14:53, Daniel Vetter wrote:
>>>>>
>>>>> I didn't know that we pass vblank waits to X clients. Either way annoying,
>>>>> since it means you need to keep things working like this for amd drivers
>>>>> forever.
>>>>
>>>> Please don't try to single out our drivers, it seems like rather your
>>>> driver is the odd man out in this case.
>>>
>>> Hm, why/where is i915 special?
>>
>> By only triggering the vblank interrupt at the end of a vertical blank
>> period, breaking the original DRM_IOCTL_WAIT_VBLANK functionality.
> 
> Ok, turns out we never did that, but we did move the drm event for page
> flip completion around. So indeed vblank wait drm event should fire at
> start of vblank if possible. I still maintain that (by default) you
> shouldn't allow a page flip to overtake a drm event.

We don't allow that AFAIK. If you think we do, please describe the
scenario you're thinking of.

The only thing I can think of is that you may be concerned about cases
where userspace calls DRM_IOCTL_MODE_PAGE_FLIP several times in a row
without calling DRM_IOCTL_WAIT_VBLANK in between. If so, I can change
patch 2 to also track page flip completions in
file_priv->last_vblank_wait. That would ensure that two consecutive
flips can never complete in the same vertical blank period.


>>>>> How bad would it really be to just always delay the page_flip past vblank?
>>>>
>>>> [...]
>>
>> [...] always delaying flip programming past vblank can cause flips to be
>> unnecessarily delayed by one frame.
> 
> Yes, but right now you show them too early, breaking existing stuff.

No, we don't! Neither before this series nor after it, nor at any point
during it.

The DRI2 code in our DDX drivers actually checks for this and complains
if a flip completes earlier than expected. We did break this before and
received bug reports about it. Mario fixed it by changing the
MASTER_UPDATE_MODE register to 3, which restricts flips to take effect
only at the beginning of vertical blank. This series makes it safe to
change it back to 0 (allowing flips to take effect anywhere in vertical
blank) and then does so.


>>>>> If that's not good enough I'd say we should add a
>>>>> faster-than-vblank-but-still-synced page_flip flag. Then userspace could
>>>>> tell you exactly whether you should always wait (no flags), or never wait
>>>>> (with this new flag).
>>>>
>>>> That would be an inferior solution compared to my series, e.g.: If
>>>> userspace calls DRM_IOCTL_MODE_PAGE_FLIP before the target vblank seqno
>>>> is reached, it cannot use the new flag, otherwise the flip might take
>>>> effect too early. However, if we are then already in the target vblank
>>>> period when the fences have signalled and we are ready to program the
>>>> flip, we have to wait for the end of vblank first, and the flip will be
>>>> delayed by one frame.
>>>>
>>>> If we're going to change the userspace interface, it would be better to
>>>> re-purpose the reserved field of struct drm_mode_crtc_page_flip for
>>>> explicitly specifying the target vblank seqno (via a new cap and flag).
>>>> Then the kernel and userspace would no longer need to second-guess each
>>>> other.
>>>>
>>>> But even if we take that route, this series would be desirable for
>>>> getting us most of the way there for existing userspace.
>>>
>>> Well that's what I mean. You'r patches here shoehorn what you want into
>>> existing api, trying to second-guess userspaces intentions inferred from
>>> what it does. Ime that tends to end in trouble. It would be much better to
>>> have a clear uabi for this. And my flag was just a suggestion.
>>>
>>> [...] At least I think the generic kms rules are:
>>>
>>> - vblank events fire at lockstep [...].
>>> - pageflip immediately after a vblank wait needs to hit the next vblank.
>>> - pageflip in a loop needs to result in at most 1 flip per vblank, and if
>>>   you're too fast then the kernel should return -EBUSY. [...]
>>
>> My series doesn't break any of these rules (or any others I'm aware of).
>> It avoids unnecessarily delaying flips in some cases within the confines
>> of these rules.
>>
>>
>>> Imo everything else (in this case: make the flip complete on the same
>>> frame as the vblank, if you hit the vblank window) needs special flags,
>>> with clear meaning of what they do. The specific flip target sounds like a
>>> good idea, except that current userspace can't be fixed, so we need to
>>> make it work without any flag.
>>
>> Hey, that was my point above! So you agree that (something like) this
>> series will be needed anyway, even if new flags are added to make it
>> more explicit? :)
> 
> Yeah I think there's a bit of confusion going on here ;-) Of course I'm
> not against fixing this, and I agree that fixing it by delaying the vblank
> drm event (like I proposed at first) is not good. What I think would be
> best to fix this:
> 
> - For all current userspace (i.e. no flags or anything) force vrefresh to
>   to be fixed, and delay page flips which hit the vblank window to be
>   after that. This way amd drivers are consistent with every other kms
>   drivers, and work like current userspace seems to expect: Wait for
>   vblank, then assume any flips will only hit after the next vblank.

This series preserves this behaviour. A flip is only allowed to complete
during the current vertical blank period if userspace either:

* expected it to complete in this vertical blank (or an earlier one).
  In other words, if the flip doesn't complete in this vertical blank,
  it is delayed (further) compared to userspace expectations.

* hasn't called DRM_IOCTL_WAIT_VBLANK at all (so apparently it doesn't
  care about when the flip completes).

It sounds like you're saying we aren't allowed to fix cases where flips
are completing later than expected by userspace, because other drivers
haven't fixed those cases yet. Quite frankly, that sucks. Nothing other
than possible hardware restrictions prevents other drivers from fixing
this as well.


> - For clients which don't care about which frame to be displayed on, add a
>   flag to page_flip (and atomic_commit) that asks for asap, but still
>   without tearing.

I explained above why such a flag wouldn't help in all cases. When I get
a chance, I'll work on a series allowing the target vblank seqno to be
specified explicitly in DRM_IOCTL_MODE_PAGE_FLIP. That should cover all
cases.


> - For variable vrefresh I think we need yet another flag (crtc property,
>   or mode flag) so that userspace can tell the kernel when it's ok to have
>   vblank times with massive jitter. Again default should be current mode.
>   Userspace can then enable variable vrefresh when it only has clients
>   which don't care when exactly they show up. As soon as something shows
>   up which asks for precise timestamps and displaying of the flip (using
>   OML_sync_control or Present), userspace can disable the variable
>   vrefresh mode again to get back to a lockstep 60Hz (or whatever it is).

Colour me skeptical about that being a good approach for variable
refresh rate, but that's another discussion which doesn't directly
affect this series.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/5] drm/amdgpu: Wait for end of last waited-for vblank before programming flip

[Daniel Vetter]

On Thu, Jun 16, 2016 at 11:15:14AM +0900, Michel Dänzer wrote:
> On 15.06.2016 18:23, Daniel Vetter wrote:
> > On Wed, Jun 15, 2016 at 05:03:41PM +0900, Michel Dänzer wrote:
> >> On 14.06.2016 17:06, Daniel Vetter wrote:
> > Yeah I think there's a bit of confusion going on here ;-) Of course I'm
> > not against fixing this, and I agree that fixing it by delaying the vblank
> > drm event (like I proposed at first) is not good. What I think would be
> > best to fix this:
> > 
> > - For all current userspace (i.e. no flags or anything) force vrefresh to
> >   to be fixed, and delay page flips which hit the vblank window to be
> >   after that. This way amd drivers are consistent with every other kms
> >   drivers, and work like current userspace seems to expect: Wait for
> >   vblank, then assume any flips will only hit after the next vblank.
> 
> This series preserves this behaviour. A flip is only allowed to complete
> during the current vertical blank period if userspace either:
> 
> * expected it to complete in this vertical blank (or an earlier one).
>   In other words, if the flip doesn't complete in this vertical blank,
>   it is delayed (further) compared to userspace expectations.
> 
> * hasn't called DRM_IOCTL_WAIT_VBLANK at all (so apparently it doesn't
>   care about when the flip completes).
> 
> It sounds like you're saying we aren't allowed to fix cases where flips
> are completing later than expected by userspace, because other drivers
> haven't fixed those cases yet. Quite frankly, that sucks. Nothing other
> than possible hardware restrictions prevents other drivers from fixing
> this as well.

I'm not objecting against fixing this. I'm objecting against fixing this
through clever inferring of what userspace wants, aka the above, instead
of an explicit flag or something. And without that flag the rule is that a
pageflip after vblank hits the next vblank and can't squeeze into the
current one.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel