[Intel-gfx] [RFC 0/7] Asynchronous flip implementation for i915

[Intel-gfx] [RFC 0/7] Asynchronous flip implementation for i915

[Karthik B S]

Without async flip support in the kernel, fullscreen apps where game
resolution is equal to the screen resolution, must perform an extra blit
per frame prior to flipping.

Asynchronous page flips will also boost the FPS of Mesa benchmarks.

Karthik B S (7):
  drm/i915: Define flip done functions and enable IER
  drm/i915: Add support for async flips in I915
  drm/i915: Make commit call blocking in case of async flips
  drm/i915: Add checks specific to async flips
  drm/i915: Add flip_done_handler definition
  drm/i915: Enable and handle flip done interrupt
  drm/i915: Do not call drm_crtc_arm_vblank_event in async flips

 drivers/gpu/drm/i915/display/intel_display.c | 55 +++++++++++++++++--
 drivers/gpu/drm/i915/display/intel_sprite.c  | 12 ++--
 drivers/gpu/drm/i915/i915_irq.c              | 58 +++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.h              |  2 +
 drivers/gpu/drm/i915/i915_reg.h              |  1 +
 5 files changed, 117 insertions(+), 11 deletions(-)

-- 
2.22.0

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

[Intel-gfx] [RFC 1/7] drm/i915: Define flip done functions and enable IER

[Karthik B S]

Add enable/disable flip done functions and enable
the flip done interrupt in IER.

Flip done interrupt is used to send the page flip event as soon as the
surface address is written as per the requirement of async flips.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/i915/i915_irq.h |  2 ++
 2 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index ecf07b0faad2..5955e737a45d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2626,6 +2626,27 @@ int bdw_enable_vblank(struct drm_crtc *crtc)
 	return 0;
 }
 
+void icl_enable_flip_done(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[pipe];
+	unsigned long irqflags;
+
+	/* Make sure that vblank is not enabled, as we are already sending
+	 * the page flip event in the flip_done_handler.
+	 */
+	if (atomic_read(&vblank->refcount) != 0)
+		drm_crtc_vblank_put(crtc);
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	bdw_enable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+
+}
+
 /* Called from drm generic code, passed 'crtc' which
  * we use as a pipe index
  */
@@ -2686,6 +2707,20 @@ void bdw_disable_vblank(struct drm_crtc *crtc)
 	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
 }
 
+
+void icl_disable_flip_done(struct drm_crtc *crtc)
+{
+	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
+	enum pipe pipe = to_intel_crtc(crtc)->pipe;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
+
+	bdw_disable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
+
+	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
+}
+
 static void ibx_irq_reset(struct drm_i915_private *dev_priv)
 {
 	struct intel_uncore *uncore = &dev_priv->uncore;
@@ -3375,7 +3410,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
 		de_port_masked |= CNL_AUX_CHANNEL_F;
 
 	de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
-					   GEN8_PIPE_FIFO_UNDERRUN;
+			  GEN8_PIPE_FIFO_UNDERRUN | GEN9_PIPE_PLANE1_FLIP_DONE;
 
 	de_port_enables = de_port_masked;
 	if (IS_GEN9_LP(dev_priv))
diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h
index 812c47a9c2d6..6fc319980dd3 100644
--- a/drivers/gpu/drm/i915/i915_irq.h
+++ b/drivers/gpu/drm/i915/i915_irq.h
@@ -114,11 +114,13 @@ int i915gm_enable_vblank(struct drm_crtc *crtc);
 int i965_enable_vblank(struct drm_crtc *crtc);
 int ilk_enable_vblank(struct drm_crtc *crtc);
 int bdw_enable_vblank(struct drm_crtc *crtc);
+void icl_enable_flip_done(struct drm_crtc *crtc);
 void i8xx_disable_vblank(struct drm_crtc *crtc);
 void i915gm_disable_vblank(struct drm_crtc *crtc);
 void i965_disable_vblank(struct drm_crtc *crtc);
 void ilk_disable_vblank(struct drm_crtc *crtc);
 void bdw_disable_vblank(struct drm_crtc *crtc);
+void icl_disable_flip_done(struct drm_crtc *crtc);
 
 void gen2_irq_reset(struct intel_uncore *uncore);
 void gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,
-- 
2.22.0

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

[Intel-gfx] [RFC 2/7] drm/i915: Add support for async flips in I915

[Karthik B S]

Enable support for async flips in I915.
Set the Async Address Update Enable bit in plane ctl
when async flip is requested.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
 drivers/gpu/drm/i915/i915_reg.h              | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index dd47eb65b563..4ce9897f5c58 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -4756,6 +4756,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
 			plane_ctl |= PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
 	}
 
+	if (crtc_state->uapi.async_flip)
+		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
+
 	plane_ctl |= skl_plane_ctl_format(fb->format->format);
 	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
 	plane_ctl |= skl_plane_ctl_rotate(rotation & DRM_MODE_ROTATE_MASK);
@@ -17738,6 +17741,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
 
 	mode_config->funcs = &intel_mode_funcs;
 
+	mode_config->async_page_flip = true;
 	/*
 	 * Maximum framebuffer dimensions, chosen to match
 	 * the maximum render engine surface size on gen4+.
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 80cf02a6eec1..42037aee9b78 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -6794,6 +6794,7 @@ enum {
 #define   PLANE_CTL_TILED_X			(1 << 10)
 #define   PLANE_CTL_TILED_Y			(4 << 10)
 #define   PLANE_CTL_TILED_YF			(5 << 10)
+#define   PLANE_CTL_ASYNC_FLIP			(1 << 9)
 #define   PLANE_CTL_FLIP_HORIZONTAL		(1 << 8)
 #define   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE	(1 << 4) /* TGL+ */
 #define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK */
-- 
2.22.0

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

[Intel-gfx] [RFC 3/7] drm/i915: Make commit call blocking in case of async flips

[Karthik B S]

Make the commit call blocking in case of async flips
so that there is no delay in completing the flip.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 4ce9897f5c58..25fad5d01e67 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15737,7 +15737,9 @@ static int intel_atomic_commit(struct drm_device *dev,
 {
 	struct intel_atomic_state *state = to_intel_atomic_state(_state);
 	struct drm_i915_private *dev_priv = to_i915(dev);
-	int ret = 0;
+	struct intel_crtc_state *new_crtc_state;
+	struct intel_crtc *crtc;
+	int ret = 0, i;
 
 	state->wakeref = intel_runtime_pm_get(&dev_priv->runtime_pm);
 
@@ -15763,10 +15765,6 @@ static int intel_atomic_commit(struct drm_device *dev,
 	 * (assuming we had any) would solve these problems.
 	 */
 	if (INTEL_GEN(dev_priv) < 9 && state->base.legacy_cursor_update) {
-		struct intel_crtc_state *new_crtc_state;
-		struct intel_crtc *crtc;
-		int i;
-
 		for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i)
 			if (new_crtc_state->wm.need_postvbl_update ||
 			    new_crtc_state->update_wm_post)
@@ -15808,6 +15806,13 @@ static int intel_atomic_commit(struct drm_device *dev,
 	drm_atomic_state_get(&state->base);
 	INIT_WORK(&state->base.commit_work, intel_atomic_commit_work);
 
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->uapi.async_flip) {
+			nonblock = false;
+			break;
+		}
+	}
+
 	i915_sw_fence_commit(&state->commit_ready);
 	if (nonblock && state->modeset) {
 		queue_work(dev_priv->modeset_wq, &state->base.commit_work);
-- 
2.22.0

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

[Intel-gfx] [RFC 4/7] drm/i915: Add checks specific to async flips

[Karthik B S]

Support added only for async flips on primary plane.
If flip is requested on any other plane, reject it.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index 25fad5d01e67..a8de08c3773e 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -14732,6 +14732,31 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
 	return false;
 }
 
+static int intel_atomic_check_async(struct intel_atomic_state *state)
+{
+	struct drm_plane *plane;
+	struct drm_plane_state *plane_state;
+	struct intel_crtc_state *crtc_state;
+	struct intel_crtc *crtc;
+	int i, j;
+
+	/*FIXME: Async flip is only supported for primary plane currently
+	 * Support for overlays to be added.
+	 */
+	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
+		if (crtc_state->uapi.async_flip) {
+			for_each_new_plane_in_state(&state->base,
+						    plane, plane_state, j) {
+				if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
+					DRM_ERROR("Async flips is NOT supported for non-primary plane\n");
+					return -EINVAL;
+				}
+			}
+		}
+	}
+	return 0;
+}
+
 /**
  * intel_atomic_check - validate state object
  * @dev: drm device
@@ -14760,6 +14785,10 @@ static int intel_atomic_check(struct drm_device *dev,
 	if (ret)
 		goto fail;
 
+	ret = intel_atomic_check_async(state);
+	if  (ret)
+		goto fail;
+
 	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
 					    new_crtc_state, i) {
 		if (!needs_modeset(new_crtc_state)) {
-- 
2.22.0

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

[Intel-gfx] [RFC 5/7] drm/i915: Add flip_done_handler definition

[Karthik B S]

Send the flip done event in the handler and disable the interrupt.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 5955e737a45d..1feda9aecf4a 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1243,6 +1243,24 @@ display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 			     u32 crc4) {}
 #endif
 
+static void flip_done_handler(struct drm_i915_private *dev_priv,
+			      unsigned int pipe)
+{
+	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
+	struct drm_crtc_state *crtc_state = crtc->base.state;
+	struct drm_device *dev = &dev_priv->drm;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&dev->event_lock, irqflags);
+
+	if (crtc_state->event->base.event->type == DRM_EVENT_FLIP_COMPLETE) {
+		drm_crtc_send_vblank_event(&crtc->base, crtc_state->event);
+		crtc_state->event = NULL;
+	}
+
+	spin_unlock_irqrestore(&dev->event_lock, irqflags);
+	icl_disable_flip_done(&crtc->base);
+}
 
 static void hsw_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
 				     enum pipe pipe)
-- 
2.22.0

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

[Intel-gfx] [RFC 6/7] drm/i915: Enable and handle flip done interrupt

[Karthik B S]

Enable flip done function is called before writing the
surface address register as the write to this register triggers
the flip done interrupt.

If the flip done event is requested then send it in the flip done handler,
and then disable the interrupt.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 7 +++++++
 drivers/gpu/drm/i915/i915_irq.c              | 3 +++
 2 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
index a8de08c3773e..757380af1f93 100644
--- a/drivers/gpu/drm/i915/display/intel_display.c
+++ b/drivers/gpu/drm/i915/display/intel_display.c
@@ -15604,6 +15604,13 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
 	if (state->modeset)
 		icl_dbuf_slice_pre_update(state);
 
+	for_each_new_intel_crtc_in_state(state, crtc, new_crtc_state, i) {
+		if (new_crtc_state->uapi.async_flip) {
+			icl_enable_flip_done(&crtc->base);
+			break;
+		}
+	}
+
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	dev_priv->display.commit_modeset_enables(state);
 
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 1feda9aecf4a..c9b1bb0e2c30 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -2363,6 +2363,9 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl)
 		if (iir & GEN8_PIPE_VBLANK)
 			intel_handle_vblank(dev_priv, pipe);
 
+		if (iir & GEN9_PIPE_PLANE1_FLIP_DONE)
+			flip_done_handler(dev_priv, pipe);
+
 		if (iir & GEN8_PIPE_CDCLK_CRC_DONE)
 			hsw_pipe_crc_irq_handler(dev_priv, pipe);
 
-- 
2.22.0

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

[Intel-gfx] [RFC 7/7] drm/i915: Do not call drm_crtc_arm_vblank_event in async flips

[Karthik B S]

Since the flip done event will be sent in the flip_done_handler,
no need to add the event to the list and delay it for later.

Signed-off-by: Karthik B S <karthik.b.s@intel.com>
---
 drivers/gpu/drm/i915/display/intel_sprite.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_sprite.c b/drivers/gpu/drm/i915/display/intel_sprite.c
index deda351719db..95193a521aa9 100644
--- a/drivers/gpu/drm/i915/display/intel_sprite.c
+++ b/drivers/gpu/drm/i915/display/intel_sprite.c
@@ -209,12 +209,14 @@ void intel_pipe_update_end(struct intel_crtc_state *new_crtc_state)
 		drm_WARN_ON(&dev_priv->drm,
 			    drm_crtc_vblank_get(&crtc->base) != 0);
 
-		spin_lock(&crtc->base.dev->event_lock);
-		drm_crtc_arm_vblank_event(&crtc->base,
-				          new_crtc_state->uapi.event);
-		spin_unlock(&crtc->base.dev->event_lock);
+		if (!new_crtc_state->uapi.async_flip) {
+			spin_lock(&crtc->base.dev->event_lock);
+			drm_crtc_arm_vblank_event(&crtc->base,
+						  new_crtc_state->uapi.event);
+			spin_unlock(&crtc->base.dev->event_lock);
 
-		new_crtc_state->uapi.event = NULL;
+			new_crtc_state->uapi.event = NULL;
+		}
 	}
 
 	local_irq_enable();
-- 
2.22.0

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

[Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for Asynchronous flip implementation for i915

[Patchwork]

== Series Details ==

Series: Asynchronous flip implementation for i915
URL   : https://patchwork.freedesktop.org/series/74386/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
acc147186833 drm/i915: Define flip done functions and enable IER
-:41: CHECK:BRACES: Blank lines aren't necessary before a close brace '}'
#41: FILE: drivers/gpu/drm/i915/i915_irq.c:2658:
+
+}

-:50: CHECK:LINE_SPACING: Please don't use multiple blank lines
#50: FILE: drivers/gpu/drm/i915/i915_irq.c:2720:
 
+

total: 0 errors, 0 warnings, 2 checks, 68 lines checked
e1403132835c drm/i915: Add support for async flips in I915
95f6839ee4fc drm/i915: Make commit call blocking in case of async flips
ea0c09f4b01b drm/i915: Add checks specific to async flips
6d21dd58995a drm/i915: Add flip_done_handler definition
de25734fbcdc drm/i915: Enable and handle flip done interrupt
2eafb08e7a12 drm/i915: Do not call drm_crtc_arm_vblank_event in async flips

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

[Intel-gfx] ✗ Fi.CI.SPARSE: warning for Asynchronous flip implementation for i915

[Patchwork]

== Series Details ==

Series: Asynchronous flip implementation for i915
URL   : https://patchwork.freedesktop.org/series/74386/
State : warning

== Summary ==

$ dim sparse origin/drm-tip
Sparse version: v0.6.0
Commit: drm/i915: Define flip done functions and enable IER
Okay!

Commit: drm/i915: Add support for async flips in I915
Okay!

Commit: drm/i915: Make commit call blocking in case of async flips
Okay!

Commit: drm/i915: Add checks specific to async flips
Okay!

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

[Intel-gfx] ✗ Fi.CI.DOCS: warning for Asynchronous flip implementation for i915

[Patchwork]

== Series Details ==

Series: Asynchronous flip implementation for i915
URL   : https://patchwork.freedesktop.org/series/74386/
State : warning

== Summary ==

$ make htmldocs 2>&1 > /dev/null | grep i915
./drivers/gpu/drm/i915/display/intel_dpll_mgr.h:285: warning: Function parameter or member 'get_freq' not described in 'intel_shared_dpll_funcs'

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

[Intel-gfx] ✗ Fi.CI.BAT: failure for Asynchronous flip implementation for i915

[Patchwork]

== Series Details ==

Series: Asynchronous flip implementation for i915
URL   : https://patchwork.freedesktop.org/series/74386/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8085 -> Patchwork_16860
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_16860 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_16860, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/index.html

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_16860:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@gt_lrc:
    - fi-snb-2520m:       [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-snb-2520m/igt@i915_selftest@live@gt_lrc.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-snb-2520m/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_busy@basic@flip:
    - fi-skl-6770hq:      [PASS][3] -> [INCOMPLETE][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-skl-6770hq/igt@kms_busy@basic@flip.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-skl-6770hq/igt@kms_busy@basic@flip.html

  * igt@runner@aborted:
    - fi-snb-2520m:       NOTRUN -> [FAIL][5]
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-snb-2520m/igt@runner@aborted.html
    - fi-byt-j1900:       NOTRUN -> [FAIL][6]
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-byt-j1900/igt@runner@aborted.html

  
Known issues
------------

  Here are the changes found in Patchwork_16860 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@kms_addfb_basic@invalid-get-prop:
    - fi-tgl-y:           [PASS][7] -> [DMESG-WARN][8] ([CI#94] / [i915#402]) +1 similar issue
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-tgl-y/igt@kms_addfb_basic@invalid-get-prop.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-tgl-y/igt@kms_addfb_basic@invalid-get-prop.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s4-devices:
    - fi-tgl-y:           [FAIL][9] ([CI#94]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-tgl-y/igt@gem_exec_suspend@basic-s4-devices.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-tgl-y/igt@gem_exec_suspend@basic-s4-devices.html

  * igt@i915_selftest@live@gt_contexts:
    - fi-snb-2520m:       [DMESG-WARN][11] -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-snb-2520m/igt@i915_selftest@live@gt_contexts.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-snb-2520m/igt@i915_selftest@live@gt_contexts.html

  * igt@kms_chamelium@dp-crc-fast:
    - fi-kbl-7500u:       [FAIL][13] ([fdo#109635] / [i915#262]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-kbl-7500u/igt@kms_chamelium@dp-crc-fast.html

  * igt@prime_self_import@basic-with_two_bos:
    - fi-tgl-y:           [DMESG-WARN][15] ([CI#94] / [i915#402]) -> [PASS][16] +1 similar issue
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8085/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/fi-tgl-y/igt@prime_self_import@basic-with_two_bos.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [CI#94]: https://gitlab.freedesktop.org/gfx-ci/i915-infra/issues/94
  [fdo#109635]: https://bugs.freedesktop.org/show_bug.cgi?id=109635
  [i915#262]: https://gitlab.freedesktop.org/drm/intel/issues/262
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#45]: https://gitlab.freedesktop.org/drm/intel/issues/45


Participating hosts (51 -> 42)
------------------------------

  Additional (1): fi-tgl-dsi 
  Missing    (10): fi-ilk-m540 fi-hsw-4200u fi-hsw-peppy fi-byt-squawks fi-bsw-cyan fi-gdg-551 fi-bsw-kefka fi-bdw-samus fi-byt-clapper fi-skl-6600u 


Build changes
-------------

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8085 -> Patchwork_16860

  CI-20190529: 20190529
  CI_DRM_8085: f731492964aa6510672f43292d4b2216b73eddeb @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5496: 00a8e400876f2c27f62ed7d418be6b55738a4ea6 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_16860: 2eafb08e7a1274550eaa97d29851803cdda31162 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

2eafb08e7a12 drm/i915: Do not call drm_crtc_arm_vblank_event in async flips
de25734fbcdc drm/i915: Enable and handle flip done interrupt
6d21dd58995a drm/i915: Add flip_done_handler definition
ea0c09f4b01b drm/i915: Add checks specific to async flips
95f6839ee4fc drm/i915: Make commit call blocking in case of async flips
e1403132835c drm/i915: Add support for async flips in I915
acc147186833 drm/i915: Define flip done functions and enable IER

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_16860/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC 1/7] drm/i915: Define flip done functions and enable IER

[Paulo Zanoni]

Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> Add enable/disable flip done functions and enable
> the flip done interrupt in IER.
> 
> Flip done interrupt is used to send the page flip event as soon as the
> surface address is written as per the requirement of async flips.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 37 ++++++++++++++++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_irq.h |  2 ++
>  2 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ecf07b0faad2..5955e737a45d 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2626,6 +2626,27 @@ int bdw_enable_vblank(struct drm_crtc *crtc)
>  	return 0;
>  }
>  
> +void icl_enable_flip_done(struct drm_crtc *crtc)


Platform prefixes indicate the first platform that is able to run this
function. In this case I can't even see which platforms will run the
function because it's only later in the series that this function will
get called. I'm not a fan of this patch splitting style where a
function gets added in patch X and then used in patch X+Y. IMHO
functions should only be introduced in patches where they are used.
This makes the code much easier to review.

So, shouldn't this be skl_enable_flip_done()?

> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[pipe];
> +	unsigned long irqflags;
> +
> +	/* Make sure that vblank is not enabled, as we are already sending
> +	 * the page flip event in the flip_done_handler.
> +	 */
> +	if (atomic_read(&vblank->refcount) != 0)
> +		drm_crtc_vblank_put(crtc);

This is the kind of thing that will be much easier to review when this
patch gets squashed in the one that makes use of these functions.

Even after reading the whole series, this put() doesn't seem correct to
me. What is the problem with having vblanks enabled? Is it because we
were sending duplicate vblank events without these lines? Where is the
get() that triggers this put()? Please help me understand this.


> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	bdw_enable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> +}
> +
>  /* Called from drm generic code, passed 'crtc' which
>   * we use as a pipe index
>   */
> @@ -2686,6 +2707,20 @@ void bdw_disable_vblank(struct drm_crtc *crtc)
>  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
>  }
>  
> +
> +void icl_disable_flip_done(struct drm_crtc *crtc)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> +
> +	bdw_disable_pipe_irq(dev_priv, pipe, GEN9_PIPE_PLANE1_FLIP_DONE);
> +
> +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +}
> +
>  static void ibx_irq_reset(struct drm_i915_private *dev_priv)
>  {
>  	struct intel_uncore *uncore = &dev_priv->uncore;
> @@ -3375,7 +3410,7 @@ static void gen8_de_irq_postinstall(struct drm_i915_private *dev_priv)
>  		de_port_masked |= CNL_AUX_CHANNEL_F;
>  
>  	de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
> -					   GEN8_PIPE_FIFO_UNDERRUN;
> +			  GEN8_PIPE_FIFO_UNDERRUN | GEN9_PIPE_PLANE1_FLIP_DONE;

This is going to set this bit for gen8 too, which is something we
probably don't want since it doesn't exist there.

The patch also does not add the handler for the interrupt, which
doesn't make sense (see my point above).

Also, don't we want to do like GEN8_PIPE_VBLANK and also set it on the
power_well_post_enable hook? If not, why? This is probably a case we
should write an IGT subtest for.

>  
>  	de_port_enables = de_port_masked;
>  	if (IS_GEN9_LP(dev_priv))
> diff --git a/drivers/gpu/drm/i915/i915_irq.h b/drivers/gpu/drm/i915/i915_irq.h
> index 812c47a9c2d6..6fc319980dd3 100644
> --- a/drivers/gpu/drm/i915/i915_irq.h
> +++ b/drivers/gpu/drm/i915/i915_irq.h
> @@ -114,11 +114,13 @@ int i915gm_enable_vblank(struct drm_crtc *crtc);
>  int i965_enable_vblank(struct drm_crtc *crtc);
>  int ilk_enable_vblank(struct drm_crtc *crtc);
>  int bdw_enable_vblank(struct drm_crtc *crtc);
> +void icl_enable_flip_done(struct drm_crtc *crtc);
>  void i8xx_disable_vblank(struct drm_crtc *crtc);
>  void i915gm_disable_vblank(struct drm_crtc *crtc);
>  void i965_disable_vblank(struct drm_crtc *crtc);
>  void ilk_disable_vblank(struct drm_crtc *crtc);
>  void bdw_disable_vblank(struct drm_crtc *crtc);
> +void icl_disable_flip_done(struct drm_crtc *crtc);
>  
>  void gen2_irq_reset(struct intel_uncore *uncore);
>  void gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,

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

Re: [Intel-gfx] [RFC 2/7] drm/i915: Add support for async flips in I915

[Paulo Zanoni]

Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> Enable support for async flips in I915.
> Set the Async Address Update Enable bit in plane ctl
> when async flip is requested.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
>  drivers/gpu/drm/i915/i915_reg.h              | 1 +
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index dd47eb65b563..4ce9897f5c58 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4756,6 +4756,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state *crtc_state,
>  			plane_ctl |= PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
>  	}
>  
> +	if (crtc_state->uapi.async_flip)
> +		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> +
>  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
>  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
>  	plane_ctl |= skl_plane_ctl_rotate(rotation & DRM_MODE_ROTATE_MASK);
> @@ -17738,6 +17741,7 @@ static void intel_mode_config_init(struct drm_i915_private *i915)
>  
>  	mode_config->funcs = &intel_mode_funcs;
>  
> +	mode_config->async_page_flip = true;

We should only enable the feature to user space after it has been fully
implemented inside the Kernel. Think about the case where git-bisect
decides to land at exactly this commit when someone is debugging a
failure unrelated to async vblanks.

Also, when features have trivial on/off switches like the line above,
it's better if the patch that enables the feature only contains the
line that toggles the on/off switch. This way, if a revert is needed,
we can just switch it to off without removing more code. Also, it
enables us to land the rest of the code while keeping the feature off
for stabilization.

Also, the line above is enabling the feature for every platform, which
is probably not a goal of this series.


>  	/*
>  	 * Maximum framebuffer dimensions, chosen to match
>  	 * the maximum render engine surface size on gen4+.
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 80cf02a6eec1..42037aee9b78 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6794,6 +6794,7 @@ enum {
>  #define   PLANE_CTL_TILED_X			(1 << 10)
>  #define   PLANE_CTL_TILED_Y			(4 << 10)
>  #define   PLANE_CTL_TILED_YF			(5 << 10)
> +#define   PLANE_CTL_ASYNC_FLIP			(1 << 9)
>  #define   PLANE_CTL_FLIP_HORIZONTAL		(1 << 8)
>  #define   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE	(1 << 4) /* TGL+ */
>  #define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK */

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

Re: [Intel-gfx] [RFC 4/7] drm/i915: Add checks specific to async flips

[Paulo Zanoni]

Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> Support added only for async flips on primary plane.
> If flip is requested on any other plane, reject it.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 29 ++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 25fad5d01e67..a8de08c3773e 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -14732,6 +14732,31 @@ static bool intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
>  	return false;
>  }
>  
> +static int intel_atomic_check_async(struct intel_atomic_state *state)
> +{
> +	struct drm_plane *plane;
> +	struct drm_plane_state *plane_state;
> +	struct intel_crtc_state *crtc_state;
> +	struct intel_crtc *crtc;
> +	int i, j;
> +
> +	/*FIXME: Async flip is only supported for primary plane currently
> +	 * Support for overlays to be added.
> +	 */
> +	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> +		if (crtc_state->uapi.async_flip) {
> +			for_each_new_plane_in_state(&state->base,
> +						    plane, plane_state, j) {
> +				if (plane->type != DRM_PLANE_TYPE_PRIMARY) {
> +					DRM_ERROR("Async flips is NOT supported for non-primary plane\n");

My understanding is that this is not a case of DRM_ERROR, since it's
just user space doing something it shouldn't.

Have we checked if xf86-video-modesetting or some other current user
space is going to try these calls on non-primary when async_flips are
enabled? Specifically, how does it react when it gets the EINVAL? We
should try to avoid the case where we release a Kernel that current
user space is not prepared for (even if it's not the Kernel's fault).


> +					return -EINVAL;
> +				}
> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
>  /**
>   * intel_atomic_check - validate state object
>   * @dev: drm device
> @@ -14760,6 +14785,10 @@ static int intel_atomic_check(struct drm_device *dev,
>  	if (ret)
>  		goto fail;
>  
> +	ret = intel_atomic_check_async(state);
> +	if  (ret)
> +		goto fail;
> +
>  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
>  					    new_crtc_state, i) {
>  		if (!needs_modeset(new_crtc_state)) {

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

Re: [Intel-gfx] [RFC 5/7] drm/i915: Add flip_done_handler definition

[Paulo Zanoni]

Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> Send the flip done event in the handler and disable the interrupt.
> 
> Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 5955e737a45d..1feda9aecf4a 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1243,6 +1243,24 @@ display_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  			     u32 crc4) {}
>  #endif
>  
> +static void flip_done_handler(struct drm_i915_private *dev_priv,
> +			      unsigned int pipe)

The compiler is going to complain that we added a static function with
no caller.

See my comment on commit 1: please squash this patch with the one that
makes use of the new function.

> +{
> +	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> +	struct drm_crtc_state *crtc_state = crtc->base.state;
> +	struct drm_device *dev = &dev_priv->drm;
> +	unsigned long irqflags;
> +
> +	spin_lock_irqsave(&dev->event_lock, irqflags);
> +
> +	if (crtc_state->event->base.event->type == DRM_EVENT_FLIP_COMPLETE) {
> +		drm_crtc_send_vblank_event(&crtc->base, crtc_state->event);
> +		crtc_state->event = NULL;
> +	}
> +
> +	spin_unlock_irqrestore(&dev->event_lock, irqflags);
> +	icl_disable_flip_done(&crtc->base);
> +}
>  
>  static void hsw_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
>  				     enum pipe pipe)

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

Re: [Intel-gfx] [RFC 0/7] Asynchronous flip implementation for i915

[Paulo Zanoni]

Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> Without async flip support in the kernel, fullscreen apps where game
> resolution is equal to the screen resolution, must perform an extra blit
> per frame prior to flipping.
> 
> Asynchronous page flips will also boost the FPS of Mesa benchmarks.


Thanks a lot for doing this work!

I did some quick smoke tests on a Gemini Lake and while this appears to
be working fine with xf86-video-modesetting, the "pageflip.c" program I
shared previously breaks when you launch it as "./pageflip -n": this
argument makes the program *not* request for page flip events (by not
setting DRM_MODE_PAGE_FLIP_EVENT) and just try to flip as fast as it
can. I didn't investigate why this breaks, but it's probably some
corner case the series is forgetting.

Also, doesn't async pageflip interact with some other display features?
Don't we need to disable at least one of FBC, PSR and/or render
compression when using async page flips?

Ville mentioned some possible interactions with SURF/OFFSET tracking
too (framebuffers not being at the start of the bo), which doesn't seem
to be covered by the series.

Thanks,
Paulo

> 
> Karthik B S (7):
>   drm/i915: Define flip done functions and enable IER
>   drm/i915: Add support for async flips in I915
>   drm/i915: Make commit call blocking in case of async flips
>   drm/i915: Add checks specific to async flips
>   drm/i915: Add flip_done_handler definition
>   drm/i915: Enable and handle flip done interrupt
>   drm/i915: Do not call drm_crtc_arm_vblank_event in async flips
> 
>  drivers/gpu/drm/i915/display/intel_display.c | 55 +++++++++++++++++--
>  drivers/gpu/drm/i915/display/intel_sprite.c  | 12 ++--
>  drivers/gpu/drm/i915/i915_irq.c              | 58 +++++++++++++++++++-
>  drivers/gpu/drm/i915/i915_irq.h              |  2 +
>  drivers/gpu/drm/i915/i915_reg.h              |  1 +
>  5 files changed, 117 insertions(+), 11 deletions(-)
> 

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

Re: [Intel-gfx] [RFC 0/7] Asynchronous flip implementation for i915

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Sent: Tuesday, March 10, 2020 5:35 AM
> To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [RFC 0/7] Asynchronous flip implementation for i915
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Without async flip support in the kernel, fullscreen apps where game
> > resolution is equal to the screen resolution, must perform an extra
> > blit per frame prior to flipping.
> >
> > Asynchronous page flips will also boost the FPS of Mesa benchmarks.
> 
> 
> Thanks a lot for doing this work!

Thanks a lot for the review.
> 
> I did some quick smoke tests on a Gemini Lake and while this appears to be
> working fine with xf86-video-modesetting, the "pageflip.c" program I shared
> previously breaks when you launch it as "./pageflip -n": this argument makes
> the program *not* request for page flip events (by not setting
> DRM_MODE_PAGE_FLIP_EVENT) and just try to flip as fast as it can. I didn't
> investigate why this breaks, but it's probably some corner case the series is
> forgetting.

I hadn't tried out this option. Thanks for pointing this out.
Will fix this in the next revision.
> 
> Also, doesn't async pageflip interact with some other display features?
> Don't we need to disable at least one of FBC, PSR and/or render compression
> when using async page flips?
> 
> Ville mentioned some possible interactions with SURF/OFFSET tracking too
> (framebuffers not being at the start of the bo), which doesn't seem to be
> covered by the series.
> 

Yes, both the above hasn't been taken care of in this series. Thanks for pointing it out.
Will check it and update in the next revision.

Thanks,
Karthik

> Thanks,
> Paulo
> 
> >
> > Karthik B S (7):
> >   drm/i915: Define flip done functions and enable IER
> >   drm/i915: Add support for async flips in I915
> >   drm/i915: Make commit call blocking in case of async flips
> >   drm/i915: Add checks specific to async flips
> >   drm/i915: Add flip_done_handler definition
> >   drm/i915: Enable and handle flip done interrupt
> >   drm/i915: Do not call drm_crtc_arm_vblank_event in async flips
> >
> >  drivers/gpu/drm/i915/display/intel_display.c | 55 +++++++++++++++++--
> > drivers/gpu/drm/i915/display/intel_sprite.c  | 12 ++--
> >  drivers/gpu/drm/i915/i915_irq.c              | 58 +++++++++++++++++++-
> >  drivers/gpu/drm/i915/i915_irq.h              |  2 +
> >  drivers/gpu/drm/i915/i915_reg.h              |  1 +
> >  5 files changed, 117 insertions(+), 11 deletions(-)
> >

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

Re: [Intel-gfx] [RFC 1/7] drm/i915: Define flip done functions and enable IER

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Sent: Tuesday, March 10, 2020 4:48 AM
> To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [RFC 1/7] drm/i915: Define flip done functions and enable IER
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Add enable/disable flip done functions and enable the flip done
> > interrupt in IER.
> >
> > Flip done interrupt is used to send the page flip event as soon as the
> > surface address is written as per the requirement of async flips.
> >
> > Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 37
> > ++++++++++++++++++++++++++++++++-
> drivers/gpu/drm/i915/i915_irq.h |
> > 2 ++
> >  2 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index ecf07b0faad2..5955e737a45d
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2626,6 +2626,27 @@ int bdw_enable_vblank(struct drm_crtc *crtc)
> >  	return 0;
> >  }
> >
> > +void icl_enable_flip_done(struct drm_crtc *crtc)
> 
> 
> Platform prefixes indicate the first platform that is able to run this function.
> In this case I can't even see which platforms will run the function because it's
> only later in the series that this function will get called. I'm not a fan of this
> patch splitting style where a function gets added in patch X and then used in
> patch X+Y. IMHO functions should only be introduced in patches where they
> are used.
> This makes the code much easier to review.

Thanks for the review.
Will update the patches as per your feedback.
> 
> So, shouldn't this be skl_enable_flip_done()?

Agreed. Will update the function name.
> 
> > +{
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +	struct drm_vblank_crtc *vblank = &dev_priv->drm.vblank[pipe];
> > +	unsigned long irqflags;
> > +
> > +	/* Make sure that vblank is not enabled, as we are already sending
> > +	 * the page flip event in the flip_done_handler.
> > +	 */
> > +	if (atomic_read(&vblank->refcount) != 0)
> > +		drm_crtc_vblank_put(crtc);
> 
> This is the kind of thing that will be much easier to review when this patch
> gets squashed in the one that makes use of these functions.

Will update the patches as per your feedback.
> 
> Even after reading the whole series, this put() doesn't seem correct to me.
> What is the problem with having vblanks enabled? Is it because we were
> sending duplicate vblank events without these lines? Where is the
> get() that triggers this put()? Please help me understand this.

Checked the code once more after your review and I agree that this wouldn't be needed as
there is no get() called for which this put() would be needed.
And as the event is sent in the flip_done_handler in this series, there is no need to disable vblank.
> 
> 
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	bdw_enable_pipe_irq(dev_priv, pipe,
> GEN9_PIPE_PLANE1_FLIP_DONE);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> > +
> > +}
> > +
> >  /* Called from drm generic code, passed 'crtc' which
> >   * we use as a pipe index
> >   */
> > @@ -2686,6 +2707,20 @@ void bdw_disable_vblank(struct drm_crtc *crtc)
> >  	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);  }
> >
> > +
> > +void icl_disable_flip_done(struct drm_crtc *crtc) {
> > +	struct drm_i915_private *dev_priv = to_i915(crtc->dev);
> > +	enum pipe pipe = to_intel_crtc(crtc)->pipe;
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> > +
> > +	bdw_disable_pipe_irq(dev_priv, pipe,
> GEN9_PIPE_PLANE1_FLIP_DONE);
> > +
> > +	spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); }
> > +
> >  static void ibx_irq_reset(struct drm_i915_private *dev_priv)  {
> >  	struct intel_uncore *uncore = &dev_priv->uncore; @@ -3375,7
> +3410,7
> > @@ static void gen8_de_irq_postinstall(struct drm_i915_private
> *dev_priv)
> >  		de_port_masked |= CNL_AUX_CHANNEL_F;
> >
> >  	de_pipe_enables = de_pipe_masked | GEN8_PIPE_VBLANK |
> > -					   GEN8_PIPE_FIFO_UNDERRUN;
> > +			  GEN8_PIPE_FIFO_UNDERRUN |
> GEN9_PIPE_PLANE1_FLIP_DONE;
> 
> This is going to set this bit for gen8 too, which is something we probably don't
> want since it doesn't exist there.

Will add a gen check here in the next revision.
> 
> The patch also does not add the handler for the interrupt, which doesn't
> make sense (see my point above).

Noted.
> 
> Also, don't we want to do like GEN8_PIPE_VBLANK and also set it on the
> power_well_post_enable hook? If not, why? This is probably a case we
> should write an IGT subtest for.

Will check this and update in the next revision.
> 
> >
> >  	de_port_enables = de_port_masked;
> >  	if (IS_GEN9_LP(dev_priv))
> > diff --git a/drivers/gpu/drm/i915/i915_irq.h
> > b/drivers/gpu/drm/i915/i915_irq.h index 812c47a9c2d6..6fc319980dd3
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.h
> > +++ b/drivers/gpu/drm/i915/i915_irq.h
> > @@ -114,11 +114,13 @@ int i915gm_enable_vblank(struct drm_crtc *crtc);
> > int i965_enable_vblank(struct drm_crtc *crtc);  int
> > ilk_enable_vblank(struct drm_crtc *crtc);  int
> > bdw_enable_vblank(struct drm_crtc *crtc);
> > +void icl_enable_flip_done(struct drm_crtc *crtc);
> >  void i8xx_disable_vblank(struct drm_crtc *crtc);  void
> > i915gm_disable_vblank(struct drm_crtc *crtc);  void
> > i965_disable_vblank(struct drm_crtc *crtc);  void
> > ilk_disable_vblank(struct drm_crtc *crtc);  void
> > bdw_disable_vblank(struct drm_crtc *crtc);
> > +void icl_disable_flip_done(struct drm_crtc *crtc);
> >
> >  void gen2_irq_reset(struct intel_uncore *uncore);  void
> > gen3_irq_reset(struct intel_uncore *uncore, i915_reg_t imr,

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

Re: [Intel-gfx] [RFC 2/7] drm/i915: Add support for async flips in I915

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Sent: Tuesday, March 10, 2020 4:48 AM
> To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [RFC 2/7] drm/i915: Add support for async flips in I915
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Enable support for async flips in I915.
> > Set the Async Address Update Enable bit in plane ctl when async flip
> > is requested.
> >
> > Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
> >  drivers/gpu/drm/i915/i915_reg.h              | 1 +
> >  2 files changed, 5 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index dd47eb65b563..4ce9897f5c58 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -4756,6 +4756,9 @@ u32 skl_plane_ctl(const struct intel_crtc_state
> *crtc_state,
> >  			plane_ctl |=
> PLANE_CTL_YUV_RANGE_CORRECTION_DISABLE;
> >  	}
> >
> > +	if (crtc_state->uapi.async_flip)
> > +		plane_ctl |= PLANE_CTL_ASYNC_FLIP;
> > +
> >  	plane_ctl |= skl_plane_ctl_format(fb->format->format);
> >  	plane_ctl |= skl_plane_ctl_tiling(fb->modifier);
> >  	plane_ctl |= skl_plane_ctl_rotate(rotation &
> DRM_MODE_ROTATE_MASK);
> > @@ -17738,6 +17741,7 @@ static void intel_mode_config_init(struct
> > drm_i915_private *i915)
> >
> >  	mode_config->funcs = &intel_mode_funcs;
> >
> > +	mode_config->async_page_flip = true;
> 
> We should only enable the feature to user space after it has been fully
> implemented inside the Kernel. Think about the case where git-bisect
> decides to land at exactly this commit when someone is debugging a failure
> unrelated to async vblanks.
> 
> Also, when features have trivial on/off switches like the line above, it's
> better if the patch that enables the feature only contains the line that toggles
> the on/off switch. This way, if a revert is needed, we can just switch it to off
> without removing more code. Also, it enables us to land the rest of the code
> while keeping the feature off for stabilization.
> 
> Also, the line above is enabling the feature for every platform, which is
> probably not a goal of this series.

Agreed. Will make the on/off part a separate patch and also add a gen check for it.
> 
> 
> >  	/*
> >  	 * Maximum framebuffer dimensions, chosen to match
> >  	 * the maximum render engine surface size on gen4+.
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h index 80cf02a6eec1..42037aee9b78
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -6794,6 +6794,7 @@ enum {
> >  #define   PLANE_CTL_TILED_X			(1 << 10)
> >  #define   PLANE_CTL_TILED_Y			(4 << 10)
> >  #define   PLANE_CTL_TILED_YF			(5 << 10)
> > +#define   PLANE_CTL_ASYNC_FLIP			(1 << 9)
> >  #define   PLANE_CTL_FLIP_HORIZONTAL		(1 << 8)
> >  #define   PLANE_CTL_MEDIA_DECOMPRESSION_ENABLE	(1 << 4) /*
> TGL+ */
> >  #define   PLANE_CTL_ALPHA_MASK			(0x3 << 4) /* Pre-GLK
> */

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

Re: [Intel-gfx] [RFC 4/7] drm/i915: Add checks specific to async flips

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Sent: Tuesday, March 10, 2020 4:49 AM
> To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [RFC 4/7] drm/i915: Add checks specific to async flips
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Support added only for async flips on primary plane.
> > If flip is requested on any other plane, reject it.
> >
> > Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_display.c | 29
> > ++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_display.c
> > b/drivers/gpu/drm/i915/display/intel_display.c
> > index 25fad5d01e67..a8de08c3773e 100644
> > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > @@ -14732,6 +14732,31 @@ static bool
> intel_cpu_transcoders_need_modeset(struct intel_atomic_state *state,
> >  	return false;
> >  }
> >
> > +static int intel_atomic_check_async(struct intel_atomic_state *state)
> > +{
> > +	struct drm_plane *plane;
> > +	struct drm_plane_state *plane_state;
> > +	struct intel_crtc_state *crtc_state;
> > +	struct intel_crtc *crtc;
> > +	int i, j;
> > +
> > +	/*FIXME: Async flip is only supported for primary plane currently
> > +	 * Support for overlays to be added.
> > +	 */
> > +	for_each_new_intel_crtc_in_state(state, crtc, crtc_state, i) {
> > +		if (crtc_state->uapi.async_flip) {
> > +			for_each_new_plane_in_state(&state->base,
> > +						    plane, plane_state, j) {
> > +				if (plane->type !=
> DRM_PLANE_TYPE_PRIMARY) {
> > +					DRM_ERROR("Async flips is NOT
> supported for non-primary
> > +plane\n");
> 
> My understanding is that this is not a case of DRM_ERROR, since it's just user
> space doing something it shouldn't.

Sure. Will fix that in the next revision.
> 
> Have we checked if xf86-video-modesetting or some other current user
> space is going to try these calls on non-primary when async_flips are
> enabled? Specifically, how does it react when it gets the EINVAL? We should
> try to avoid the case where we release a Kernel that current user space is not
> prepared for (even if it's not the Kernel's fault).

Will check the user space behavior and update accordingly in the next revision. 
> 
> 
> > +					return -EINVAL;
> > +				}
> > +			}
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  /**
> >   * intel_atomic_check - validate state object
> >   * @dev: drm device
> > @@ -14760,6 +14785,10 @@ static int intel_atomic_check(struct
> drm_device *dev,
> >  	if (ret)
> >  		goto fail;
> >
> > +	ret = intel_atomic_check_async(state);
> > +	if  (ret)
> > +		goto fail;
> > +
> >  	for_each_oldnew_intel_crtc_in_state(state, crtc, old_crtc_state,
> >  					    new_crtc_state, i) {
> >  		if (!needs_modeset(new_crtc_state)) {

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

Re: [Intel-gfx] [RFC 5/7] drm/i915: Add flip_done_handler definition

[B S, Karthik]

> -----Original Message-----
> From: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Sent: Tuesday, March 10, 2020 4:49 AM
> To: B S, Karthik <karthik.b.s@intel.com>; intel-gfx@lists.freedesktop.org
> Cc: ville.syrjala@linux.intel.com; Kulkarni, Vandita
> <vandita.kulkarni@intel.com>; Shankar, Uma <uma.shankar@intel.com>
> Subject: Re: [RFC 5/7] drm/i915: Add flip_done_handler definition
> 
> Em sex, 2020-03-06 às 17:09 +0530, Karthik B S escreveu:
> > Send the flip done event in the handler and disable the interrupt.
> >
> > Signed-off-by: Karthik B S <karthik.b.s@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_irq.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c index 5955e737a45d..1feda9aecf4a
> > 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -1243,6 +1243,24 @@ display_pipe_crc_irq_handler(struct
> drm_i915_private *dev_priv,
> >  			     u32 crc4) {}
> >  #endif
> >
> > +static void flip_done_handler(struct drm_i915_private *dev_priv,
> > +			      unsigned int pipe)
> 
> The compiler is going to complain that we added a static function with no
> caller.
> 
> See my comment on commit 1: please squash this patch with the one that
> makes use of the new function.

Sure. Will restructure the patches as per your feedback. Thanks.
> 
> > +{
> > +	struct intel_crtc *crtc = intel_get_crtc_for_pipe(dev_priv, pipe);
> > +	struct drm_crtc_state *crtc_state = crtc->base.state;
> > +	struct drm_device *dev = &dev_priv->drm;
> > +	unsigned long irqflags;
> > +
> > +	spin_lock_irqsave(&dev->event_lock, irqflags);
> > +
> > +	if (crtc_state->event->base.event->type ==
> DRM_EVENT_FLIP_COMPLETE) {
> > +		drm_crtc_send_vblank_event(&crtc->base, crtc_state-
> >event);
> > +		crtc_state->event = NULL;
> > +	}
> > +
> > +	spin_unlock_irqrestore(&dev->event_lock, irqflags);
> > +	icl_disable_flip_done(&crtc->base);
> > +}
> >
> >  static void hsw_pipe_crc_irq_handler(struct drm_i915_private *dev_priv,
> >  				     enum pipe pipe)

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