[PATCH 1/5] drm/i915: Enable runtime pm

[PATCH 1/5] drm/i915: Enable runtime pm

[Rodrigo Vivi]

From: Daniel Vetter <daniel.vetter@ffwll.ch>

Like with every other feature that's not enabled by default we break
runtime pm support way too often by accident because the overall test
coverage isn't great. And it's been almost 2 years since we enabled
the power well code by default

commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
Date:   Wed Jul 3 17:12:13 2013 -0300

    drm/i915: switch disable_power_well default value to 1

It's really more than overdue for runtime pm itself to follow!

Note that in practice this wont do a hole lot yet, since we're still
gated on snd-hda-intel doing proper runtime pm. But I've discussed
this with Liam and we agreed that this needs to be done. And the audio
team is working to hold up their end of this bargain.

And the justification for updating the autosuspend delay to 100ms:
Quick measurment shows that we can do a full rpm cycle in about 5ms,
which means the delay should still be really conservative from a power
conservation pov. The only workload that would suffer from ping-pong
is also only gpu/compute with all screens off. 100ms should cover any
kind of latency with submitting follow-up batches.

Cc: Takashi Iwai <tiwai@suse.de>
Cc: Liam Girdwood <liam.r.girdwood@intel.com>
Cc: Yang, Libin <libin.yang@intel.com>
Cc: Lin, Mengdong <mengdong.lin@intel.com>
Cc: Li, Jocelyn <jocelyn.li@intel.com>
Cc: Kaskinen, Tanu <tanu.kaskinen@intel.com>
Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 1a45385..2628b21 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1831,9 +1831,10 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
 		return;
 	}
 
-	pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
+	pm_runtime_set_autosuspend_delay(device, 100);
 	pm_runtime_mark_last_busy(device);
 	pm_runtime_use_autosuspend(device);
+	pm_runtime_allow(device);
 
 	pm_runtime_put_autosuspend(device);
 }
-- 
2.1.0

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

[PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

By Spec we should just mask memup and hotplug detection
for hardware tracking cases. However we always masked
LPSP that is for low power tracking support because
without it PSR was constantly exiting and never really
getting activated.

Now with runtime PM being enabled by default Matthew
reported that he was facing missed screen updates. So
let's remove this undesirable mask and let HW tracking
take care of cases like this were power saving features
are also running.

WARNING: With this patch PSR depends on Audio and GPU
runtime PM to be properly enabled, working on "auto".
If either audio runtime PM or gpu runtime pm are not
properly set PSR will constant Exit and Performance
Counter will be 0.

But the best thing of this patch is that with one more
HW tracking working the risks of missed blank screen
are minimized at most.

This affects just core platforms where PSR exit are also
helped by HW tracking: Haswell, Broadwell and Skylake
for now.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Matthew Garrett <mjg59@srcf.ucam.org via codon.org.uk>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5ee0fa5..6549d58 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 
 		/* Avoid continuous PSR exit by masking memup and hpd */
 		I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
-			   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
+			   EDP_PSR_DEBUG_MASK_HPD);
 
 		/* Enable PSR on the panel */
 		hsw_psr_enable_sink(intel_dp);
-- 
2.1.0

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

[PATCH 3/5] drm/i915: Remove unused ring argument from frontbuffer invalidate and busy functions.

[Rodrigo Vivi]

This patch doesn't have any functional change, but organize fruntbuffer
invalidate and busy by removing unecesarry signature argument for ring.

It was unsed on mark_fb_busy and only used on fb_obj_invalidate for the
same ORIGIN_CS usage. So let's clean it a bit

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c            | 10 +++++-----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c |  2 +-
 drivers/gpu/drm/i915/intel_drv.h           |  1 -
 drivers/gpu/drm/i915/intel_fbdev.c         |  4 ++--
 drivers/gpu/drm/i915/intel_frontbuffer.c   | 14 +++++---------
 5 files changed, 13 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 248fd1a..49beca2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -350,7 +350,7 @@ i915_gem_phys_pwrite(struct drm_i915_gem_object *obj,
 	if (ret)
 		return ret;
 
-	intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
+	intel_fb_obj_invalidate(obj, ORIGIN_CPU);
 	if (__copy_from_user_inatomic_nocache(vaddr, user_data, args->size)) {
 		unsigned long unwritten;
 
@@ -804,7 +804,7 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
 
 	offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
 
-	intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
+	intel_fb_obj_invalidate(obj, ORIGIN_GTT);
 
 	while (remain > 0) {
 		/* Operation in this page
@@ -948,7 +948,7 @@ i915_gem_shmem_pwrite(struct drm_device *dev,
 	if (ret)
 		return ret;
 
-	intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
+	intel_fb_obj_invalidate(obj, ORIGIN_CPU);
 
 	i915_gem_object_pin_pages(obj);
 
@@ -3939,7 +3939,7 @@ i915_gem_object_set_to_gtt_domain(struct drm_i915_gem_object *obj, bool write)
 	}
 
 	if (write)
-		intel_fb_obj_invalidate(obj, NULL, ORIGIN_GTT);
+		intel_fb_obj_invalidate(obj, ORIGIN_GTT);
 
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
@@ -4212,7 +4212,7 @@ i915_gem_object_set_to_cpu_domain(struct drm_i915_gem_object *obj, bool write)
 	}
 
 	if (write)
-		intel_fb_obj_invalidate(obj, NULL, ORIGIN_CPU);
+		intel_fb_obj_invalidate(obj, ORIGIN_CPU);
 
 	trace_i915_gem_object_change_domain(obj,
 					    old_read_domains,
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 3336e1c..edb8c45 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -1038,7 +1038,7 @@ i915_gem_execbuffer_move_to_active(struct list_head *vmas,
 			obj->dirty = 1;
 			i915_gem_request_assign(&obj->last_write_req, req);
 
-			intel_fb_obj_invalidate(obj, ring, ORIGIN_CS);
+			intel_fb_obj_invalidate(obj, ORIGIN_CS);
 
 			/* update for the implicit flush after a batch */
 			obj->base.write_domain &= ~I915_GEM_GPU_DOMAINS;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcafefc..64fb9fe 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -955,7 +955,6 @@ void bxt_ddi_vswing_sequence(struct drm_device *dev, u32 level,
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-			     struct intel_engine_cs *ring,
 			     enum fb_op_origin origin);
 void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 				    unsigned frontbuffer_bits);
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 6372cfc..8382146 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -89,7 +89,7 @@ static int intel_fbdev_blank(int blank, struct fb_info *info)
 		 * now until we solve this for real.
 		 */
 		mutex_lock(&fb_helper->dev->struct_mutex);
-		intel_fb_obj_invalidate(ifbdev->fb->obj, NULL, ORIGIN_GTT);
+		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
 		mutex_unlock(&fb_helper->dev->struct_mutex);
 	}
 
@@ -115,7 +115,7 @@ static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
 		 * now until we solve this for real.
 		 */
 		mutex_lock(&fb_helper->dev->struct_mutex);
-		intel_fb_obj_invalidate(ifbdev->fb->obj, NULL, ORIGIN_GTT);
+		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
 		mutex_unlock(&fb_helper->dev->struct_mutex);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index 57095f5..bdf0d57 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -98,14 +98,12 @@ static void intel_increase_pllclock(struct drm_device *dev,
  * intel_mark_fb_busy - mark given planes as busy
  * @dev: DRM device
  * @frontbuffer_bits: bits for the affected planes
- * @ring: optional ring for asynchronous commands
  *
  * This function gets called every time the screen contents change. It can be
  * used to keep e.g. the update rate at the nominal refresh rate with DRRS.
  */
 static void intel_mark_fb_busy(struct drm_device *dev,
-			       unsigned frontbuffer_bits,
-			       struct intel_engine_cs *ring)
+			       unsigned frontbuffer_bits)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum pipe pipe;
@@ -121,17 +119,15 @@ static void intel_mark_fb_busy(struct drm_device *dev,
 /**
  * intel_fb_obj_invalidate - invalidate frontbuffer object
  * @obj: GEM object to invalidate
- * @ring: set for asynchronous rendering
  * @origin: which operation caused the invalidation
  *
  * This function gets called every time rendering on the given object starts and
  * frontbuffer caching (fbc, low refresh rate for DRRS, panel self refresh) must
- * be invalidated. If @ring is non-NULL any subsequent invalidation will be delayed
+ * be invalidated. For ORIGIN_CS any subsequent invalidation will be delayed
  * until the rendering completes or a flip on this frontbuffer plane is
  * scheduled.
  */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
-			     struct intel_engine_cs *ring,
 			     enum fb_op_origin origin)
 {
 	struct drm_device *dev = obj->base.dev;
@@ -142,7 +138,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 	if (!obj->frontbuffer_bits)
 		return;
 
-	if (ring) {
+	if (origin == ORIGIN_CS) {
 		mutex_lock(&dev_priv->fb_tracking.lock);
 		dev_priv->fb_tracking.busy_bits
 			|= obj->frontbuffer_bits;
@@ -151,7 +147,7 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
 		mutex_unlock(&dev_priv->fb_tracking.lock);
 	}
 
-	intel_mark_fb_busy(dev, obj->frontbuffer_bits, ring);
+	intel_mark_fb_busy(dev, obj->frontbuffer_bits);
 
 	intel_psr_invalidate(dev, obj->frontbuffer_bits);
 	intel_edp_drrs_invalidate(dev, obj->frontbuffer_bits);
@@ -179,7 +175,7 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
-	intel_mark_fb_busy(dev, frontbuffer_bits, NULL);
+	intel_mark_fb_busy(dev, frontbuffer_bits);
 
 	intel_edp_drrs_flush(dev, frontbuffer_bits);
 	intel_psr_flush(dev, frontbuffer_bits);
-- 
2.1.0

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

[PATCH 4/5] drm/i915: Invalidate frontbuffer bits on FBDEV sync

[Rodrigo Vivi]

Before this we had some duct tapes to cover known cases where
FBDEV would cause a frontbuffer flush so we invalidate it again.

However other cases appeared like the boot splash screen doing
modeset and flushing it. So let's fix it for all cases.

FBDEV ops provides a function to fb_sync that was designed
to wait for blit idle. We don't need to wait for blit idle for
the operations, but we can use this function to let frontbuffer
tracking know that fbdev is about to do something.

So this patch introduces a reliable way to know when fbdev is
performing any operation.

I could've use ORIGIN_FBDEV to set fbdev_running bool inside
 the invalidate function, however I decided to let it on fbdev
so we can use the single lock to know when we need to invalidate
minimizing the struct_mutex locks and invalidates themselves.
So actual invalidate happens only on the first fbdev frontbuffer
touch only, or whenever needed. Like if the splash screen
called a modeset during boot the fbdev will invalidate on the next
screen drawn so there will be no risk of missing screen updates
if PSR is enabled.

The fbdev_running unset is happening on frontbuffer tracking code
when a async flip completes. Since fbdev has no reliable place
to tell when it got paused we can use this place that will happen
if something else completed a modeset. The risk of false positive
exist but is minimal since any proper alternation will go through
this path. Also false positive while we don't get the propper
modeset is better than the risk of miss screen updates.

Althoguth fbdev presumes that all callbacks work from atomic
context I don't believe that any "wait for idle" is atomic. So I
also removed the FIXME comments we had for using struct_mutext
there on fb_ops.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c      |   5 ++
 drivers/gpu/drm/i915/i915_drv.h          |   2 +
 drivers/gpu/drm/i915/intel_fbdev.c       | 106 ++++++++++---------------------
 drivers/gpu/drm/i915/intel_frontbuffer.c |  19 ++++++
 4 files changed, 59 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index c49fe2a..e3adddb 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2376,6 +2376,11 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 	}
 	mutex_unlock(&dev_priv->psr.lock);
 
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	seq_printf(m, "FBDEV running: %s\n",
+		   yesno(dev_priv->fb_tracking.fbdev_running));
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
 	intel_runtime_pm_put(dev_priv);
 	return 0;
 }
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 491ef0c..f1478f5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -888,6 +888,7 @@ enum fb_op_origin {
 	ORIGIN_CPU,
 	ORIGIN_CS,
 	ORIGIN_FLIP,
+	ORIGIN_FBDEV,
 };
 
 struct i915_fbc {
@@ -1627,6 +1628,7 @@ struct i915_frontbuffer_tracking {
 	 */
 	unsigned busy_bits;
 	unsigned flip_bits;
+	bool fbdev_running;
 };
 
 struct i915_wa_reg {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 8382146..4a96c20 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -45,92 +45,52 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
-static int intel_fbdev_set_par(struct fb_info *info)
-{
-	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
-	int ret;
-
-	ret = drm_fb_helper_set_par(info);
-
-	if (ret == 0) {
-		/*
-		 * FIXME: fbdev presumes that all callbacks also work from
-		 * atomic contexts and relies on that for emergency oops
-		 * printing. KMS totally doesn't do that and the locking here is
-		 * by far not the only place this goes wrong.  Ignore this for
-		 * now until we solve this for real.
-		 */
-		mutex_lock(&fb_helper->dev->struct_mutex);
-		ret = i915_gem_object_set_to_gtt_domain(ifbdev->fb->obj,
-							true);
-		mutex_unlock(&fb_helper->dev->struct_mutex);
-	}
-
-	return ret;
-}
-
-static int intel_fbdev_blank(int blank, struct fb_info *info)
-{
-	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
-	int ret;
-
-	ret = drm_fb_helper_blank(blank, info);
-
-	if (ret == 0) {
-		/*
-		 * FIXME: fbdev presumes that all callbacks also work from
-		 * atomic contexts and relies on that for emergency oops
-		 * printing. KMS totally doesn't do that and the locking here is
-		 * by far not the only place this goes wrong.  Ignore this for
-		 * now until we solve this for real.
-		 */
-		mutex_lock(&fb_helper->dev->struct_mutex);
-		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
-		mutex_unlock(&fb_helper->dev->struct_mutex);
-	}
-
-	return ret;
-}
-
-static int intel_fbdev_pan_display(struct fb_var_screeninfo *var,
-				   struct fb_info *info)
+/*
+ * FBDEV fb_sync op was designed to wait for blit idle and it is optional.
+ *
+ * We don't have to wait for blit idle, but before any fb operation
+ * we need to make sure we have frontbuffer tracking and its power saving
+ * feature consumers are aware that fbdev is running and constantly touching
+ * frontbuffer.
+ */
+static int intel_fbdev_sync(struct fb_info *info)
 {
 	struct drm_fb_helper *fb_helper = info->par;
-	struct intel_fbdev *ifbdev =
-		container_of(fb_helper, struct intel_fbdev, helper);
-
-	int ret;
-	ret = drm_fb_helper_pan_display(var, info);
+	struct drm_device *dev = fb_helper->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	bool needs_invalidate = false;
 
-	if (ret == 0) {
-		/*
-		 * FIXME: fbdev presumes that all callbacks also work from
-		 * atomic contexts and relies on that for emergency oops
-		 * printing. KMS totally doesn't do that and the locking here is
-		 * by far not the only place this goes wrong.  Ignore this for
-		 * now until we solve this for real.
-		 */
-		mutex_lock(&fb_helper->dev->struct_mutex);
-		intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
-		mutex_unlock(&fb_helper->dev->struct_mutex);
+	/*
+	 * In order to minimize the frontbuffer invalidates
+	 * and struct_mutex locks let's check if it is really needed.
+	 * Frontbuffer tracking is responsible for setting fbdev_running
+	 * to false in case it received a async flip, but if we are still
+	 * touching frontbuffer we invalidate it again.
+	 */
+	mutex_lock(&dev_priv->fb_tracking.lock);
+	needs_invalidate = !dev_priv->fb_tracking.fbdev_running;
+	dev_priv->fb_tracking.fbdev_running = true;
+	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	if (needs_invalidate) {
+		mutex_lock(&dev->struct_mutex);
+		intel_fb_obj_invalidate(dev_priv->fbdev->fb->obj, ORIGIN_FBDEV);
+		mutex_unlock(&dev->struct_mutex);
 	}
 
-	return ret;
+	return 0;
 }
 
 static struct fb_ops intelfb_ops = {
 	.owner = THIS_MODULE,
 	.fb_check_var = drm_fb_helper_check_var,
-	.fb_set_par = intel_fbdev_set_par,
+	.fb_set_par = drm_fb_helper_set_par,
 	.fb_fillrect = cfb_fillrect,
 	.fb_copyarea = cfb_copyarea,
 	.fb_imageblit = cfb_imageblit,
-	.fb_pan_display = intel_fbdev_pan_display,
-	.fb_blank = intel_fbdev_blank,
+	.fb_sync = intel_fbdev_sync,
+	.fb_pan_display = drm_fb_helper_pan_display,
+	.fb_blank = drm_fb_helper_blank,
 	.fb_setcmap = drm_fb_helper_setcmap,
 	.fb_debug_enter = drm_fb_helper_debug_enter,
 	.fb_debug_leave = drm_fb_helper_debug_leave,
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index bdf0d57..6cdfc766 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -172,6 +172,16 @@ void intel_frontbuffer_flush(struct drm_device *dev,
 
 	/* Delay flushing when rings are still busy.*/
 	mutex_lock(&dev_priv->fb_tracking.lock);
+
+	/*
+	 * If FBDEV is runnig frontbuffer is happening and nothing else
+	 * should flush frontbuffer
+	 */
+	if (dev_priv->fb_tracking.fbdev_running) {
+		mutex_unlock(&dev_priv->fb_tracking.lock);
+		return;
+	}
+
 	frontbuffer_bits &= ~dev_priv->fb_tracking.busy_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
 
@@ -259,6 +269,15 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	mutex_lock(&dev_priv->fb_tracking.lock);
+
+	/*
+	 * Let's assume that if this asynchronous flip happened
+	 * FBDEV might not be in use anymore.
+	 * If this is not the case fb_sync will happen on next
+	 * frontbuffer touch and invalidate bits again
+	 */
+	dev_priv->fb_tracking.fbdev_running = false;
+
 	/* Mask any cancelled flips. */
 	frontbuffer_bits &= dev_priv->fb_tracking.flip_bits;
 	dev_priv->fb_tracking.flip_bits &= ~frontbuffer_bits;
-- 
2.1.0

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

[PATCH 5/5] drm/i915: Enable PSR by default.

[Rodrigo Vivi]

With a reliable frontbuffer tracking and all instability corner cases solved
let's re-enabled PSR by default on all supported platforms.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_params.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
index 8ac5a1b..e864e67 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = {
 	.enable_execlists = -1,
 	.enable_hangcheck = true,
 	.enable_ppgtt = -1,
-	.enable_psr = 0,
+	.enable_psr = 1,
 	.preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
 	.disable_power_well = 1,
 	.enable_ips = 1,
@@ -124,7 +124,7 @@ MODULE_PARM_DESC(enable_execlists,
 	"(-1=auto [default], 0=disabled, 1=enabled)");
 
 module_param_named(enable_psr, i915.enable_psr, int, 0600);
-MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
+MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
 
 module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
 MODULE_PARM_DESC(preliminary_hw_support,
-- 
2.1.0

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

Re: [PATCH 1/5] drm/i915: Enable runtime pm

[Rodrigo Vivi]

I understand this patch is yet under discussion. I just re-sent to
warn that the following one depends on this. Otherwise it is better to
remove it and proceed with the last 3 patches of the series.

Thanks

On Thu, Jun 18, 2015 at 11:43 AM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> From: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Like with every other feature that's not enabled by default we break
> runtime pm support way too often by accident because the overall test
> coverage isn't great. And it's been almost 2 years since we enabled
> the power well code by default
>
> commit bf51d5e2cda5d36d98e4b46ac7fca9461e512c41
> Author: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Date:   Wed Jul 3 17:12:13 2013 -0300
>
>     drm/i915: switch disable_power_well default value to 1
>
> It's really more than overdue for runtime pm itself to follow!
>
> Note that in practice this wont do a hole lot yet, since we're still
> gated on snd-hda-intel doing proper runtime pm. But I've discussed
> this with Liam and we agreed that this needs to be done. And the audio
> team is working to hold up their end of this bargain.
>
> And the justification for updating the autosuspend delay to 100ms:
> Quick measurment shows that we can do a full rpm cycle in about 5ms,
> which means the delay should still be really conservative from a power
> conservation pov. The only workload that would suffer from ping-pong
> is also only gpu/compute with all screens off. 100ms should cover any
> kind of latency with submitting follow-up batches.
>
> Cc: Takashi Iwai <tiwai@suse.de>
> Cc: Liam Girdwood <liam.r.girdwood@intel.com>
> Cc: Yang, Libin <libin.yang@intel.com>
> Cc: Lin, Mengdong <mengdong.lin@intel.com>
> Cc: Li, Jocelyn <jocelyn.li@intel.com>
> Cc: Kaskinen, Tanu <tanu.kaskinen@intel.com>
> Cc: Zanoni, Paulo R <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 1a45385..2628b21 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -1831,9 +1831,10 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv)
>                 return;
>         }
>
> -       pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */
> +       pm_runtime_set_autosuspend_delay(device, 100);
>         pm_runtime_mark_last_busy(device);
>         pm_runtime_use_autosuspend(device);
> +       pm_runtime_allow(device);
>
>         pm_runtime_put_autosuspend(device);
>  }
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/5] drm/i915: Enable PSR by default.

[Paulo Zanoni]

2015-06-18 15:43 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> With a reliable frontbuffer tracking and all instability corner cases solved
> let's re-enabled PSR by default on all supported platforms.

Are we now passing all the PSR tests from kms_frontbuffer_tracking too?

>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index 8ac5a1b..e864e67 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = {
>         .enable_execlists = -1,
>         .enable_hangcheck = true,
>         .enable_ppgtt = -1,
> -       .enable_psr = 0,
> +       .enable_psr = 1,
>         .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>         .disable_power_well = 1,
>         .enable_ips = 1,
> @@ -124,7 +124,7 @@ MODULE_PARM_DESC(enable_execlists,
>         "(-1=auto [default], 0=disabled, 1=enabled)");
>
>  module_param_named(enable_psr, i915.enable_psr, int, 0600);
> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
>
>  module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
>  MODULE_PARM_DESC(preliminary_hw_support,
> --
> 2.1.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Daniel Vetter]

On Thu, Jun 18, 2015 at 8:43 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> By Spec we should just mask memup and hotplug detection
> for hardware tracking cases. However we always masked
> LPSP that is for low power tracking support because
> without it PSR was constantly exiting and never really
> getting activated.
>
> Now with runtime PM being enabled by default Matthew
> reported that he was facing missed screen updates. So
> let's remove this undesirable mask and let HW tracking
> take care of cases like this were power saving features
> are also running.
>
> WARNING: With this patch PSR depends on Audio and GPU
> runtime PM to be properly enabled, working on "auto".
> If either audio runtime PM or gpu runtime pm are not
> properly set PSR will constant Exit and Performance
> Counter will be 0.
>
> But the best thing of this patch is that with one more
> HW tracking working the risks of missed blank screen
> are minimized at most.
>
> This affects just core platforms where PSR exit are also
> helped by HW tracking: Haswell, Broadwell and Skylake
> for now.
>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Matthew Garrett <mjg59@srcf.ucam.org via codon.org.uk>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

I guess I don't really understand your description, but it does sound
strange ... runtime pm enabling from my patch is only about D3, power
well changes are still done. And as long as we have anything enabled
(even with PSR) we'll prevent D3.

So the only thing I can think of is that somehow D3 wreaks something
in the PSR setup and that's causing issues. Unfortunately I have no
idea about our hw details around PSR and D3, so no idea. Maybe Art has
some?

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5ee0fa5..6549d58 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>
>                 /* Avoid continuous PSR exit by masking memup and hpd */

Need to adjust the comment.

>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
> -                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
> +                          EDP_PSR_DEBUG_MASK_HPD);
>
>                 /* Enable PSR on the panel */
>                 hsw_psr_enable_sink(intel_dp);
> --
> 2.1.0
>



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

On Fri, Jun 19, 2015 at 1:32 PM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> On Thu, Jun 18, 2015 at 8:43 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>> By Spec we should just mask memup and hotplug detection
>> for hardware tracking cases. However we always masked
>> LPSP that is for low power tracking support because
>> without it PSR was constantly exiting and never really
>> getting activated.
>>
>> Now with runtime PM being enabled by default Matthew
>> reported that he was facing missed screen updates. So
>> let's remove this undesirable mask and let HW tracking
>> take care of cases like this were power saving features
>> are also running.
>>
>> WARNING: With this patch PSR depends on Audio and GPU
>> runtime PM to be properly enabled, working on "auto".
>> If either audio runtime PM or gpu runtime pm are not
>> properly set PSR will constant Exit and Performance
>> Counter will be 0.
>>
>> But the best thing of this patch is that with one more
>> HW tracking working the risks of missed blank screen
>> are minimized at most.
>>
>> This affects just core platforms where PSR exit are also
>> helped by HW tracking: Haswell, Broadwell and Skylake
>> for now.
>>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Matthew Garrett <mjg59@srcf.ucam.org via codon.org.uk>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> I guess I don't really understand your description, but it does sound
> strange ... runtime pm enabling from my patch is only about D3, power
> well changes are still done. And as long as we have anything enabled
> (even with PSR) we'll prevent D3.
>
> So the only thing I can think of is that somehow D3 wreaks something
> in the PSR setup and that's causing issues. Unfortunately I have no
> idea about our hw details around PSR and D3, so no idea. Maybe Art has
> some?

I don't know this relation as well. When I found this LPSP maks that
made PSR working it was totally by forcing all masks and start
removing one by one up to the point that this Low Power something did
the trick. At that time Artur had told about power well handling
enabled, but now after Mathew reported that issue I noticed this Low
power flag was also related to runtime PM...

>
>> ---
>>  drivers/gpu/drm/i915/intel_psr.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> index 5ee0fa5..6549d58 100644
>> --- a/drivers/gpu/drm/i915/intel_psr.c
>> +++ b/drivers/gpu/drm/i915/intel_psr.c
>> @@ -400,7 +400,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>>
>>                 /* Avoid continuous PSR exit by masking memup and hpd */
>
> Need to adjust the comment.

not actually... memup and hpd are still there... comment matched spec
but never matched the code... now it matches.

>
>>                 I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>> -                          EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>> +                          EDP_PSR_DEBUG_MASK_HPD);
>>
>>                 /* Enable PSR on the panel */
>>                 hsw_psr_enable_sink(intel_dp);
>> --
>> 2.1.0
>>
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Invalidate frontbuffer bits on FBDEV sync

[Daniel Vetter]

On Thu, Jun 18, 2015 at 11:43:25AM -0700, Rodrigo Vivi wrote:
> @@ -259,6 +269,15 @@ void intel_frontbuffer_flip_complete(struct drm_device *dev,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	mutex_lock(&dev_priv->fb_tracking.lock);
> +
> +	/*
> +	 * Let's assume that if this asynchronous flip happened
> +	 * FBDEV might not be in use anymore.
> +	 * If this is not the case fb_sync will happen on next
> +	 * frontbuffer touch and invalidate bits again
> +	 */
> +	dev_priv->fb_tracking.fbdev_running = false;

This pretty much destroys frontbuffer tracking for everyone if they boot
up with fbdev.

Since this seems to be epic fun indeed I think what we need to do is:
- Fill out the ->dirty callback.
- Extend the drm fbdev helpers to call ->dirty at all suitable places if
  needed.
 
For an example of how to do this all see udl/udl_fb.c. Imo a good patch
would be to replace all the udl special casing with the new fbdev helpers,
just as a demonstration patch. i915 isn't the only driver where every
frontbuffer change must involve some manual driver action, there's no
reason we need to roll our own solution.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/5] drm/i915: Remove unused ring argument from frontbuffer invalidate and busy functions.

[Daniel Vetter]

On Thu, Jun 18, 2015 at 11:43:24AM -0700, Rodrigo Vivi wrote:
> This patch doesn't have any functional change, but organize fruntbuffer
> invalidate and busy by removing unecesarry signature argument for ring.
> 
> It was unsed on mark_fb_busy and only used on fb_obj_invalidate for the
> same ORIGIN_CS usage. So let's clean it a bit
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/5] drm/i915: Invalidate frontbuffer bits on FBDEV sync

[Rodrigo Vivi]

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

it didn't destroy the frontbuffer tracking with x running... fb_sync wasn't
being called... but it indeed destroyed with igt So I agree this is not the
solution to go with...

I'm going to check dirty callback... thanks for the suggestion...



On Mon, Jun 22, 2015 at 6:56 AM Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Jun 18, 2015 at 11:43:25AM -0700, Rodrigo Vivi wrote:
> > @@ -259,6 +269,15 @@ void intel_frontbuffer_flip_complete(struct
> drm_device *dev,
> >       struct drm_i915_private *dev_priv = dev->dev_private;
> >
> >       mutex_lock(&dev_priv->fb_tracking.lock);
> > +
> > +     /*
> > +      * Let's assume that if this asynchronous flip happened
> > +      * FBDEV might not be in use anymore.
> > +      * If this is not the case fb_sync will happen on next
> > +      * frontbuffer touch and invalidate bits again
> > +      */
> > +     dev_priv->fb_tracking.fbdev_running = false;
>
> This pretty much destroys frontbuffer tracking for everyone if they boot
> up with fbdev.
>
> Since this seems to be epic fun indeed I think what we need to do is:
> - Fill out the ->dirty callback.
> - Extend the drm fbdev helpers to call ->dirty at all suitable places if
>   needed.
>
> For an example of how to do this all see udl/udl_fb.c. Imo a good patch
> would be to replace all the udl special casing with the new fbdev helpers,
> just as a demonstration patch. i915 isn't the only driver where every
> frontbuffer change must involve some manual driver action, there's no
> reason we need to roll our own solution.
> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 2549 bytes --]

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

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

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Runyan, Arthur J]

-- Daniel
>> I guess I don't really understand your description, but it does sound
>> strange ... runtime pm enabling from my patch is only about D3, power
>> well changes are still done. And as long as we have anything enabled
>> (even with PSR) we'll prevent D3.
>>
>> So the only thing I can think of is that somehow D3 wreaks something
>> in the PSR setup and that's causing issues. Unfortunately I have no
>> idea about our hw details around PSR and D3, so no idea. Maybe Art has
>> some?
>
-- Rodrigo
>I don't know this relation as well. When I found this LPSP maks that
>made PSR working it was totally by forcing all masks and start
>removing one by one up to the point that this Low Power something did
>the trick. At that time Artur had told about power well handling
>enabled, but now after Mathew reported that issue I noticed this Low
>power flag was also related to runtime PM...
>

Let me see if I understand what is happening.  Runtime PM seemed to cause PSR to miss some screen updates when you had LPSP masked, then you stopped masking  LPSP and it fixed the missing updates?
My first guess is that you are not in LPSP at this point, so removing the mask is effectively disabling PSR, which prevents PSR from missing screen updates.   Are you still getting good PSR residency with LPSP masked in this situation?  

The LPSP mask is only there fro debug.  We don't normally want PSR to enter without LPSP because there can be undesirable things like clocks stopping which could break the display audio controller and codec.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

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

On Mon, Jun 22, 2015 at 3:31 PM Runyan, Arthur J <arthur.j.runyan@intel.com>
wrote:

> -- Daniel
> >> I guess I don't really understand your description, but it does sound
> >> strange ... runtime pm enabling from my patch is only about D3, power
> >> well changes are still done. And as long as we have anything enabled
> >> (even with PSR) we'll prevent D3.
> >>
> >> So the only thing I can think of is that somehow D3 wreaks something
> >> in the PSR setup and that's causing issues. Unfortunately I have no
> >> idea about our hw details around PSR and D3, so no idea. Maybe Art has
> >> some?
> >
> -- Rodrigo
> >I don't know this relation as well. When I found this LPSP maks that
> >made PSR working it was totally by forcing all masks and start
> >removing one by one up to the point that this Low Power something did
> >the trick. At that time Artur had told about power well handling
> >enabled, but now after Mathew reported that issue I noticed this Low
> >power flag was also related to runtime PM...
> >
>
> Let me see if I understand what is happening.  Runtime PM seemed to cause
> PSR to miss some screen updates when you had LPSP masked, then you stopped
> masking  LPSP and it fixed the missing updates?
>

Yes.


> My first guess is that you are not in LPSP at this point, so removing the
> mask is effectively disabling PSR, which prevents PSR from missing screen
> updates.   Are you still getting good PSR residency with LPSP masked in
> this situation?
>

Without LPSP masked residency is 0 until I enable runtime_pm from i915 and
audio. But once they get enabled residency counter increases fast.

Also if I have wireless searching for network or trying to connect
residency stay 0, but once connection gets stablished or stop trying to
connect than PSR residency start increasing again.


>
> The LPSP mask is only there fro debug.  We don't normally want PSR to
> enter without LPSP because there can be undesirable things like clocks
> stopping which could break the display audio controller and codec.
>

Yeah, I know. I just masked because I could never get PSR count bigger than
0 without this mask previously...

[-- Attachment #1.2: Type: text/html, Size: 2914 bytes --]

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

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

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Runyan, Arthur J]

>From: Rodrigo Vivi [mailto:rodrigo.vivi@gmail.com] 
>>On Mon, Jun 22, 2015 at 3:31 PM Runyan, Arthur J <arthur.j.runyan@intel.com> wrote:
>>-- Daniel
>>>> I guess I don't really understand your description, but it does sound
>>>> strange ... runtime pm enabling from my patch is only about D3, power
>>>> well changes are still done. And as long as we have anything enabled
>>>> (even with PSR) we'll prevent D3.
>>>>
>>>> So the only thing I can think of is that somehow D3 wreaks something
>>>> in the PSR setup and that's causing issues. Unfortunately I have no
>>>> idea about our hw details around PSR and D3, so no idea. Maybe Art has
>>>> some?
>>>
>>-- Rodrigo
>>>I don't know this relation as well. When I found this LPSP maks that
>>>made PSR working it was totally by forcing all masks and start
>>>removing one by one up to the point that this Low Power something did
>>>the trick. At that time Artur had told about power well handling
>>>enabled, but now after Mathew reported that issue I noticed this Low
>>>power flag was also related to runtime PM...

>>Let me see if I understand what is happening.  Runtime PM seemed to cause PSR to miss some screen updates when you had LPSP masked, then you stopped masking  LPSP and it fixed the missing updates?
>Yes.
 
>>My first guess is that you are not in LPSP at this point, so removing the mask is effectively disabling PSR, which prevents PSR from missing screen updates.   Are you still getting good PSR residency with LPSP masked in this situation?
>Without LPSP masked residency is 0 until I enable runtime_pm from i915 and audio. But once they get enabled residency counter increases fast.
It sounds like you aren't really getting LPSP without runtime_pm.  Probably because the audio codec has to be in D3 to allow LPSP.  So I think this is a general problem with missing screen updates when PSR is entered, and the runtime_pm is modulating when PSR can be entered.  
Do you have a case where PSR residency is increassing and the screen updates are not missed?
Are these flips or are they front buffer modifications?

>Also if I have wireless searching for network or trying to connect residency stay 0, but once connection gets stablished or stop trying to connect than PSR residency start increasing again.
The wireless searching is causing frequent flips or other screen udpates?  
Also the residency counter measurement of time is not accurate since it doesn't account for time when clocks get stopped, but you should see it making some progress if PSR is really entered.

>>The LPSP mask is only there fro debug.  We don't normally want PSR to enter without LPSP because there can be undesirable things like clocks stopping which could break the display audio controller and codec.
>Yeah, I know. I just masked because I could never get PSR count bigger than 0 without this mask previously...
 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/5] drm/i915: PSR: Remove Low Power HW tracking mask.

[Rodrigo Vivi]

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

On Tue, Jun 23, 2015 at 11:40 AM Runyan, Arthur J <arthur.j.runyan@intel.com>
wrote:

> >From: Rodrigo Vivi [mailto:rodrigo.vivi@gmail.com]
> >>On Mon, Jun 22, 2015 at 3:31 PM Runyan, Arthur J <
> arthur.j.runyan@intel.com> wrote:
> >>-- Daniel
> >>>> I guess I don't really understand your description, but it does sound
> >>>> strange ... runtime pm enabling from my patch is only about D3, power
> >>>> well changes are still done. And as long as we have anything enabled
> >>>> (even with PSR) we'll prevent D3.
> >>>>
> >>>> So the only thing I can think of is that somehow D3 wreaks something
> >>>> in the PSR setup and that's causing issues. Unfortunately I have no
> >>>> idea about our hw details around PSR and D3, so no idea. Maybe Art has
> >>>> some?
> >>>
> >>-- Rodrigo
> >>>I don't know this relation as well. When I found this LPSP maks that
> >>>made PSR working it was totally by forcing all masks and start
> >>>removing one by one up to the point that this Low Power something did
> >>>the trick. At that time Artur had told about power well handling
> >>>enabled, but now after Mathew reported that issue I noticed this Low
> >>>power flag was also related to runtime PM...
>
> >>Let me see if I understand what is happening.  Runtime PM seemed to
> cause PSR to miss some screen updates when you had LPSP masked, then you
> stopped masking  LPSP and it fixed the missing updates?
> >Yes.
>
> >>My first guess is that you are not in LPSP at this point, so removing
> the mask is effectively disabling PSR, which prevents PSR from missing
> screen updates.   Are you still getting good PSR residency with LPSP masked
> in this situation?
> >Without LPSP masked residency is 0 until I enable runtime_pm from i915
> and audio. But once they get enabled residency counter increases fast.
> It sounds like you aren't really getting LPSP without runtime_pm.
> Probably because the audio codec has to be in D3 to allow LPSP.  So I think
> this is a general problem with missing screen updates when PSR is entered,
> and the runtime_pm is modulating when PSR can be entered.
>

I believe this address Daniel's doubt. So we are fine and we need this
patch if runtime_pms are enabled otherwise PSR wont work.


> Do you have a case where PSR residency is increassing and the screen
> updates are not missed?
>

yes, with runtime_pm enabled (auto) on i915 and on audio and no wireless
activity PSR residency increases and we don't miss any screen update.


> Are these flips or are they front buffer modifications?
>

The reported issue when LPSP is masked and runtime_pm is working is with
frontbuffer modifications.


>
> >Also if I have wireless searching for network or trying to connect
> residency stay 0, but once connection gets stablished or stop trying to
> connect than PSR residency start increasing again.
> The wireless searching is causing frequent flips or other screen udpates?
>

(Witout LPSP mask and rutime_pm enabled) The wireless search blocks PSR
residency on 0. But screen updates happen normally as expected. When it
stop trying connecting residency start increasing and screen updates still
work as expected.


> Also the residency counter measurement of time is not accurate since it
> doesn't account for time when clocks get stopped, but you should see it
> making some progress if PSR is really entered.
>

Yeah, it works as expected.


>
> >>The LPSP mask is only there fro debug.  We don't normally want PSR to
> enter without LPSP because there can be undesirable things like clocks
> stopping which could break the display audio controller and codec.
> >Yeah, I know. I just masked because I could never get PSR count bigger
> than 0 without this mask previously...
>
>

Thanks,
Rodrigo.

[-- Attachment #1.2: Type: text/html, Size: 5237 bytes --]

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

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

Re: [PATCH 5/5] drm/i915: Enable PSR by default.

[Paulo Zanoni]

2015-06-18 15:54 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> 2015-06-18 15:43 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
>> With a reliable frontbuffer tracking and all instability corner cases solved
>> let's re-enabled PSR by default on all supported platforms.
>
> Are we now passing all the PSR tests from kms_frontbuffer_tracking too?

Ok so I discussed this with Rodrigo today, and all the failures I was
seeing were due to bugs on the test suite. I have local IGT fixes for
these failures, and I could verify that, with those fixes, all the
current tests pass on the current -nightly tree (i.e., I'm not testing
this patch series!).

>
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
>> index 8ac5a1b..e864e67 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = {
>>         .enable_execlists = -1,
>>         .enable_hangcheck = true,
>>         .enable_ppgtt = -1,
>> -       .enable_psr = 0,
>> +       .enable_psr = 1,
>>         .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
>>         .disable_power_well = 1,
>>         .enable_ips = 1,
>> @@ -124,7 +124,7 @@ MODULE_PARM_DESC(enable_execlists,
>>         "(-1=auto [default], 0=disabled, 1=enabled)");
>>
>>  module_param_named(enable_psr, i915.enable_psr, int, 0600);
>> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
>> +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
>>
>>  module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
>>  MODULE_PARM_DESC(preliminary_hw_support,
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni



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

Re: [PATCH 5/5] drm/i915: Enable PSR by default.

[Vivi, Rodrigo]

On Wed, 2015-06-24 at 18:48 -0300, Paulo Zanoni wrote:
> 2015-06-18 15:54 GMT-03:00 Paulo Zanoni <przanoni@gmail.com>:
> > 2015-06-18 15:43 GMT-03:00 Rodrigo Vivi <rodrigo.vivi@intel.com>:
> >> With a reliable frontbuffer tracking and all instability corner cases solved
> >> let's re-enabled PSR by default on all supported platforms.
> >
> > Are we now passing all the PSR tests from kms_frontbuffer_tracking too?
> 
> Ok so I discussed this with Rodrigo today, and all the failures I was
> seeing were due to bugs on the test suite. I have local IGT fixes for
> these failures, and I could verify that, with those fixes, all the
> current tests pass on the current -nightly tree (i.e., I'm not testing
> this patch series!).

Thanks Paulo. Nevermind about this series... you were right... this
series would break all tests anyway... ;)
> 
> >
> >>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_params.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> >> index 8ac5a1b..e864e67 100644
> >> --- a/drivers/gpu/drm/i915/i915_params.c
> >> +++ b/drivers/gpu/drm/i915/i915_params.c
> >> @@ -37,7 +37,7 @@ struct i915_params i915 __read_mostly = {
> >>         .enable_execlists = -1,
> >>         .enable_hangcheck = true,
> >>         .enable_ppgtt = -1,
> >> -       .enable_psr = 0,
> >> +       .enable_psr = 1,
> >>         .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT),
> >>         .disable_power_well = 1,
> >>         .enable_ips = 1,
> >> @@ -124,7 +124,7 @@ MODULE_PARM_DESC(enable_execlists,
> >>         "(-1=auto [default], 0=disabled, 1=enabled)");
> >>
> >>  module_param_named(enable_psr, i915.enable_psr, int, 0600);
> >> -MODULE_PARM_DESC(enable_psr, "Enable PSR (default: false)");
> >> +MODULE_PARM_DESC(enable_psr, "Enable PSR (default: true)");
> >>
> >>  module_param_named(preliminary_hw_support, i915.preliminary_hw_support, int, 0600);
> >>  MODULE_PARM_DESC(preliminary_hw_support,
> >> --
> >> 2.1.0
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > --
> > Paulo Zanoni
> 
> 
> 

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