All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
@ 2018-03-07  3:34 Dhinakaran Pandiyan
  2018-03-07  3:34 ` [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags Dhinakaran Pandiyan
                   ` (4 more replies)
  0 siblings, 5 replies; 19+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-07  3:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: Paulo Zanoni, Pandiyan, Dhinakaran

From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>

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.

v2:
Rebased due to Ville adding intel_plane_pin_fb().
Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)

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 | 9 +++++++--
 drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
 drivers/gpu/drm/i915/intel_overlay.c | 1 +
 4 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a5bd07338b46..e4c5a1a22d8b 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,
@@ -4129,9 +4130,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 331084082545..91ce8a0522a3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 		return;
 	}
 
+	obj = intel_fb_obj(fb);
+	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
+
 	plane_state->src_x = 0;
 	plane_state->src_y = 0;
 	plane_state->src_w = fb->width << 16;
@@ -2871,7 +2874,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;
 
@@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
 	if (ret)
 		return ret;
 
+	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
+
 	if (!new_state->fence) { /* implicit fencing */
 		struct dma_fence *fence;
 
@@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	if (ret)
 		goto out_unlock;
 
-	old_fb = old_plane_state->fb;
+	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
 
+	old_fb = old_plane_state->fb;
 	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
 			  intel_plane->frontbuffer_bit);
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6f12adc06365..65a3313723c9 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -221,6 +221,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
 		goto out_unlock;
 	}
 
+	fb = &ifbdev->fb->base;
+	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
+
 	info = drm_fb_helper_alloc_fbi(helper);
 	if (IS_ERR(info)) {
 		DRM_ERROR("Failed to allocate fb_info\n");
@@ -230,8 +233,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 36671a937fa4..c2f10d899329 100644
--- a/drivers/gpu/drm/i915/intel_overlay.c
+++ b/drivers/gpu/drm/i915/intel_overlay.c
@@ -807,6 +807,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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
@ 2018-03-07  3:34 ` Dhinakaran Pandiyan
  2018-03-07 22:53   ` Chris Wilson
  2018-03-07  3:34 ` [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking Dhinakaran Pandiyan
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-07  3:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: Rodrigo Vivi, Dhinakaran Pandiyan, Paulo Zanoni

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. Use origin == ORIGIN_FLIP so that features can ignore
cursor updates in their frontbuffer_flush implementations.

 /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 91ce8a0522a3..18b08e263ee1 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
 	if (ret)
 		goto out_unlock;
 
-	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
+	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
 
 	old_fb = old_plane_state->fb;
 	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
-- 
2.14.1

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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.
  2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
  2018-03-07  3:34 ` [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags Dhinakaran Pandiyan
@ 2018-03-07  3:34 ` Dhinakaran Pandiyan
  2018-03-12 23:16   ` Souza, Jose
  2018-03-07  4:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Patchwork
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 19+ messages in thread
From: Dhinakaran Pandiyan @ 2018-03-07  3:34 UTC (permalink / raw)
  To: intel-gfx; +Cc: Dhinakaran Pandiyan, Rodrigo Vivi

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 7eec99d7fad4..6aab929f354c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -772,6 +772,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 1a7c5addcec3..fdf7dd100dfe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1873,7 +1873,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 3a8d3d06c26a..7fff0a0eceb4 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 	}
 
 	might_sleep();
-	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 05770790a4e9..16b77e19c28e 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -948,6 +948,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
@@ -957,7 +958,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;
@@ -965,6 +966,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);
@@ -1005,6 +1009,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);
@@ -1081,6 +1088,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

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
  2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
  2018-03-07  3:34 ` [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags Dhinakaran Pandiyan
  2018-03-07  3:34 ` [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking Dhinakaran Pandiyan
@ 2018-03-07  4:10 ` Patchwork
  2018-03-07  6:26 ` ✓ Fi.CI.IGT: " Patchwork
  2018-03-07 22:46 ` [PATCH v2 1/3] " Rodrigo Vivi
  4 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-03-07  4:10 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

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

---- Known issues:

Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

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:423s
fi-bdw-gvtdvm    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:422s
fi-blb-e6850     total:288  pass:223  dwarn:1   dfail:0   fail:0   skip:64  time:370s
fi-bsw-n3050     total:288  pass:242  dwarn:0   dfail:0   fail:0   skip:46  time:500s
fi-bwr-2160      total:288  pass:183  dwarn:0   dfail:0   fail:0   skip:105 time:278s
fi-bxt-j4205     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:492s
fi-byt-j1900     total:288  pass:253  dwarn:0   dfail:0   fail:0   skip:35  time:480s
fi-byt-n2820     total:288  pass:249  dwarn:0   dfail:0   fail:0   skip:39  time:467s
fi-cfl-8700k     total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:403s
fi-cfl-s2        total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:578s
fi-cfl-u         total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:508s
fi-elk-e7500     total:288  pass:229  dwarn:0   dfail:0   fail:0   skip:59  time:417s
fi-gdg-551       total:288  pass:179  dwarn:0   dfail:0   fail:1   skip:108 time:286s
fi-glk-1         total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:513s
fi-hsw-4770      total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:398s
fi-ilk-650       total:288  pass:228  dwarn:0   dfail:0   fail:0   skip:60  time:409s
fi-ivb-3520m     total:288  pass:259  dwarn:0   dfail:0   fail:0   skip:29  time:455s
fi-ivb-3770      total:288  pass:255  dwarn:0   dfail:0   fail:0   skip:33  time:421s
fi-kbl-7500u     total:288  pass:263  dwarn:1   dfail:0   fail:0   skip:24  time:470s
fi-kbl-7560u     total:108  pass:104  dwarn:0   dfail:0   fail:0   skip:3  
fi-kbl-7567u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:464s
fi-kbl-r         total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:504s
fi-pnv-d510      total:288  pass:222  dwarn:1   dfail:0   fail:0   skip:65  time:592s
fi-skl-6260u     total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:436s
fi-skl-6600u     total:288  pass:261  dwarn:0   dfail:0   fail:0   skip:27  time:521s
fi-skl-6700hq    total:288  pass:262  dwarn:0   dfail:0   fail:0   skip:26  time:533s
fi-skl-6700k2    total:288  pass:264  dwarn:0   dfail:0   fail:0   skip:24  time:494s
fi-skl-6770hq    total:288  pass:268  dwarn:0   dfail:0   fail:0   skip:20  time:495s
fi-skl-guc       total:288  pass:260  dwarn:0   dfail:0   fail:0   skip:28  time:423s
fi-skl-gvtdvm    total:288  pass:265  dwarn:0   dfail:0   fail:0   skip:23  time:426s
fi-snb-2520m     total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:520s
fi-snb-2600      total:288  pass:248  dwarn:0   dfail:0   fail:0   skip:40  time:388s

8a4eb4556f66d7ab7ad512d5c3d239da8de6b750 drm-tip: 2018y-03m-06d-22h-59m-29s UTC integration manifest
a75e6390e4b3 drm/i915/psr: Use more PSR HW tracking.
132639104cfb drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
e6ae9ffdc6f2 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_8255/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* ✓ Fi.CI.IGT: success for series starting with [v2,1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
  2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
                   ` (2 preceding siblings ...)
  2018-03-07  4:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Patchwork
@ 2018-03-07  6:26 ` Patchwork
  2018-03-07 22:46 ` [PATCH v2 1/3] " Rodrigo Vivi
  4 siblings, 0 replies; 19+ messages in thread
From: Patchwork @ 2018-03-07  6:26 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx

== Series Details ==

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

== Summary ==

---- Known issues:

Test kms_chv_cursor_fail:
        Subgroup pipe-b-256x256-top-edge:
                dmesg-warn -> PASS       (shard-snb) fdo#105185 +3
Test kms_flip:
        Subgroup 2x-plain-flip-ts-check:
                fail       -> PASS       (shard-hsw) fdo#100368 +2
        Subgroup modeset-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
Test kms_frontbuffer_tracking:
        Subgroup fbc-suspend:
                pass       -> FAIL       (shard-apl) fdo#101623
Test kms_rotation_crc:
        Subgroup primary-rotation-180:
                fail       -> PASS       (shard-snb) fdo#103925
Test kms_sysfs_edid_timing:
                warn       -> PASS       (shard-apl) fdo#100047
Test perf:
        Subgroup polling:
                fail       -> PASS       (shard-hsw) fdo#102252 +1

fdo#105185 https://bugs.freedesktop.org/show_bug.cgi?id=105185
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#101623 https://bugs.freedesktop.org/show_bug.cgi?id=101623
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
fdo#102252 https://bugs.freedesktop.org/show_bug.cgi?id=102252

shard-apl        total:3467 pass:1826 dwarn:1   dfail:0   fail:8   skip:1632 time:12329s
shard-hsw        total:3467 pass:1770 dwarn:1   dfail:0   fail:4   skip:1691 time:11883s
shard-snb        total:3467 pass:1361 dwarn:4   dfail:0   fail:1   skip:2101 time:7019s
Blacklisted hosts:
shard-kbl        total:3381 pass:1900 dwarn:1   dfail:0   fail:8   skip:1471 time:9091s

== Logs ==

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
  2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
                   ` (3 preceding siblings ...)
  2018-03-07  6:26 ` ✓ Fi.CI.IGT: " Patchwork
@ 2018-03-07 22:46 ` Rodrigo Vivi
  2018-03-07 22:54   ` Pandiyan, Dhinakaran
  4 siblings, 1 reply; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-07 22:46 UTC (permalink / raw)
  To: Dhinakaran Pandiyan; +Cc: intel-gfx, Paulo Zanoni

On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> 
> 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.
> 
> v2:
> Rebased due to Ville adding intel_plane_pin_fb().
> Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> 
> 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 | 9 +++++++--
>  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
>  drivers/gpu/drm/i915/intel_overlay.c | 1 +
>  4 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a5bd07338b46..e4c5a1a22d8b 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,
> @@ -4129,9 +4130,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() */

Although flush by some definition here is invalidate + flush
I see flush operations as the operation to be performed at the "end-of-frame".
After the operation was done and whatever had to be modified was modified.

I see you patch changing the flush to the creation and begin of the fb handling
like on creating and init fb functions. I believe by that time we are not ready
to exit PSR yet.

What am I missing?

>  	__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 331084082545..91ce8a0522a3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
>  		return;
>  	}
>  
> +	obj = intel_fb_obj(fb);
> +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> +
>  	plane_state->src_x = 0;
>  	plane_state->src_y = 0;
>  	plane_state->src_w = fb->width << 16;
> @@ -2871,7 +2874,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;
>  
> @@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
>  	if (ret)
>  		return ret;
>  
> +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> +
>  	if (!new_state->fence) { /* implicit fencing */
>  		struct dma_fence *fence;
>  
> @@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>  	if (ret)
>  		goto out_unlock;
>  
> -	old_fb = old_plane_state->fb;
> +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
>  
> +	old_fb = old_plane_state->fb;
>  	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
>  			  intel_plane->frontbuffer_bit);
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> index 6f12adc06365..65a3313723c9 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -221,6 +221,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
>  		goto out_unlock;
>  	}
>  
> +	fb = &ifbdev->fb->base;
> +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> +
>  	info = drm_fb_helper_alloc_fbi(helper);
>  	if (IS_ERR(info)) {
>  		DRM_ERROR("Failed to allocate fb_info\n");
> @@ -230,8 +233,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 36671a937fa4..c2f10d899329 100644
> --- a/drivers/gpu/drm/i915/intel_overlay.c
> +++ b/drivers/gpu/drm/i915/intel_overlay.c
> @@ -807,6 +807,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07  3:34 ` [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags Dhinakaran Pandiyan
@ 2018-03-07 22:53   ` Chris Wilson
  2018-03-07 23:10     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 19+ messages in thread
From: Chris Wilson @ 2018-03-07 22:53 UTC (permalink / raw)
  To: Dhinakaran Pandiyan, intel-gfx; +Cc: Dhinakaran

Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> 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. Use origin == ORIGIN_FLIP so that features can ignore
> cursor updates in their frontbuffer_flush implementations.
> 
>  /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 91ce8a0522a3..18b08e263ee1 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
>         if (ret)
>                 goto out_unlock;
>  
> -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);

What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
aiui.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
  2018-03-07 22:46 ` [PATCH v2 1/3] " Rodrigo Vivi
@ 2018-03-07 22:54   ` Pandiyan, Dhinakaran
  2018-03-07 23:20     ` Rodrigo Vivi
  0 siblings, 1 reply; 19+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-07 22:54 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: intel-gfx, Zanoni, Paulo R




On Wed, 2018-03-07 at 14:46 -0800, Rodrigo Vivi wrote:
> On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> > 
> > 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.
> > 
> > v2:
> > Rebased due to Ville adding intel_plane_pin_fb().
> > Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> > 
> > 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 | 9 +++++++--
> >  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
> >  drivers/gpu/drm/i915/intel_overlay.c | 1 +
> >  4 files changed, 15 insertions(+), 9 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > index a5bd07338b46..e4c5a1a22d8b 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,
> > @@ -4129,9 +4130,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() */
> 
> Although flush by some definition here is invalidate + flush
> I see flush operations as the operation to be performed at the "end-of-frame".
> After the operation was done and whatever had to be modified was modified.
> 
> I see you patch changing the flush to the creation and begin of the fb handling
> like on creating and init fb functions. I believe by that time we are not ready
> to exit PSR yet.
> 
> What am I missing?


The functions that you are referring to call
i915_gem_object_pin_to_display_plane() which in turn calls frontbuffer
flush. This patch is just a refactor, doesn't change whether flush() is
called. I am trying to move it up the call stack so that flush() can be
selectively removed depending on the call site.




> 
> >  	__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 331084082545..91ce8a0522a3 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> >  		return;
> >  	}
> >  
> > +	obj = intel_fb_obj(fb);
> > +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +
> >  	plane_state->src_x = 0;
> >  	plane_state->src_y = 0;
> >  	plane_state->src_w = fb->width << 16;
> > @@ -2871,7 +2874,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;
> >  
> > @@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> >  	if (ret)
> >  		return ret;
> >  
> > +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > +
> >  	if (!new_state->fence) { /* implicit fencing */
> >  		struct dma_fence *fence;
> >  
> > @@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> >  	if (ret)
> >  		goto out_unlock;
> >  
> > -	old_fb = old_plane_state->fb;
> > +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> >  
> > +	old_fb = old_plane_state->fb;
> >  	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> >  			  intel_plane->frontbuffer_bit);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 6f12adc06365..65a3313723c9 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -221,6 +221,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> >  		goto out_unlock;
> >  	}
> >  
> > +	fb = &ifbdev->fb->base;
> > +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > +
> >  	info = drm_fb_helper_alloc_fbi(helper);
> >  	if (IS_ERR(info)) {
> >  		DRM_ERROR("Failed to allocate fb_info\n");
> > @@ -230,8 +233,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 36671a937fa4..c2f10d899329 100644
> > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > @@ -807,6 +807,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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07 22:53   ` Chris Wilson
@ 2018-03-07 23:10     ` Pandiyan, Dhinakaran
  2018-03-07 23:23       ` Rodrigo Vivi
  2018-03-09  1:11       ` Pandiyan, Dhinakaran
  0 siblings, 2 replies; 19+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-07 23:10 UTC (permalink / raw)
  To: chris; +Cc: Dhinakaran, intel-gfx

On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > cursor updates in their frontbuffer_flush implementations.
> > 
> >  /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 91ce8a0522a3..18b08e263ee1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> >         if (ret)
> >                 goto out_unlock;
> >  
> > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> 
> What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> aiui.
> -Chris


That was the idea but there's a problem with not knowing if PSR exit is
fully complete before we begin updating the plane registers in
pipe_update_start().

Let's say PSR was active and display is in DC6. A flip comes in, without
_flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
enabling that happens in pipe_update_start. We immediately follow that
with programming the plane MMIO's without checking if PSR fully exited.
If PSR and DC6 happen to exit while we were in the middle of programming
plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
partially programmed registers. _flush(DIRTYFB) gives us an opportunity
to exit PSR fully by starting early.

As for legacy_cursor_update(), since there is no vblank enabling
involved, we avoid updating the MMIO's in the midst of PSR exit


> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display
  2018-03-07 22:54   ` Pandiyan, Dhinakaran
@ 2018-03-07 23:20     ` Rodrigo Vivi
  0 siblings, 0 replies; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-07 23:20 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx, Zanoni, Paulo R

On Wed, Mar 07, 2018 at 10:54:28PM +0000, Pandiyan, Dhinakaran wrote:
> 
> 
> 
> On Wed, 2018-03-07 at 14:46 -0800, Rodrigo Vivi wrote:
> > On Tue, Mar 06, 2018 at 07:34:18PM -0800, Dhinakaran Pandiyan wrote:
> > > From: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
> > > 
> > > 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.
> > > 
> > > v2:
> > > Rebased due to Ville adding intel_plane_pin_fb().
> > > Minor code reordering as fb_obj_flush doesn't need struct_mutex (Chris)
> > > 
> > > 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 | 9 +++++++--
> > >  drivers/gpu/drm/i915/intel_fbdev.c   | 5 +++--
> > >  drivers/gpu/drm/i915/intel_overlay.c | 1 +
> > >  4 files changed, 15 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> > > index a5bd07338b46..e4c5a1a22d8b 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,
> > > @@ -4129,9 +4130,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() */
> > 
> > Although flush by some definition here is invalidate + flush
> > I see flush operations as the operation to be performed at the "end-of-frame".
> > After the operation was done and whatever had to be modified was modified.
> > 
> > I see you patch changing the flush to the creation and begin of the fb handling
> > like on creating and init fb functions. I believe by that time we are not ready
> > to exit PSR yet.
> > 
> > What am I missing?
> 
> 
> The functions that you are referring to call
> i915_gem_object_pin_to_display_plane() which in turn calls frontbuffer
> flush. This patch is just a refactor, doesn't change whether flush() is
> called. I am trying to move it up the call stack so that flush() can be
> selectively removed depending on the call site.

Oh! now it made more sense...

probably this phrase is more clear than the current commit message for a
commit message.

But I checked the code and I agree it is right, so:

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>



> 
> 
> 
> 
> > 
> > >  	__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 331084082545..91ce8a0522a3 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -2858,6 +2858,9 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
> > >  		return;
> > >  	}
> > >  
> > > +	obj = intel_fb_obj(fb);
> > > +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > > +
> > >  	plane_state->src_x = 0;
> > >  	plane_state->src_y = 0;
> > >  	plane_state->src_w = fb->width << 16;
> > > @@ -2871,7 +2874,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;
> > >  
> > > @@ -12785,6 +12787,8 @@ intel_prepare_plane_fb(struct drm_plane *plane,
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > +	intel_fb_obj_flush(obj, ORIGIN_DIRTYFB);
> > > +
> > >  	if (!new_state->fence) { /* implicit fencing */
> > >  		struct dma_fence *fence;
> > >  
> > > @@ -13172,8 +13176,9 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > >  	if (ret)
> > >  		goto out_unlock;
> > >  
> > > -	old_fb = old_plane_state->fb;
> > > +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > >  
> > > +	old_fb = old_plane_state->fb;
> > >  	i915_gem_track_fb(intel_fb_obj(old_fb), intel_fb_obj(fb),
> > >  			  intel_plane->frontbuffer_bit);
> > >  
> > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
> > > index 6f12adc06365..65a3313723c9 100644
> > > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > > @@ -221,6 +221,9 @@ static int intelfb_create(struct drm_fb_helper *helper,
> > >  		goto out_unlock;
> > >  	}
> > >  
> > > +	fb = &ifbdev->fb->base;
> > > +	intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > +
> > >  	info = drm_fb_helper_alloc_fbi(helper);
> > >  	if (IS_ERR(info)) {
> > >  		DRM_ERROR("Failed to allocate fb_info\n");
> > > @@ -230,8 +233,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 36671a937fa4..c2f10d899329 100644
> > > --- a/drivers/gpu/drm/i915/intel_overlay.c
> > > +++ b/drivers/gpu/drm/i915/intel_overlay.c
> > > @@ -807,6 +807,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
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07 23:10     ` Pandiyan, Dhinakaran
@ 2018-03-07 23:23       ` Rodrigo Vivi
  2018-03-07 23:43         ` Rodrigo Vivi
  2018-03-09  1:11       ` Pandiyan, Dhinakaran
  1 sibling, 1 reply; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-07 23:23 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: Dhinakaran, intel-gfx

On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > > cursor updates in their frontbuffer_flush implementations.
> > > 
> > >  /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 91ce8a0522a3..18b08e263ee1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > >         if (ret)
> > >                 goto out_unlock;
> > >  
> > > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > 
> > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > aiui.
> > -Chris
> 
> 
> That was the idea but there's a problem with not knowing if PSR exit is
> fully complete before we begin updating the plane registers in
> pipe_update_start().
> 
> Let's say PSR was active and display is in DC6. A flip comes in, without
> _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> enabling that happens in pipe_update_start. We immediately follow that
> with programming the plane MMIO's without checking if PSR fully exited.
> If PSR and DC6 happen to exit while we were in the middle of programming
> plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> to exit PSR fully by starting early.
> 
> As for legacy_cursor_update(), since there is no vblank enabling
> involved, we avoid updating the MMIO's in the midst of PSR exit

I don't believe you will be ever in a case that you write to any register
and get any flip or anything without exiting DC6 before that happens.

Or the CSR mechanism of DC6 will be simply wrong.

Would this be enough?

> 
> 
> > _______________________________________________
> > 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07 23:23       ` Rodrigo Vivi
@ 2018-03-07 23:43         ` Rodrigo Vivi
  2018-03-08  0:21           ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-07 23:43 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: Dhinakaran, intel-gfx

On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote:
> > On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > > > cursor updates in their frontbuffer_flush implementations.
> > > > 
> > > >  /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 91ce8a0522a3..18b08e263ee1 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > >         if (ret)
> > > >                 goto out_unlock;
> > > >  
> > > > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > 
> > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > aiui.
> > > -Chris
> > 
> > 
> > That was the idea but there's a problem with not knowing if PSR exit is
> > fully complete before we begin updating the plane registers in
> > pipe_update_start().
> > 
> > Let's say PSR was active and display is in DC6. A flip comes in, without
> > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > enabling that happens in pipe_update_start. We immediately follow that
> > with programming the plane MMIO's without checking if PSR fully exited.
> > If PSR and DC6 happen to exit while we were in the middle of programming
> > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > to exit PSR fully by starting early.
> > 
> > As for legacy_cursor_update(), since there is no vblank enabling
> > involved, we avoid updating the MMIO's in the midst of PSR exit
> 
> I don't believe you will be ever in a case that you write to any register
> and get any flip or anything without exiting DC6 before that happens.
> 
> Or the CSR mechanism of DC6 will be simply wrong.
> 
> Would this be enough?

ok... just ignore my previous comment...
I believe we can move with the safest side and maybe revisit this later.

From what I remember of the FBC nuke needs as well I believe this is
the right move.

Although I'm asking myself now if we are not changing the meaning of the
ORIGINS here. Shouldn't we add a new origin and update the handling?

of "flip" is a good description for this call?

> 
> > 
> > 
> > > _______________________________________________
> > > 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
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07 23:43         ` Rodrigo Vivi
@ 2018-03-08  0:21           ` Pandiyan, Dhinakaran
  2018-03-09  1:16             ` Rodrigo Vivi
  0 siblings, 1 reply; 19+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-08  0:21 UTC (permalink / raw)
  To: Vivi, Rodrigo; +Cc: Dhinakaran, intel-gfx

On Wed, 2018-03-07 at 15:43 -0800, Rodrigo Vivi wrote:
> On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> > On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote:
> > > On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> > > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > > > > cursor updates in their frontbuffer_flush implementations.
> > > > > 
> > > > >  /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 91ce8a0522a3..18b08e263ee1 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > > >         if (ret)
> > > > >                 goto out_unlock;
> > > > >  
> > > > > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > > 
> > > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > > aiui.
> > > > -Chris
> > > 
> > > 
> > > That was the idea but there's a problem with not knowing if PSR exit is
> > > fully complete before we begin updating the plane registers in
> > > pipe_update_start().
> > > 
> > > Let's say PSR was active and display is in DC6. A flip comes in, without
> > > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > > enabling that happens in pipe_update_start. We immediately follow that
> > > with programming the plane MMIO's without checking if PSR fully exited.
> > > If PSR and DC6 happen to exit while we were in the middle of programming
> > > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > > to exit PSR fully by starting early.
> > > 
> > > As for legacy_cursor_update(), since there is no vblank enabling
> > > involved, we avoid updating the MMIO's in the midst of PSR exit
> > 
> > I don't believe you will be ever in a case that you write to any register
> > and get any flip or anything without exiting DC6 before that happens.
> > 
> > Or the CSR mechanism of DC6 will be simply wrong.
> > 
> > Would this be enough?
> 
> ok... just ignore my previous comment...
> I believe we can move with the safest side and maybe revisit this later.
> 
> From what I remember of the FBC nuke needs as well I believe this is
> the right move.
> 
> Although I'm asking myself now if we are not changing the meaning of the
> ORIGINS here. Shouldn't we add a new origin and update the handling?
> 
> of "flip" is a good description for this call?
> 

The action taken for this frontbuffer_flush() is the same as origin ==
FLIP. I think it is better to add a new one when we want the features
(psr,fbc) to distinguish and react differently. 


> > 
> > > 
> > > 
> > > > _______________________________________________
> > > > 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
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-07 23:10     ` Pandiyan, Dhinakaran
  2018-03-07 23:23       ` Rodrigo Vivi
@ 2018-03-09  1:11       ` Pandiyan, Dhinakaran
  1 sibling, 0 replies; 19+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-09  1:11 UTC (permalink / raw)
  To: chris; +Cc: intel-gfx




On Wed, 2018-03-07 at 15:34 -0800, Dhinakaran Pandiyan wrote:
> On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > > cursor updates in their frontbuffer_flush implementations.
> > > 
> > >  /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 91ce8a0522a3..18b08e263ee1 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > >         if (ret)
> > >                 goto out_unlock;
> > >  
> > > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > 
> > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > aiui.
> > -Chris
> 
> 
> That was the idea but there's a problem with not knowing if PSR exit is
> fully complete before we begin updating the plane registers in
> pipe_update_start().
> 
> Let's say PSR was active and display is in DC6. A flip comes in, without
> _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> enabling that happens in pipe_update_start.

Correction, _flush(DIRTYFB) in prepare_plane_fb() has no effect because
the gem object has no frontbuffer bits set. We should just remove that
misleading call.

>  We immediately follow that
> with programming the plane MMIO's without checking if PSR fully exited.
> If PSR and DC6 happen to exit while we were in the middle of programming
> plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> to exit PSR fully by starting early.
> 
> As for legacy_cursor_update(), since there is no vblank enabling
> involved, we avoid updating the MMIO's in the midst of PSR exit
> 
> 
> > _______________________________________________
> > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags.
  2018-03-08  0:21           ` Pandiyan, Dhinakaran
@ 2018-03-09  1:16             ` Rodrigo Vivi
  0 siblings, 0 replies; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-09  1:16 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: Dhinakaran, intel-gfx

On Wed, Mar 07, 2018 at 04:21:53PM -0800, Pandiyan, Dhinakaran wrote:
> On Wed, 2018-03-07 at 15:43 -0800, Rodrigo Vivi wrote:
> > On Wed, Mar 07, 2018 at 03:23:13PM -0800, Rodrigo Vivi wrote:
> > > On Wed, Mar 07, 2018 at 11:10:35PM +0000, Pandiyan, Dhinakaran wrote:
> > > > On Wed, 2018-03-07 at 22:53 +0000, Chris Wilson wrote:
> > > > > Quoting Dhinakaran Pandiyan (2018-03-07 03:34:19)
> > > > > > 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. Use origin == ORIGIN_FLIP so that features can ignore
> > > > > > cursor updates in their frontbuffer_flush implementations.
> > > > > > 
> > > > > >  /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 91ce8a0522a3..18b08e263ee1 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -13176,7 +13176,7 @@ intel_legacy_cursor_update(struct drm_plane *plane,
> > > > > >         if (ret)
> > > > > >                 goto out_unlock;
> > > > > >  
> > > > > > -       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_DIRTYFB);
> > > > > > +       intel_fb_obj_flush(intel_fb_obj(fb), ORIGIN_FLIP);
> > > > > 
> > > > > What about prepare_plane? That should reduce to ORIGIN_FLIP as well,
> > > > > aiui.
> > > > > -Chris
> > > > 
> > > > 
> > > > That was the idea but there's a problem with not knowing if PSR exit is
> > > > fully complete before we begin updating the plane registers in
> > > > pipe_update_start().
> > > > 
> > > > Let's say PSR was active and display is in DC6. A flip comes in, without
> > > > _flush(DIRTYFB) in prepare_plane_fb(), PSR exit is delayed until vblank
> > > > enabling that happens in pipe_update_start. We immediately follow that
> > > > with programming the plane MMIO's without checking if PSR fully exited.
> > > > If PSR and DC6 happen to exit while we were in the middle of programming
> > > > plane MMIO's, the resulting vblank toggle (from PSR exit) might activate
> > > > partially programmed registers. _flush(DIRTYFB) gives us an opportunity
> > > > to exit PSR fully by starting early.
> > > > 
> > > > As for legacy_cursor_update(), since there is no vblank enabling
> > > > involved, we avoid updating the MMIO's in the midst of PSR exit
> > > 
> > > I don't believe you will be ever in a case that you write to any register
> > > and get any flip or anything without exiting DC6 before that happens.
> > > 
> > > Or the CSR mechanism of DC6 will be simply wrong.
> > > 
> > > Would this be enough?
> > 
> > ok... just ignore my previous comment...
> > I believe we can move with the safest side and maybe revisit this later.
> > 
> > From what I remember of the FBC nuke needs as well I believe this is
> > the right move.
> > 
> > Although I'm asking myself now if we are not changing the meaning of the
> > ORIGINS here. Shouldn't we add a new origin and update the handling?
> > 
> > of "flip" is a good description for this call?
> > 
> 
> The action taken for this frontbuffer_flush() is the same as origin ==
> FLIP. I think it is better to add a new one when we want the features
> (psr,fbc) to distinguish and react differently. 

Maybe we could change the names from ORIGIN_something to something more
meaninful about the operation to be performed then...

Anyways I don't want to force increase the number of origins and duplicate
the code on a niptic and I don't have better suggestions for names...

So I guess we move fwd with this for now...


Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>




> 
> 
> > > 
> > > > 
> > > > 
> > > > > _______________________________________________
> > > > > 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
> > > _______________________________________________
> > > 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.
  2018-03-07  3:34 ` [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking Dhinakaran Pandiyan
@ 2018-03-12 23:16   ` Souza, Jose
  2018-03-12 23:19     ` Pandiyan, Dhinakaran
  0 siblings, 1 reply; 19+ messages in thread
From: Souza, Jose @ 2018-03-12 23:16 UTC (permalink / raw)
  To: intel-gfx, Pandiyan, Dhinakaran; +Cc: Vivi, Rodrigo

What if FBC is disabled? Or FBC can not be activate by any of the
reasons in intel_fbc_can_activate(). The hardware tracking would never
trigger a PSR exit by it self?!

On Tue, 2018-03-06 at 19:34 -0800, Dhinakaran Pandiyan wrote:
> 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 7eec99d7fad4..6aab929f354c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -772,6 +772,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 1a7c5addcec3..fdf7dd100dfe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1873,7 +1873,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 3a8d3d06c26a..7fff0a0eceb4 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct
> drm_i915_gem_object *obj,
>  	}
>  
>  	might_sleep();
> -	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 05770790a4e9..16b77e19c28e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -948,6 +948,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
> @@ -957,7 +958,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;
> @@ -965,6 +966,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);
> @@ -1005,6 +1009,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);
> @@ -1081,6 +1088,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.
  2018-03-12 23:16   ` Souza, Jose
@ 2018-03-12 23:19     ` Pandiyan, Dhinakaran
  2018-03-13  0:58       ` Souza, Jose
  0 siblings, 1 reply; 19+ messages in thread
From: Pandiyan, Dhinakaran @ 2018-03-12 23:19 UTC (permalink / raw)
  To: Souza, Jose; +Cc: intel-gfx, Vivi, Rodrigo


On Mon, 2018-03-12 at 23:16 +0000, Souza, Jose wrote:
> What if FBC is disabled? Or FBC can not be activate by any of the
> reasons in intel_fbc_can_activate(). The hardware tracking would never
> trigger a PSR exit by it self?!
> 

Only frontbuffer tracking is tied to FBC, PSR exit on plane/pipe MMIO
writes, vblank enable should happen even without FBC.



> On Tue, 2018-03-06 at 19:34 -0800, Dhinakaran Pandiyan wrote:
> > 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 7eec99d7fad4..6aab929f354c 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -772,6 +772,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 1a7c5addcec3..fdf7dd100dfe 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1873,7 +1873,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 3a8d3d06c26a..7fff0a0eceb4 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct
> > drm_i915_gem_object *obj,
> >  	}
> >  
> >  	might_sleep();
> > -	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 05770790a4e9..16b77e19c28e 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -948,6 +948,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
> > @@ -957,7 +958,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;
> > @@ -965,6 +966,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);
> > @@ -1005,6 +1009,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);
> > @@ -1081,6 +1088,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.
  2018-03-12 23:19     ` Pandiyan, Dhinakaran
@ 2018-03-13  0:58       ` Souza, Jose
  2018-03-13 20:50         ` Rodrigo Vivi
  0 siblings, 1 reply; 19+ messages in thread
From: Souza, Jose @ 2018-03-13  0:58 UTC (permalink / raw)
  To: Pandiyan, Dhinakaran; +Cc: intel-gfx, Vivi, Rodrigo

On Mon, 2018-03-12 at 16:19 -0700, Pandiyan, Dhinakaran wrote:
> On Mon, 2018-03-12 at 23:16 +0000, Souza, Jose wrote:
> > What if FBC is disabled? Or FBC can not be activate by any of the
> > reasons in intel_fbc_can_activate(). The hardware tracking would
> > never
> > trigger a PSR exit by it self?!
> > 
> 
> Only frontbuffer tracking is tied to FBC, PSR exit on plane/pipe MMIO
> writes, vblank enable should happen even without FBC.

Oh now I got it.

> 
> 
> 
> > On Tue, 2018-03-06 at 19:34 -0800, Dhinakaran Pandiyan wrote:
> > > 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
> > > >

Reviewed-by: Jose Roberto de Souza <jose.souza@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 7eec99d7fad4..6aab929f354c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -772,6 +772,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 1a7c5addcec3..fdf7dd100dfe 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -1873,7 +1873,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 3a8d3d06c26a..7fff0a0eceb4 100644
> > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > @@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct
> > > drm_i915_gem_object *obj,
> > >  	}
> > >  
> > >  	might_sleep();
> > > -	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 05770790a4e9..16b77e19c28e 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -948,6 +948,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
> > > @@ -957,7 +958,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;
> > > @@ -965,6 +966,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);
> > > @@ -1005,6 +1009,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);
> > > @@ -1081,6 +1088,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;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking.
  2018-03-13  0:58       ` Souza, Jose
@ 2018-03-13 20:50         ` Rodrigo Vivi
  0 siblings, 0 replies; 19+ messages in thread
From: Rodrigo Vivi @ 2018-03-13 20:50 UTC (permalink / raw)
  To: Souza, Jose; +Cc: intel-gfx, Pandiyan, Dhinakaran

On Tue, Mar 13, 2018 at 12:58:59AM +0000, Souza, Jose wrote:
> On Mon, 2018-03-12 at 16:19 -0700, Pandiyan, Dhinakaran wrote:
> > On Mon, 2018-03-12 at 23:16 +0000, Souza, Jose wrote:
> > > What if FBC is disabled? Or FBC can not be activate by any of the
> > > reasons in intel_fbc_can_activate(). The hardware tracking would
> > > never
> > > trigger a PSR exit by it self?!
> > > 
> > 
> > Only frontbuffer tracking is tied to FBC, PSR exit on plane/pipe MMIO
> > writes, vblank enable should happen even without FBC.
> 
> Oh now I got it.
> 
> > 
> > 
> > 
> > > On Tue, 2018-03-06 at 19:34 -0800, Dhinakaran Pandiyan wrote:
> > > > 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
> > > > >
> 
> Reviewed-by: Jose Roberto de Souza <jose.souza@intel.com>

thanks. pushed to dinq

> 
> > > > ---
> > > >  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 7eec99d7fad4..6aab929f354c 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > @@ -772,6 +772,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 1a7c5addcec3..fdf7dd100dfe 100644
> > > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > > @@ -1873,7 +1873,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 3a8d3d06c26a..7fff0a0eceb4 100644
> > > > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > > > @@ -80,7 +80,7 @@ void __intel_fb_obj_invalidate(struct
> > > > drm_i915_gem_object *obj,
> > > >  	}
> > > >  
> > > >  	might_sleep();
> > > > -	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 05770790a4e9..16b77e19c28e 100644
> > > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > > @@ -948,6 +948,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
> > > > @@ -957,7 +958,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;
> > > > @@ -965,6 +966,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);
> > > > @@ -1005,6 +1009,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);
> > > > @@ -1081,6 +1088,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;
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2018-03-13 20:51 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-03-07  3:34 [PATCH v2 1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Dhinakaran Pandiyan
2018-03-07  3:34 ` [PATCH v2 2/3] drm/i915/frontbuffer: HW tracking for cursor moves to fix PSR lags Dhinakaran Pandiyan
2018-03-07 22:53   ` Chris Wilson
2018-03-07 23:10     ` Pandiyan, Dhinakaran
2018-03-07 23:23       ` Rodrigo Vivi
2018-03-07 23:43         ` Rodrigo Vivi
2018-03-08  0:21           ` Pandiyan, Dhinakaran
2018-03-09  1:16             ` Rodrigo Vivi
2018-03-09  1:11       ` Pandiyan, Dhinakaran
2018-03-07  3:34 ` [PATCH v2 3/3] drm/i915/psr: Use more PSR HW tracking Dhinakaran Pandiyan
2018-03-12 23:16   ` Souza, Jose
2018-03-12 23:19     ` Pandiyan, Dhinakaran
2018-03-13  0:58       ` Souza, Jose
2018-03-13 20:50         ` Rodrigo Vivi
2018-03-07  4:10 ` ✓ Fi.CI.BAT: success for series starting with [v2,1/3] drm/i915/frontbuffer: Pull frontbuffer_flush out of gem_obj_pin_to_display Patchwork
2018-03-07  6:26 ` ✓ Fi.CI.IGT: " Patchwork
2018-03-07 22:46 ` [PATCH v2 1/3] " Rodrigo Vivi
2018-03-07 22:54   ` Pandiyan, Dhinakaran
2018-03-07 23:20     ` Rodrigo Vivi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.