[PATCH 0/6] drm: Explicit target vblank seqno for page flips

[PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Michel Dänzer]

The purpose of this series is to allow drivers to avoid unnecessarily
delaying page flips, by explicitly telling the driver which vblank seqno
a flip is supposed to take effect in.

Patch 1 sets the target to the vblank seqno after the current one when
the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called, preserving the ioctl
contract with existing userspace.

Patches 2-4 take advantage of this in the amdgpu and radeon drivers, by
allowing a flip to be programmed and executed during the target vertical
blank period (or a later one).

Patch 6 extends the ioctl with new flags, which allow userspace to
explicitly specify the target vblank seqno. This can also avoid delaying
flips in some cases where we are already in the target vertical blank
period when the ioctl is called.

[PATCH 1/6] drm: Add page_flip_target CRTC hook
[PATCH 2/6] drm/amdgpu: Provide page_flip_target hook
[PATCH 3/6] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again
[PATCH 4/6] drm/radeon: Provide page_flip_target hook
[PATCH 5/6] drm/radeon: Set MASTER_UPDATE_MODE to 0 again
[PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/6] drm: Add page_flip_target CRTC hook

[Michel Dänzer]

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

Mostly the same as the existing page_flip hook, but takes an additional
parameter specifying the target vertical blank period when the flip
should take effect.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++----
 include/drm/drm_crtc.h     | 14 ++++++++++++++
 2 files changed, 33 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9d3f80e..15ad7e2 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
+	u32 target_vblank = 0;
 	int ret = -EINVAL;
 
 	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
@@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
+	if (crtc->funcs->page_flip_target) {
+		int r;
+
+		r = drm_crtc_vblank_get(crtc);
+		if (r)
+			return r;
+
+		target_vblank = drm_crtc_vblank_count(crtc) +
+			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
+	} else if (crtc->funcs->page_flip == NULL)
+		return -EINVAL;
+
 	drm_modeset_lock_crtc(crtc, crtc->primary);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
@@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	if (crtc->funcs->page_flip == NULL)
-		goto out;
-
 	fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
 	if (!fb) {
 		ret = -ENOENT;
@@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 	crtc->primary->old_fb = crtc->primary->fb;
-	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
+	if (crtc->funcs->page_flip_target)
+		ret = crtc->funcs->page_flip_target(crtc, fb, e,
+						    page_flip->flags,
+						    target_vblank);
+	else
+		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
 	if (ret) {
 		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
 			drm_event_cancel_free(dev, &e->base);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 9e6ab4a..ae1d9b6 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -579,6 +579,20 @@ struct drm_crtc_funcs {
 			 uint32_t flags);
 
 	/**
+	 * @page_flip_target:
+	 *
+	 * Same as @page_flip but with an additional parameter specifying the
+	 * target vertical blank period when the flip should take effect.
+	 *
+	 * Note that the core code calls drm_crtc_vblank_get before this entry
+	 * point.
+	 */
+	int (*page_flip_target)(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags, uint32_t target);
+
+	/**
 	 * @set_property:
 	 *
 	 * This is the legacy entry point to update a property attached to the
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/6] drm/amdgpu: Provide page_flip_target hook

[Michel Dänzer]

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

Now we can program a flip during a vertical blank period, if it's the
one targeted by the flip (or a later one). This allows simplifying
amdgpu_flip_work_func considerably.

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 | 98 +++++++++--------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h    |  8 +--
 drivers/gpu/drm/amd/amdgpu/dce_v10_0.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v11_0.c      |  2 +-
 drivers/gpu/drm/amd/amdgpu/dce_v8_0.c       |  2 +-
 6 files changed, 39 insertions(+), 76 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index eb09037..71f4a4c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -718,10 +718,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				target_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 7dbe8d0..ce1f2bf 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,16 +63,17 @@ 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];
 
 	struct drm_crtc *crtc = &amdgpuCrtc->base;
 	unsigned long flags;
-	unsigned i, repcnt = 4;
-	int vpos, hpos, stat, min_udelay = 0;
-	struct drm_vblank_crtc *vblank = &crtc->dev->vblank[work->crtc_id];
+	unsigned i;
+	int vpos, hpos;
 
 	if (amdgpu_flip_handle_fence(work, &work->excl))
 		return;
@@ -81,55 +82,23 @@ static void amdgpu_flip_work_func(struct work_struct *__work)
 		if (amdgpu_flip_handle_fence(work, &work->shared[i]))
 			return;
 
-	/* We borrow the event spin lock for protecting flip_status */
-	spin_lock_irqsave(&crtc->dev->event_lock, flags);
-
-	/* If this happens to execute within the "virtually extended" vblank
-	 * interval before the start of the real vblank interval then it needs
-	 * to delay programming the mmio flip until the real vblank is entered.
-	 * This prevents completing a flip too early due to the way we fudge
-	 * our vblank counter and vblank timestamps in order to work around the
-	 * problem that the hw fires vblank interrupts before actual start of
-	 * vblank (when line buffer refilling is done for a frame). It
-	 * complements the fudging logic in amdgpu_get_crtc_scanoutpos() for
-	 * timestamping and amdgpu_get_vblank_counter_kms() for vblank counts.
-	 *
-	 * In practice this won't execute very often unless on very fast
-	 * machines because the time window for this to happen is very small.
+	/* Wait until we're out of the vertical blank period before the one
+	 * targeted by the flip
 	 */
-	while (amdgpuCrtc->enabled && --repcnt) {
-		/* GET_DISTANCE_TO_VBLANKSTART returns distance to real vblank
-		 * start in hpos, and to the "fudged earlier" vblank start in
-		 * vpos.
-		 */
-		stat = amdgpu_get_crtc_scanoutpos(adev->ddev, work->crtc_id,
-						  GET_DISTANCE_TO_VBLANKSTART,
-						  &vpos, &hpos, NULL, NULL,
-						  &crtc->hwmode);
-
-		if ((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
-		    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE) ||
-		    !(vpos >= 0 && hpos <= 0))
-			break;
-
-		/* Sleep at least until estimated real start of hw vblank */
-		min_udelay = (-hpos + 1) * max(vblank->linedur_ns / 1000, 5);
-		if (min_udelay > vblank->framedur_ns / 2000) {
-			/* Don't wait ridiculously long - something is wrong */
-			repcnt = 0;
-			break;
-		}
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-		usleep_range(min_udelay, 2 * min_udelay);
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
-	};
+	if (amdgpuCrtc->enabled &&
+	    (amdgpu_get_crtc_scanoutpos(adev->ddev, work->crtc_id, 0,
+					&vpos, &hpos, NULL, NULL,
+					&crtc->hwmode)
+	     & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK)) ==
+	    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK) &&
+	    (int)(work->target_vblank -
+		  amdgpu_get_vblank_counter_kms(adev->ddev, amdgpuCrtc->crtc_id)) > 0) {
+		schedule_delayed_work(&work->flip_work, usecs_to_jiffies(1000));
+		return;
+	}
 
-	if (!repcnt)
-		DRM_DEBUG_DRIVER("Delay problem on crtc %d: min_udelay %d, "
-				 "framedur %d, linedur %d, stat %d, vpos %d, "
-				 "hpos %d\n", work->crtc_id, min_udelay,
-				 vblank->framedur_ns / 1000,
-				 vblank->linedur_ns / 1000, stat, vpos, hpos);
+	/* We borrow the event spin lock for protecting flip_status */
+	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 
 	/* Do the flip (mmio) */
 	adev->mode_info.funcs->page_flip(adev, work->crtc_id, work->base, work->async);
@@ -169,10 +138,10 @@ static void amdgpu_unpin_work_func(struct work_struct *__work)
 	kfree(work);
 }
 
-int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
-			  struct drm_framebuffer *fb,
-			  struct drm_pending_vblank_event *event,
-			  uint32_t page_flip_flags)
+int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
+				 struct drm_framebuffer *fb,
+				 struct drm_pending_vblank_event *event,
+				 uint32_t page_flip_flags, uint32_t target)
 {
 	struct drm_device *dev = crtc->dev;
 	struct amdgpu_device *adev = dev->dev_private;
@@ -191,7 +160,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;
@@ -237,12 +206,8 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 	amdgpu_bo_unreserve(new_rbo);
 
 	work->base = base;
-
-	r = drm_crtc_vblank_get(crtc);
-	if (r) {
-		DRM_ERROR("failed to get vblank before flip\n");
-		goto pflip_cleanup;
-	}
+	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
+		amdgpu_get_vblank_counter_kms(dev, work->crtc_id);
 
 	/* we borrow the event spin lock for protecting flip_wrok */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
@@ -250,7 +215,7 @@ int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
 		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 		r = -EBUSY;
-		goto vblank_cleanup;
+		goto pflip_cleanup;
 	}
 
 	amdgpu_crtc->pflip_status = AMDGPU_FLIP_PENDING;
@@ -262,12 +227,9 @@ 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:
-	drm_crtc_vblank_put(crtc);
-
 pflip_cleanup:
 	if (unlikely(amdgpu_bo_reserve(new_rbo, false) != 0)) {
 		DRM_ERROR("failed to reserve new rbo in error path\n");
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 6b1d7d3..4552d80 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -587,10 +587,10 @@ int amdgpu_align_pitch(struct amdgpu_device *adev, int width, int bpp, bool tile
 void amdgpu_print_display_setup(struct drm_device *dev);
 int amdgpu_modeset_create_props(struct amdgpu_device *adev);
 int amdgpu_crtc_set_config(struct drm_mode_set *set);
-int amdgpu_crtc_page_flip(struct drm_crtc *crtc,
-			  struct drm_framebuffer *fb,
-			  struct drm_pending_vblank_event *event,
-			  uint32_t page_flip_flags);
+int amdgpu_crtc_page_flip_target(struct drm_crtc *crtc,
+				 struct drm_framebuffer *fb,
+				 struct drm_pending_vblank_event *event,
+				 uint32_t page_flip_flags, uint32_t target);
 extern const struct drm_mode_config_funcs amdgpu_mode_funcs;
 
 #endif
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index c1b04e9..a158942 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2698,7 +2698,7 @@ static const struct drm_crtc_funcs dce_v10_0_crtc_funcs = {
 	.gamma_set = dce_v10_0_crtc_gamma_set,
 	.set_config = amdgpu_crtc_set_config,
 	.destroy = dce_v10_0_crtc_destroy,
-	.page_flip = amdgpu_crtc_page_flip,
+	.page_flip_target = amdgpu_crtc_page_flip_target,
 };
 
 static void dce_v10_0_crtc_dpms(struct drm_crtc *crtc, int mode)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index d4bf133..4455d63 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2708,7 +2708,7 @@ static const struct drm_crtc_funcs dce_v11_0_crtc_funcs = {
 	.gamma_set = dce_v11_0_crtc_gamma_set,
 	.set_config = amdgpu_crtc_set_config,
 	.destroy = dce_v11_0_crtc_destroy,
-	.page_flip = amdgpu_crtc_page_flip,
+	.page_flip_target = amdgpu_crtc_page_flip_target,
 };
 
 static void dce_v11_0_crtc_dpms(struct drm_crtc *crtc, int mode)
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 4fdfab1..ad4884a 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2552,7 +2552,7 @@ static const struct drm_crtc_funcs dce_v8_0_crtc_funcs = {
 	.gamma_set = dce_v8_0_crtc_gamma_set,
 	.set_config = amdgpu_crtc_set_config,
 	.destroy = dce_v8_0_crtc_destroy,
-	.page_flip = amdgpu_crtc_page_flip,
+	.page_flip_target = amdgpu_crtc_page_flip_target,
 };
 
 static void dce_v8_0_crtc_dpms(struct drm_crtc *crtc, int mode)
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/6] 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 get to
amdgpu_flip_work_func -> adev->mode_info.funcs->page_flip during a
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  | 4 ++--
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index a158942..ddf5c7a 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 4455d63..97ae822 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2250,8 +2250,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 ad4884a..9a876db 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2137,8 +2137,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

[PATCH 4/6] drm/radeon: Provide page_flip_target hook

[Michel Dänzer]

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

Now we can program a flip during a vertical blank period, if it's the
one targeted by the flip (or a later one). This allows simplifying
radeon_flip_work_func considerably.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/radeon/radeon.h         |  1 +
 drivers/gpu/drm/radeon/radeon_display.c | 89 +++++++++------------------------
 2 files changed, 25 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
index 5633ee3..1b0dcad 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -742,6 +742,7 @@ struct radeon_flip_work {
 	struct work_struct		unpin_work;
 	struct radeon_device		*rdev;
 	int				crtc_id;
+	u32				target_vblank;
 	uint64_t			base;
 	struct drm_pending_vblank_event *event;
 	struct radeon_bo		*old_rbo;
diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
index 5f1cd69..bd9d995 100644
--- a/drivers/gpu/drm/radeon/radeon_display.c
+++ b/drivers/gpu/drm/radeon/radeon_display.c
@@ -400,14 +400,13 @@ static void radeon_flip_work_func(struct work_struct *__work)
 	struct radeon_flip_work *work =
 		container_of(__work, struct radeon_flip_work, flip_work);
 	struct radeon_device *rdev = work->rdev;
+	struct drm_device *dev = rdev->ddev;
 	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[work->crtc_id];
 
 	struct drm_crtc *crtc = &radeon_crtc->base;
 	unsigned long flags;
 	int r;
-	int vpos, hpos, stat, min_udelay = 0;
-	unsigned repcnt = 4;
-	struct drm_vblank_crtc *vblank = &crtc->dev->vblank[work->crtc_id];
+	int vpos, hpos;
 
 	down_read(&rdev->exclusive_lock);
 	if (work->fence) {
@@ -438,59 +437,25 @@ static void radeon_flip_work_func(struct work_struct *__work)
 		work->fence = NULL;
 	}
 
+	/* Wait until we're out of the vertical blank period before the one
+	 * targeted by the flip
+	 */
+	while (radeon_crtc->enabled &&
+	       (radeon_get_crtc_scanoutpos(dev, work->crtc_id, 0,
+					   &vpos, &hpos, NULL, NULL,
+					   &crtc->hwmode)
+		& (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK)) ==
+	       (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK) &&
+	       (int)(work->target_vblank -
+		     dev->driver->get_vblank_counter(dev, work->crtc_id)) > 0)
+		usleep_range(1000, 2000);
+
 	/* We borrow the event spin lock for protecting flip_status */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
 
 	/* set the proper interrupt */
 	radeon_irq_kms_pflip_irq_get(rdev, radeon_crtc->crtc_id);
 
-	/* If this happens to execute within the "virtually extended" vblank
-	 * interval before the start of the real vblank interval then it needs
-	 * to delay programming the mmio flip until the real vblank is entered.
-	 * This prevents completing a flip too early due to the way we fudge
-	 * our vblank counter and vblank timestamps in order to work around the
-	 * problem that the hw fires vblank interrupts before actual start of
-	 * vblank (when line buffer refilling is done for a frame). It
-	 * complements the fudging logic in radeon_get_crtc_scanoutpos() for
-	 * timestamping and radeon_get_vblank_counter_kms() for vblank counts.
-	 *
-	 * In practice this won't execute very often unless on very fast
-	 * machines because the time window for this to happen is very small.
-	 */
-	while (radeon_crtc->enabled && --repcnt) {
-		/* GET_DISTANCE_TO_VBLANKSTART returns distance to real vblank
-		 * start in hpos, and to the "fudged earlier" vblank start in
-		 * vpos.
-		 */
-		stat = radeon_get_crtc_scanoutpos(rdev->ddev, work->crtc_id,
-						  GET_DISTANCE_TO_VBLANKSTART,
-						  &vpos, &hpos, NULL, NULL,
-						  &crtc->hwmode);
-
-		if ((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
-		    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE) ||
-		    !(vpos >= 0 && hpos <= 0))
-			break;
-
-		/* Sleep at least until estimated real start of hw vblank */
-		min_udelay = (-hpos + 1) * max(vblank->linedur_ns / 1000, 5);
-		if (min_udelay > vblank->framedur_ns / 2000) {
-			/* Don't wait ridiculously long - something is wrong */
-			repcnt = 0;
-			break;
-		}
-		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
-		usleep_range(min_udelay, 2 * min_udelay);
-		spin_lock_irqsave(&crtc->dev->event_lock, flags);
-	};
-
-	if (!repcnt)
-		DRM_DEBUG_DRIVER("Delay problem on crtc %d: min_udelay %d, "
-				 "framedur %d, linedur %d, stat %d, vpos %d, "
-				 "hpos %d\n", work->crtc_id, min_udelay,
-				 vblank->framedur_ns / 1000,
-				 vblank->linedur_ns / 1000, stat, vpos, hpos);
-
 	/* do the flip (mmio) */
 	radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base, work->async);
 
@@ -499,10 +464,11 @@ static void radeon_flip_work_func(struct work_struct *__work)
 	up_read(&rdev->exclusive_lock);
 }
 
-static int radeon_crtc_page_flip(struct drm_crtc *crtc,
-				 struct drm_framebuffer *fb,
-				 struct drm_pending_vblank_event *event,
-				 uint32_t page_flip_flags)
+static int radeon_crtc_page_flip_target(struct drm_crtc *crtc,
+					struct drm_framebuffer *fb,
+					struct drm_pending_vblank_event *event,
+					uint32_t page_flip_flags,
+					uint32_t target)
 {
 	struct drm_device *dev = crtc->dev;
 	struct radeon_device *rdev = dev->dev_private;
@@ -599,12 +565,8 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 		base &= ~7;
 	}
 	work->base = base;
-
-	r = drm_crtc_vblank_get(crtc);
-	if (r) {
-		DRM_ERROR("failed to get vblank before flip\n");
-		goto pflip_cleanup;
-	}
+	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
+		dev->driver->get_vblank_counter(dev, work->crtc_id);
 
 	/* We borrow the event spin lock for protecting flip_work */
 	spin_lock_irqsave(&crtc->dev->event_lock, flags);
@@ -613,7 +575,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
 		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
 		r = -EBUSY;
-		goto vblank_cleanup;
+		goto pflip_cleanup;
 	}
 	radeon_crtc->flip_status = RADEON_FLIP_PENDING;
 	radeon_crtc->flip_work = work;
@@ -626,9 +588,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
 	queue_work(radeon_crtc->flip_queue, &work->flip_work);
 	return 0;
 
-vblank_cleanup:
-	drm_crtc_vblank_put(crtc);
-
 pflip_cleanup:
 	if (unlikely(radeon_bo_reserve(new_rbo, false) != 0)) {
 		DRM_ERROR("failed to reserve new rbo in error path\n");
@@ -697,7 +656,7 @@ static const struct drm_crtc_funcs radeon_crtc_funcs = {
 	.gamma_set = radeon_crtc_gamma_set,
 	.set_config = radeon_crtc_set_config,
 	.destroy = radeon_crtc_destroy,
-	.page_flip = radeon_crtc_page_flip,
+	.page_flip_target = radeon_crtc_page_flip_target,
 };
 
 static void radeon_crtc_init(struct drm_device *dev, int index)
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 5/6] drm/radeon: 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 get to
radeon_flip_work_func -> adev->mode_info.funcs->page_flip during a
vertical blank period.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/radeon/atombios_crtc.c | 8 ++++----
 drivers/gpu/drm/radeon/evergreen.c     | 3 +--
 drivers/gpu/drm/radeon/rv515.c         | 3 +--
 3 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/radeon/atombios_crtc.c b/drivers/gpu/drm/radeon/atombios_crtc.c
index a97abc8..2029e35 100644
--- a/drivers/gpu/drm/radeon/atombios_crtc.c
+++ b/drivers/gpu/drm/radeon/atombios_crtc.c
@@ -1433,8 +1433,8 @@ static int dce4_crtc_do_set_base(struct drm_crtc *crtc,
 	WREG32(EVERGREEN_VIEWPORT_SIZE + radeon_crtc->crtc_offset,
 	       (viewport_w << 16) | viewport_h);
 
-	/* set pageflip to happen only at start of vblank interval (front porch) */
-	WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 3);
+	/* set pageflip to happen anywhere in vblank interval */
+	WREG32(EVERGREEN_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
 
 	if (!atomic && fb && fb != crtc->primary->fb) {
 		radeon_fb = to_radeon_framebuffer(fb);
@@ -1632,8 +1632,8 @@ static int avivo_crtc_do_set_base(struct drm_crtc *crtc,
 	WREG32(AVIVO_D1MODE_VIEWPORT_SIZE + radeon_crtc->crtc_offset,
 	       (viewport_w << 16) | viewport_h);
 
-	/* set pageflip to happen only at start of vblank interval (front porch) */
-	WREG32(AVIVO_D1MODE_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 3);
+	/* set pageflip to happen anywhere in vblank interval */
+	WREG32(AVIVO_D1MODE_MASTER_UPDATE_MODE + radeon_crtc->crtc_offset, 0);
 
 	if (!atomic && fb && fb != crtc->primary->fb) {
 		radeon_fb = to_radeon_framebuffer(fb);
diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon/evergreen.c
index db275b7..f95db0c 100644
--- a/drivers/gpu/drm/radeon/evergreen.c
+++ b/drivers/gpu/drm/radeon/evergreen.c
@@ -2878,9 +2878,8 @@ void evergreen_mc_resume(struct radeon_device *rdev, struct evergreen_mc_save *s
 	for (i = 0; i < rdev->num_crtc; i++) {
 		if (save->crtc_enabled[i]) {
 			tmp = RREG32(EVERGREEN_MASTER_UPDATE_MODE + crtc_offsets[i]);
-			if ((tmp & 0x7) != 3) {
+			if ((tmp & 0x7) != 0) {
 				tmp &= ~0x7;
-				tmp |= 0x3;
 				WREG32(EVERGREEN_MASTER_UPDATE_MODE + crtc_offsets[i], tmp);
 			}
 			tmp = RREG32(EVERGREEN_GRPH_UPDATE + crtc_offsets[i]);
diff --git a/drivers/gpu/drm/radeon/rv515.c b/drivers/gpu/drm/radeon/rv515.c
index c55d653..76c55c5 100644
--- a/drivers/gpu/drm/radeon/rv515.c
+++ b/drivers/gpu/drm/radeon/rv515.c
@@ -406,9 +406,8 @@ void rv515_mc_resume(struct radeon_device *rdev, struct rv515_mc_save *save)
 	for (i = 0; i < rdev->num_crtc; i++) {
 		if (save->crtc_enabled[i]) {
 			tmp = RREG32(AVIVO_D1MODE_MASTER_UPDATE_MODE + crtc_offsets[i]);
-			if ((tmp & 0x7) != 3) {
+			if ((tmp & 0x7) != 0) {
 				tmp &= ~0x7;
-				tmp |= 0x3;
 				WREG32(AVIVO_D1MODE_MASTER_UPDATE_MODE + crtc_offsets[i], tmp);
 			}
 			tmp = RREG32(AVIVO_D1GRPH_UPDATE + crtc_offsets[i]);
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Michel Dänzer]

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

These flags allow userspace to explicitly specify the target vertical
blank period when a flip should take effect.

Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---

Note that the previous patches in this series can avoid delaying page
flips in some cases even without this patch or any corresponding
userspace changes. Here are examples of how userspace can take advantage
of this patch to avoid delaying page flips in even more cases:

https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b

https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f

 drivers/gpu/drm/drm_crtc.c  | 46 +++++++++++++++++++++++++++++++++++++++------
 drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
 include/uapi/drm/drm.h      |  1 +
 include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++----
 4 files changed, 69 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 15ad7e2..b2fd860 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5421,11 +5421,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
-	u32 target_vblank = 0;
+	u32 target_vblank = page_flip->sequence;
 	int ret = -EINVAL;
 
-	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
-	    page_flip->reserved != 0)
+	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
+		return -EINVAL;
+
+	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
+		return -EINVAL;
+
+	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
+	 * can be specified
+	 */
+	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
 		return -EINVAL;
 
 	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
@@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -ENOENT;
 
 	if (crtc->funcs->page_flip_target) {
+		u32 current_vblank;
 		int r;
 
 		r = drm_crtc_vblank_get(crtc);
 		if (r)
 			return r;
 
-		target_vblank = drm_crtc_vblank_count(crtc) +
-			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
-	} else if (crtc->funcs->page_flip == NULL)
+		current_vblank = drm_crtc_vblank_count(crtc);
+
+		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
+		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
+			if ((int)(target_vblank - current_vblank) > 1) {
+				DRM_DEBUG("Invalid absolute flip target %u, "
+					  "must be <= %u\n", target_vblank,
+					  current_vblank + 1);
+				drm_crtc_vblank_put(crtc);
+				return -EINVAL;
+			}
+			break;
+		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
+			if (target_vblank != 0 && target_vblank != 1) {
+				DRM_DEBUG("Invalid relative flip target %u, "
+					  "must be 0 or 1\n", target_vblank);
+				drm_crtc_vblank_put(crtc);
+				return -EINVAL;
+			}
+			target_vblank += current_vblank;
+			break;
+		default:
+			target_vblank = current_vblank +
+				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
+			break;
+		}
+	} else if (crtc->funcs->page_flip == NULL ||
+		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
 		return -EINVAL;
 
 	drm_modeset_lock_crtc(crtc, crtc->primary);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 33af4a5..0099d2a 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
 static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
 	struct drm_get_cap *req = data;
+	struct drm_crtc *crtc;
 
 	req->value = 0;
 	switch (req->capability) {
@@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_ASYNC_PAGE_FLIP:
 		req->value = dev->mode_config.async_page_flip;
 		break;
+	case DRM_CAP_PAGE_FLIP_TARGET:
+		req->value = 1;
+		drm_for_each_crtc(crtc, dev) {
+			if (!crtc->funcs->page_flip_target)
+				req->value = 0;
+		}
+		break;
 	case DRM_CAP_CURSOR_WIDTH:
 		if (dev->mode_config.cursor_width)
 			req->value = dev->mode_config.cursor_width;
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 452675f..b2c5284 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -646,6 +646,7 @@ struct drm_gem_open {
 #define DRM_CAP_CURSOR_WIDTH		0x8
 #define DRM_CAP_CURSOR_HEIGHT		0x9
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
+#define DRM_CAP_PAGE_FLIP_TARGET	0x11
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 49a7265..175aa1f 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -520,7 +520,13 @@ struct drm_color_lut {
 
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
-#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
+#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
+#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
+#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
+				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
+#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
+				  DRM_MODE_PAGE_FLIP_ASYNC | \
+				  DRM_MODE_PAGE_FLIP_TARGET)
 
 /*
  * Request a page flip on the specified crtc.
@@ -543,15 +549,25 @@ struct drm_color_lut {
  * 'as soon as possible', meaning that it not delay waiting for vblank.
  * This may cause tearing on the screen.
  *
- * The reserved field must be zero until we figure out something
- * clever to use it for.
+ * The sequence field must be zero unless either of the
+ * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
+ * the ABSOLUTE flag is specified, the sequence field denotes the absolute
+ * vblank sequence when the flip should take effect. When the RELATIVE
+ * flag is specified, the sequence field denotes the relative (to the
+ * current one when the ioctl is called) vblank sequence when the flip
+ * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
+ * make sure the vblank sequence before the target one has passed before
+ * calling this ioctl. The purpose of the
+ * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
+ * the target for when code dealing with a page flip runs during a
+ * vertical blank period.
  */
 
 struct drm_mode_crtc_page_flip {
 	__u32 crtc_id;
 	__u32 fb_id;
 	__u32 flags;
-	__u32 reserved;
+	__u32 sequence;
 	__u64 user_data;
 };
 
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Christian König]

Nice, looking forward to this for quite a while now.

I'm rather busy and not so deep into the display stuff anyway, so the 
whole set is Acked-by: Christian König <christian.koenig@amd.com>.

Let me know if I should take a deeper look as well.

Regards,
Christian.

Am 04.08.2016 um 05:39 schrieb Michel Dänzer:
> The purpose of this series is to allow drivers to avoid unnecessarily
> delaying page flips, by explicitly telling the driver which vblank seqno
> a flip is supposed to take effect in.
>
> Patch 1 sets the target to the vblank seqno after the current one when
> the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called, preserving the ioctl
> contract with existing userspace.
>
> Patches 2-4 take advantage of this in the amdgpu and radeon drivers, by
> allowing a flip to be programmed and executed during the target vertical
> blank period (or a later one).
>
> Patch 6 extends the ioctl with new flags, which allow userspace to
> explicitly specify the target vblank seqno. This can also avoid delaying
> flips in some cases where we are already in the target vertical blank
> period when the ioctl is called.
>
> [PATCH 1/6] drm: Add page_flip_target CRTC hook
> [PATCH 2/6] drm/amdgpu: Provide page_flip_target hook
> [PATCH 3/6] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again
> [PATCH 4/6] drm/radeon: Provide page_flip_target hook
> [PATCH 5/6] drm/radeon: Set MASTER_UPDATE_MODE to 0 again
> [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Ville Syrjälä]

On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> These flags allow userspace to explicitly specify the target vertical
> blank period when a flip should take effect.

I like the idea. Atomic could use this too, although there we'd need to
potentially pass in a target vblank for each crtc taking part of the
operation, or I suppose you could pass it only for, say, a single crtc
in case you only want ot sync to that one.

One thing I've pondered in the past is the OML_sync_control modulo stuff.
Looks like what you have here won't free up userspace from doing the
wait_vblank+flip sequence if it wants to implement the modulo behaviour.
And of course it's still not guaranteed to honor the modulo if, for
instance, the flip ioctl itself gets blocked on a mutex for a wee bit too
long and misses the target. So should we just implement the modulo stuff
in the kernel instead?

> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> Note that the previous patches in this series can avoid delaying page
> flips in some cases even without this patch or any corresponding
> userspace changes. Here are examples of how userspace can take advantage
> of this patch to avoid delaying page flips in even more cases:
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f
> 
>  drivers/gpu/drm/drm_crtc.c  | 46 +++++++++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
>  include/uapi/drm/drm.h      |  1 +
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++----
>  4 files changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 15ad7e2..b2fd860 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,11 +5421,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> -	u32 target_vblank = 0;
> +	u32 target_vblank = page_flip->sequence;
>  	int ret = -EINVAL;
>  
> -	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> -	    page_flip->reserved != 0)
> +	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
> +		return -EINVAL;
> +
> +	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
> +		return -EINVAL;
> +
> +	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
> +	 * can be specified
> +	 */
> +	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
>  		return -EINVAL;
>  
>  	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
> @@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		return -ENOENT;
>  
>  	if (crtc->funcs->page_flip_target) {
> +		u32 current_vblank;
>  		int r;
>  
>  		r = drm_crtc_vblank_get(crtc);
>  		if (r)
>  			return r;
>  
> -		target_vblank = drm_crtc_vblank_count(crtc) +
> -			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> -	} else if (crtc->funcs->page_flip == NULL)
> +		current_vblank = drm_crtc_vblank_count(crtc);
> +
> +		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
> +		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
> +			if ((int)(target_vblank - current_vblank) > 1) {
> +				DRM_DEBUG("Invalid absolute flip target %u, "
> +					  "must be <= %u\n", target_vblank,
> +					  current_vblank + 1);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			break;
> +		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
> +			if (target_vblank != 0 && target_vblank != 1) {
> +				DRM_DEBUG("Invalid relative flip target %u, "
> +					  "must be 0 or 1\n", target_vblank);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			target_vblank += current_vblank;
> +			break;
> +		default:
> +			target_vblank = current_vblank +
> +				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +			break;
> +		}
> +	} else if (crtc->funcs->page_flip == NULL ||
> +		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
>  		return -EINVAL;
>  
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 33af4a5..0099d2a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
>  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  {
>  	struct drm_get_cap *req = data;
> +	struct drm_crtc *crtc;
>  
>  	req->value = 0;
>  	switch (req->capability) {
> @@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_ASYNC_PAGE_FLIP:
>  		req->value = dev->mode_config.async_page_flip;
>  		break;
> +	case DRM_CAP_PAGE_FLIP_TARGET:
> +		req->value = 1;
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!crtc->funcs->page_flip_target)
> +				req->value = 0;
> +		}
> +		break;
>  	case DRM_CAP_CURSOR_WIDTH:
>  		if (dev->mode_config.cursor_width)
>  			req->value = dev->mode_config.cursor_width;
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 452675f..b2c5284 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -646,6 +646,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_WIDTH		0x8
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
> +#define DRM_CAP_PAGE_FLIP_TARGET	0x11
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 49a7265..175aa1f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -520,7 +520,13 @@ struct drm_color_lut {
>  
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
> +#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> +#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
> +#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
> +				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> +#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
> +				  DRM_MODE_PAGE_FLIP_ASYNC | \
> +				  DRM_MODE_PAGE_FLIP_TARGET)
>  
>  /*
>   * Request a page flip on the specified crtc.
> @@ -543,15 +549,25 @@ struct drm_color_lut {
>   * 'as soon as possible', meaning that it not delay waiting for vblank.
>   * This may cause tearing on the screen.
>   *
> - * The reserved field must be zero until we figure out something
> - * clever to use it for.
> + * The sequence field must be zero unless either of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
> + * the ABSOLUTE flag is specified, the sequence field denotes the absolute
> + * vblank sequence when the flip should take effect. When the RELATIVE
> + * flag is specified, the sequence field denotes the relative (to the
> + * current one when the ioctl is called) vblank sequence when the flip
> + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
> + * make sure the vblank sequence before the target one has passed before
> + * calling this ioctl. The purpose of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
> + * the target for when code dealing with a page flip runs during a
> + * vertical blank period.
>   */
>  
>  struct drm_mode_crtc_page_flip {
>  	__u32 crtc_id;
>  	__u32 fb_id;
>  	__u32 flags;
> -	__u32 reserved;
> +	__u32 sequence;
>  	__u64 user_data;
>  };
>  
> -- 
> 2.8.1

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Daniel Stone]

Hi,

On 4 August 2016 at 04:39, Michel Dänzer <michel@daenzer.net> wrote:
> Patch 6 extends the ioctl with new flags, which allow userspace to
> explicitly specify the target vblank seqno. This can also avoid delaying
> flips in some cases where we are already in the target vertical blank
> period when the ioctl is called.

Is there open userspace for this? What's the behaviour vs. modeset:
does the modeset request block until the last-requested flip is
complete? If so, is there some kind of upper bound on the number of
blank periods to wait for? Is wraparound handled properly? Is all this
tested somewhere?

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

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Michel Dänzer]

On 04.08.2016 18:51, Daniel Stone wrote:
> On 4 August 2016 at 04:39, Michel Dänzer <michel@daenzer.net> wrote:
>> Patch 6 extends the ioctl with new flags, which allow userspace to
>> explicitly specify the target vblank seqno. This can also avoid delaying
>> flips in some cases where we are already in the target vertical blank
>> period when the ioctl is called.
> 
> Is there open userspace for this?

Sure, referenced in patch 6:

https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b

https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f


> What's the behaviour vs. modeset: does the modeset request block until
> the last-requested flip is complete? If so, is there some kind of upper
> bound on the number of blank periods to wait for?

Did you read the patch? :)

The only change compared to the existing ioctl is that userspace can ask
for a flip to take effect in the current vblank seqno. The code added by
the patch checks for target vblank seqno > current vblank seqno + 1 and
returns -EINVAL in that case. This is also documented in drm_mode.h.


> Is all this tested somewhere?

Yes, I've been using it for a while on all my machines.


-- 
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 0/6] drm: Explicit target vblank seqno for page flips

[Daniel Stone]

On 4 August 2016 at 11:01, Michel Dänzer <michel@daenzer.net> wrote:
> On 04.08.2016 18:51, Daniel Stone wrote:
>> On 4 August 2016 at 04:39, Michel Dänzer <michel@daenzer.net> wrote:
>>> Patch 6 extends the ioctl with new flags, which allow userspace to
>>> explicitly specify the target vblank seqno. This can also avoid delaying
>>> flips in some cases where we are already in the target vertical blank
>>> period when the ioctl is called.
>>
>> Is there open userspace for this?
>
> Sure, referenced in patch 6:
>
> https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
>
> https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f
>
>
>> What's the behaviour vs. modeset: does the modeset request block until
>> the last-requested flip is complete? If so, is there some kind of upper
>> bound on the number of blank periods to wait for?
>
> Did you read the patch? :)

Nope, for some reason my mailer decided it didn't like it, but I did
find it in the archives.

> The only change compared to the existing ioctl is that userspace can ask
> for a flip to take effect in the current vblank seqno. The code added by
> the patch checks for target vblank seqno > current vblank seqno + 1 and
> returns -EINVAL in that case. This is also documented in drm_mode.h.

Is there any particular benefit to having split absolute/relative
modes in this case? Personally I'm struggling to see the use of
relative.

>> Is all this tested somewhere?
>
> Yes, I've been using it for a while on all my machines.

I mean in a test suite. :)

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

Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Daniel Vetter]

On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> Mostly the same as the existing page_flip hook, but takes an additional
> parameter specifying the target vertical blank period when the flip
> should take effect.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++----
>  include/drm/drm_crtc.h     | 14 ++++++++++++++
>  2 files changed, 33 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9d3f80e..15ad7e2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> +	u32 target_vblank = 0;
>  	int ret = -EINVAL;
>  
>  	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	if (!crtc)
>  		return -ENOENT;
>  
> +	if (crtc->funcs->page_flip_target) {
> +		int r;
> +
> +		r = drm_crtc_vblank_get(crtc);

This leaks when e.g framebuffer_lookup fails.
-Daniel

> +		if (r)
> +			return r;
> +
> +		target_vblank = drm_crtc_vblank_count(crtc) +
> +			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +	} else if (crtc->funcs->page_flip == NULL)
> +		return -EINVAL;
> +
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
>  	if (crtc->primary->fb == NULL) {
>  		/* The framebuffer is currently unbound, presumably
> @@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		goto out;
>  	}
>  
> -	if (crtc->funcs->page_flip == NULL)
> -		goto out;
> -
>  	fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
>  	if (!fb) {
>  		ret = -ENOENT;
> @@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	}
>  
>  	crtc->primary->old_fb = crtc->primary->fb;
> -	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> +	if (crtc->funcs->page_flip_target)
> +		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> +						    page_flip->flags,
> +						    target_vblank);
> +	else
> +		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
>  	if (ret) {
>  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
>  			drm_event_cancel_free(dev, &e->base);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 9e6ab4a..ae1d9b6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
>  			 uint32_t flags);
>  
>  	/**
> +	 * @page_flip_target:
> +	 *
> +	 * Same as @page_flip but with an additional parameter specifying the
> +	 * target vertical blank period when the flip should take effect.
> +	 *
> +	 * Note that the core code calls drm_crtc_vblank_get before this entry
> +	 * point.

I think you should add "Drivers must drop that reference again by calling
drm_crtc_vblank_put()."

It's a bit icky that we have this difference (both compared to old page
flip, but also atomic doesn't grab a vblank reference either). But really
can't be avoided, at least if we implement the relative mode.
-Daniel

> +	 */
> +	int (*page_flip_target)(struct drm_crtc *crtc,
> +				struct drm_framebuffer *fb,
> +				struct drm_pending_vblank_event *event,
> +				uint32_t flags, uint32_t target);
> +
> +	/**
>  	 * @set_property:
>  	 *
>  	 * This is the legacy entry point to update a property attached to the
> -- 
> 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 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Daniel Vetter]

On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
> 
> These flags allow userspace to explicitly specify the target vertical
> blank period when a flip should take effect.
> 
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
> 
> Note that the previous patches in this series can avoid delaying page
> flips in some cases even without this patch or any corresponding
> userspace changes. Here are examples of how userspace can take advantage
> of this patch to avoid delaying page flips in even more cases:
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
> 
> https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f
> 
>  drivers/gpu/drm/drm_crtc.c  | 46 +++++++++++++++++++++++++++++++++++++++------
>  drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
>  include/uapi/drm/drm.h      |  1 +
>  include/uapi/drm/drm_mode.h | 24 +++++++++++++++++++----
>  4 files changed, 69 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 15ad7e2..b2fd860 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,11 +5421,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> -	u32 target_vblank = 0;
> +	u32 target_vblank = page_flip->sequence;
>  	int ret = -EINVAL;
>  
> -	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> -	    page_flip->reserved != 0)
> +	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
> +		return -EINVAL;
> +
> +	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
> +		return -EINVAL;
> +
> +	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
> +	 * can be specified
> +	 */
> +	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
>  		return -EINVAL;
>  
>  	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
> @@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		return -ENOENT;
>  
>  	if (crtc->funcs->page_flip_target) {
> +		u32 current_vblank;
>  		int r;
>  
>  		r = drm_crtc_vblank_get(crtc);
>  		if (r)
>  			return r;
>  
> -		target_vblank = drm_crtc_vblank_count(crtc) +
> -			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> -	} else if (crtc->funcs->page_flip == NULL)
> +		current_vblank = drm_crtc_vblank_count(crtc);
> +
> +		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
> +		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
> +			if ((int)(target_vblank - current_vblank) > 1) {
> +				DRM_DEBUG("Invalid absolute flip target %u, "
> +					  "must be <= %u\n", target_vblank,
> +					  current_vblank + 1);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			break;
> +		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
> +			if (target_vblank != 0 && target_vblank != 1) {
> +				DRM_DEBUG("Invalid relative flip target %u, "
> +					  "must be 0 or 1\n", target_vblank);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			target_vblank += current_vblank;
> +			break;
> +		default:
> +			target_vblank = current_vblank +
> +				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +			break;
> +		}
> +	} else if (crtc->funcs->page_flip == NULL ||
> +		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
>  		return -EINVAL;
>  
>  	drm_modeset_lock_crtc(crtc, crtc->primary);
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 33af4a5..0099d2a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
>  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  {
>  	struct drm_get_cap *req = data;
> +	struct drm_crtc *crtc;
>  
>  	req->value = 0;
>  	switch (req->capability) {
> @@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_ASYNC_PAGE_FLIP:
>  		req->value = dev->mode_config.async_page_flip;
>  		break;
> +	case DRM_CAP_PAGE_FLIP_TARGET:
> +		req->value = 1;
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!crtc->funcs->page_flip_target)
> +				req->value = 0;
> +		}
> +		break;
>  	case DRM_CAP_CURSOR_WIDTH:
>  		if (dev->mode_config.cursor_width)
>  			req->value = dev->mode_config.cursor_width;
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 452675f..b2c5284 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -646,6 +646,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_WIDTH		0x8
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
> +#define DRM_CAP_PAGE_FLIP_TARGET	0x11
>  
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 49a7265..175aa1f 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -520,7 +520,13 @@ struct drm_color_lut {
>  
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
> +#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> +#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
> +#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
> +				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> +#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
> +				  DRM_MODE_PAGE_FLIP_ASYNC | \
> +				  DRM_MODE_PAGE_FLIP_TARGET)
>  
>  /*
>   * Request a page flip on the specified crtc.
> @@ -543,15 +549,25 @@ struct drm_color_lut {
>   * 'as soon as possible', meaning that it not delay waiting for vblank.
>   * This may cause tearing on the screen.
>   *
> - * The reserved field must be zero until we figure out something
> - * clever to use it for.
> + * The sequence field must be zero unless either of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
> + * the ABSOLUTE flag is specified, the sequence field denotes the absolute
> + * vblank sequence when the flip should take effect. When the RELATIVE
> + * flag is specified, the sequence field denotes the relative (to the
> + * current one when the ioctl is called) vblank sequence when the flip
> + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
> + * make sure the vblank sequence before the target one has passed before
> + * calling this ioctl. The purpose of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
> + * the target for when code dealing with a page flip runs during a
> + * vertical blank period.
>   */
>  
>  struct drm_mode_crtc_page_flip {
>  	__u32 crtc_id;
>  	__u32 fb_id;
>  	__u32 flags;
> -	__u32 reserved;
> +	__u32 sequence;

Might break abi somewhere. I think it'd be better to create a
struct drm_mode_crtc_page_flip2 with the renamed field. Otherwise this
looks great, and the only other quibble I have is that we extend the old
page_flip path here instead of atomic. But given all the fun we have
trying to port i915 to atomic I fully understand that you can't simply
first port amdgpu over ;-)

With or without the above (but strongingly in support of a new struct):

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  	__u64 user_data;
>  };
>  
> -- 
> 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 1/6] drm: Add page_flip_target CRTC hook

[Daniel Vetter]

On Thu, Aug 04, 2016 at 12:47:38PM +0200, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:
> > From: Michel Dänzer <michel.daenzer@amd.com>
> > 
> > Mostly the same as the existing page_flip hook, but takes an additional
> > parameter specifying the target vertical blank period when the flip
> > should take effect.
> > 
> > Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++----
> >  include/drm/drm_crtc.h     | 14 ++++++++++++++
> >  2 files changed, 33 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index 9d3f80e..15ad7e2 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	struct drm_crtc *crtc;
> >  	struct drm_framebuffer *fb = NULL;
> >  	struct drm_pending_vblank_event *e = NULL;
> > +	u32 target_vblank = 0;
> >  	int ret = -EINVAL;
> >  
> >  	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> > @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	if (!crtc)
> >  		return -ENOENT;
> >  
> > +	if (crtc->funcs->page_flip_target) {
> > +		int r;
> > +
> > +		r = drm_crtc_vblank_get(crtc);
> 
> This leaks when e.g framebuffer_lookup fails.
> -Daniel
> 
> > +		if (r)
> > +			return r;
> > +
> > +		target_vblank = drm_crtc_vblank_count(crtc) +
> > +			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> > +	} else if (crtc->funcs->page_flip == NULL)
> > +		return -EINVAL;
> > +
> >  	drm_modeset_lock_crtc(crtc, crtc->primary);
> >  	if (crtc->primary->fb == NULL) {
> >  		/* The framebuffer is currently unbound, presumably
> > @@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  		goto out;
> >  	}
> >  
> > -	if (crtc->funcs->page_flip == NULL)
> > -		goto out;
> > -
> >  	fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
> >  	if (!fb) {
> >  		ret = -ENOENT;
> > @@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >  	}
> >  
> >  	crtc->primary->old_fb = crtc->primary->fb;
> > -	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> > +	if (crtc->funcs->page_flip_target)
> > +		ret = crtc->funcs->page_flip_target(crtc, fb, e,
> > +						    page_flip->flags,
> > +						    target_vblank);
> > +	else
> > +		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> >  	if (ret) {
> >  		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
> >  			drm_event_cancel_free(dev, &e->base);
> > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> > index 9e6ab4a..ae1d9b6 100644
> > --- a/include/drm/drm_crtc.h
> > +++ b/include/drm/drm_crtc.h
> > @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
> >  			 uint32_t flags);
> >  
> >  	/**
> > +	 * @page_flip_target:
> > +	 *
> > +	 * Same as @page_flip but with an additional parameter specifying the
> > +	 * target vertical blank period when the flip should take effect.

*absolute target vertical blank period as reported by
drm_crtc_vblank_count()

would imo be a good addition here.

> > +	 *
> > +	 * Note that the core code calls drm_crtc_vblank_get before this entry
> > +	 * point.
> 
> I think you should add "Drivers must drop that reference again by calling
> drm_crtc_vblank_put()."

Also, who should drop the reference in case ->page_flip_target fails?

> 
> It's a bit icky that we have this difference (both compared to old page
> flip, but also atomic doesn't grab a vblank reference either). But really
> can't be avoided, at least if we implement the relative mode.

With all issues addressed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
-Daniel

> -Daniel
> 
> > +	 */
> > +	int (*page_flip_target)(struct drm_crtc *crtc,
> > +				struct drm_framebuffer *fb,
> > +				struct drm_pending_vblank_event *event,
> > +				uint32_t flags, uint32_t target);
> > +
> > +	/**
> >  	 * @set_property:
> >  	 *
> >  	 * This is the legacy entry point to update a property attached to the
> > -- 
> > 2.8.1
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

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

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Ville Syrjälä]

On Thu, Aug 04, 2016 at 11:12:27AM +0100, Daniel Stone wrote:
> On 4 August 2016 at 11:01, Michel Dänzer <michel@daenzer.net> wrote:
> > On 04.08.2016 18:51, Daniel Stone wrote:
> >> On 4 August 2016 at 04:39, Michel Dänzer <michel@daenzer.net> wrote:
> >>> Patch 6 extends the ioctl with new flags, which allow userspace to
> >>> explicitly specify the target vblank seqno. This can also avoid delaying
> >>> flips in some cases where we are already in the target vertical blank
> >>> period when the ioctl is called.
> >>
> >> Is there open userspace for this?
> >
> > Sure, referenced in patch 6:
> >
> > https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
> >
> > https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f
> >
> >
> >> What's the behaviour vs. modeset: does the modeset request block until
> >> the last-requested flip is complete? If so, is there some kind of upper
> >> bound on the number of blank periods to wait for?
> >
> > Did you read the patch? :)
> 
> Nope, for some reason my mailer decided it didn't like it, but I did
> find it in the archives.
> 
> > The only change compared to the existing ioctl is that userspace can ask
> > for a flip to take effect in the current vblank seqno. The code added by
> > the patch checks for target vblank seqno > current vblank seqno + 1 and
> > returns -EINVAL in that case. This is also documented in drm_mode.h.
> 
> Is there any particular benefit to having split absolute/relative
> modes in this case? Personally I'm struggling to see the use of
> relative.

Last time I read the egl spec it specified swap interval as
RELATIVE=<interval>, whereas GLX_whatever specified the more sensible
ABSOLUTE=<last+interval> behaviour.

I suppose that means Mesa is technically not egl compliant with DRI2/3.

> 
> >> Is all this tested somewhere?
> >
> > Yes, I've been using it for a while on all my machines.
> 
> I mean in a test suite. :)
> 
> Cheers,
> Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Alex Deucher]

On Wed, Aug 3, 2016 at 11:39 PM, Michel Dänzer <michel@daenzer.net> wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> Mostly the same as the existing page_flip hook, but takes an additional
> parameter specifying the target vertical blank period when the flip
> should take effect.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 23 +++++++++++++++++++----
>  include/drm/drm_crtc.h     | 14 ++++++++++++++
>  2 files changed, 33 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 9d3f80e..15ad7e2 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         struct drm_crtc *crtc;
>         struct drm_framebuffer *fb = NULL;
>         struct drm_pending_vblank_event *e = NULL;
> +       u32 target_vblank = 0;
>         int ret = -EINVAL;
>
>         if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         if (!crtc)
>                 return -ENOENT;
>
> +       if (crtc->funcs->page_flip_target) {
> +               int r;
> +
> +               r = drm_crtc_vblank_get(crtc);
> +               if (r)
> +                       return r;
> +
> +               target_vblank = drm_crtc_vblank_count(crtc) +
> +                       !(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +       } else if (crtc->funcs->page_flip == NULL)
> +               return -EINVAL;
> +

According to kernel coding style, this last block should have parens
because the previous block did.  With that and Daniel's comments
addressed:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Alex

>         drm_modeset_lock_crtc(crtc, crtc->primary);
>         if (crtc->primary->fb == NULL) {
>                 /* The framebuffer is currently unbound, presumably
> @@ -5444,9 +5457,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>                 goto out;
>         }
>
> -       if (crtc->funcs->page_flip == NULL)
> -               goto out;
> -
>         fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
>         if (!fb) {
>                 ret = -ENOENT;
> @@ -5487,7 +5497,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>         }
>
>         crtc->primary->old_fb = crtc->primary->fb;
> -       ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
> +       if (crtc->funcs->page_flip_target)
> +               ret = crtc->funcs->page_flip_target(crtc, fb, e,
> +                                                   page_flip->flags,
> +                                                   target_vblank);
> +       else
> +               ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
>         if (ret) {
>                 if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
>                         drm_event_cancel_free(dev, &e->base);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 9e6ab4a..ae1d9b6 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
>                          uint32_t flags);
>
>         /**
> +        * @page_flip_target:
> +        *
> +        * Same as @page_flip but with an additional parameter specifying the
> +        * target vertical blank period when the flip should take effect.
> +        *
> +        * Note that the core code calls drm_crtc_vblank_get before this entry
> +        * point.
> +        */
> +       int (*page_flip_target)(struct drm_crtc *crtc,
> +                               struct drm_framebuffer *fb,
> +                               struct drm_pending_vblank_event *event,
> +                               uint32_t flags, uint32_t target);
> +
> +       /**
>          * @set_property:
>          *
>          * This is the legacy entry point to update a property attached to the
> --
> 2.8.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Alex Deucher]

On Wed, Aug 3, 2016 at 11:39 PM, Michel Dänzer <michel@daenzer.net> wrote:
> The purpose of this series is to allow drivers to avoid unnecessarily
> delaying page flips, by explicitly telling the driver which vblank seqno
> a flip is supposed to take effect in.
>
> Patch 1 sets the target to the vblank seqno after the current one when
> the DRM_IOCTL_MODE_PAGE_FLIP ioctl is called, preserving the ioctl
> contract with existing userspace.
>
> Patches 2-4 take advantage of this in the amdgpu and radeon drivers, by
> allowing a flip to be programmed and executed during the target vertical
> blank period (or a later one).
>
> Patch 6 extends the ioctl with new flags, which allow userspace to
> explicitly specify the target vblank seqno. This can also avoid delaying
> flips in some cases where we are already in the target vertical blank
> period when the ioctl is called.
>
> [PATCH 1/6] drm: Add page_flip_target CRTC hook
> [PATCH 2/6] drm/amdgpu: Provide page_flip_target hook
> [PATCH 3/6] drm/amdgpu: Set MASTER_UPDATE_MODE to 0 again
> [PATCH 4/6] drm/radeon: Provide page_flip_target hook
> [PATCH 5/6] drm/radeon: Set MASTER_UPDATE_MODE to 0 again
> [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE

Small comment on patch 1.  The rest are:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

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

Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Michel Dänzer]

On 05.08.2016 00:13, Alex Deucher wrote:
> On Wed, Aug 3, 2016 at 11:39 PM, Michel Dänzer <michel@daenzer.net> wrote:
>>
>> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>         if (!crtc)
>>                 return -ENOENT;
>>
>> +       if (crtc->funcs->page_flip_target) {
>> +               int r;
>> +
>> +               r = drm_crtc_vblank_get(crtc);
>> +               if (r)
>> +                       return r;
>> +
>> +               target_vblank = drm_crtc_vblank_count(crtc) +
>> +                       !(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
>> +       } else if (crtc->funcs->page_flip == NULL)
>> +               return -EINVAL;
>> +
> 
> According to kernel coding style, this last block should have parens
> because the previous block did.

Right, I can never remember which project wants this which way. :)


> With that and Daniel's comments addressed:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>

Thanks! I'll send out a v2 patch.


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

Re: [PATCH 0/6] drm: Explicit target vblank seqno for page flips

[Michel Dänzer]

On 04.08.2016 19:12, Daniel Stone wrote:
> On 4 August 2016 at 11:01, Michel Dänzer <michel@daenzer.net> wrote:
>> On 04.08.2016 18:51, Daniel Stone wrote:
>>> On 4 August 2016 at 04:39, Michel Dänzer <michel@daenzer.net> wrote:
>>>> Patch 6 extends the ioctl with new flags, which allow userspace to
>>>> explicitly specify the target vblank seqno. This can also avoid delaying
>>>> flips in some cases where we are already in the target vertical blank
>>>> period when the ioctl is called.
>>>
>>> Is there open userspace for this?
>>
>> Sure, referenced in patch 6:
>>
>> https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b
>>
>> https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f

[...]

>> The only change compared to the existing ioctl is that userspace can ask
>> for a flip to take effect in the current vblank seqno. The code added by
>> the patch checks for target vblank seqno > current vblank seqno + 1 and
>> returns -EINVAL in that case. This is also documented in drm_mode.h.
> 
> Is there any particular benefit to having split absolute/relative
> modes in this case? Personally I'm struggling to see the use of
> relative.

See the userspace patches above. It allows userspace to set the target
to the current/next vblank seqno without a DRM_IOCTL_WAIT_VBLANK
round-trip (which could also result in the target getting delayed by one
frame compared to using a relative target directly).


>>> Is all this tested somewhere?
>>
>> Yes, I've been using it for a while on all my machines.
> 
> I mean in a test suite. :)

Not yet. Are you thinking of intel-gpu-tools, or something else?


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

Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Michel Dänzer]

On 04.08.2016 20:01, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 12:47:38PM +0200, Daniel Vetter wrote:
>> On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:
>>>
>>> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>>>  	if (!crtc)
>>>  		return -ENOENT;
>>>  
>>> +	if (crtc->funcs->page_flip_target) {
>>> +		int r;
>>> +
>>> +		r = drm_crtc_vblank_get(crtc);
>>
>> This leaks when e.g framebuffer_lookup fails.

Good catch.


>>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
>>> index 9e6ab4a..ae1d9b6 100644
>>> --- a/include/drm/drm_crtc.h
>>> +++ b/include/drm/drm_crtc.h
>>> @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
>>>  			 uint32_t flags);
>>>  
>>>  	/**
>>> +	 * @page_flip_target:
>>> +	 *
>>> +	 * Same as @page_flip but with an additional parameter specifying the
>>> +	 * target vertical blank period when the flip should take effect.
> 
> *absolute target vertical blank period as reported by
> drm_crtc_vblank_count()
> 
> would imo be a good addition here.

[...]

>>> +	 *
>>> +	 * Note that the core code calls drm_crtc_vblank_get before this entry
>>> +	 * point.
>>
>> I think you should add "Drivers must drop that reference again by calling
>> drm_crtc_vblank_put()."

Thanks for the suggestions.


> Also, who should drop the reference in case ->page_flip_target fails?

The core DRM code.


> With all issues addressed:
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks! I'll send out a v2 patch with your and Alex's feedback
addressed. Will it be okay to merge this via Alex's tree?


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

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Michel Dänzer]

On 04.08.2016 19:56, Daniel Vetter wrote:
> On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> @@ -543,15 +549,25 @@ struct drm_color_lut {
>>   * 'as soon as possible', meaning that it not delay waiting for vblank.
>>   * This may cause tearing on the screen.
>>   *
>> - * The reserved field must be zero until we figure out something
>> - * clever to use it for.
>> + * The sequence field must be zero unless either of the
>> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
>> + * the ABSOLUTE flag is specified, the sequence field denotes the absolute
>> + * vblank sequence when the flip should take effect. When the RELATIVE
>> + * flag is specified, the sequence field denotes the relative (to the
>> + * current one when the ioctl is called) vblank sequence when the flip
>> + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
>> + * make sure the vblank sequence before the target one has passed before
>> + * calling this ioctl. The purpose of the
>> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
>> + * the target for when code dealing with a page flip runs during a
>> + * vertical blank period.
>>   */
>>  
>>  struct drm_mode_crtc_page_flip {
>>  	__u32 crtc_id;
>>  	__u32 fb_id;
>>  	__u32 flags;
>> -	__u32 reserved;
>> +	__u32 sequence;
> 
> Might break abi somewhere. I think it'd be better to create a
> struct drm_mode_crtc_page_flip2 with the renamed field.

It doesn't break ABI. I guess you mean there might be userspace code
referencing the reserved field? Such code would have to be open-coding
drmModePageFlip instead of using it. And it turns out you're right, e.g.
SNA actually does that and would fail to compile... Will be fixed in v2.


> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Thanks! Will it be okay to merge this via Alex's tree?


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

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags

[Michel Dänzer]

On 04.08.2016 16:42, Ville Syrjälä wrote:
> On Thu, Aug 04, 2016 at 12:39:41PM +0900, Michel Dänzer wrote:
>> From: Michel Dänzer <michel.daenzer@amd.com>
>>
>> These flags allow userspace to explicitly specify the target vertical
>> blank period when a flip should take effect.
> 
> I like the idea. Atomic could use this too, although there we'd need to
> potentially pass in a target vblank for each crtc taking part of the
> operation, or I suppose you could pass it only for, say, a single crtc
> in case you only want ot sync to that one.

Yes, userspace tends to just synchronize to one CRTC and doesn't
particularly care about any others performing the same flip.


> One thing I've pondered in the past is the OML_sync_control modulo stuff.
> Looks like what you have here won't free up userspace from doing the
> wait_vblank+flip sequence if it wants to implement the modulo behaviour.
> And of course it's still not guaranteed to honor the modulo if, for
> instance, the flip ioctl itself gets blocked on a mutex for a wee bit too
> long and misses the target. So should we just implement the modulo stuff
> in the kernel instead?

That might be nice, but I'm afraid it would be rather more complex than
this patch for various reasons. E.g., in no particular order:

The modulo can't be passed to the ioctl without extending the struct
used by the ioctl, which could be tricky to get right in all corner cases.

The current driver interface of the Present extension code in xserver
wouldn't allow making use of it, the xserver code already handles the
modulo before calling into the driver.

The kernel driver interface would have to change significantly as well,
it would be basically the driver's responsibility to handle the modulo.
It would also be rather tricky to make it work reliably with our
hardware, because we can't reliably determine in advance when a flip
will take effect in the hardware.


Overall, it seems like too much effort for too little gain with the
legacy KMS API. Might be worth tackling for atomic though.


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

[PATCH 1/6] drm: Add page_flip_target CRTC hook v2

[Michel Dänzer]

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

Mostly the same as the existing page_flip hook, but takes an additional
parameter specifying the target vertical blank period when the flip
should take effect.

v2:
* Add curly braces around else statement corresponding to an if block
  with curly braces (Alex Deucher)
* Call drm_crtc_vblank_put in the error case (Daniel Vetter)
* Clarify entry point documentation comment (Daniel Vetter)

Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_crtc.c | 26 ++++++++++++++++++++++----
 include/drm/drm_crtc.h     | 18 ++++++++++++++++++
 2 files changed, 40 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 9d3f80e..d6491ef 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5421,6 +5421,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
+	u32 target_vblank = 0;
 	int ret = -EINVAL;
 
 	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
@@ -5434,6 +5435,19 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	if (!crtc)
 		return -ENOENT;
 
+	if (crtc->funcs->page_flip_target) {
+		int r;
+
+		r = drm_crtc_vblank_get(crtc);
+		if (r)
+			return r;
+
+		target_vblank = drm_crtc_vblank_count(crtc) +
+			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
+	} else if (crtc->funcs->page_flip == NULL) {
+		return -EINVAL;
+	}
+
 	drm_modeset_lock_crtc(crtc, crtc->primary);
 	if (crtc->primary->fb == NULL) {
 		/* The framebuffer is currently unbound, presumably
@@ -5444,9 +5458,6 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		goto out;
 	}
 
-	if (crtc->funcs->page_flip == NULL)
-		goto out;
-
 	fb = drm_framebuffer_lookup(dev, page_flip->fb_id);
 	if (!fb) {
 		ret = -ENOENT;
@@ -5487,7 +5498,12 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 	crtc->primary->old_fb = crtc->primary->fb;
-	ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
+	if (crtc->funcs->page_flip_target)
+		ret = crtc->funcs->page_flip_target(crtc, fb, e,
+						    page_flip->flags,
+						    target_vblank);
+	else
+		ret = crtc->funcs->page_flip(crtc, fb, e, page_flip->flags);
 	if (ret) {
 		if (page_flip->flags & DRM_MODE_PAGE_FLIP_EVENT)
 			drm_event_cancel_free(dev, &e->base);
@@ -5500,6 +5516,8 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 	}
 
 out:
+	if (ret)
+		drm_crtc_vblank_put(crtc);
 	if (fb)
 		drm_framebuffer_unreference(fb);
 	if (crtc->primary->old_fb)
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 9e6ab4a..1eaf2e1 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -579,6 +579,24 @@ struct drm_crtc_funcs {
 			 uint32_t flags);
 
 	/**
+	 * @page_flip_target:
+	 *
+	 * Same as @page_flip but with an additional parameter specifying the
+	 * absolute target vertical blank period (as reported by
+	 * drm_crtc_vblank_count()) when the flip should take effect.
+	 *
+	 * Note that the core code calls drm_crtc_vblank_get before this entry
+	 * point, and will call drm_crtc_vblank_put if this entry point returns
+	 * any non-0 error code. It's the driver's responsibility to call
+	 * drm_crtc_vblank_put after this entry point returns 0, typically when
+	 * the flip completes.
+	 */
+	int (*page_flip_target)(struct drm_crtc *crtc,
+				struct drm_framebuffer *fb,
+				struct drm_pending_vblank_event *event,
+				uint32_t flags, uint32_t target);
+
+	/**
 	 * @set_property:
 	 *
 	 * This is the legacy entry point to update a property attached to the
-- 
2.8.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2

[Michel Dänzer]

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

These flags allow userspace to explicitly specify the target vertical
blank period when a flip should take effect.

v2:
* Add new struct drm_mode_crtc_page_flip_target instead of modifying
  struct drm_mode_crtc_page_flip, to make sure all existing userspace
  code keeps compiling (Daniel Vetter)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
---
 drivers/gpu/drm/drm_crtc.c  | 48 ++++++++++++++++++++++++++++++++++++++-------
 drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
 include/uapi/drm/drm.h      |  1 +
 include/uapi/drm/drm_mode.h | 39 +++++++++++++++++++++++++++++++++---
 4 files changed, 86 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index d6491ef..3e1a63d 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -5417,15 +5417,23 @@ out:
 int drm_mode_page_flip_ioctl(struct drm_device *dev,
 			     void *data, struct drm_file *file_priv)
 {
-	struct drm_mode_crtc_page_flip *page_flip = data;
+	struct drm_mode_crtc_page_flip_target *page_flip = data;
 	struct drm_crtc *crtc;
 	struct drm_framebuffer *fb = NULL;
 	struct drm_pending_vblank_event *e = NULL;
-	u32 target_vblank = 0;
+	u32 target_vblank = page_flip->sequence;
 	int ret = -EINVAL;
 
-	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
-	    page_flip->reserved != 0)
+	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
+		return -EINVAL;
+
+	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
+		return -EINVAL;
+
+	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
+	 * can be specified
+	 */
+	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
 		return -EINVAL;
 
 	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
@@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
 		return -ENOENT;
 
 	if (crtc->funcs->page_flip_target) {
+		u32 current_vblank;
 		int r;
 
 		r = drm_crtc_vblank_get(crtc);
 		if (r)
 			return r;
 
-		target_vblank = drm_crtc_vblank_count(crtc) +
-			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
-	} else if (crtc->funcs->page_flip == NULL) {
+		current_vblank = drm_crtc_vblank_count(crtc);
+
+		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
+		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
+			if ((int)(target_vblank - current_vblank) > 1) {
+				DRM_DEBUG("Invalid absolute flip target %u, "
+					  "must be <= %u\n", target_vblank,
+					  current_vblank + 1);
+				drm_crtc_vblank_put(crtc);
+				return -EINVAL;
+			}
+			break;
+		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
+			if (target_vblank != 0 && target_vblank != 1) {
+				DRM_DEBUG("Invalid relative flip target %u, "
+					  "must be 0 or 1\n", target_vblank);
+				drm_crtc_vblank_put(crtc);
+				return -EINVAL;
+			}
+			target_vblank += current_vblank;
+			break;
+		default:
+			target_vblank = current_vblank +
+				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
+			break;
+		}
+	} else if (crtc->funcs->page_flip == NULL ||
+		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) {
 		return -EINVAL;
 	}
 
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 33af4a5..0099d2a 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
 static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
 {
 	struct drm_get_cap *req = data;
+	struct drm_crtc *crtc;
 
 	req->value = 0;
 	switch (req->capability) {
@@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
 	case DRM_CAP_ASYNC_PAGE_FLIP:
 		req->value = dev->mode_config.async_page_flip;
 		break;
+	case DRM_CAP_PAGE_FLIP_TARGET:
+		req->value = 1;
+		drm_for_each_crtc(crtc, dev) {
+			if (!crtc->funcs->page_flip_target)
+				req->value = 0;
+		}
+		break;
 	case DRM_CAP_CURSOR_WIDTH:
 		if (dev->mode_config.cursor_width)
 			req->value = dev->mode_config.cursor_width;
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 452675f..b2c5284 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -646,6 +646,7 @@ struct drm_gem_open {
 #define DRM_CAP_CURSOR_WIDTH		0x8
 #define DRM_CAP_CURSOR_HEIGHT		0x9
 #define DRM_CAP_ADDFB2_MODIFIERS	0x10
+#define DRM_CAP_PAGE_FLIP_TARGET	0x11
 
 /** DRM_IOCTL_GET_CAP ioctl argument type */
 struct drm_get_cap {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 49a7265..df0e350 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -520,7 +520,13 @@ struct drm_color_lut {
 
 #define DRM_MODE_PAGE_FLIP_EVENT 0x01
 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
-#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
+#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
+#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
+#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
+				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
+#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
+				  DRM_MODE_PAGE_FLIP_ASYNC | \
+				  DRM_MODE_PAGE_FLIP_TARGET)
 
 /*
  * Request a page flip on the specified crtc.
@@ -543,8 +549,7 @@ struct drm_color_lut {
  * 'as soon as possible', meaning that it not delay waiting for vblank.
  * This may cause tearing on the screen.
  *
- * The reserved field must be zero until we figure out something
- * clever to use it for.
+ * The reserved field must be zero.
  */
 
 struct drm_mode_crtc_page_flip {
@@ -555,6 +560,34 @@ struct drm_mode_crtc_page_flip {
 	__u64 user_data;
 };
 
+/*
+ * Request a page flip on the specified crtc.
+ *
+ * Same as struct drm_mode_crtc_page_flip, but supports new flags and
+ * re-purposes the reserved field:
+ *
+ * The sequence field must be zero unless either of the
+ * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
+ * the ABSOLUTE flag is specified, the sequence field denotes the absolute
+ * vblank sequence when the flip should take effect. When the RELATIVE
+ * flag is specified, the sequence field denotes the relative (to the
+ * current one when the ioctl is called) vblank sequence when the flip
+ * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
+ * make sure the vblank sequence before the target one has passed before
+ * calling this ioctl. The purpose of the
+ * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
+ * the target for when code dealing with a page flip runs during a
+ * vertical blank period.
+ */
+
+struct drm_mode_crtc_page_flip_target {
+	__u32 crtc_id;
+	__u32 fb_id;
+	__u32 flags;
+	__u32 sequence;
+	__u64 user_data;
+};
+
 /* create a dumb scanout buffer */
 struct drm_mode_create_dumb {
 	__u32 height;
-- 
2.8.1

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

Re: [PATCH 1/6] drm: Add page_flip_target CRTC hook

[Daniel Vetter]

On Mon, Aug 08, 2016 at 12:54:45PM +0900, Michel Dänzer wrote:
> On 04.08.2016 20:01, Daniel Vetter wrote:
> > On Thu, Aug 04, 2016 at 12:47:38PM +0200, Daniel Vetter wrote:
> >> On Thu, Aug 04, 2016 at 12:39:36PM +0900, Michel Dänzer wrote:
> >>>
> >>> @@ -5434,6 +5435,18 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
> >>>  	if (!crtc)
> >>>  		return -ENOENT;
> >>>  
> >>> +	if (crtc->funcs->page_flip_target) {
> >>> +		int r;
> >>> +
> >>> +		r = drm_crtc_vblank_get(crtc);
> >>
> >> This leaks when e.g framebuffer_lookup fails.
> 
> Good catch.
> 
> 
> >>> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> >>> index 9e6ab4a..ae1d9b6 100644
> >>> --- a/include/drm/drm_crtc.h
> >>> +++ b/include/drm/drm_crtc.h
> >>> @@ -579,6 +579,20 @@ struct drm_crtc_funcs {
> >>>  			 uint32_t flags);
> >>>  
> >>>  	/**
> >>> +	 * @page_flip_target:
> >>> +	 *
> >>> +	 * Same as @page_flip but with an additional parameter specifying the
> >>> +	 * target vertical blank period when the flip should take effect.
> > 
> > *absolute target vertical blank period as reported by
> > drm_crtc_vblank_count()
> > 
> > would imo be a good addition here.
> 
> [...]
> 
> >>> +	 *
> >>> +	 * Note that the core code calls drm_crtc_vblank_get before this entry
> >>> +	 * point.
> >>
> >> I think you should add "Drivers must drop that reference again by calling
> >> drm_crtc_vblank_put()."
> 
> Thanks for the suggestions.
> 
> 
> > Also, who should drop the reference in case ->page_flip_target fails?
> 
> The core DRM code.

Please add that to the kerneldoc, too, when the code is fixed.
 
> > With all issues addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> Thanks! I'll send out a v2 patch with your and Alex's feedback
> addressed. Will it be okay to merge this via Alex's tree?

Sure, ack on merging through amdgpu. Would be good to then send an earlier
pull request with it to avoid conflicts when it's too long outside of
drm-next.
-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/6] drm/radeon: Provide page_flip_target hook

[Mario Kleiner]

Hi Michel,

sorry for the super-late reply, i was just catching up with all the 
mails and discussions, starting in June, leading to this patch set.

Looks all pretty good.

I'll look at this radeon patch and 2/6 for amdgpu later this week when i 
have a fresh brain and enough "obsessive compulsive time", to make sure 
all the magic wrt. "virtually extended vblank" and the fudging logic is 
fine.

I'll then also run it through my timing tests. I assume the 
ati/amdgpu-ddx patches and libdrm patches in your freedesktop home are 
all i need for testing?

-mario


On 08/04/2016 05:39 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> Now we can program a flip during a vertical blank period, if it's the
> one targeted by the flip (or a later one). This allows simplifying
> radeon_flip_work_func considerably.
>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/radeon/radeon.h         |  1 +
>  drivers/gpu/drm/radeon/radeon_display.c | 89 +++++++++------------------------
>  2 files changed, 25 insertions(+), 65 deletions(-)
>
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 5633ee3..1b0dcad 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -742,6 +742,7 @@ struct radeon_flip_work {
>  	struct work_struct		unpin_work;
>  	struct radeon_device		*rdev;
>  	int				crtc_id;
> +	u32				target_vblank;
>  	uint64_t			base;
>  	struct drm_pending_vblank_event *event;
>  	struct radeon_bo		*old_rbo;
> diff --git a/drivers/gpu/drm/radeon/radeon_display.c b/drivers/gpu/drm/radeon/radeon_display.c
> index 5f1cd69..bd9d995 100644
> --- a/drivers/gpu/drm/radeon/radeon_display.c
> +++ b/drivers/gpu/drm/radeon/radeon_display.c
> @@ -400,14 +400,13 @@ static void radeon_flip_work_func(struct work_struct *__work)
>  	struct radeon_flip_work *work =
>  		container_of(__work, struct radeon_flip_work, flip_work);
>  	struct radeon_device *rdev = work->rdev;
> +	struct drm_device *dev = rdev->ddev;
>  	struct radeon_crtc *radeon_crtc = rdev->mode_info.crtcs[work->crtc_id];
>
>  	struct drm_crtc *crtc = &radeon_crtc->base;
>  	unsigned long flags;
>  	int r;
> -	int vpos, hpos, stat, min_udelay = 0;
> -	unsigned repcnt = 4;
> -	struct drm_vblank_crtc *vblank = &crtc->dev->vblank[work->crtc_id];
> +	int vpos, hpos;
>
>  	down_read(&rdev->exclusive_lock);
>  	if (work->fence) {
> @@ -438,59 +437,25 @@ static void radeon_flip_work_func(struct work_struct *__work)
>  		work->fence = NULL;
>  	}
>
> +	/* Wait until we're out of the vertical blank period before the one
> +	 * targeted by the flip
> +	 */
> +	while (radeon_crtc->enabled &&
> +	       (radeon_get_crtc_scanoutpos(dev, work->crtc_id, 0,
> +					   &vpos, &hpos, NULL, NULL,
> +					   &crtc->hwmode)
> +		& (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK)) ==
> +	       (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_IN_VBLANK) &&
> +	       (int)(work->target_vblank -
> +		     dev->driver->get_vblank_counter(dev, work->crtc_id)) > 0)
> +		usleep_range(1000, 2000);
> +
>  	/* We borrow the event spin lock for protecting flip_status */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
>
>  	/* set the proper interrupt */
>  	radeon_irq_kms_pflip_irq_get(rdev, radeon_crtc->crtc_id);
>
> -	/* If this happens to execute within the "virtually extended" vblank
> -	 * interval before the start of the real vblank interval then it needs
> -	 * to delay programming the mmio flip until the real vblank is entered.
> -	 * This prevents completing a flip too early due to the way we fudge
> -	 * our vblank counter and vblank timestamps in order to work around the
> -	 * problem that the hw fires vblank interrupts before actual start of
> -	 * vblank (when line buffer refilling is done for a frame). It
> -	 * complements the fudging logic in radeon_get_crtc_scanoutpos() for
> -	 * timestamping and radeon_get_vblank_counter_kms() for vblank counts.
> -	 *
> -	 * In practice this won't execute very often unless on very fast
> -	 * machines because the time window for this to happen is very small.
> -	 */
> -	while (radeon_crtc->enabled && --repcnt) {
> -		/* GET_DISTANCE_TO_VBLANKSTART returns distance to real vblank
> -		 * start in hpos, and to the "fudged earlier" vblank start in
> -		 * vpos.
> -		 */
> -		stat = radeon_get_crtc_scanoutpos(rdev->ddev, work->crtc_id,
> -						  GET_DISTANCE_TO_VBLANKSTART,
> -						  &vpos, &hpos, NULL, NULL,
> -						  &crtc->hwmode);
> -
> -		if ((stat & (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE)) !=
> -		    (DRM_SCANOUTPOS_VALID | DRM_SCANOUTPOS_ACCURATE) ||
> -		    !(vpos >= 0 && hpos <= 0))
> -			break;
> -
> -		/* Sleep at least until estimated real start of hw vblank */
> -		min_udelay = (-hpos + 1) * max(vblank->linedur_ns / 1000, 5);
> -		if (min_udelay > vblank->framedur_ns / 2000) {
> -			/* Don't wait ridiculously long - something is wrong */
> -			repcnt = 0;
> -			break;
> -		}
> -		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> -		usleep_range(min_udelay, 2 * min_udelay);
> -		spin_lock_irqsave(&crtc->dev->event_lock, flags);
> -	};
> -
> -	if (!repcnt)
> -		DRM_DEBUG_DRIVER("Delay problem on crtc %d: min_udelay %d, "
> -				 "framedur %d, linedur %d, stat %d, vpos %d, "
> -				 "hpos %d\n", work->crtc_id, min_udelay,
> -				 vblank->framedur_ns / 1000,
> -				 vblank->linedur_ns / 1000, stat, vpos, hpos);
> -
>  	/* do the flip (mmio) */
>  	radeon_page_flip(rdev, radeon_crtc->crtc_id, work->base, work->async);
>
> @@ -499,10 +464,11 @@ static void radeon_flip_work_func(struct work_struct *__work)
>  	up_read(&rdev->exclusive_lock);
>  }
>
> -static int radeon_crtc_page_flip(struct drm_crtc *crtc,
> -				 struct drm_framebuffer *fb,
> -				 struct drm_pending_vblank_event *event,
> -				 uint32_t page_flip_flags)
> +static int radeon_crtc_page_flip_target(struct drm_crtc *crtc,
> +					struct drm_framebuffer *fb,
> +					struct drm_pending_vblank_event *event,
> +					uint32_t page_flip_flags,
> +					uint32_t target)
>  {
>  	struct drm_device *dev = crtc->dev;
>  	struct radeon_device *rdev = dev->dev_private;
> @@ -599,12 +565,8 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  		base &= ~7;
>  	}
>  	work->base = base;
> -
> -	r = drm_crtc_vblank_get(crtc);
> -	if (r) {
> -		DRM_ERROR("failed to get vblank before flip\n");
> -		goto pflip_cleanup;
> -	}
> +	work->target_vblank = target - drm_crtc_vblank_count(crtc) +
> +		dev->driver->get_vblank_counter(dev, work->crtc_id);
>
>  	/* We borrow the event spin lock for protecting flip_work */
>  	spin_lock_irqsave(&crtc->dev->event_lock, flags);
> @@ -613,7 +575,7 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  		DRM_DEBUG_DRIVER("flip queue: crtc already busy\n");
>  		spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
>  		r = -EBUSY;
> -		goto vblank_cleanup;
> +		goto pflip_cleanup;
>  	}
>  	radeon_crtc->flip_status = RADEON_FLIP_PENDING;
>  	radeon_crtc->flip_work = work;
> @@ -626,9 +588,6 @@ static int radeon_crtc_page_flip(struct drm_crtc *crtc,
>  	queue_work(radeon_crtc->flip_queue, &work->flip_work);
>  	return 0;
>
> -vblank_cleanup:
> -	drm_crtc_vblank_put(crtc);
> -
>  pflip_cleanup:
>  	if (unlikely(radeon_bo_reserve(new_rbo, false) != 0)) {
>  		DRM_ERROR("failed to reserve new rbo in error path\n");
> @@ -697,7 +656,7 @@ static const struct drm_crtc_funcs radeon_crtc_funcs = {
>  	.gamma_set = radeon_crtc_gamma_set,
>  	.set_config = radeon_crtc_set_config,
>  	.destroy = radeon_crtc_destroy,
> -	.page_flip = radeon_crtc_page_flip,
> +	.page_flip_target = radeon_crtc_page_flip_target,
>  };
>
>  static void radeon_crtc_init(struct drm_device *dev, int index)
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2

[Mario Kleiner]

Cc'ing Daniel Stone and Pekka Paalanen, because this relates to wayland.

Wrt. having a new pageflip parameter struct, i wonder if it wouldn't 
make sense to then already prepare some space in it for specifying an 
absolute target time, e.g., in u64 microseconds? Or make this part of 
the atomic pageflip framework?

In Wayland we now have the presentation_feedback extension (maybe it got 
a new name?), which is great to get feedback when and how presentation 
was completed, essentially the equivalent of X11's GLX_INTEL_swap_events 
+ some useful extra info / the feedback half of OML_sync_control.

That was supposed to be complemented by a presentation_queue extension 
which would provide the other half of OML_sync_control, the part where 
an app can tell the compositor when and how to present surface updates. 
I remember that a year ago inclusion of that extension was blocked on 
some other more urgent important blocker bugs? Did this make progress?
For timing sensitive applications such an extension is even more 
important in a wayland world than under X11. On X11 most desktops allow 
unredirecting fullscreen windows, so an app can drive the flip timing 
rather direct. At least on Weston as i remember it from my last tests a 
year ago, that wasn't possible, and an app that doesn't want to present 
at each video refresh, but at specific target times had to guess what 
the specific compositors scheduling strategy might be and then "play 
games" wrt. to timing surface commit's to trick the compositor into sort 
of doing the right thing - very fragile.

Anyway, the idea of presentation_queue was to specify the requested time 
of presentation as actual time, not as a target vblank count. This 
because applications that care about precise presentation timing, like 
my kind of neuroscience/medical visual stimulation software, or also 
video players, or e.g., at least the VDPAU video presentation api 
"think" in absolute time, not in refresh cycles. Also a target vblank 
count for presentation is less meaningful than a target time for things 
like variable framerate displays/adaptive sync, or displays which don't 
have a classic refresh cycle at all. It might also be useful if one 
thinks about something like VR compositors, where precise timing control 
could help for some of the tricks ("time warping" iirc?) they use to 
hide/cope with latency from head tracking -> display.

It would be nice if the kernel could help compositors which would 
implement presentation_queue or similar to be robust/precise in face of 
future stuff like Freesync, or of added delays due to Optimus/Prime 
hybrid-graphics setups. If we wanted to synchronize presentation of 
multiple displays in a Freesync type display setup, absolute target 
times could also be helpful.

-mario

On 08/08/2016 09:23 AM, Michel Dänzer wrote:
> From: Michel Dänzer <michel.daenzer@amd.com>
>
> These flags allow userspace to explicitly specify the target vertical
> blank period when a flip should take effect.
>
> v2:
> * Add new struct drm_mode_crtc_page_flip_target instead of modifying
>   struct drm_mode_crtc_page_flip, to make sure all existing userspace
>   code keeps compiling (Daniel Vetter)
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Michel Dänzer <michel.daenzer@amd.com>
> ---
>  drivers/gpu/drm/drm_crtc.c  | 48 ++++++++++++++++++++++++++++++++++++++-------
>  drivers/gpu/drm/drm_ioctl.c |  8 ++++++++
>  include/uapi/drm/drm.h      |  1 +
>  include/uapi/drm/drm_mode.h | 39 +++++++++++++++++++++++++++++++++---
>  4 files changed, 86 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index d6491ef..3e1a63d 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -5417,15 +5417,23 @@ out:
>  int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  			     void *data, struct drm_file *file_priv)
>  {
> -	struct drm_mode_crtc_page_flip *page_flip = data;
> +	struct drm_mode_crtc_page_flip_target *page_flip = data;
>  	struct drm_crtc *crtc;
>  	struct drm_framebuffer *fb = NULL;
>  	struct drm_pending_vblank_event *e = NULL;
> -	u32 target_vblank = 0;
> +	u32 target_vblank = page_flip->sequence;
>  	int ret = -EINVAL;
>
> -	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS ||
> -	    page_flip->reserved != 0)
> +	if (page_flip->flags & ~DRM_MODE_PAGE_FLIP_FLAGS)
> +		return -EINVAL;
> +
> +	if (page_flip->sequence != 0 && !(page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET))
> +		return -EINVAL;
> +
> +	/* Only one of the DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags
> +	 * can be specified
> +	 */
> +	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) == DRM_MODE_PAGE_FLIP_TARGET)
>  		return -EINVAL;
>
>  	if ((page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC) && !dev->mode_config.async_page_flip)
> @@ -5436,15 +5444,41 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev,
>  		return -ENOENT;
>
>  	if (crtc->funcs->page_flip_target) {
> +		u32 current_vblank;
>  		int r;
>
>  		r = drm_crtc_vblank_get(crtc);
>  		if (r)
>  			return r;
>
> -		target_vblank = drm_crtc_vblank_count(crtc) +
> -			!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> -	} else if (crtc->funcs->page_flip == NULL) {
> +		current_vblank = drm_crtc_vblank_count(crtc);
> +
> +		switch (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET) {
> +		case DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE:
> +			if ((int)(target_vblank - current_vblank) > 1) {
> +				DRM_DEBUG("Invalid absolute flip target %u, "
> +					  "must be <= %u\n", target_vblank,
> +					  current_vblank + 1);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			break;
> +		case DRM_MODE_PAGE_FLIP_TARGET_RELATIVE:
> +			if (target_vblank != 0 && target_vblank != 1) {
> +				DRM_DEBUG("Invalid relative flip target %u, "
> +					  "must be 0 or 1\n", target_vblank);
> +				drm_crtc_vblank_put(crtc);
> +				return -EINVAL;
> +			}
> +			target_vblank += current_vblank;
> +			break;
> +		default:
> +			target_vblank = current_vblank +
> +				!(page_flip->flags & DRM_MODE_PAGE_FLIP_ASYNC);
> +			break;
> +		}
> +	} else if (crtc->funcs->page_flip == NULL ||
> +		   (page_flip->flags & DRM_MODE_PAGE_FLIP_TARGET)) {
>  		return -EINVAL;
>  	}
>
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index 33af4a5..0099d2a 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -228,6 +228,7 @@ static int drm_getstats(struct drm_device *dev, void *data,
>  static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_priv)
>  {
>  	struct drm_get_cap *req = data;
> +	struct drm_crtc *crtc;
>
>  	req->value = 0;
>  	switch (req->capability) {
> @@ -254,6 +255,13 @@ static int drm_getcap(struct drm_device *dev, void *data, struct drm_file *file_
>  	case DRM_CAP_ASYNC_PAGE_FLIP:
>  		req->value = dev->mode_config.async_page_flip;
>  		break;
> +	case DRM_CAP_PAGE_FLIP_TARGET:
> +		req->value = 1;
> +		drm_for_each_crtc(crtc, dev) {
> +			if (!crtc->funcs->page_flip_target)
> +				req->value = 0;
> +		}
> +		break;
>  	case DRM_CAP_CURSOR_WIDTH:
>  		if (dev->mode_config.cursor_width)
>  			req->value = dev->mode_config.cursor_width;
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 452675f..b2c5284 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -646,6 +646,7 @@ struct drm_gem_open {
>  #define DRM_CAP_CURSOR_WIDTH		0x8
>  #define DRM_CAP_CURSOR_HEIGHT		0x9
>  #define DRM_CAP_ADDFB2_MODIFIERS	0x10
> +#define DRM_CAP_PAGE_FLIP_TARGET	0x11
>
>  /** DRM_IOCTL_GET_CAP ioctl argument type */
>  struct drm_get_cap {
> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
> index 49a7265..df0e350 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -520,7 +520,13 @@ struct drm_color_lut {
>
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
> -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC)
> +#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4
> +#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8
> +#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \
> +				   DRM_MODE_PAGE_FLIP_TARGET_RELATIVE)
> +#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \
> +				  DRM_MODE_PAGE_FLIP_ASYNC | \
> +				  DRM_MODE_PAGE_FLIP_TARGET)
>
>  /*
>   * Request a page flip on the specified crtc.
> @@ -543,8 +549,7 @@ struct drm_color_lut {
>   * 'as soon as possible', meaning that it not delay waiting for vblank.
>   * This may cause tearing on the screen.
>   *
> - * The reserved field must be zero until we figure out something
> - * clever to use it for.
> + * The reserved field must be zero.
>   */
>
>  struct drm_mode_crtc_page_flip {
> @@ -555,6 +560,34 @@ struct drm_mode_crtc_page_flip {
>  	__u64 user_data;
>  };
>
> +/*
> + * Request a page flip on the specified crtc.
> + *
> + * Same as struct drm_mode_crtc_page_flip, but supports new flags and
> + * re-purposes the reserved field:
> + *
> + * The sequence field must be zero unless either of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When
> + * the ABSOLUTE flag is specified, the sequence field denotes the absolute
> + * vblank sequence when the flip should take effect. When the RELATIVE
> + * flag is specified, the sequence field denotes the relative (to the
> + * current one when the ioctl is called) vblank sequence when the flip
> + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to
> + * make sure the vblank sequence before the target one has passed before
> + * calling this ioctl. The purpose of the
> + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify
> + * the target for when code dealing with a page flip runs during a
> + * vertical blank period.
> + */
> +
> +struct drm_mode_crtc_page_flip_target {
> +	__u32 crtc_id;
> +	__u32 fb_id;
> +	__u32 flags;
> +	__u32 sequence;
> +	__u64 user_data;
> +};
> +
>  /* create a dumb scanout buffer */
>  struct drm_mode_create_dumb {
>  	__u32 height;
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2

[Michel Dänzer]

On 16/08/16 10:32 AM, Mario Kleiner wrote:
> Cc'ing Daniel Stone and Pekka Paalanen, because this relates to wayland.
> 
> Wrt. having a new pageflip parameter struct, i wonder if it wouldn't
> make sense to then already prepare some space in it for specifying an
> absolute target time, e.g., in u64 microseconds? Or make this part of
> the atomic pageflip framework?

I feel like this is too much hassle for the DRM_IOCTL_MODE_PAGE_FLIP
ioctl, probably makes sense to only deal with this in the atomic API.


> In Wayland we now have the presentation_feedback extension (maybe it got
> a new name?), which is great to get feedback when and how presentation
> was completed, essentially the equivalent of X11's GLX_INTEL_swap_events
> + some useful extra info / the feedback half of OML_sync_control.
> 
> That was supposed to be complemented by a presentation_queue extension
> which would provide the other half of OML_sync_control, the part where
> an app can tell the compositor when and how to present surface updates.
> I remember that a year ago inclusion of that extension was blocked on
> some other more urgent important blocker bugs? Did this make progress?
> For timing sensitive applications such an extension is even more
> important in a wayland world than under X11. On X11 most desktops allow
> unredirecting fullscreen windows, so an app can drive the flip timing
> rather direct. At least on Weston as i remember it from my last tests a
> year ago, that wasn't possible, and an app that doesn't want to present
> at each video refresh, but at specific target times had to guess what
> the specific compositors scheduling strategy might be and then "play
> games" wrt. to timing surface commit's to trick the compositor into sort
> of doing the right thing - very fragile.
> 
> Anyway, the idea of presentation_queue was to specify the requested time
> of presentation as actual time, not as a target vblank count. This
> because applications that care about precise presentation timing, like
> my kind of neuroscience/medical visual stimulation software, or also
> video players, or e.g., at least the VDPAU video presentation api
> "think" in absolute time, not in refresh cycles. Also a target vblank
> count for presentation is less meaningful than a target time for things
> like variable framerate displays/adaptive sync, or displays which don't
> have a classic refresh cycle at all. It might also be useful if one
> thinks about something like VR compositors, where precise timing control
> could help for some of the tricks ("time warping" iirc?) they use to
> hide/cope with latency from head tracking -> display.
> 
> It would be nice if the kernel could help compositors which would
> implement presentation_queue or similar to be robust/precise in face of
> future stuff like Freesync, or of added delays due to Optimus/Prime
> hybrid-graphics setups. If we wanted to synchronize presentation of
> multiple displays in a Freesync type display setup, absolute target
> times could also be helpful.

I agree with all of this though.

I'd like to add that the X11 Present extension specification already
includes support for specifying a target time instead of a target
refresh cycle. This isn't implemented yet though, but it should be
relatively straightforward to implement using the kind of kernel API you
describe.


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

Re: [PATCH 4/6] drm/radeon: Provide page_flip_target hook

[Michel Dänzer]

On 16/08/16 09:35 AM, Mario Kleiner wrote:
> Hi Michel,
> 
> sorry for the super-late reply, i was just catching up with all the
> mails and discussions, starting in June, leading to this patch set.
> 
> Looks all pretty good.
> 
> I'll look at this radeon patch and 2/6 for amdgpu later this week when i
> have a fresh brain and enough "obsessive compulsive time", to make sure
> all the magic wrt. "virtually extended vblank" and the fudging logic is
> fine.

Excellent!


> I'll then also run it through my timing tests. I assume the
> ati/amdgpu-ddx patches and libdrm patches in your freedesktop home are
> all i need for testing?

Yes, although it would also be nice to test with unmodified userspace
and make sure there are no regressions with that.


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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 6/6] drm: Add DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags v2

[Pekka Paalanen]

[-- Attachment #1.1: Type: text/plain, Size: 4284 bytes --]

On Tue, 16 Aug 2016 10:46:13 +0900
Michel Dänzer <michel@daenzer.net> wrote:

> On 16/08/16 10:32 AM, Mario Kleiner wrote:
> > Cc'ing Daniel Stone and Pekka Paalanen, because this relates to wayland.
> > 
> > Wrt. having a new pageflip parameter struct, i wonder if it wouldn't
> > make sense to then already prepare some space in it for specifying an
> > absolute target time, e.g., in u64 microseconds? Or make this part of
> > the atomic pageflip framework?  
> 
> I feel like this is too much hassle for the DRM_IOCTL_MODE_PAGE_FLIP
> ioctl, probably makes sense to only deal with this in the atomic API.
> 
> 
> > In Wayland we now have the presentation_feedback extension (maybe it got
> > a new name?), which is great to get feedback when and how presentation
> > was completed, essentially the equivalent of X11's GLX_INTEL_swap_events
> > + some useful extra info / the feedback half of OML_sync_control.
> > 
> > That was supposed to be complemented by a presentation_queue extension
> > which would provide the other half of OML_sync_control, the part where
> > an app can tell the compositor when and how to present surface updates.
> > I remember that a year ago inclusion of that extension was blocked on
> > some other more urgent important blocker bugs? Did this make progress?

Hi,

sorry, I'm pretty poor at following dri-devel@.

Yeah, the Wayland extension for queueing frames to be presented at
specific times has not been going anywhere for a long time. IIRC the
blockers have since been resolved, now it would need dusting off and
seeing how it interacts with those protocol additions. I wasn't too
happy with the design at the time, either, because of the question of
which state to queue and which not.

Nowadays presentation_feedback lives in
https://cgit.freedesktop.org/wayland/wayland-protocols/tree/stable/presentation-time
and has been declared stable.

> > For timing sensitive applications such an extension is even more
> > important in a wayland world than under X11. On X11 most desktops allow
> > unredirecting fullscreen windows, so an app can drive the flip timing
> > rather direct. At least on Weston as i remember it from my last tests a
> > year ago, that wasn't possible, and an app that doesn't want to present
> > at each video refresh, but at specific target times had to guess what
> > the specific compositors scheduling strategy might be and then "play
> > games" wrt. to timing surface commit's to trick the compositor into sort
> > of doing the right thing - very fragile.
> > 
> > Anyway, the idea of presentation_queue was to specify the requested time
> > of presentation as actual time, not as a target vblank count. This
> > because applications that care about precise presentation timing, like
> > my kind of neuroscience/medical visual stimulation software, or also
> > video players, or e.g., at least the VDPAU video presentation api
> > "think" in absolute time, not in refresh cycles. Also a target vblank
> > count for presentation is less meaningful than a target time for things
> > like variable framerate displays/adaptive sync, or displays which don't
> > have a classic refresh cycle at all. It might also be useful if one
> > thinks about something like VR compositors, where precise timing control
> > could help for some of the tricks ("time warping" iirc?) they use to
> > hide/cope with latency from head tracking -> display.
> > 
> > It would be nice if the kernel could help compositors which would
> > implement presentation_queue or similar to be robust/precise in face of
> > future stuff like Freesync, or of added delays due to Optimus/Prime
> > hybrid-graphics setups. If we wanted to synchronize presentation of
> > multiple displays in a Freesync type display setup, absolute target
> > times could also be helpful.  
> 
> I agree with all of this though.

I'm very happy to hear the idea has support!


Thanks,
pq


> I'd like to add that the X11 Present extension specification already
> includes support for specifying a target time instead of a target
> refresh cycle. This isn't implemented yet though, but it should be
> relatively straightforward to implement using the kind of kernel API you
> describe.

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH 4/6] drm/radeon: Provide page_flip_target hook

[Mario Kleiner]

Hi Michel,

all your patches, both the already merged kernel bits in radeon/amdgpu 
and also all the userspace bits in libdrm/ati-ddx/amdgpu-ddx are all

Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>

I successfully tested with old/current userspace and the new userspace 
patches from your own libdrm/ati-ddx/amdgpu-ddx repos, under radeon-kms 
with HD-5770 and amdgpu-kms with R9 380, under DRI2 and DRI3/Present 
with the new userspace and at least DRI3/Present with the old/current 
userspace (can't quite remember if i also tested with DRI2 on 
old/current userspace, but probably). Hardware measured timing tests all 
work fine.

So all is good, except for pre-DCE4 without pflip irqs, but for that see 
the patchset i just sent out, which should make those old asics work 
well as well.

-mario

On 08/16/2016 03:49 AM, Michel Dänzer wrote:
> On 16/08/16 09:35 AM, Mario Kleiner wrote:
>> Hi Michel,
>>
>> sorry for the super-late reply, i was just catching up with all the
>> mails and discussions, starting in June, leading to this patch set.
>>
>> Looks all pretty good.
>>
>> I'll look at this radeon patch and 2/6 for amdgpu later this week when i
>> have a fresh brain and enough "obsessive compulsive time", to make sure
>> all the magic wrt. "virtually extended vblank" and the fudging logic is
>> fine.
>
> Excellent!
>
>
>> I'll then also run it through my timing tests. I assume the
>> ati/amdgpu-ddx patches and libdrm patches in your freedesktop home are
>> all i need for testing?
>
> Yes, although it would also be nice to test with unmodified userspace
> and make sure there are no regressions with that.
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/6] drm/radeon: Provide page_flip_target hook

[Michel Dänzer]

On 17/09/16 09:41 PM, Mario Kleiner wrote:
> Hi Michel,
> 
> all your patches, both the already merged kernel bits in radeon/amdgpu
> and also all the userspace bits in libdrm/ati-ddx/amdgpu-ddx are all
> 
> Reviewed-and-tested-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> 
> I successfully tested with old/current userspace and the new userspace
> patches from your own libdrm/ati-ddx/amdgpu-ddx repos, under radeon-kms
> with HD-5770 and amdgpu-kms with R9 380, under DRI2 and DRI3/Present
> with the new userspace and at least DRI3/Present with the old/current
> userspace (can't quite remember if i also tested with DRI2 on
> old/current userspace, but probably). Hardware measured timing tests all
> work fine.
> 
> So all is good, except for pre-DCE4 without pflip irqs, but for that see
> the patchset i just sent out, which should make those old asics work
> well as well.

Thanks for the testing and patches!


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