[PATCH 1/9] drm/i915: Use HAS_PSR to avoid unecessary interactions.

[PATCH 1/9] drm/i915: Use HAS_PSR to avoid unecessary interactions.

[Rodrigo Vivi]

Let's be more conservative and protect platforms that don't
support PSR from unecessary interactions.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b3f97f2..da65adc 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1747,11 +1747,6 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if (!HAS_PSR(dev)) {
-		DRM_DEBUG_KMS("PSR not supported on this platform\n");
-		return false;
-	}
-
 	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
 	    (dig_port->port != PORT_A)) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
@@ -1824,6 +1819,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 
+	if (!HAS_PSR(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		return;
+	}
+
 	if (intel_edp_psr_match_conditions(intel_dp) &&
 	    !intel_edp_is_psr_enabled(dev))
 		intel_edp_psr_do_enable(intel_dp);
@@ -1851,6 +1851,9 @@ void intel_edp_psr_update(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_dp *intel_dp = NULL;
 
+	if (!HAS_PSR(dev))
+		return;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
-- 
1.9.3

[PATCH 2/9] drm/i915: Don't let update_psr function actually enable PSR.

[Rodrigo Vivi]

Being more conservative by enabling PSR only on psr_enable function.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index da65adc..a7b4ac9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1805,9 +1805,6 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 	    intel_edp_is_psr_enabled(dev))
 		return;
 
-	/* Setup PSR once */
-	intel_edp_psr_setup(intel_dp);
-
 	/* Enable PSR on the panel */
 	intel_edp_psr_enable_sink(intel_dp);
 
@@ -1824,6 +1821,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	/* Setup PSR once */
+	intel_edp_psr_setup(intel_dp);
+
 	if (intel_edp_psr_match_conditions(intel_dp) &&
 	    !intel_edp_is_psr_enabled(dev))
 		intel_edp_psr_do_enable(intel_dp);
@@ -1848,12 +1848,16 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 
 void intel_edp_psr_update(struct drm_device *dev)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_encoder *encoder;
 	struct intel_dp *intel_dp = NULL;
 
 	if (!HAS_PSR(dev))
 		return;
 
+	if (!dev_priv->psr.setup_done)
+		return;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
-- 
1.9.3

[PATCH 3/9] drm/i915: Do not try to enable PSR when Panel doesn't suport it.

[Rodrigo Vivi]

Also do not cache aux info. That info could be related to another panel.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index a7b4ac9..68289ce 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1613,11 +1613,9 @@ static void intel_dp_get_config(struct intel_encoder *encoder,
 	}
 }
 
-static bool is_edp_psr(struct drm_device *dev)
+static bool is_edp_psr(struct intel_dp *intel_dp)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	return dev_priv->psr.sink_support;
+	return intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED;
 }
 
 static bool intel_edp_is_psr_enabled(struct drm_device *dev)
@@ -1821,6 +1819,11 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (!is_edp_psr(intel_dp)) {
+		DRM_DEBUG_KMS("PSR not supported by this panel\n");
+		return;
+	}
+
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
@@ -1862,9 +1865,6 @@ void intel_edp_psr_update(struct drm_device *dev)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
 
-			if (!is_edp_psr(dev))
-				return;
-
 			if (!intel_edp_psr_match_conditions(intel_dp))
 				intel_edp_psr_disable(intel_dp);
 			else
-- 
1.9.3

[PATCH 4/9] drm/i915: Force PSR exit by inactivating it.

[Rodrigo Vivi]

The perfect solution for psr_exit is the hardware tracking the changes and
doing the psr exit by itself. This scenario works for HSW and BDW with some
environments like Gnome and Wayland.

However there are many other scenarios that this isn't true. Mainly one right
now is KDE users on HSW and BDW with PSR on. User would miss many screen
updates. For instances any key typed could be seen only when mouse cursor is
moved. So this patch introduces the ability of trigger PSR exit on kernel side
on some common cases that.

Most of the cases are coverred by psr_exit at set_domain. The remaining cases
are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
mark_busy.

The downside here might be reducing the residency time on the cases this
already work very wall like Gnome environment. But so far let's get focused
on fixinge issues sio PSR couild be used for everybody and we could even
get it enabled by default. Later we can add some alternatives to choose the
level of PSR efficiency over boot flag of even over crtc property.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/i915_gem.c      |  6 +++
 drivers/gpu/drm/i915/intel_display.c | 20 +++++++-
 drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 5 files changed, 111 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f36d9eb..64d520f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -637,6 +637,9 @@ struct i915_psr {
 	bool sink_support;
 	bool source_ok;
 	bool setup_done;
+	bool enabled;
+	bool active;
+	struct delayed_work work;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1dc4b74..3637add 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
+	intel_edp_psr_exit(dev, true);
+
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..70dc433 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5017,6 +5017,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
  * consider. */
 void intel_connector_dpms(struct drm_connector *connector, int mode)
 {
+	struct drm_device *dev = connector->dev;
+	struct intel_encoder *intel_encoder;
 	/* All the simple cases only support two dpms states. */
 	if (mode != DRM_MODE_DPMS_ON)
 		mode = DRM_MODE_DPMS_OFF;
@@ -5027,9 +5029,15 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
 	connector->dpms = mode;
 
 	/* Only need to change hw state when actually enabled */
-	if (connector->encoder)
-		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
+	if (connector->encoder) {
+
+		intel_encoder = to_intel_encoder(connector->encoder);
+		intel_encoder_dpms(intel_encoder, mode);
 
+		if (intel_encoder->type == INTEL_OUTPUT_EDP &&
+		    mode == DRM_MODE_DPMS_OFF)
+			intel_edp_psr_exit(dev, false);
+	}
 	intel_modeset_check_state(connector->dev);
 }
 
@@ -8826,12 +8834,15 @@ out:
 	intel_runtime_pm_put(dev_priv);
 }
 
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
+	intel_edp_psr_exit(dev, true);
+
 	if (!i915.powersave)
 		return;
 
@@ -9300,6 +9311,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	/* Exit PSR early in page flip */
+	intel_edp_psr_exit(dev, true);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -11491,6 +11505,8 @@ static void intel_setup_outputs(struct drm_device *dev)
 	if (SUPPORTS_TV(dev))
 		intel_tv_init(dev);
 
+	intel_edp_psr_init(dev);
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
 		encoder->base.possible_crtcs = encoder->crtc_mask;
 		encoder->base.possible_clones =
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 68289ce..f7b2414 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_psr_match_conditions(intel_dp) ||
-	    intel_edp_is_psr_enabled(dev))
+	if (intel_edp_is_psr_enabled(dev))
 		return;
 
 	/* Enable PSR on the panel */
@@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 
 	/* Enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
+
+	dev_priv->psr.enabled = true;
+	dev_priv->psr.active = true;
 }
 
 void intel_edp_psr_enable(struct intel_dp *intel_dp)
@@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
-	if (intel_edp_psr_match_conditions(intel_dp) &&
-	    !intel_edp_is_psr_enabled(dev))
+	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
 
@@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_is_psr_enabled(dev))
+	if (!dev_priv->psr.enabled)
 		return;
 
 	I915_WRITE(EDP_PSR_CTL(dev),
@@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
 		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
+
+	dev_priv->psr.enabled = false;
 }
 
 void intel_edp_psr_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *encoder;
-	struct intel_dp *intel_dp = NULL;
 
 	if (!HAS_PSR(dev))
 		return;
@@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
 	if (!dev_priv->psr.setup_done)
 		return;
 
+	intel_edp_psr_exit(dev, true);
+}
+
+void intel_edp_psr_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), psr.work.work);
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
@@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
 			if (!intel_edp_psr_match_conditions(intel_dp))
 				intel_edp_psr_disable(intel_dp);
 			else
-				if (!intel_edp_is_psr_enabled(dev))
-					intel_edp_psr_do_enable(intel_dp);
+				intel_edp_psr_do_enable(intel_dp);
+		}
+}
+
+void intel_edp_psr_inactivate(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *intel_crtc;
+	struct intel_dp *intel_dp = NULL;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = to_intel_crtc(encoder->base.crtc);
+
+			dev_priv->psr.active = false;
+
+			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
+				   & ~EDP_PSR_ENABLE);
 		}
+	}
+}
+
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	if (!dev_priv->psr.setup_done)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->psr.work);
+
+	if (dev_priv->psr.active)
+		intel_edp_psr_inactivate(dev);
+
+	if (schedule_back)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(100));
+}
+
+void intel_edp_psr_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
 }
 
 static void intel_disable_dp(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1904ef8..bd90661 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
+void intel_edp_psr_init(struct drm_device *dev);
+
 
 /* intel_dsi.c */
 void intel_dsi_init(struct drm_device *dev);
-- 
1.9.3

[PATCH 5/9] drm/i915: BDW PSR: Add single frame update support.

[Rodrigo Vivi]

When link is in stand by and PSR exit is triggered by a primary or sprite
plane flip this mode allows only one single updated frame to be send to
display than get back to PSR immediately.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 1 +
 drivers/gpu/drm/i915/intel_dp.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 05e2541..9ce017b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2409,6 +2409,7 @@ enum punit_power_well {
 #define EDP_PSR_BASE(dev)                       (IS_HASWELL(dev) ? 0x64800 : 0x6f800)
 #define EDP_PSR_CTL(dev)			(EDP_PSR_BASE(dev) + 0)
 #define   EDP_PSR_ENABLE			(1<<31)
+#define   BDW_PSR_SINGLE_FRAME			(1<<30)
 #define   EDP_PSR_LINK_DISABLE			(0<<27)
 #define   EDP_PSR_LINK_STANDBY			(1<<27)
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_MASK	(3<<25)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f7b2414..0d9c7b6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1723,6 +1723,7 @@ static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		val |= EDP_PSR_TP1_TIME_0us;
 		val |= EDP_PSR_SKIP_AUX_EXIT;
+		val |= IS_BROADWELL(dev) ? BDW_PSR_SINGLE_FRAME : 0;
 	} else
 		val |= EDP_PSR_LINK_DISABLE;
 
-- 
1.9.3

[PATCH 6/9] drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.

[Rodrigo Vivi]

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0d9c7b6..ba020fd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1776,6 +1776,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	/* Below limitations aren't valid for Broadwell */
+	if (IS_BROADWELL(dev))
+		goto out;
+
 	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {
 		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
 		return false;
@@ -1792,6 +1796,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+ out:
 	dev_priv->psr.source_ok = true;
 	return true;
 }
-- 
1.9.3

[PATCH 7/9] drm/i915: BDW PSR: Remove DDIA limitation for Broadwell.

[Rodrigo Vivi]

Broadwell has a PSR per transcoder, where DDIA supports
link disable and link standby modes while other
transcoders only support link standby.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index ba020fd..b6b2640 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1683,16 +1683,21 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t aux_clock_divider;
 	int precharge = 0x3;
 	int msg_size = 5;       /* Header(4) + Message(1) */
+	bool only_standby = false;
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
+	if (IS_BROADWELL(dev) && dig_port->port != PORT_A)
+		only_standby = true;
+
 	/* Enable PSR in sink */
-	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT)
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby)
 		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
 				   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
 	else
@@ -1711,14 +1716,19 @@ static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_enable_source(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint32_t max_sleep_time = 0x1f;
 	uint32_t idle_frames = 1;
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
+	bool only_standby = false;
 
-	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
+	if (IS_BROADWELL(dev) && dig_port->port != PORT_A)
+		only_standby = true;
+
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby) {
 		val |= EDP_PSR_LINK_STANDBY;
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
 		val |= EDP_PSR_TP1_TIME_0us;
@@ -1746,8 +1756,13 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 	dev_priv->psr.source_ok = false;
 
-	if ((intel_encoder->type != INTEL_OUTPUT_EDP) ||
-	    (dig_port->port != PORT_A)) {
+	if (!HAS_PSR(dev)) {
+		DRM_DEBUG_KMS("PSR not supported on this platform\n");
+		return false;
+	}
+
+	if (IS_HASWELL(dev) && (intel_encoder->type != INTEL_OUTPUT_EDP ||
+				dig_port->port != PORT_A)) {
 		DRM_DEBUG_KMS("HSW ties PSR to DDI A (eDP)\n");
 		return false;
 	}
-- 
1.9.3

[PATCH 8/9] drm/i915: Improve PSR debugfs status.

[Rodrigo Vivi]

Now we have the active/inactive state for exit and this actually changes the
HW enable bit the status was a bit confusing for users. So let's provide
more info.

Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 7b83297..a8b8140 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1973,10 +1973,12 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 
 	seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
 	seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
+	seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
+	seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
 
 	enabled = HAS_PSR(dev) &&
 		I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
-	seq_printf(m, "Enabled: %s\n", yesno(enabled));
+	seq_printf(m, "HW Enabled & Active bit: %s\n", yesno(enabled));
 
 	if (HAS_PSR(dev))
 		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
-- 
1.9.3

[PATCH 9/9] drm/i915: PSR HSW: update after enabling sprite.

[Rodrigo Vivi]

On the current structure HSW doesn't support PSR with sprites enabled
but sprites can be enabled after PSR was enabled what would cause
user to miss screen updates.

v2: move it to update_plane.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_sprite.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 1b66ddc..404335d 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
 		mutex_unlock(&dev->struct_mutex);
 	}
 
+	intel_edp_psr_update(dev);
+
 	return 0;
 }
 
-- 
1.9.3

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

Re: [PATCH 4/9] drm/i915: Force PSR exit by inactivating it.

[Daniel Vetter]

On Thu, Jun 12, 2014 at 10:16:41AM -0700, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
> 
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
> 
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.
> 
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>  drivers/gpu/drm/i915/i915_gem.c      |  6 +++
>  drivers/gpu/drm/i915/intel_display.c | 20 +++++++-
>  drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  5 files changed, 111 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..64d520f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -637,6 +637,9 @@ struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
>  	bool setup_done;
> +	bool enabled;
> +	bool active;
> +	struct delayed_work work;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1dc4b74..3637add 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	/* Try to flush the object off the GPU without holding the lock.
>  	 * We will repeat the flush holding the lock in the normal manner
>  	 * to catch cases where we are gazumped.
> @@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> @@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..70dc433 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5017,6 +5017,8 @@ static void intel_connector_check_state(struct intel_connector *connector)
>   * consider. */
>  void intel_connector_dpms(struct drm_connector *connector, int mode)
>  {
> +	struct drm_device *dev = connector->dev;
> +	struct intel_encoder *intel_encoder;
>  	/* All the simple cases only support two dpms states. */
>  	if (mode != DRM_MODE_DPMS_ON)
>  		mode = DRM_MODE_DPMS_OFF;
> @@ -5027,9 +5029,15 @@ void intel_connector_dpms(struct drm_connector *connector, int mode)
>  	connector->dpms = mode;
>  
>  	/* Only need to change hw state when actually enabled */
> -	if (connector->encoder)
> -		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
> +	if (connector->encoder) {
> +
> +		intel_encoder = to_intel_encoder(connector->encoder);
> +		intel_encoder_dpms(intel_encoder, mode);
>  
> +		if (intel_encoder->type == INTEL_OUTPUT_EDP &&
> +		    mode == DRM_MODE_DPMS_OFF)
> +			intel_edp_psr_exit(dev, false);

Nope. intel_connector_dpms is just the generic implementation we have for
forwarding the drm core ->dpms interface to our own stuff. Sprinkling
random things in here is not ok.

This either needs to be in the crtc_disable or in an encoder->disable
function.

Sorry I didn't spot this earlier.

First two patches merged.
-Daniel

> +	}
>  	intel_modeset_check_state(connector->dev);
>  }
>  
> @@ -8826,12 +8834,15 @@ out:
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_crtc *crtc;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	if (!i915.powersave)
>  		return;
>  
> @@ -9300,6 +9311,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	/* Exit PSR early in page flip */
> +	intel_edp_psr_exit(dev, true);
> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -11491,6 +11505,8 @@ static void intel_setup_outputs(struct drm_device *dev)
>  	if (SUPPORTS_TV(dev))
>  		intel_tv_init(dev);
>  
> +	intel_edp_psr_init(dev);
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
>  		encoder->base.possible_crtcs = encoder->crtc_mask;
>  		encoder->base.possible_clones =
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 68289ce..f7b2414 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>  
>  static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  {
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_psr_match_conditions(intel_dp) ||
> -	    intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_is_psr_enabled(dev))
>  		return;
>  
>  	/* Enable PSR on the panel */
> @@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  
>  	/* Enable PSR on the host */
>  	intel_edp_psr_enable_source(intel_dp);
> +
> +	dev_priv->psr.enabled = true;
> +	dev_priv->psr.active = true;
>  }
>  
>  void intel_edp_psr_enable(struct intel_dp *intel_dp)
> @@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>  	/* Setup PSR once */
>  	intel_edp_psr_setup(intel_dp);
>  
> -	if (intel_edp_psr_match_conditions(intel_dp) &&
> -	    !intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_psr_match_conditions(intel_dp))
>  		intel_edp_psr_do_enable(intel_dp);
>  }
>  
> @@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_is_psr_enabled(dev))
> +	if (!dev_priv->psr.enabled)
>  		return;
>  
>  	I915_WRITE(EDP_PSR_CTL(dev),
> @@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
>  		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
>  		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> +	dev_priv->psr.enabled = false;
>  }
>  
>  void intel_edp_psr_update(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_encoder *encoder;
> -	struct intel_dp *intel_dp = NULL;
>  
>  	if (!HAS_PSR(dev))
>  		return;
> @@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
>  	if (!dev_priv->psr.setup_done)
>  		return;
>  
> +	intel_edp_psr_exit(dev, true);
> +}
> +
> +void intel_edp_psr_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv), psr.work.work);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>  		if (encoder->type == INTEL_OUTPUT_EDP) {
>  			intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
>  			if (!intel_edp_psr_match_conditions(intel_dp))
>  				intel_edp_psr_disable(intel_dp);
>  			else
> -				if (!intel_edp_is_psr_enabled(dev))
> -					intel_edp_psr_do_enable(intel_dp);
> +				intel_edp_psr_do_enable(intel_dp);
> +		}
> +}
> +
> +void intel_edp_psr_inactivate(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->base.dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		if (encoder->type == INTEL_OUTPUT_EDP) {
> +
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +			intel_crtc = to_intel_crtc(encoder->base.crtc);
> +
> +			dev_priv->psr.active = false;
> +
> +			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
> +				   & ~EDP_PSR_ENABLE);
>  		}
> +	}
> +}
> +
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	if (!dev_priv->psr.setup_done)
> +		return;
> +
> +	cancel_delayed_work_sync(&dev_priv->psr.work);
> +
> +	if (dev_priv->psr.active)
> +		intel_edp_psr_inactivate(dev);
> +
> +	if (schedule_back)
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(100));
> +}
> +
> +void intel_edp_psr_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
>  }
>  
>  static void intel_disable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1904ef8..bd90661 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  void intel_edp_psr_update(struct drm_device *dev);
>  void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
> +void intel_edp_psr_init(struct drm_device *dev);
> +
>  
>  /* intel_dsi.c */
>  void intel_dsi_init(struct drm_device *dev);
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 9/9] drm/i915: PSR HSW: update after enabling sprite.

[Daniel Vetter]

On Thu, Jun 12, 2014 at 10:16:46AM -0700, Rodrigo Vivi wrote:
> On the current structure HSW doesn't support PSR with sprites enabled
> but sprites can be enabled after PSR was enabled what would cause
> user to miss screen updates.
> 
> v2: move it to update_plane.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Merged the other patches, too (except patch 4). I need to chat with you
about locking ;-)
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 1b66ddc..404335d 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>  		mutex_unlock(&dev->struct_mutex);
>  	}
>  
> +	intel_edp_psr_update(dev);
> +
>  	return 0;
>  }
>  
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/9] drm/i915: BDW PSR: Remove limitations that aren't valid for BDW.

[Daniel Vetter]

On Thu, Jun 12, 2014 at 10:16:43AM -0700, Rodrigo Vivi wrote:
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0d9c7b6..ba020fd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1776,6 +1776,10 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> +	/* Below limitations aren't valid for Broadwell */
> +	if (IS_BROADWELL(dev))
> +		goto out;
> +
>  	if (I915_READ(SPRCTL(intel_crtc->pipe)) & SPRITE_ENABLE) {

The removal of the sprite checks is tricky, since currently our psr test
coverage is a bit spotty. Same for cursor (and you can blame Chris a bit
for that). So we need similar nasty tests like with primary plane:
1. setplane/setcursor, wait for psr entry, change with gtt mmap write
2. setplane/setcursor, wait for psr entry, change with pwrite upload
3. setplane/setcursor, wait for psr entry, change with blt/rendercpy

Use-case 3. is currently not used, but Chris has code that does both 1&2.
So we need to update the testcase a bit. Might be worth to refactor it
into a list of buffer object upload methods and a list of tests, but not
sure.

Essentially this means we need the frontbuffer tracking you have for byt
(since it doesn't work well there) also on hsw/bdw for the cursor/sprite
stuff. Upside is that we can enable psr also with sprites on hsw.
-Daniel

>  		DRM_DEBUG_KMS("PSR condition failed: Sprite is Enabled\n");
>  		return false;
> @@ -1792,6 +1796,7 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>  		return false;
>  	}
>  
> + out:
>  	dev_priv->psr.source_ok = true;
>  	return true;
>  }
> -- 
> 1.9.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 9/9] drm/i915: PSR HSW: update after enabling sprite.

[Jani Nikula]

On Fri, 13 Jun 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Jun 12, 2014 at 10:16:46AM -0700, Rodrigo Vivi wrote:
>> On the current structure HSW doesn't support PSR with sprites enabled
>> but sprites can be enabled after PSR was enabled what would cause
>> user to miss screen updates.
>> 
>> v2: move it to update_plane.
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>
> Merged the other patches, too (except patch 4). I need to chat with you
> about locking ;-)

Patch 8 depends on patch 4.


  CC      drivers/gpu/drm/i915/i915_debugfs.o
drivers/gpu/drm/i915/i915_debugfs.c: In function ‘i915_edp_psr_status’:
drivers/gpu/drm/i915/i915_debugfs.c:1976:52: error: ‘struct i915_psr’ has no member named ‘enabled’
  seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
                                                    ^
drivers/gpu/drm/i915/i915_debugfs.c:1977:51: error: ‘struct i915_psr’ has no member named ‘active’
  seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
                                                   ^
drivers/gpu/drm/i915/i915_debugfs.c: In function ‘pipe_crc_set_source’:
drivers/gpu/drm/i915/i915_debugfs.c:2935:3: warning: passing argument 1 of ‘mutex_lock’ from incompatible pointer type [enabled by default]
   mutex_lock(&crtc->base.mutex);
   ^
In file included from include/linux/seq_file.h:7:0,
                 from drivers/gpu/drm/i915/i915_debugfs.c:29:
include/linux/mutex.h:158:13: note: expected ‘struct mutex *’ but argument is of type ‘struct drm_modeset_lock *’
 extern void mutex_lock(struct mutex *lock);
             ^
drivers/gpu/drm/i915/i915_debugfs.c:2938:3: warning: passing argument 1 of ‘mutex_unlock’ from incompatible pointer type [enabled by default]
   mutex_unlock(&crtc->base.mutex);
   ^
In file included from include/linux/seq_file.h:7:0,
                 from drivers/gpu/drm/i915/i915_debugfs.c:29:
include/linux/mutex.h:175:13: note: expected ‘struct mutex *’ but argument is of type ‘struct drm_modeset_lock *’
 extern void mutex_unlock(struct mutex *lock);
             ^
make[4]: *** [drivers/gpu/drm/i915/i915_debugfs.o] Error 1
make[3]: *** [drivers/gpu/drm/i915] Error 2
make[2]: *** [drivers/gpu/drm] Error 2
make[1]: *** [drivers/gpu] Error 2
make[1]: *** Waiting for unfinished jobs....
make: *** [drivers] Error 2




> -Daniel
>
>> ---
>>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
>> index 1b66ddc..404335d 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
>>  		mutex_unlock(&dev->struct_mutex);
>>  	}
>>  
>> +	intel_edp_psr_update(dev);
>> +
>>  	return 0;
>>  }
>>  
>> -- 
>> 1.9.3
>> 
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Force PSR exit by inactivating it.

[Rodrigo Vivi]

The perfect solution for psr_exit is the hardware tracking the changes and
doing the psr exit by itself. This scenario works for HSW and BDW with some
environments like Gnome and Wayland.

However there are many other scenarios that this isn't true. Mainly one right
now is KDE users on HSW and BDW with PSR on. User would miss many screen
updates. For instances any key typed could be seen only when mouse cursor is
moved. So this patch introduces the ability of trigger PSR exit on kernel side
on some common cases that.

Most of the cases are coverred by psr_exit at set_domain. The remaining cases
are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
mark_busy.

The downside here might be reducing the residency time on the cases this
already work very wall like Gnome environment. But so far let's get focused
on fixinge issues sio PSR couild be used for everybody and we could even
get it enabled by default. Later we can add some alternatives to choose the
level of PSR efficiency over boot flag of even over crtc property.

v2: remove exit from connector_dpms. Daniel pointed this is the wrong way and
also this isn't needed for BDW and HSW anyway.

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  3 ++
 drivers/gpu/drm/i915/i915_gem.c      |  6 +++
 drivers/gpu/drm/i915/intel_display.c |  8 ++++
 drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |  3 ++
 5 files changed, 101 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f36d9eb..64d520f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -637,6 +637,9 @@ struct i915_psr {
 	bool sink_support;
 	bool source_ok;
 	bool setup_done;
+	bool enabled;
+	bool active;
+	struct delayed_work work;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1dc4b74..3637add 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
 		goto unlock;
 	}
 
+	intel_edp_psr_exit(dev, true);
+
 	/* Try to flush the object off the GPU without holding the lock.
 	 * We will repeat the flush holding the lock in the normal manner
 	 * to catch cases where we are gazumped.
@@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
@@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
 	if (ret)
 		return ret;
 
+	intel_edp_psr_exit(dev, true);
+
 	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
 	if (&obj->base == NULL) {
 		ret = -ENOENT;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5cbb28..d48314c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8826,12 +8826,15 @@ out:
 	intel_runtime_pm_put(dev_priv);
 }
 
+
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 			struct intel_engine_cs *ring)
 {
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
+	intel_edp_psr_exit(dev, true);
+
 	if (!i915.powersave)
 		return;
 
@@ -9300,6 +9303,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	if (work == NULL)
 		return -ENOMEM;
 
+	/* Exit PSR early in page flip */
+	intel_edp_psr_exit(dev, true);
+
 	work->event = event;
 	work->crtc = crtc;
 	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
@@ -11491,6 +11497,8 @@ static void intel_setup_outputs(struct drm_device *dev)
 	if (SUPPORTS_TV(dev))
 		intel_tv_init(dev);
 
+	intel_edp_psr_init(dev);
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
 		encoder->base.possible_crtcs = encoder->crtc_mask;
 		encoder->base.possible_clones =
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 68289ce..f7b2414 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
 
 static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = intel_dig_port->base.base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_psr_match_conditions(intel_dp) ||
-	    intel_edp_is_psr_enabled(dev))
+	if (intel_edp_is_psr_enabled(dev))
 		return;
 
 	/* Enable PSR on the panel */
@@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
 
 	/* Enable PSR on the host */
 	intel_edp_psr_enable_source(intel_dp);
+
+	dev_priv->psr.enabled = true;
+	dev_priv->psr.active = true;
 }
 
 void intel_edp_psr_enable(struct intel_dp *intel_dp)
@@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
 	/* Setup PSR once */
 	intel_edp_psr_setup(intel_dp);
 
-	if (intel_edp_psr_match_conditions(intel_dp) &&
-	    !intel_edp_is_psr_enabled(dev))
+	if (intel_edp_psr_match_conditions(intel_dp))
 		intel_edp_psr_do_enable(intel_dp);
 }
 
@@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
-	if (!intel_edp_is_psr_enabled(dev))
+	if (!dev_priv->psr.enabled)
 		return;
 
 	I915_WRITE(EDP_PSR_CTL(dev),
@@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
 	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
 		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
 		DRM_ERROR("Timed out waiting for PSR Idle State\n");
+
+	dev_priv->psr.enabled = false;
 }
 
 void intel_edp_psr_update(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_encoder *encoder;
-	struct intel_dp *intel_dp = NULL;
 
 	if (!HAS_PSR(dev))
 		return;
@@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
 	if (!dev_priv->psr.setup_done)
 		return;
 
+	intel_edp_psr_exit(dev, true);
+}
+
+void intel_edp_psr_work(struct work_struct *work)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(work, typeof(*dev_priv), psr.work.work);
+	struct drm_device *dev = dev_priv->dev;
+	struct intel_encoder *encoder;
+	struct intel_dp *intel_dp = NULL;
+
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
 		if (encoder->type == INTEL_OUTPUT_EDP) {
 			intel_dp = enc_to_intel_dp(&encoder->base);
@@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
 			if (!intel_edp_psr_match_conditions(intel_dp))
 				intel_edp_psr_disable(intel_dp);
 			else
-				if (!intel_edp_is_psr_enabled(dev))
-					intel_edp_psr_do_enable(intel_dp);
+				intel_edp_psr_do_enable(intel_dp);
+		}
+}
+
+void intel_edp_psr_inactivate(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_connector *connector;
+	struct intel_encoder *encoder;
+	struct intel_crtc *intel_crtc;
+	struct intel_dp *intel_dp = NULL;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->base.dpms != DRM_MODE_DPMS_ON)
+			continue;
+
+		encoder = to_intel_encoder(connector->base.encoder);
+		if (encoder->type == INTEL_OUTPUT_EDP) {
+
+			intel_dp = enc_to_intel_dp(&encoder->base);
+			intel_crtc = to_intel_crtc(encoder->base.crtc);
+
+			dev_priv->psr.active = false;
+
+			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
+				   & ~EDP_PSR_ENABLE);
 		}
+	}
+}
+
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	if (!dev_priv->psr.setup_done)
+		return;
+
+	cancel_delayed_work_sync(&dev_priv->psr.work);
+
+	if (dev_priv->psr.active)
+		intel_edp_psr_inactivate(dev);
+
+	if (schedule_back)
+		schedule_delayed_work(&dev_priv->psr.work,
+				      msecs_to_jiffies(100));
+}
+
+void intel_edp_psr_init(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+
+	if (!HAS_PSR(dev))
+		return;
+
+	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
 }
 
 static void intel_disable_dp(struct intel_encoder *encoder)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1904ef8..bd90661 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
 void intel_edp_psr_disable(struct intel_dp *intel_dp);
 void intel_edp_psr_update(struct drm_device *dev);
 void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
+void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
+void intel_edp_psr_init(struct drm_device *dev);
+
 
 /* intel_dsi.c */
 void intel_dsi_init(struct drm_device *dev);
-- 
1.9.3

Re: [PATCH 9/9] drm/i915: PSR HSW: update after enabling sprite.

[Daniel Vetter]

On Fri, Jun 13, 2014 at 03:02:21PM +0300, Jani Nikula wrote:
> On Fri, 13 Jun 2014, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, Jun 12, 2014 at 10:16:46AM -0700, Rodrigo Vivi wrote:
> >> On the current structure HSW doesn't support PSR with sprites enabled
> >> but sprites can be enabled after PSR was enabled what would cause
> >> user to miss screen updates.
> >> 
> >> v2: move it to update_plane.
> >> 
> >> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> >> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >
> > Merged the other patches, too (except patch 4). I need to chat with you
> > about locking ;-)
> 
> Patch 8 depends on patch 4.

Patch 8 dropped again.

/me hides under a really big rock
-Daniel

> 
> 
>   CC      drivers/gpu/drm/i915/i915_debugfs.o
> drivers/gpu/drm/i915/i915_debugfs.c: In function ‘i915_edp_psr_status’:
> drivers/gpu/drm/i915/i915_debugfs.c:1976:52: error: ‘struct i915_psr’ has no member named ‘enabled’
>   seq_printf(m, "Enabled: %s\n", yesno(dev_priv->psr.enabled));
>                                                     ^
> drivers/gpu/drm/i915/i915_debugfs.c:1977:51: error: ‘struct i915_psr’ has no member named ‘active’
>   seq_printf(m, "Active: %s\n", yesno(dev_priv->psr.active));
>                                                    ^
> drivers/gpu/drm/i915/i915_debugfs.c: In function ‘pipe_crc_set_source’:
> drivers/gpu/drm/i915/i915_debugfs.c:2935:3: warning: passing argument 1 of ‘mutex_lock’ from incompatible pointer type [enabled by default]
>    mutex_lock(&crtc->base.mutex);
>    ^
> In file included from include/linux/seq_file.h:7:0,
>                  from drivers/gpu/drm/i915/i915_debugfs.c:29:
> include/linux/mutex.h:158:13: note: expected ‘struct mutex *’ but argument is of type ‘struct drm_modeset_lock *’
>  extern void mutex_lock(struct mutex *lock);
>              ^
> drivers/gpu/drm/i915/i915_debugfs.c:2938:3: warning: passing argument 1 of ‘mutex_unlock’ from incompatible pointer type [enabled by default]
>    mutex_unlock(&crtc->base.mutex);
>    ^
> In file included from include/linux/seq_file.h:7:0,
>                  from drivers/gpu/drm/i915/i915_debugfs.c:29:
> include/linux/mutex.h:175:13: note: expected ‘struct mutex *’ but argument is of type ‘struct drm_modeset_lock *’
>  extern void mutex_unlock(struct mutex *lock);
>              ^
> make[4]: *** [drivers/gpu/drm/i915/i915_debugfs.o] Error 1
> make[3]: *** [drivers/gpu/drm/i915] Error 2
> make[2]: *** [drivers/gpu/drm] Error 2
> make[1]: *** [drivers/gpu] Error 2
> make[1]: *** Waiting for unfinished jobs....
> make: *** [drivers] Error 2
> 
> 
> 
> 
> > -Daniel
> >
> >> ---
> >>  drivers/gpu/drm/i915/intel_sprite.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> >> index 1b66ddc..404335d 100644
> >> --- a/drivers/gpu/drm/i915/intel_sprite.c
> >> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> >> @@ -1051,6 +1051,8 @@ intel_update_plane(struct drm_plane *plane, struct drm_crtc *crtc,
> >>  		mutex_unlock(&dev->struct_mutex);
> >>  	}
> >>  
> >> +	intel_edp_psr_update(dev);
> >> +
> >>  	return 0;
> >>  }
> >>  
> >> -- 
> >> 1.9.3
> >> 
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> > -- 
> > 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
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
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] drm/i915: Force PSR exit by inactivating it.

[Daniel Vetter]

On Fri, Jun 13, 2014 at 05:10:03AM -0700, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
> 
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
> 
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.
> 
> v2: remove exit from connector_dpms. Daniel pointed this is the wrong way and
> also this isn't needed for BDW and HSW anyway.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Ok, locking doesn't exist here which is bad, but as long as we don't
enable psr it won't blow up.

The other issue is that psr_exit doesn't key of the bo that's getting
busy, which means is psr residency will be completely down the toilet
compared to what we could achieve.

Well, queued for -next, thanks for the patch.
-Daniel
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>  drivers/gpu/drm/i915/i915_gem.c      |  6 +++
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  5 files changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..64d520f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -637,6 +637,9 @@ struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
>  	bool setup_done;
> +	bool enabled;
> +	bool active;
> +	struct delayed_work work;
>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1dc4b74..3637add 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	/* Try to flush the object off the GPU without holding the lock.
>  	 * We will repeat the flush holding the lock in the normal manner
>  	 * to catch cases where we are gazumped.
> @@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> @@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5cbb28..d48314c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8826,12 +8826,15 @@ out:
>  	intel_runtime_pm_put(dev_priv);
>  }
>  
> +
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>  			struct intel_engine_cs *ring)
>  {
>  	struct drm_device *dev = obj->base.dev;
>  	struct drm_crtc *crtc;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	if (!i915.powersave)
>  		return;
>  
> @@ -9300,6 +9303,9 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	if (work == NULL)
>  		return -ENOMEM;
>  
> +	/* Exit PSR early in page flip */
> +	intel_edp_psr_exit(dev, true);


> +
>  	work->event = event;
>  	work->crtc = crtc;
>  	work->old_fb_obj = to_intel_framebuffer(old_fb)->obj;
> @@ -11491,6 +11497,8 @@ static void intel_setup_outputs(struct drm_device *dev)
>  	if (SUPPORTS_TV(dev))
>  		intel_tv_init(dev);
>  
> +	intel_edp_psr_init(dev);
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
>  		encoder->base.possible_crtcs = encoder->crtc_mask;
>  		encoder->base.possible_clones =
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 68289ce..f7b2414 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1797,10 +1797,11 @@ static bool intel_edp_psr_match_conditions(struct intel_dp *intel_dp)
>  
>  static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  {
> -	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> +	struct drm_device *dev = intel_dig_port->base.base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_psr_match_conditions(intel_dp) ||
> -	    intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_is_psr_enabled(dev))
>  		return;
>  
>  	/* Enable PSR on the panel */
> @@ -1808,6 +1809,9 @@ static void intel_edp_psr_do_enable(struct intel_dp *intel_dp)
>  
>  	/* Enable PSR on the host */
>  	intel_edp_psr_enable_source(intel_dp);
> +
> +	dev_priv->psr.enabled = true;
> +	dev_priv->psr.active = true;
>  }
>  
>  void intel_edp_psr_enable(struct intel_dp *intel_dp)
> @@ -1827,8 +1831,7 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp)
>  	/* Setup PSR once */
>  	intel_edp_psr_setup(intel_dp);
>  
> -	if (intel_edp_psr_match_conditions(intel_dp) &&
> -	    !intel_edp_is_psr_enabled(dev))
> +	if (intel_edp_psr_match_conditions(intel_dp))
>  		intel_edp_psr_do_enable(intel_dp);
>  }
>  
> @@ -1837,7 +1840,7 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  
> -	if (!intel_edp_is_psr_enabled(dev))
> +	if (!dev_priv->psr.enabled)
>  		return;
>  
>  	I915_WRITE(EDP_PSR_CTL(dev),
> @@ -1847,13 +1850,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
>  	if (_wait_for((I915_READ(EDP_PSR_STATUS_CTL(dev)) &
>  		       EDP_PSR_STATUS_STATE_MASK) == 0, 2000, 10))
>  		DRM_ERROR("Timed out waiting for PSR Idle State\n");
> +
> +	dev_priv->psr.enabled = false;
>  }
>  
>  void intel_edp_psr_update(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct intel_encoder *encoder;
> -	struct intel_dp *intel_dp = NULL;
>  
>  	if (!HAS_PSR(dev))
>  		return;
> @@ -1861,6 +1864,17 @@ void intel_edp_psr_update(struct drm_device *dev)
>  	if (!dev_priv->psr.setup_done)
>  		return;
>  
> +	intel_edp_psr_exit(dev, true);
> +}
> +
> +void intel_edp_psr_work(struct work_struct *work)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(work, typeof(*dev_priv), psr.work.work);
> +	struct drm_device *dev = dev_priv->dev;
> +	struct intel_encoder *encoder;
> +	struct intel_dp *intel_dp = NULL;
> +
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
>  		if (encoder->type == INTEL_OUTPUT_EDP) {
>  			intel_dp = enc_to_intel_dp(&encoder->base);
> @@ -1868,9 +1882,66 @@ void intel_edp_psr_update(struct drm_device *dev)
>  			if (!intel_edp_psr_match_conditions(intel_dp))
>  				intel_edp_psr_disable(intel_dp);
>  			else
> -				if (!intel_edp_is_psr_enabled(dev))
> -					intel_edp_psr_do_enable(intel_dp);
> +				intel_edp_psr_do_enable(intel_dp);
> +		}
> +}
> +
> +void intel_edp_psr_inactivate(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_connector *connector;
> +	struct intel_encoder *encoder;
> +	struct intel_crtc *intel_crtc;
> +	struct intel_dp *intel_dp = NULL;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +
> +		if (connector->base.dpms != DRM_MODE_DPMS_ON)
> +			continue;
> +
> +		encoder = to_intel_encoder(connector->base.encoder);
> +		if (encoder->type == INTEL_OUTPUT_EDP) {
> +
> +			intel_dp = enc_to_intel_dp(&encoder->base);
> +			intel_crtc = to_intel_crtc(encoder->base.crtc);
> +
> +			dev_priv->psr.active = false;
> +
> +			I915_WRITE(EDP_PSR_CTL(dev), I915_READ(EDP_PSR_CTL(dev))
> +				   & ~EDP_PSR_ENABLE);
>  		}
> +	}
> +}
> +
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	if (!dev_priv->psr.setup_done)
> +		return;
> +
> +	cancel_delayed_work_sync(&dev_priv->psr.work);
> +
> +	if (dev_priv->psr.active)
> +		intel_edp_psr_inactivate(dev);
> +
> +	if (schedule_back)
> +		schedule_delayed_work(&dev_priv->psr.work,
> +				      msecs_to_jiffies(100));
> +}
> +
> +void intel_edp_psr_init(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +
> +	if (!HAS_PSR(dev))
> +		return;
> +
> +	INIT_DELAYED_WORK(&dev_priv->psr.work, intel_edp_psr_work);
>  }
>  
>  static void intel_disable_dp(struct intel_encoder *encoder)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 1904ef8..bd90661 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -831,6 +831,9 @@ void intel_edp_psr_enable(struct intel_dp *intel_dp);
>  void intel_edp_psr_disable(struct intel_dp *intel_dp);
>  void intel_edp_psr_update(struct drm_device *dev);
>  void intel_dp_set_drrs_state(struct drm_device *dev, int refresh_rate);
> +void intel_edp_psr_exit(struct drm_device *dev, bool schedule_back);
> +void intel_edp_psr_init(struct drm_device *dev);
> +
>  
>  /* intel_dsi.c */
>  void intel_dsi_init(struct drm_device *dev);
> -- 
> 1.9.3
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Force PSR exit by inactivating it.

[Chris Wilson]

On Fri, Jun 13, 2014 at 05:10:03AM -0700, Rodrigo Vivi wrote:
> The perfect solution for psr_exit is the hardware tracking the changes and
> doing the psr exit by itself. This scenario works for HSW and BDW with some
> environments like Gnome and Wayland.
> 
> However there are many other scenarios that this isn't true. Mainly one right
> now is KDE users on HSW and BDW with PSR on. User would miss many screen
> updates. For instances any key typed could be seen only when mouse cursor is
> moved. So this patch introduces the ability of trigger PSR exit on kernel side
> on some common cases that.
> 
> Most of the cases are coverred by psr_exit at set_domain. The remaining cases
> are coverred by triggering it at set_domain, busy_ioctl, sw_finish and
> mark_busy.
> 
> The downside here might be reducing the residency time on the cases this
> already work very wall like Gnome environment. But so far let's get focused
> on fixinge issues sio PSR couild be used for everybody and we could even
> get it enabled by default. Later we can add some alternatives to choose the
> level of PSR efficiency over boot flag of even over crtc property.
> 
> v2: remove exit from connector_dpms. Daniel pointed this is the wrong way and
> also this isn't needed for BDW and HSW anyway.
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Reviewed-by: Vijay Purushothaman <vijay.a.purushothaman@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  3 ++
>  drivers/gpu/drm/i915/i915_gem.c      |  6 +++
>  drivers/gpu/drm/i915/intel_display.c |  8 ++++
>  drivers/gpu/drm/i915/intel_dp.c      | 91 ++++++++++++++++++++++++++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |  3 ++
>  5 files changed, 101 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f36d9eb..64d520f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -637,6 +637,9 @@ struct i915_psr {
>  	bool sink_support;
>  	bool source_ok;
>  	bool setup_done;
> +	bool enabled;
> +	bool active;
> +	struct delayed_work work;

What is your locking strategy?

>  };
>  
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 1dc4b74..3637add 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1395,6 +1395,8 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data,
>  		goto unlock;
>  	}
>  
> +	intel_edp_psr_exit(dev, true);

This is broken as you reenable PSR. As last time, it is perfectly valid
for the client to continue to write through the GTT onto the scanout
indefinitely after a single set-domain call. Bonus points for ignoring
earlier review about pin_display and checking for writes, and instead
doing it for everything.

> +
>  	/* Try to flush the object off the GPU without holding the lock.
>  	 * We will repeat the flush holding the lock in the normal manner
>  	 * to catch cases where we are gazumped.
> @@ -1440,6 +1442,8 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);
> +
>  	obj = to_intel_bo(drm_gem_object_lookup(dev, file, args->handle));
>  	if (&obj->base == NULL) {
>  		ret = -ENOENT;
> @@ -4234,6 +4238,8 @@ i915_gem_busy_ioctl(struct drm_device *dev, void *data,
>  	if (ret)
>  		return ret;
>  
> +	intel_edp_psr_exit(dev, true);

EVERY SINGLE CALL TO BUSY?

This is instead of tracking front buffer writes? Which you do anyway.
You interchange invalidate/flush patterns for psr exit with no
explanation as to what the strategy is meant to be.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre