[PATCH 1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[PATCH 1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Dhinakaran Pandiyan]

i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to
DIRTYFB. The callers however are at a vantage point to decide if hardware
frontbuffer tracking can do the flush for us. For example, legacy cursor
updates, like flips, write to MMIO registers, which then triggers PSR flush
by the hardware. Moving frontbuffer_flush out will enable us to skip a
software initiated flush by setting origin to FLIP. Thanks to Chris for the
idea.

Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c      |  9 ++++-----
 drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
 drivers/gpu/drm/i915/intel_fbdev.c   |  6 +++---
 drivers/gpu/drm/i915/intel_overlay.c |  1 +
 4 files changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fc68b35854df..22b598e1e0b9 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
 }
 
 /*
- * Prepare buffer for display plane (scanout, cursors, etc).
- * Can be called from an uninterruptible phase (modesetting) and allows
- * any flushes to be pipelined (for pageflips).
+ * Prepare buffer for display plane (scanout, cursors, etc). Can be called from
+ * an uninterruptible phase (modesetting) and allows any flushes to be pipelined
+ * (for pageflips). We only flush the caches while preparing the buffer for
+ * display, the callers are responsible for frontbuffer flush.
  */
 struct i915_vma *
 i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
@@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
 
 	vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
 
-	/* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
 	__i915_gem_object_flush_for_display(obj);
-	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
 
 	/* It should now be out of any other write domains, and we can update
 	 * the domain values for our changes.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 286a9591d179..2b70714ead0f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	return;
 
 valid_fb:
+	obj = intel_fb_obj(fb);
 	mutex_lock(&dev->struct_mutex);
 	intel_state->vma =
 		intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
+
+	if (!IS_ERR(intel_state->vma))
+		intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
 	mutex_unlock(&dev->struct_mutex);
+
 	if (IS_ERR(intel_state->vma)) {
 		DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
 			  intel_crtc->pipe, PTR_ERR(intel_state->vma));
@@ -2832,7 +2837,6 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 	intel_state->base.src = drm_plane_state_src(plane_state);
 	intel_state->base.dst = drm_plane_state_dest(plane_state);
 
-	obj = intel_fb_obj(fb);
 	if (i915_gem_object_is_tiled(obj))
 		dev_priv->preserve_bios_swizzle = true;
 
@@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		struct i915_vma *vma;
 
 		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
-		if (!IS_ERR(vma))
+		if (!IS_ERR(vma)) {
 			to_intel_plane_state(new_state)->vma = vma;
-		else
+			intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
+		} else {
 			ret =  PTR_ERR(vma);
+		}
 	}
 
 	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
@@ -13136,6 +13142,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 			goto out_unlock;
 		}
 
+		intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
 		to_intel_plane_state(new_plane_state)->vma = vma;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index da48af11eb6b..6104d0eadbbc 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -204,6 +204,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		sizes->fb_height = intel_fb->base.height;
 	}
 
+	fb = &ifbdev->fb->base;
 	mutex_lock(&dev->struct_mutex);
 	intel_runtime_pm_get(dev_priv);
 
@@ -211,11 +212,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	 * This also validates that any existing fb inherited from the
 	 * BIOS is suitable for own access.
 	 */
-	vma = intel_pin_and_fence_fb_obj(&ifbdev->fb->base, DRM_MODE_ROTATE_0);
+	vma = intel_pin_and_fence_fb_obj(fb, DRM_MODE_ROTATE_0);
 	if (IS_ERR(vma)) {
 		ret = PTR_ERR(vma);
 		goto out_unlock;
 	}
+	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
 
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
@@ -226,8 +228,6 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	info->par = helper;
 
-	fb = &ifbdev->fb->base;
-
 	ifbdev->helper.fb = fb;
 
 	strcpy(info->fix.id, "inteldrmfb");
diff --git a/drivers/gpu/drm/i915/intel_overlay.c b/drivers/gpu/drm/i915/intel_overlay.c
index 41e9465d44a8..16bfb68f8e42 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -806,6 +806,7 @@ static int intel_overlay_do_put_image(struct intel_overlay *overlay,
 		ret = PTR_ERR(vma);
 		goto out_pin_section;
 	}
+	intel_fb_obj_flush(new_bo, ORIGIN_DIRTYFB);
 
 	ret = i915_vma_put_fence(vma);
 	if (ret)
-- 
2.14.1

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

[PATCH 2/5] drm/i915/psr: Use more PSR HW tracking.

[Dhinakaran Pandiyan]

From: Rodrigo Vivi <rodrigo.vivi@intel.com>

So far we are using frontbuffer tracking for everything
and ignoring that PSR has a HW capable HW tracking for many
modern usages of GPU on Core platforms and newer Atom ones.

One reason for that is that we were trying to keep same
infrastructure in place for VLV/CHV than the rest of platforms.
But also because when this infrastructure was created
the front-buffer-tracking origin wasn't that good and stable
how it is today after Paulo reworked it to attend FBC cases.

However this PSR implementation without HW tracking died
on gen8LP. And newer platforms are starting to demand more HW
tracking specially with PSR2 cases in mind.

By disabling and re-enabling PSR totally every time we believe
someone is going to change the front buffer content we don't
allow PSR HW tracking to do this job and specially compromising
the whole idea of PSR2 case where the HW tracking detect only
the damaged area and do a partial screen update.

So, from now on, on the platforms that has hw_tracking let's
rely more on HW tracking.

This also is the case in used by other drivers and more validated
by SV teams. So I hope that this will lead us to less misterious
bugs.

v2: Only do this for platform that actually has hw tracking.

v3 from DK
Do this only for flips, small gradual changes are better.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Jim Bride <jim.bride@linux.intel.com>
Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |  1 +
 drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_psr.c         | 10 +++++++++-
 4 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 90eca42ab2b8..31aae988d515 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -770,6 +770,7 @@ struct i915_psr {
 	bool y_cord_support;
 	bool colorimetry_support;
 	bool alpm;
+	bool has_hw_tracking;
 
 	void (*enable_source)(struct intel_dp *,
 			      const struct intel_crtc_state *);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 898064e8bea7..7192045c5149 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1862,7 +1862,8 @@ void intel_psr_enable(struct intel_dp *intel_dp,
 void intel_psr_disable(struct intel_dp *intel_dp,
 		      const struct intel_crtc_state *old_crtc_state);
 void intel_psr_invalidate(struct drm_i915_private *dev_priv,
-			  unsigned frontbuffer_bits);
+			  unsigned frontbuffer_bits,
+			  enum fb_op_origin origin);
 void intel_psr_flush(struct drm_i915_private *dev_priv,
 		     unsigned frontbuffer_bits,
 		     enum fb_op_origin origin);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index fcfc217e754e..efda1af9a5b3 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -79,7 +79,7 @@ void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 		spin_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_psr_invalidate(dev_priv, frontbuffer_bits);
+	intel_psr_invalidate(dev_priv, frontbuffer_bits, origin);
 	intel_edp_drrs_invalidate(dev_priv, frontbuffer_bits);
 	intel_fbc_invalidate(dev_priv, frontbuffer_bits, origin);
 }
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2ef374f936b9..2a31c7cbdb41 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -824,6 +824,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
  * intel_psr_invalidate - Invalidade PSR
  * @dev_priv: i915 device
  * @frontbuffer_bits: frontbuffer plane tracking bits
+ * @origin: which operation caused the invalidate
  *
  * Since the hardware frontbuffer tracking has gaps we need to integrate
  * with the software frontbuffer tracking. This function gets called every
@@ -833,7 +834,7 @@ void intel_psr_single_frame_update(struct drm_i915_private *dev_priv,
  * Dirty frontbuffers relevant to PSR are tracked in busy_frontbuffer_bits."
  */
 void intel_psr_invalidate(struct drm_i915_private *dev_priv,
-			  unsigned frontbuffer_bits)
+			  unsigned frontbuffer_bits, enum fb_op_origin origin)
 {
 	struct drm_crtc *crtc;
 	enum pipe pipe;
@@ -841,6 +842,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
 	if (!CAN_PSR(dev_priv))
 		return;
 
+	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
+		return;
+
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -881,6 +885,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
 	if (!CAN_PSR(dev_priv))
 		return;
 
+	if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
+		return;
+
 	mutex_lock(&dev_priv->psr.lock);
 	if (!dev_priv->psr.enabled) {
 		mutex_unlock(&dev_priv->psr.lock);
@@ -957,6 +964,7 @@ void intel_psr_init(struct drm_i915_private *dev_priv)
 		dev_priv->psr.activate = vlv_psr_activate;
 		dev_priv->psr.setup_vsc = vlv_psr_setup_vsc;
 	} else {
+		dev_priv->psr.has_hw_tracking = true;
 		dev_priv->psr.enable_source = hsw_psr_enable_source;
 		dev_priv->psr.disable_source = hsw_psr_disable;
 		dev_priv->psr.enable_sink = hsw_psr_enable_sink;
-- 
2.14.1

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

[PATCH 3/5] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.

[Dhinakaran Pandiyan]

DRM_IOCTL_MODE_CURSOR results in frontbuffer flush before the cursor
plane MMIOs are written to. But this flush should not be necessary for
PSR as hardware tracking triggers PSR exit when MMIOs are written. As
for FBC, the spec says "Flips or changes to plane size and panning" cause
FBC to be nuked. /sys/kernel/debug/dri/0/i915_fbc_status shows
"Compressing: yes" when I move the cursor around.

v3: Use ORIGIN_FLIP now that pin_to_display does not flush frontbuffer.
v2: Update comment in i915_gem_object_pin_to_display_plane. (Chris)

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2b70714ead0f..24ca43424c44 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13142,7 +13142,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 			goto out_unlock;
 		}
 
-		intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
+		intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
 		to_intel_plane_state(new_plane_state)->vma = vma;
 	}
 
-- 
2.14.1

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

[PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Dhinakaran Pandiyan]

Preparing a framebuffer should not require a flush. _post_plane_update()
takes care of flushing when a flip is scheduled, this should be
sufficient for PSR and FBC.

Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 24ca43424c44..c611855bf05a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 		struct i915_vma *vma;
 
 		vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
-		if (!IS_ERR(vma)) {
+		if (!IS_ERR(vma))
 			to_intel_plane_state(new_state)->vma = vma;
-			intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
-		} else {
+		else
 			ret =  PTR_ERR(vma);
-		}
 	}
 
 	i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
-- 
2.14.1

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

[PATCH 5/5] drm/i915/psr: Wait for PSR transition to complete before exiting.

[Dhinakaran Pandiyan]

With fbdev, screen freezes after a few continuous PSR exit->enter cycles.
Printing out the PSR status register clearly showed this freeze coincided
with exiting when the hardware is in a transitory state. So wait for a max
of 100 ms (~6 frames) for PSR to become active and then exit.

Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 2a31c7cbdb41..d6669f5f890f 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -738,6 +738,17 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
 			WARN_ON(!(val & EDP_PSR2_ENABLE));
 			I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
 		} else {
+			/* Wait for about 6 frames in case we just enabled PSR,
+			 * this prevents the screen from freezing as the HW does
+			 * not seem to be able to back off cleanly it is already
+			 * trying to enter PSR.
+			 */
+			intel_wait_for_register(dev_priv,
+						EDP_PSR_STATUS,
+						EDP_PSR_STATUS_STATE_MASK,
+						EDP_PSR_STATUS_STATE_SRDENT,
+						100);
+
 			val = I915_READ(EDP_PSR_CTL);
 			WARN_ON(!(val & EDP_PSR_ENABLE));
 			I915_WRITE(EDP_PSR_CTL, val & ~EDP_PSR_ENABLE);
-- 
2.14.1

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

✗ Fi.CI.CHECKPATCH: warning for series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
URL   : https://patchwork.freedesktop.org/series/38410/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
28b7f4f93d2f drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
8721f68002db drm/i915/psr: Use more PSR HW tracking.
-:65: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#65: FILE: drivers/gpu/drm/i915/intel_drv.h:1865:
+			  unsigned frontbuffer_bits,

-:100: WARNING: Prefer 'unsigned int' to bare use of 'unsigned'
#100: FILE: drivers/gpu/drm/i915/intel_psr.c:837:
+			  unsigned frontbuffer_bits, enum fb_op_origin origin)

total: 0 errors, 2 warnings, 0 checks, 64 lines checked
787bd9dab50d drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
478be48e1ec8 drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()
c1b5cfe64bc4 drm/i915/psr: Wait for PSR transition to complete before exiting.

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

✓ Fi.CI.BAT: success for series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
URL   : https://patchwork.freedesktop.org/series/38410/
State : success

== Summary ==

Series 38410v1 series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
https://patchwork.freedesktop.org/api/1.0/series/38410/revisions/1/mbox/

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                pass       -> FAIL       (fi-gdg-551) fdo#102575
Test prime_vgem:
        Subgroup basic-fence-flip:
                pass       -> FAIL       (fi-ilk-650) fdo#104008

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:288  pass:267  dwarn:0   dfail:0   fail:0   skip:21  time:425s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:429s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:376s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:497s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:289s
fi-bxt-dsi       total:288  pass:258  dwarn:0   dfail:0   fail:0   skip:30  time:483s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:490s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:475s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:463s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:569s
fi-cnl-y3        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:581s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:419s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:283s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:519s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:393s
fi-ilk-650       total:288  pass:227  dwarn:0   dfail:0   fail:1   skip:60  time:417s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:463s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:460s
fi-kbl-7560u     total:288  pass:269  dwarn:0   dfail:0   fail:0   skip:19  time:492s
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:500s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:593s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:433s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:526s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:472s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:417s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:430s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:533s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:399s
Blacklisted hosts:
fi-glk-dsi       total:117  pass:105  dwarn:0   dfail:0   fail:0   skip:12 

5b7aaf330f88098deefe45bc69af7908e9db382d drm-tip: 2018y-02m-16d-00h-00m-51s UTC integration manifest
c1b5cfe64bc4 drm/i915/psr: Wait for PSR transition to complete before exiting.
478be48e1ec8 drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()
787bd9dab50d drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
8721f68002db drm/i915/psr: Use more PSR HW tracking.
28b7f4f93d2f drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

== Logs ==

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

Re: [PATCH 1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Chris Wilson]

Quoting Dhinakaran Pandiyan (2018-02-16 04:33:18)
> i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to
> DIRTYFB. The callers however are at a vantage point to decide if hardware
> frontbuffer tracking can do the flush for us. For example, legacy cursor
> updates, like flips, write to MMIO registers, which then triggers PSR flush
> by the hardware. Moving frontbuffer_flush out will enable us to skip a
> software initiated flush by setting origin to FLIP. Thanks to Chris for the
> idea.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      |  9 ++++-----
>  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
>  drivers/gpu/drm/i915/intel_fbdev.c   |  6 +++---
>  drivers/gpu/drm/i915/intel_overlay.c |  1 +
>  4 files changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fc68b35854df..22b598e1e0b9 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
>  }
>  
>  /*
> - * Prepare buffer for display plane (scanout, cursors, etc).
> - * Can be called from an uninterruptible phase (modesetting) and allows
> - * any flushes to be pipelined (for pageflips).
> + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from
> + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined
> + * (for pageflips). We only flush the caches while preparing the buffer for
> + * display, the callers are responsible for frontbuffer flush.
>   */
>  struct i915_vma *
>  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
>  
>         vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
>  
> -       /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
>         __i915_gem_object_flush_for_display(obj);
> -       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
>  
>         /* It should now be out of any other write domains, and we can update
>          * the domain values for our changes.
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 286a9591d179..2b70714ead0f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>         return;
>  
>  valid_fb:
> +       obj = intel_fb_obj(fb);
>         mutex_lock(&dev->struct_mutex);
>         intel_state->vma =
>                 intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> +
> +       if (!IS_ERR(intel_state->vma))
> +               intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);

The fb_obj_flush doesn't need to be inside the lock, so you can do it
after the normal error testing (or just do it since the flush doesn't
depend on having the vma bound).

>         mutex_unlock(&dev->struct_mutex);
> +
>         if (IS_ERR(intel_state->vma)) {
>                 DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
>                           intel_crtc->pipe, PTR_ERR(intel_state->vma));
> @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>                 struct i915_vma *vma;
>  
>                 vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> -               if (!IS_ERR(vma))
> +               if (!IS_ERR(vma)) {
>                         to_intel_plane_state(new_state)->vma = vma;
> -               else
> +                       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> +               } else {
>                         ret =  PTR_ERR(vma);

Would
	if (IS_ERR(vma)) {
		ret = PTR_ERR(vma);
		break;
	}

	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
	to_intel_plane_state(new_state)->vma = vma;

look neater here?

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

Re: [PATCH 2/5] drm/i915/psr: Use more PSR HW tracking.

[Chris Wilson]

Quoting Dhinakaran Pandiyan (2018-02-16 04:33:19)
> From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> So far we are using frontbuffer tracking for everything
> and ignoring that PSR has a HW capable HW tracking for many
> modern usages of GPU on Core platforms and newer Atom ones.
> 
> One reason for that is that we were trying to keep same
> infrastructure in place for VLV/CHV than the rest of platforms.
> But also because when this infrastructure was created
> the front-buffer-tracking origin wasn't that good and stable
> how it is today after Paulo reworked it to attend FBC cases.
> 
> However this PSR implementation without HW tracking died
> on gen8LP. And newer platforms are starting to demand more HW
> tracking specially with PSR2 cases in mind.
> 
> By disabling and re-enabling PSR totally every time we believe
> someone is going to change the front buffer content we don't
> allow PSR HW tracking to do this job and specially compromising
> the whole idea of PSR2 case where the HW tracking detect only
> the damaged area and do a partial screen update.
> 
> So, from now on, on the platforms that has hw_tracking let's
> rely more on HW tracking.
> 
> This also is the case in used by other drivers and more validated
> by SV teams. So I hope that this will lead us to less misterious
> bugs.
> 
> v2: Only do this for platform that actually has hw tracking.
> 
> v3 from DK
> Do this only for flips, small gradual changes are better.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h          |  1 +
>  drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
>  drivers/gpu/drm/i915/intel_psr.c         | 10 +++++++++-
>  4 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 90eca42ab2b8..31aae988d515 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -770,6 +770,7 @@ struct i915_psr {
>         bool y_cord_support;
>         bool colorimetry_support;
>         bool alpm;
> +       bool has_hw_tracking;

Time for some bool:1 compaction?

> @@ -841,6 +842,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
>         if (!CAN_PSR(dev_priv))
>                 return;
>  
> +       if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> +               return;
> +
>         mutex_lock(&dev_priv->psr.lock);
>         if (!dev_priv->psr.enabled) {
>                 mutex_unlock(&dev_priv->psr.lock);
> @@ -881,6 +885,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
>         if (!CAN_PSR(dev_priv))
>                 return;
>  
> +       if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> +               return;
> +

Much easier for the causal reader to understand :)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Chris Wilson]

Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> Preparing a framebuffer should not require a flush. _post_plane_update()
> takes care of flushing when a flip is scheduled, this should be
> sufficient for PSR and FBC.

Makes sense.
 
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

Also
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
to validate the flow through atomic.
-Chris

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 24ca43424c44..c611855bf05a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>                 struct i915_vma *vma;
>  
>                 vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> -               if (!IS_ERR(vma)) {
> +               if (!IS_ERR(vma))
>                         to_intel_plane_state(new_state)->vma = vma;
> -                       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> -               } else {
> +               else
>                         ret =  PTR_ERR(vma);
> -               }
>         }
>  
>         i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> -- 
> 2.14.1
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915/psr: Wait for PSR transition to complete before exiting.

[Chris Wilson]

Quoting Dhinakaran Pandiyan (2018-02-16 04:33:22)
> With fbdev, screen freezes after a few continuous PSR exit->enter cycles.
> Printing out the PSR status register clearly showed this freeze coincided
> with exiting when the hardware is in a transitory state. So wait for a max
> of 100 ms (~6 frames) for PSR to become active and then exit.
> 
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 2a31c7cbdb41..d6669f5f890f 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -738,6 +738,17 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
>                         WARN_ON(!(val & EDP_PSR2_ENABLE));
>                         I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
>                 } else {
> +                       /* Wait for about 6 frames in case we just enabled PSR,
> +                        * this prevents the screen from freezing as the HW does
> +                        * not seem to be able to back off cleanly it is already
> +                        * trying to enter PSR.
> +                        */
> +                       intel_wait_for_register(dev_priv,
> +                                               EDP_PSR_STATUS,
> +                                               EDP_PSR_STATUS_STATE_MASK,
> +                                               EDP_PSR_STATUS_STATE_SRDENT,
> +                                               100);

I'm going to suggest that you want a DRM_DEBUG_KMS() (_DRIVER? Not sure
what works best for PSR/frontbuffer-tracking) at least for spotting
trouble when it times out. I would start with a DRM_ERROR for a few CI
passes just to see what's happening on our machines before toning it
down for production.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Patchwork]

== Series Details ==

Series: series starting with [1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
URL   : https://patchwork.freedesktop.org/series/38410/
State : success

== Summary ==

Test kms_flip:
        Subgroup dpms-vs-vblank-race-interruptible:
                fail       -> PASS       (shard-hsw) fdo#103060 +1

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060

shard-apl        total:3397 pass:1767 dwarn:1   dfail:0   fail:20  skip:1608 time:12477s
shard-hsw        total:3427 pass:1759 dwarn:1   dfail:0   fail:10  skip:1656 time:11764s
shard-snb        total:3427 pass:1347 dwarn:1   dfail:0   fail:12  skip:2067 time:6690s

== Logs ==

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

Re: [PATCH 5/5] drm/i915/psr: Wait for PSR transition to complete before exiting.

[Pandiyan, Dhinakaran]

On Fri, 2018-02-16 at 08:58 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:22)
> > With fbdev, screen freezes after a few continuous PSR exit->enter cycles.
> > Printing out the PSR status register clearly showed this freeze coincided
> > with exiting when the hardware is in a transitory state. So wait for a max
> > of 100 ms (~6 frames) for PSR to become active and then exit.
> > 
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 11 +++++++++++
> >  1 file changed, 11 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 2a31c7cbdb41..d6669f5f890f 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -738,6 +738,17 @@ static void intel_psr_exit(struct drm_i915_private *dev_priv)
> >                         WARN_ON(!(val & EDP_PSR2_ENABLE));
> >                         I915_WRITE(EDP_PSR2_CTL, val & ~EDP_PSR2_ENABLE);
> >                 } else {
> > +                       /* Wait for about 6 frames in case we just enabled PSR,
> > +                        * this prevents the screen from freezing as the HW does
> > +                        * not seem to be able to back off cleanly it is already
> > +                        * trying to enter PSR.
> > +                        */
> > +                       intel_wait_for_register(dev_priv,
> > +                                               EDP_PSR_STATUS,
> > +                                               EDP_PSR_STATUS_STATE_MASK,
> > +                                               EDP_PSR_STATUS_STATE_SRDENT,
> > +                                               100);
> 
> I'm going to suggest that you want a DRM_DEBUG_KMS() (_DRIVER? Not sure
> what works best for PSR/frontbuffer-tracking) at least for spotting
> trouble when it times out. I would start with a DRM_ERROR for a few CI
> passes just to see what's happening on our machines before toning it
> down for production.

Thanks for the suggestion, makes sense. I don't think we have a test to
emulate a blinking fbcon cursor, which happens to trigger continuous PSR
flushes. I'll look into it.


-DK


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

Re: [PATCH 1/5] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display

[Pandiyan, Dhinakaran]

On Fri, 2018-02-16 at 08:52 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:18)
> > i915_gem_obj_pin_to_display() calls frontbuffer_flush with origin set to
> > DIRTYFB. The callers however are at a vantage point to decide if hardware
> > frontbuffer tracking can do the flush for us. For example, legacy cursor
> > updates, like flips, write to MMIO registers, which then triggers PSR flush
> > by the hardware. Moving frontbuffer_flush out will enable us to skip a
> > software initiated flush by setting origin to FLIP. Thanks to Chris for the
> > idea.
> > 
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem.c      |  9 ++++-----
> >  drivers/gpu/drm/i915/intel_display.c | 13 ++++++++++---
> >  drivers/gpu/drm/i915/intel_fbdev.c   |  6 +++---
> >  drivers/gpu/drm/i915/intel_overlay.c |  1 +
> >  4 files changed, 18 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index fc68b35854df..22b598e1e0b9 100644
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4071,9 +4071,10 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data,
> >  }
> >  
> >  /*
> > - * Prepare buffer for display plane (scanout, cursors, etc).
> > - * Can be called from an uninterruptible phase (modesetting) and allows
> > - * any flushes to be pipelined (for pageflips).
> > + * Prepare buffer for display plane (scanout, cursors, etc). Can be called from
> > + * an uninterruptible phase (modesetting) and allows any flushes to be pipelined
> > + * (for pageflips). We only flush the caches while preparing the buffer for
> > + * display, the callers are responsible for frontbuffer flush.
> >   */
> >  struct i915_vma *
> >  i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> > @@ -4139,9 +4140,7 @@ i915_gem_object_pin_to_display_plane(struct drm_i915_gem_object *obj,
> >  
> >         vma->display_alignment = max_t(u64, vma->display_alignment, alignment);
> >  
> > -       /* Treat this as an end-of-frame, like intel_user_framebuffer_dirty() */
> >         __i915_gem_object_flush_for_display(obj);
> > -       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> >  
> >         /* It should now be out of any other write domains, and we can update
> >          * the domain values for our changes.
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 286a9591d179..2b70714ead0f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2806,10 +2806,15 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >         return;
> >  
> >  valid_fb:
> > +       obj = intel_fb_obj(fb);
> >         mutex_lock(&dev->struct_mutex);
> >         intel_state->vma =
> >                 intel_pin_and_fence_fb_obj(fb, primary->state->rotation);
> > +
> > +       if (!IS_ERR(intel_state->vma))
> > +               intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> 
> The fb_obj_flush doesn't need to be inside the lock, so you can do it
> after the normal error testing (or just do it since the flush doesn't
> depend on having the vma bound).

Okay, that answers the question I meant to ask. Decided to err on the
side of caution.
 
> 
> >         mutex_unlock(&dev->struct_mutex);
> > +
> >         if (IS_ERR(intel_state->vma)) {
> >                 DRM_ERROR("failed to pin boot fb on pipe %d: %li\n",
> >                           intel_crtc->pipe, PTR_ERR(intel_state->vma));
> > @@ -12713,10 +12717,12 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >                 struct i915_vma *vma;
> >  
> >                 vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > -               if (!IS_ERR(vma))
> > +               if (!IS_ERR(vma)) {
> >                         to_intel_plane_state(new_state)->vma = vma;
> > -               else
> > +                       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +               } else {
> >                         ret =  PTR_ERR(vma);
> 
> Would
> 	if (IS_ERR(vma)) {
> 		ret = PTR_ERR(vma);
> 		break;
There's no loop around this, but this change goes away in [Patch 4/5].


> 	}
> 
> 	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> 	to_intel_plane_state(new_state)->vma = vma;
> 
> look neater here?
> 
> Lgtm,
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Pandiyan, Dhinakaran]

On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > Preparing a framebuffer should not require a flush. _post_plane_update()
> > takes care of flushing when a flip is scheduled, this should be
> > sufficient for PSR and FBC.
> 
> Makes sense.
>  
I also think this might speed up the flips a bit by avoiding flushes. 

> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> 
> Also
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> to validate the flow through atomic.
> -Chris
> 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 24ca43424c44..c611855bf05a 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12717,12 +12717,10 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >                 struct i915_vma *vma;
> >  
> >                 vma = intel_pin_and_fence_fb_obj(fb, new_state->rotation);
> > -               if (!IS_ERR(vma)) {
> > +               if (!IS_ERR(vma))
> >                         to_intel_plane_state(new_state)->vma = vma;
> > -                       intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > -               } else {
> > +               else
> >                         ret =  PTR_ERR(vma);
> > -               }
> >         }
> >  
> >         i915_gem_object_wait_priority(obj, 0, I915_PRIORITY_DISPLAY);
> > -- 
> > 2.14.1
> > 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Maarten Lankhorst]

Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
>> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
>>> Preparing a framebuffer should not require a flush. _post_plane_update()
>>> takes care of flushing when a flip is scheduled, this should be
>>> sufficient for PSR and FBC.
>> Makes sense.
>>  
> I also think this might speed up the flips a bit by avoiding flushes. 
>
>>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
>> Also
>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> to validate the flow through atomic.
>> -Chris
>>
Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?


Then again, seems like frontbuffer tracking should be done per crtc..
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Pandiyan, Dhinakaran]

On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> >>> takes care of flushing when a flip is scheduled, this should be
> >>> sufficient for PSR and FBC.
> >> Makes sense.
> >>  
> > I also think this might speed up the flips a bit by avoiding flushes. 
> >
> >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> >> Also
> >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> to validate the flow through atomic.
> >> -Chris
> >>
> Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> 

I have no context why it was removed, I'll have to understand that
change and get back to you.

> 
> Then again, seems like frontbuffer tracking should be done per crtc..
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > >>> takes care of flushing when a flip is scheduled, this should be
> > >>> sufficient for PSR and FBC.
> > >> Makes sense.
> > >>  
> > > I also think this might speed up the flips a bit by avoiding flushes. 
> > >
> > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > >> Also
> > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > >> to validate the flow through atomic.
> > >> -Chris
> > >>
> > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > 
> 
> I have no context why it was removed, I'll have to understand that
> change and get back to you.

Since we supposedly have hw nuke for both fbc and psr there doesn't seem
to be much need to do anything for flips. I guess DRRS is the only
thing that kinda needs it (not really, just avoids flipping with the
slow timings). But I think DRRS should really be tied into the vblank
stuff somehow so that we switch to the fast timings whenever a vblank
interrupts are enabled.

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > >>> takes care of flushing when a flip is scheduled, this should be
> > > >>> sufficient for PSR and FBC.
> > > >> Makes sense.
> > > >>  
> > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > >
> > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > >> Also
> > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > >> to validate the flow through atomic.
> > > >> -Chris
> > > >>
> > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > 
> > 
> > I have no context why it was removed, I'll have to understand that
> > change and get back to you.
> 
> Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> to be much need to do anything for flips. I guess DRRS is the only
> thing that kinda needs it (not really, just avoids flipping with the
> slow timings). But I think DRRS should really be tied into the vblank
> stuff somehow so that we switch to the fast timings whenever a vblank
> interrupts are enabled.

Oh, I guess VLV/CHV PSR is what would need this. To do that properly
(ie. main link off) I think we'd basically need to do a full modeset
when exiting PSR, so it should probably handled somewhere higher up
during modeset, and for other uses the frontbuffer tracking
should perhaps just schedule a work to do the full modeset.

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Pandiyan, Dhinakaran]

On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > 
> > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > >>> sufficient for PSR and FBC.
> > > > >> Makes sense.
> > > > >>  
> > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > >
> > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > >> Also
> > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > >> to validate the flow through atomic.
> > > > >> -Chris
> > > > >>
> > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > 
> > > 
> > > I have no context why it was removed, I'll have to understand that
> > > change and get back to you.
> > 
> > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > to be much need to do anything for flips. I guess DRRS is the only
> > thing that kinda needs it (not really, just avoids flipping with the
> > slow timings). But I think DRRS should really be tied into the vblank
> > stuff somehow so that we switch to the fast timings whenever a vblank
> > interrupts are enabled.
> 
> Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> (ie. main link off) I think we'd basically need to do a full modeset
> when exiting PSR, so it should probably handled somewhere higher up
> during modeset, and for other uses the frontbuffer tracking
> should perhaps just schedule a work to do the full modeset.
> 
The mention of "full modeset" got me thinking. I believe you said full
modeset because the link needs to be trained on PSR exit if it was off.
But, link off isn't supported on VLV/CHV

else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
	/* On VLV and CHV only standby mode is supported. */
	dev_priv->psr.link_standby = true;


So we are good here?



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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > 
> > > > 
> > > > 
> > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > >>> sufficient for PSR and FBC.
> > > > > >> Makes sense.
> > > > > >>  
> > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > >
> > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > >> Also
> > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > >> to validate the flow through atomic.
> > > > > >> -Chris
> > > > > >>
> > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > 
> > > > 
> > > > I have no context why it was removed, I'll have to understand that
> > > > change and get back to you.
> > > 
> > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > to be much need to do anything for flips. I guess DRRS is the only
> > > thing that kinda needs it (not really, just avoids flipping with the
> > > slow timings). But I think DRRS should really be tied into the vblank
> > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > interrupts are enabled.
> > 
> > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > (ie. main link off) I think we'd basically need to do a full modeset
> > when exiting PSR, so it should probably handled somewhere higher up
> > during modeset, and for other uses the frontbuffer tracking
> > should perhaps just schedule a work to do the full modeset.
> > 
> The mention of "full modeset" got me thinking. I believe you said full
> modeset because the link needs to be trained on PSR exit if it was off.
> But, link off isn't supported on VLV/CHV
> 
> else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> 	/* On VLV and CHV only standby mode is supported. */
> 	dev_priv->psr.link_standby = true;

I think that's just because we've been lazy and done it. I think even
with the link off we'd need to reprogram all planes at least.

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > 
> > 
> > 
> > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > >>> sufficient for PSR and FBC.
> > > > > > >> Makes sense.
> > > > > > >>  
> > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > >
> > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > >> Also
> > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > >> to validate the flow through atomic.
> > > > > > >> -Chris
> > > > > > >>
> > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > 
> > > > > 
> > > > > I have no context why it was removed, I'll have to understand that
> > > > > change and get back to you.
> > > > 
> > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > interrupts are enabled.
> > > 
> > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > (ie. main link off) I think we'd basically need to do a full modeset
> > > when exiting PSR, so it should probably handled somewhere higher up
> > > during modeset, and for other uses the frontbuffer tracking
> > > should perhaps just schedule a work to do the full modeset.
> > > 
> > The mention of "full modeset" got me thinking. I believe you said full
> > modeset because the link needs to be trained on PSR exit if it was off.
> > But, link off isn't supported on VLV/CHV
> > 
> > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > 	/* On VLV and CHV only standby mode is supported. */
> > 	dev_priv->psr.link_standby = true;
> 
> I think that's just because we've been lazy and done it. I think even
> with the link off we'd need to reprogram all planes at least.

Not had coffee yet today, and it shows. Let me rewrite that entire thing:

I think that's just because we've been lazy and not done it (link off mode).
I think even without the link off we'd need to reprogram all planes at least.

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Rodrigo Vivi]

On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > 
> > > 
> > > 
> > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > >> Makes sense.
> > > > > > > >>  
> > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > >
> > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > >> Also
> > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > >> to validate the flow through atomic.
> > > > > > > >> -Chris
> > > > > > > >>
> > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > 
> > > > > > 
> > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > change and get back to you.
> > > > > 
> > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > interrupts are enabled.
> > > > 
> > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > during modeset, and for other uses the frontbuffer tracking
> > > > should perhaps just schedule a work to do the full modeset.
> > > > 
> > > The mention of "full modeset" got me thinking. I believe you said full
> > > modeset because the link needs to be trained on PSR exit if it was off.
> > > But, link off isn't supported on VLV/CHV
> > > 
> > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > 	/* On VLV and CHV only standby mode is supported. */
> > > 	dev_priv->psr.link_standby = true;
> > 
> > I think that's just because we've been lazy and done it. I think even
> > with the link off we'd need to reprogram all planes at least.
> 
> Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> 
> I think that's just because we've been lazy and not done it (link off mode).

I agree with you here. This was probably exactly what was missing.
But, how to do this without flickering (blinking?!) the screen?

> I think even without the link off we'd need to reprogram all planes at least.

on VLV/CHV or everywhere? and why do you think that?

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote:
> On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > > 
> > > > 
> > > > 
> > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > > >> Makes sense.
> > > > > > > > >>  
> > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > > >
> > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > >> Also
> > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > >> to validate the flow through atomic.
> > > > > > > > >> -Chris
> > > > > > > > >>
> > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > > 
> > > > > > > 
> > > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > > change and get back to you.
> > > > > > 
> > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > > interrupts are enabled.
> > > > > 
> > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > > during modeset, and for other uses the frontbuffer tracking
> > > > > should perhaps just schedule a work to do the full modeset.
> > > > > 
> > > > The mention of "full modeset" got me thinking. I believe you said full
> > > > modeset because the link needs to be trained on PSR exit if it was off.
> > > > But, link off isn't supported on VLV/CHV
> > > > 
> > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > 	/* On VLV and CHV only standby mode is supported. */
> > > > 	dev_priv->psr.link_standby = true;
> > > 
> > > I think that's just because we've been lazy and done it. I think even
> > > with the link off we'd need to reprogram all planes at least.
> > 
> > Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> > 
> > I think that's just because we've been lazy and not done it (link off mode).
> 
> I agree with you here. This was probably exactly what was missing.
> But, how to do this without flickering (blinking?!) the screen?
> 
> > I think even without the link off we'd need to reprogram all planes at least.
> 
> on VLV/CHV or everywhere? and why do you think that?

VLV/CHV. I believe the hw implements PSR by simplty turning off the
planes (and pipes+dplls for link off), but it doesn't have any automagic
stuff for recovering the original state. It's all up to the driver.

IIRC Rodrigo even confirmed this at some point, or at least he had to
trigger a plane update to get something visible on the screen.

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Rodrigo Vivi]

On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote:
> On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote:
> > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > > > 
> > > > > 
> > > > > 
> > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > > > 
> > > > > > > > 
> > > > > > > > 
> > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > > > >> Makes sense.
> > > > > > > > > >>  
> > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > > > >
> > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > > >> Also
> > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > >> to validate the flow through atomic.
> > > > > > > > > >> -Chris
> > > > > > > > > >>
> > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > > > 
> > > > > > > > 
> > > > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > > > change and get back to you.
> > > > > > > 
> > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > > > interrupts are enabled.
> > > > > > 
> > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > > > during modeset, and for other uses the frontbuffer tracking
> > > > > > should perhaps just schedule a work to do the full modeset.
> > > > > > 
> > > > > The mention of "full modeset" got me thinking. I believe you said full
> > > > > modeset because the link needs to be trained on PSR exit if it was off.
> > > > > But, link off isn't supported on VLV/CHV
> > > > > 
> > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > 	/* On VLV and CHV only standby mode is supported. */
> > > > > 	dev_priv->psr.link_standby = true;
> > > > 
> > > > I think that's just because we've been lazy and done it. I think even
> > > > with the link off we'd need to reprogram all planes at least.
> > > 
> > > Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> > > 
> > > I think that's just because we've been lazy and not done it (link off mode).
> > 
> > I agree with you here. This was probably exactly what was missing.
> > But, how to do this without flickering (blinking?!) the screen?
> > 
> > > I think even without the link off we'd need to reprogram all planes at least.
> > 
> > on VLV/CHV or everywhere? and why do you think that?
> 
> VLV/CHV. I believe the hw implements PSR by simplty turning off the
> planes (and pipes+dplls for link off), but it doesn't have any automagic
> stuff for recovering the original state. It's all up to the driver.
> 
> IIRC Rodrigo even confirmed this at some point, or at least he had to
> trigger a plane update to get something visible on the screen.

My memory is terrible. But this makes so much sense and in sync
with some vague memory flashes here :)

And probably what blocked me for looking there besides the
other bugs on core platforms was my disbelieve that would
be possible without seeing any blink of a blank screen when
exiting PSR. :/

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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Pandiyan, Dhinakaran]

On Thu, 2018-03-01 at 11:00 -0800, Rodrigo Vivi wrote:
> On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote:
> > > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > > > > >> Makes sense.
> > > > > > > > > > >>  
> > > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > > > > >
> > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > > > >> Also
> > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > > >> to validate the flow through atomic.
> > > > > > > > > > >> -Chris
> > > > > > > > > > >>
> > > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > > > > change and get back to you.
> > > > > > > > 
> > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > > > > interrupts are enabled.
> > > > > > > 
> > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > > > > during modeset, and for other uses the frontbuffer tracking
> > > > > > > should perhaps just schedule a work to do the full modeset.
> > > > > > > 
> > > > > > The mention of "full modeset" got me thinking. I believe you said full
> > > > > > modeset because the link needs to be trained on PSR exit if it was off.
> > > > > > But, link off isn't supported on VLV/CHV
> > > > > > 
> > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > 	/* On VLV and CHV only standby mode is supported. */
> > > > > > 	dev_priv->psr.link_standby = true;
> > > > > 
> > > > > I think that's just because we've been lazy and done it. I think even
> > > > > with the link off we'd need to reprogram all planes at least.
> > > > 
> > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> > > > 
> > > > I think that's just because we've been lazy and not done it (link off mode).
> > > 
> > > I agree with you here. This was probably exactly what was missing.
> > > But, how to do this without flickering (blinking?!) the screen?
> > > 
> > > > I think even without the link off we'd need to reprogram all planes at least.
> > > 
> > > on VLV/CHV or everywhere? and why do you think that?
> > 
> > VLV/CHV. I believe the hw implements PSR by simplty turning off the
> > planes (and pipes+dplls for link off), but it doesn't have any automagic
> > stuff for recovering the original state. It's all up to the driver.
> > 
> > IIRC Rodrigo even confirmed this at some point, or at least he had to
> > trigger a plane update to get something visible on the screen.
> 
> My memory is terrible. But this makes so much sense and in sync
> with some vague memory flashes here :)
> 
> And probably what blocked me for looking there besides the
> other bugs on core platforms was my disbelieve that would
> be possible without seeing any blink of a blank screen when
> exiting PSR. :/
> 

We've got a couple of options here -
1) Drop this patch because it breaks VLV/CHV link standby. 

2) Keep this patch because VLV/CHV link standby is already broken (no
plane programming on exit). Follow this up with reworking PSR on
VLV/CHV.


3) Or should this be -
if (VLV() || CHV)
	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);




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

Re: [PATCH 4/5] drm/i915/frontbuffer: Remove early frontbuffer flush in prepare_plane_fb()

[Ville Syrjälä]

On Thu, Mar 01, 2018 at 11:00:40AM -0800, Rodrigo Vivi wrote:
> On Thu, Mar 01, 2018 at 08:43:05PM +0200, Ville Syrjälä wrote:
> > On Thu, Mar 01, 2018 at 10:35:48AM -0800, Rodrigo Vivi wrote:
> > > On Thu, Mar 01, 2018 at 01:22:50PM +0200, Ville Syrjälä wrote:
> > > > On Thu, Mar 01, 2018 at 12:51:45PM +0200, Ville Syrjälä wrote:
> > > > > On Wed, Feb 28, 2018 at 11:38:56PM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > On Wed, 2018-02-28 at 22:38 +0200, Ville Syrjälä wrote:
> > > > > > > On Wed, Feb 28, 2018 at 10:28:13PM +0200, Ville Syrjälä wrote:
> > > > > > > > On Sat, Feb 24, 2018 at 03:24:55AM +0000, Pandiyan, Dhinakaran wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On Mon, 2018-02-19 at 10:07 +0100, Maarten Lankhorst wrote:
> > > > > > > > > > Op 16-02-18 om 20:27 schreef Pandiyan, Dhinakaran:
> > > > > > > > > > > On Fri, 2018-02-16 at 08:55 +0000, Chris Wilson wrote:
> > > > > > > > > > >> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:21)
> > > > > > > > > > >>> Preparing a framebuffer should not require a flush. _post_plane_update()
> > > > > > > > > > >>> takes care of flushing when a flip is scheduled, this should be
> > > > > > > > > > >>> sufficient for PSR and FBC.
> > > > > > > > > > >> Makes sense.
> > > > > > > > > > >>  
> > > > > > > > > > > I also think this might speed up the flips a bit by avoiding flushes. 
> > > > > > > > > > >
> > > > > > > > > > >>> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > > > > > > > > >>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > > > > > >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > > > > > > >>> Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > > > > > > > > >> Also
> > > > > > > > > > >> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > > > > > >> to validate the flow through atomic.
> > > > > > > > > > >> -Chris
> > > > > > > > > > >>
> > > > > > > > > > Page flips used to do intel_frontbuffer_flip_prepare here, followed by intel_frontbuffer_flip_complete. I think it would make sense to change the patch to do that?
> > > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > I have no context why it was removed, I'll have to understand that
> > > > > > > > > change and get back to you.
> > > > > > > > 
> > > > > > > > Since we supposedly have hw nuke for both fbc and psr there doesn't seem
> > > > > > > > to be much need to do anything for flips. I guess DRRS is the only
> > > > > > > > thing that kinda needs it (not really, just avoids flipping with the
> > > > > > > > slow timings). But I think DRRS should really be tied into the vblank
> > > > > > > > stuff somehow so that we switch to the fast timings whenever a vblank
> > > > > > > > interrupts are enabled.
> > > > > > > 
> > > > > > > Oh, I guess VLV/CHV PSR is what would need this. To do that properly
> > > > > > > (ie. main link off) I think we'd basically need to do a full modeset
> > > > > > > when exiting PSR, so it should probably handled somewhere higher up
> > > > > > > during modeset, and for other uses the frontbuffer tracking
> > > > > > > should perhaps just schedule a work to do the full modeset.
> > > > > > > 
> > > > > > The mention of "full modeset" got me thinking. I believe you said full
> > > > > > modeset because the link needs to be trained on PSR exit if it was off.
> > > > > > But, link off isn't supported on VLV/CHV
> > > > > > 
> > > > > > else if (IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv))
> > > > > > 	/* On VLV and CHV only standby mode is supported. */
> > > > > > 	dev_priv->psr.link_standby = true;
> > > > > 
> > > > > I think that's just because we've been lazy and done it. I think even
> > > > > with the link off we'd need to reprogram all planes at least.
> > > > 
> > > > Not had coffee yet today, and it shows. Let me rewrite that entire thing:
> > > > 
> > > > I think that's just because we've been lazy and not done it (link off mode).
> > > 
> > > I agree with you here. This was probably exactly what was missing.
> > > But, how to do this without flickering (blinking?!) the screen?
> > > 
> > > > I think even without the link off we'd need to reprogram all planes at least.
> > > 
> > > on VLV/CHV or everywhere? and why do you think that?
> > 
> > VLV/CHV. I believe the hw implements PSR by simplty turning off the
> > planes (and pipes+dplls for link off), but it doesn't have any automagic
> > stuff for recovering the original state. It's all up to the driver.
> > 
> > IIRC Rodrigo even confirmed this at some point, or at least he had to
> > trigger a plane update to get something visible on the screen.
> 
> My memory is terrible. But this makes so much sense and in sync
> with some vague memory flashes here :)
> 
> And probably what blocked me for looking there besides the
> other bugs on core platforms was my disbelieve that would
> be possible without seeing any blink of a blank screen when
> exiting PSR. :/

I suppose we should start the pipe/planes back up, and only then we can
tell the sink to stop using the old stored frame.

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

Re: [PATCH 2/5] drm/i915/psr: Use more PSR HW tracking.

[Pandiyan, Dhinakaran]

On Fri, 2018-02-16 at 08:54 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-02-16 04:33:19)
> > From: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > 
> > So far we are using frontbuffer tracking for everything
> > and ignoring that PSR has a HW capable HW tracking for many
> > modern usages of GPU on Core platforms and newer Atom ones.
> > 
> > One reason for that is that we were trying to keep same
> > infrastructure in place for VLV/CHV than the rest of platforms.
> > But also because when this infrastructure was created
> > the front-buffer-tracking origin wasn't that good and stable
> > how it is today after Paulo reworked it to attend FBC cases.
> > 
> > However this PSR implementation without HW tracking died
> > on gen8LP. And newer platforms are starting to demand more HW
> > tracking specially with PSR2 cases in mind.
> > 
> > By disabling and re-enabling PSR totally every time we believe
> > someone is going to change the front buffer content we don't
> > allow PSR HW tracking to do this job and specially compromising
> > the whole idea of PSR2 case where the HW tracking detect only
> > the damaged area and do a partial screen update.
> > 
> > So, from now on, on the platforms that has hw_tracking let's
> > rely more on HW tracking.
> > 
> > This also is the case in used by other drivers and more validated
> > by SV teams. So I hope that this will lead us to less misterious
> > bugs.
> > 
> > v2: Only do this for platform that actually has hw tracking.
> > 
> > v3 from DK
> > Do this only for flips, small gradual changes are better.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Jim Bride <jim.bride@linux.intel.com>
> > Cc: Vathsala Nagaraju <vathsala.nagaraju@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h          |  1 +
> >  drivers/gpu/drm/i915/intel_drv.h         |  3 ++-
> >  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 +-
> >  drivers/gpu/drm/i915/intel_psr.c         | 10 +++++++++-
> >  4 files changed, 13 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 90eca42ab2b8..31aae988d515 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -770,6 +770,7 @@ struct i915_psr {
> >         bool y_cord_support;
> >         bool colorimetry_support;
> >         bool alpm;
> > +       bool has_hw_tracking;
> 
> Time for some bool:1 compaction?

Oddly it increases the binary size, so I didn't make this change in the
new version I sent out -
https://patchwork.freedesktop.org/series/39502/

I also left out
Patch 5/5: The mod_timer patch that Andy sent avoids the problem this
patch works around.
Patch 4/5: frontbuffer flush during prepare_fb() might be necessary to
exit PSR early (before we start updating pipe registers) 


> 
> > @@ -841,6 +842,9 @@ void intel_psr_invalidate(struct drm_i915_private *dev_priv,
> >         if (!CAN_PSR(dev_priv))
> >                 return;
> >  
> > +       if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> > +               return;
> > +
> >         mutex_lock(&dev_priv->psr.lock);
> >         if (!dev_priv->psr.enabled) {
> >                 mutex_unlock(&dev_priv->psr.lock);
> > @@ -881,6 +885,9 @@ void intel_psr_flush(struct drm_i915_private *dev_priv,
> >         if (!CAN_PSR(dev_priv))
> >                 return;
> >  
> > +       if (dev_priv->psr.has_hw_tracking && origin == ORIGIN_FLIP)
> > +               return;
> > +
> 
> Much easier for the causal reader to understand :)
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx