[PATCH 1/5] drm/i915: PSR: Remove wrong LINK_DISABLE.

[PATCH 1/5] drm/i915: PSR: Remove wrong LINK_DISABLE.

[Rodrigo Vivi]

This wrong logic and useless define came from first versions and
came along with all rework. Just now I notice how ugly, wrong and
useless this is.

val is already defined as 0 anyway and logic is completelly wrong
and useless. So let's starting the link_standby fix with this
cleaning.

Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h  | 1 -
 drivers/gpu/drm/i915/intel_psr.c | 3 +--
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 596e8eb..9f88b21 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2684,7 +2684,6 @@ enum skl_disp_power_wells {
 #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)
 #define   EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES	(0<<25)
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 27608ce..db95b39 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -269,8 +269,7 @@ static void hsw_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;
-	} else
-		val |= EDP_PSR_LINK_DISABLE;
+	}
 
 	I915_WRITE(EDP_PSR_CTL(dev), val |
 		   (IS_BROADWELL(dev) ? 0 : link_entry_time) |
-- 
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: Fix DP_PSR_NO_TRAIN_ON_EXIT logic

[Rodrigo Vivi]

Since the beginning there is a missunderstanding on the meaning of this
dpcd bit.
This bit shouldn't indicate whether to use link standby or not, but just
be used to configure TP1, TP2 and TP3 times and tell hw aux should be skiped
since HW is the responsible one.

Even with help of frontbuffer tracking, HW is still fully responsible for
PSR exit logic with/without DP training.

DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the training, but
it doesn't tell to avoid TP patterns, so we will send minimal TP1 and avoid
TP2. It also means that sink itself can take up to 5 idle frames for training.
6 in our case since we might be off by 1. So we also increment idle_frames by 4
here.

v2: Fix and improve commit message (Durga).
v3: Use minimal TP1 time avoiding TP2 and increase idle frame.

Cc: Durgadoss R <durgadoss.r@intel.com>
Cc: Arthur Runyan <arthur.j.runyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index db95b39..0e3b652 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -264,11 +264,17 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-	if (dev_priv->psr.link_standby) {
+	if (dev_priv->psr.link_standby)
 		val |= EDP_PSR_LINK_STANDBY;
+
+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
+		/* It doesn't mean we shouldn't send TPS patters, so let's
+		   send the minimal TP1 possible and skip TP2. */
+		val |= EDP_PSR_TP1_TIME_100us;
 		val |= EDP_PSR_TP2_TP3_TIME_0us;
-		val |= EDP_PSR_TP1_TIME_0us;
 		val |= EDP_PSR_SKIP_AUX_EXIT;
+		/* Sink should be able to train with the 5 or 6 idle patterns */
+		idle_frames += 4;
 	}
 
 	I915_WRITE(EDP_PSR_CTL(dev), val |
@@ -381,8 +387,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	/* First we check VBT, but we must respect sink and source
 	 * known restrictions */
 	dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
-	if ((intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) ||
-	    (IS_BROADWELL(dev) && intel_dig_port->port != PORT_A))
+	if (IS_BROADWELL(dev) && intel_dig_port->port != PORT_A)
 		dev_priv->psr.link_standby = true;
 
 	dev_priv->psr.busy_frontbuffer_bits = 0;
-- 
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: PSR: deprecate link_standby support for core platforms.

[Rodrigo Vivi]

On Haswell and Broadwell with link in standby when exit event happens
between vblank and VSC packet, PSR exit on panel but DPA transmitter
still sends black pixel. When this condition hits, panel will intermittently
display black frame.

The known W/A for this case involve the of single_frame update
that isn't supported on Haswell and to be supported on Broadwell
3 other workarounds would be required. So it is better and safe to
just deprecate link_standby for now.

Also, link fully off saves more power than link_standby and afwk
no OEM is requesting link standby on VBT. There is no reason for that.

For Skylake let's just consider it behaves like Broadwell until
we prove otherwise.

v2: Fix commit message (Durga).

v3: Fix conflict with PSR2.

Reference: HSD: bdwgfx/1912559
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Reviewed-by: Durgadoss R <durgadoss.r@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c |  3 ---
 drivers/gpu/drm/i915/i915_drv.h     |  1 -
 drivers/gpu/drm/i915/intel_psr.c    | 26 ++++++++++----------------
 3 files changed, 10 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 8446ef4..2897968 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2358,9 +2358,6 @@ static int i915_edp_psr_status(struct seq_file *m, void *data)
 		}
 	seq_puts(m, "\n");
 
-	seq_printf(m, "Link standby: %s\n",
-		   yesno((bool)dev_priv->psr.link_standby));
-
 	/* CHV PSR has no kind of performance counter */
 	if (HAS_DDI(dev)) {
 		psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dcdd17a..a7513e1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -881,7 +881,6 @@ struct i915_psr {
 	bool active;
 	struct delayed_work work;
 	unsigned busy_frontbuffer_bits;
-	bool link_standby;
 	bool psr2_support;
 	bool aux_frame_sync;
 };
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 0e3b652..5cd374b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -170,13 +170,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 
 	aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 0);
 
-	/* Enable PSR in sink */
-	if (dev_priv->psr.link_standby)
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
-	else
-		drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
-				   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
+			   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
 
 	/* Enable AUX frame sync at sink */
 	if (dev_priv->psr.aux_frame_sync)
@@ -214,6 +209,8 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
 		   (precharge << DP_AUX_CH_CTL_PRECHARGE_2US_SHIFT) |
 		   (aux_clock_divider << DP_AUX_CH_CTL_BIT_CLOCK_2X_SHIFT));
 	}
+
+	drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG, DP_PSR_ENABLE);
 }
 
 static void vlv_psr_enable_source(struct intel_dp *intel_dp)
@@ -264,9 +261,6 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	uint32_t val = 0x0;
 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
 
-	if (dev_priv->psr.link_standby)
-		val |= EDP_PSR_LINK_STANDBY;
-
 	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
 		/* It doesn't mean we shouldn't send TPS patters, so let's
 		   send the minimal TP1 possible and skip TP2. */
@@ -325,6 +319,12 @@ static bool intel_psr_match_conditions(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	if (!IS_VALLEYVIEW(dev) && ((dev_priv->vbt.psr.full_link) ||
+				    (dig_port->port != PORT_A))) {
+		DRM_DEBUG_KMS("PSR condition failed: Link Standby requested/needed but not supported on this platform\n");
+		return false;
+	}
+
 	dev_priv->psr.source_ok = true;
 	return true;
 }
@@ -384,12 +384,6 @@ void intel_psr_enable(struct intel_dp *intel_dp)
 	if (!intel_psr_match_conditions(intel_dp))
 		goto unlock;
 
-	/* First we check VBT, but we must respect sink and source
-	 * known restrictions */
-	dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
-	if (IS_BROADWELL(dev) && intel_dig_port->port != PORT_A)
-		dev_priv->psr.link_standby = true;
-
 	dev_priv->psr.busy_frontbuffer_bits = 0;
 
 	if (HAS_DDI(dev)) {
-- 
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: PSR VLV: Add single frame update.

[Rodrigo Vivi]

According to spec: "In PSR HW or SW mode, SW set this bit before writing
registers for a flip. It will be self-clear when it gets to the PSR
active state."

Some versions of spec mention that this is needed when in
"Persistent mode" but define it as same as "SW mode". Since this
fix the page flip case let's assume this is exactly what we need.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h         |  1 +
 drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
 drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++
 3 files changed, 45 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 7a0aa24..9c5d1cd 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev,
 void intel_psr_flush(struct drm_device *dev,
 			 unsigned frontbuffer_bits);
 void intel_psr_init(struct drm_device *dev);
+void intel_psr_single_frame_update(struct drm_device *dev);
 
 /* intel_runtime_pm.c */
 int intel_power_domains_init(struct drm_i915_private *);
diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
index a20cffb..57095f5 100644
--- a/drivers/gpu/drm/i915/intel_frontbuffer.c
+++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
@@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
 	/* Remove stale busy bits due to the old buffer. */
 	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
 	mutex_unlock(&dev_priv->fb_tracking.lock);
+
+	intel_psr_single_frame_update(dev);
 }
 
 /**
diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 5cd374b..5ee0fa5 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev)
 }
 
 /**
+ * intel_psr_single_frame_update - Single Frame Update
+ * @dev: DRM device
+ *
+ * Some platforms support a single frame update feature that is used to
+ * send and update only one frame on Remote Frame Buffer.
+ * So far it is only implemented for Valleyview and Cherryview because
+ * hardware requires this to be done before a page flip.
+ */
+void intel_psr_single_frame_update(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct drm_crtc *crtc;
+	enum pipe pipe;
+	u32 val;
+
+	/*
+	 * Single frame update is already supported on BDW+ but it requires
+	 * many W/A and it isn't really needed.
+	 */
+	if (!IS_VALLEYVIEW(dev))
+		return;
+
+	mutex_lock(&dev_priv->psr.lock);
+	if (!dev_priv->psr.enabled) {
+		mutex_unlock(&dev_priv->psr.lock);
+		return;
+	}
+
+	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
+	pipe = to_intel_crtc(crtc)->pipe;
+	val = I915_READ(VLV_PSRCTL(pipe));
+
+	/*
+	 * We need to set this bit before writing registers for a flip.
+	 * This bit will be self-clear when it gets to the PSR active state.
+	 */
+	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
+
+	mutex_unlock(&dev_priv->psr.lock);
+}
+
+/**
  * intel_psr_invalidate - Invalidade PSR
  * @dev: DRM device
  * @frontbuffer_bits: frontbuffer plane tracking 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 bb64415..935951f 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,
@@ -123,7 +123,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 5/5] drm/i915: Enable PSR by default.

[Matthew Garrett]

I'm seeing the same behaviour with this patchset. After boot, X works 
fine but I get a rolling display on fbcon (the contents appear to be 
moving horizontally very quickly around the middle of the screen). If 
the screen is turned off and on again, X now only updates the screen 
once every second or so but fbcon works. If I suspend and resume, things 
go back to the working state until the next screen power cycle.

-- 
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
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.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6180
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                 -5              276/276              271/276
ILK                                  301/301              301/301
SNB                                  316/316              316/316
IVB                                  328/328              328/328
BYT                                  285/285              285/285
HSW                                  394/394              394/394
BDW                                  321/321              321/321
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
*PNV  igt@gem_fence_thrash@bo-write-verify-none      PASS(2)      FAIL(1)PASS(1)
*PNV  igt@gem_fence_thrash@bo-write-verify-threaded-none      PASS(2)      FAIL(1)PASS(1)
*PNV  igt@gem_fence_thrash@bo-write-verify-x      PASS(2)      FAIL(1)PASS(1)
*PNV  igt@gem_fence_thrash@bo-write-verify-y      PASS(2)      FAIL(1)PASS(1)
 PNV  igt@gen3_render_mixed_blits      FAIL(4)PASS(4)      FAIL(1)PASS(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
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.

[Rodrigo Vivi]

Hi Matthew,

Could you please check if you can reproduce your issue using this
branch: http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=intel_psr

Also,
1. What Platform are you using? BDW?
2. What desktop environment do you use?
3. What is your resolution?
4. Is IPS running?
5. Could you please grab dmesg with drm.debug=0xe?
6. Could you please paste a sequence of  cat
/sys/kernel/debug/dri/0/i915_edp_psr_status
when facing the fbcon and X issues?

Thanks in advance,
Rodrigo.

On Fri, Apr 10, 2015 at 3:05 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
> I'm seeing the same behaviour with this patchset. After boot, X works
> fine but I get a rolling display on fbcon (the contents appear to be
> moving horizontally very quickly around the middle of the screen). If
> the screen is turned off and on again, X now only updates the screen
> once every second or so but fbcon works. If I suspend and resume, things
> go back to the working state until the next screen power cycle.
>
> --
> Matthew Garrett | mjg59@srcf.ucam.org
> _______________________________________________
> 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.

[Rodrigo Vivi]

Another questions,

Are you using powertop --auto-tune?

If so, can you please try to repdoruce X slowness issue on these 2 scenarios:
1. without doing the powertop auto-tune and psr enabled.
2. with powertop auto-tune but with PSR disabled by i915.enable_psr=0

Thanks in advance,
Rodrigo.


On Mon, Apr 13, 2015 at 4:09 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> Hi Matthew,
>
> Could you please check if you can reproduce your issue using this
> branch: http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=intel_psr
>
> Also,
> 1. What Platform are you using? BDW?
> 2. What desktop environment do you use?
> 3. What is your resolution?
> 4. Is IPS running?
> 5. Could you please grab dmesg with drm.debug=0xe?
> 6. Could you please paste a sequence of  cat
> /sys/kernel/debug/dri/0/i915_edp_psr_status
> when facing the fbcon and X issues?
>
> Thanks in advance,
> Rodrigo.
>
> On Fri, Apr 10, 2015 at 3:05 PM, Matthew Garrett <mjg59@srcf.ucam.org> wrote:
>> I'm seeing the same behaviour with this patchset. After boot, X works
>> fine but I get a rolling display on fbcon (the contents appear to be
>> moving horizontally very quickly around the middle of the screen). If
>> the screen is turned off and on again, X now only updates the screen
>> once every second or so but fbcon works. If I suspend and resume, things
>> go back to the working state until the next screen power cycle.
>>
>> --
>> Matthew Garrett | mjg59@srcf.ucam.org
>> _______________________________________________
>> 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



-- 
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 2/5] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic

[R, Durgadoss]

>-----Original Message-----
>From: Vivi, Rodrigo
>Sent: Friday, April 10, 2015 11:45 PM
>To: intel-gfx@lists.freedesktop.org
>Cc: Vivi, Rodrigo; R, Durgadoss; Runyan, Arthur J
>Subject: [PATCH 2/5] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic
>
>Since the beginning there is a missunderstanding on the meaning of this
>dpcd bit.
>This bit shouldn't indicate whether to use link standby or not, but just
>be used to configure TP1, TP2 and TP3 times and tell hw aux should be skiped
>since HW is the responsible one.
>
>Even with help of frontbuffer tracking, HW is still fully responsible for
>PSR exit logic with/without DP training.
>
>DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the training, but
>it doesn't tell to avoid TP patterns, so we will send minimal TP1 and avoid
>TP2. It also means that sink itself can take up to 5 idle frames for training.
>6 in our case since we might be off by 1. So we also increment idle_frames by 4
>here.
>
>v2: Fix and improve commit message (Durga).
>v3: Use minimal TP1 time avoiding TP2 and increase idle frame.
>
>Cc: Durgadoss R <durgadoss.r@intel.com>
>Cc: Arthur Runyan <arthur.j.runyan@intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
>---
> drivers/gpu/drm/i915/intel_psr.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>index db95b39..0e3b652 100644
>--- a/drivers/gpu/drm/i915/intel_psr.c
>+++ b/drivers/gpu/drm/i915/intel_psr.c
>@@ -264,11 +264,17 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
> 	uint32_t val = 0x0;
> 	const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>
>-	if (dev_priv->psr.link_standby) {
>+	if (dev_priv->psr.link_standby)
> 		val |= EDP_PSR_LINK_STANDBY;
>+
>+	if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
>+		/* It doesn't mean we shouldn't send TPS patters, so let's
>+		   send the minimal TP1 possible and skip TP2. */

We can fix the multi-line comment style though,

Reviewed-by: Durgadoss R <durgadoss.r@intel.com>

Thanks,
Durga

>+		val |= EDP_PSR_TP1_TIME_100us;
> 		val |= EDP_PSR_TP2_TP3_TIME_0us;
>-		val |= EDP_PSR_TP1_TIME_0us;
> 		val |= EDP_PSR_SKIP_AUX_EXIT;
>+		/* Sink should be able to train with the 5 or 6 idle patterns */
>+		idle_frames += 4;
> 	}
>
> 	I915_WRITE(EDP_PSR_CTL(dev), val |
>@@ -381,8 +387,7 @@ void intel_psr_enable(struct intel_dp *intel_dp)
> 	/* First we check VBT, but we must respect sink and source
> 	 * known restrictions */
> 	dev_priv->psr.link_standby = dev_priv->vbt.psr.full_link;
>-	if ((intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) ||
>-	    (IS_BROADWELL(dev) && intel_dig_port->port != PORT_A))
>+	if (IS_BROADWELL(dev) && intel_dig_port->port != PORT_A)
> 		dev_priv->psr.link_standby = true;
>
> 	dev_priv->psr.busy_frontbuffer_bits = 0;
>--
>2.1.0

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

Re: [PATCH 4/5] drm/i915: PSR VLV: Add single frame update.

[Daniel Vetter]

On Fri, Apr 10, 2015 at 11:15:10AM -0700, Rodrigo Vivi wrote:
> According to spec: "In PSR HW or SW mode, SW set this bit before writing
> registers for a flip. It will be self-clear when it gets to the PSR
> active state."
> 
> Some versions of spec mention that this is needed when in
> "Persistent mode" but define it as same as "SW mode". Since this
> fix the page flip case let's assume this is exactly what we need.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

Merged up to this one to dinq, will wait with the final enable until the
bug Matthew reported is a bit clearer.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_drv.h         |  1 +
>  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
>  drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++
>  3 files changed, 45 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 7a0aa24..9c5d1cd 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev,
>  void intel_psr_flush(struct drm_device *dev,
>  			 unsigned frontbuffer_bits);
>  void intel_psr_init(struct drm_device *dev);
> +void intel_psr_single_frame_update(struct drm_device *dev);
>  
>  /* intel_runtime_pm.c */
>  int intel_power_domains_init(struct drm_i915_private *);
> diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> index a20cffb..57095f5 100644
> --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> @@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
>  	/* Remove stale busy bits due to the old buffer. */
>  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
>  	mutex_unlock(&dev_priv->fb_tracking.lock);
> +
> +	intel_psr_single_frame_update(dev);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 5cd374b..5ee0fa5 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev)
>  }
>  
>  /**
> + * intel_psr_single_frame_update - Single Frame Update
> + * @dev: DRM device
> + *
> + * Some platforms support a single frame update feature that is used to
> + * send and update only one frame on Remote Frame Buffer.
> + * So far it is only implemented for Valleyview and Cherryview because
> + * hardware requires this to be done before a page flip.
> + */
> +void intel_psr_single_frame_update(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct drm_crtc *crtc;
> +	enum pipe pipe;
> +	u32 val;
> +
> +	/*
> +	 * Single frame update is already supported on BDW+ but it requires
> +	 * many W/A and it isn't really needed.
> +	 */
> +	if (!IS_VALLEYVIEW(dev))
> +		return;
> +
> +	mutex_lock(&dev_priv->psr.lock);
> +	if (!dev_priv->psr.enabled) {
> +		mutex_unlock(&dev_priv->psr.lock);
> +		return;
> +	}
> +
> +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> +	pipe = to_intel_crtc(crtc)->pipe;
> +	val = I915_READ(VLV_PSRCTL(pipe));
> +
> +	/*
> +	 * We need to set this bit before writing registers for a flip.
> +	 * This bit will be self-clear when it gets to the PSR active state.
> +	 */
> +	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
> +
> +	mutex_unlock(&dev_priv->psr.lock);
> +}
> +
> +/**
>   * intel_psr_invalidate - Invalidade PSR
>   * @dev: DRM device
>   * @frontbuffer_bits: frontbuffer plane tracking bits
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: PSR VLV: Add single frame update.

[Vivi, Rodrigo]

On Tue, 2015-04-14 at 20:04 +0200, Daniel Vetter wrote:
> On Fri, Apr 10, 2015 at 11:15:10AM -0700, Rodrigo Vivi wrote:
> > According to spec: "In PSR HW or SW mode, SW set this bit before writing
> > registers for a flip. It will be self-clear when it gets to the PSR
> > active state."
> > 
> > Some versions of spec mention that this is needed when in
> > "Persistent mode" but define it as same as "SW mode". Since this
> > fix the page flip case let's assume this is exactly what we need.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> Merged up to this one to dinq, will wait with the final enable until the
> bug Matthew reported is a bit clearer.

Thank you! I agree it is safe to wait,
but I believe the fbcon roling screen is the ips issue with resolution
and clock.
and the slowness on X typping is something else that is triggered with
some set that powertop --auto-tune uses, because I'm also facing this
even with PSR disabled. I'll investigate that anyway.

So I'm confident Matthew's issues aren't PSR related.

Thanks again,
Rodrigo.


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h         |  1 +
> >  drivers/gpu/drm/i915/intel_frontbuffer.c |  2 ++
> >  drivers/gpu/drm/i915/intel_psr.c         | 42 ++++++++++++++++++++++++++++++++
> >  3 files changed, 45 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > index 7a0aa24..9c5d1cd 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev,
> >  void intel_psr_flush(struct drm_device *dev,
> >  			 unsigned frontbuffer_bits);
> >  void intel_psr_init(struct drm_device *dev);
> > +void intel_psr_single_frame_update(struct drm_device *dev);
> >  
> >  /* intel_runtime_pm.c */
> >  int intel_power_domains_init(struct drm_i915_private *);
> > diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > index a20cffb..57095f5 100644
> > --- a/drivers/gpu/drm/i915/intel_frontbuffer.c
> > +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c
> > @@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev,
> >  	/* Remove stale busy bits due to the old buffer. */
> >  	dev_priv->fb_tracking.busy_bits &= ~frontbuffer_bits;
> >  	mutex_unlock(&dev_priv->fb_tracking.lock);
> > +
> > +	intel_psr_single_frame_update(dev);
> >  }
> >  
> >  /**
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> > index 5cd374b..5ee0fa5 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev)
> >  }
> >  
> >  /**
> > + * intel_psr_single_frame_update - Single Frame Update
> > + * @dev: DRM device
> > + *
> > + * Some platforms support a single frame update feature that is used to
> > + * send and update only one frame on Remote Frame Buffer.
> > + * So far it is only implemented for Valleyview and Cherryview because
> > + * hardware requires this to be done before a page flip.
> > + */
> > +void intel_psr_single_frame_update(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	struct drm_crtc *crtc;
> > +	enum pipe pipe;
> > +	u32 val;
> > +
> > +	/*
> > +	 * Single frame update is already supported on BDW+ but it requires
> > +	 * many W/A and it isn't really needed.
> > +	 */
> > +	if (!IS_VALLEYVIEW(dev))
> > +		return;
> > +
> > +	mutex_lock(&dev_priv->psr.lock);
> > +	if (!dev_priv->psr.enabled) {
> > +		mutex_unlock(&dev_priv->psr.lock);
> > +		return;
> > +	}
> > +
> > +	crtc = dp_to_dig_port(dev_priv->psr.enabled)->base.base.crtc;
> > +	pipe = to_intel_crtc(crtc)->pipe;
> > +	val = I915_READ(VLV_PSRCTL(pipe));
> > +
> > +	/*
> > +	 * We need to set this bit before writing registers for a flip.
> > +	 * This bit will be self-clear when it gets to the PSR active state.
> > +	 */
> > +	I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE);
> > +
> > +	mutex_unlock(&dev_priv->psr.lock);
> > +}
> > +
> > +/**
> >   * intel_psr_invalidate - Invalidade PSR
> >   * @dev: DRM device
> >   * @frontbuffer_bits: frontbuffer plane tracking bits
> > -- 
> > 2.1.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 

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

[Matthew Garrett]

On Mon, Apr 13, 2015 at 04:46:29PM -0700, Rodrigo Vivi wrote:
> Another questions,
> 
> Are you using powertop --auto-tune?
> 
> If so, can you please try to repdoruce X slowness issue on these 2 scenarios:
> 1. without doing the powertop auto-tune and psr enabled.

Ah! Yes, this is the problem. If runtime PM is enabled on the i915 PCI 
device (and the HDMI HDA device), things break. If it's disabled, 
everything works fine. I hope that helps narrow it down!

-- 
Matthew Garrett | mjg59@srcf.ucam.org
_______________________________________________
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 Sat, 2015-04-18 at 08:27 +0100, Matthew Garrett wrote:
> On Mon, Apr 13, 2015 at 04:46:29PM -0700, Rodrigo Vivi wrote:
> > Another questions,
> > 
> > Are you using powertop --auto-tune?
> > 
> > If so, can you please try to repdoruce X slowness issue on these 2 scenarios:
> > 1. without doing the powertop auto-tune and psr enabled.
> 
> Ah! Yes, this is the problem. If runtime PM is enabled on the i915 PCI 
> device (and the HDMI HDA device), things break. If it's disabled, 
> everything works fine. I hope that helps narrow it down!
> 

Are you using external USB keyboard and mouse? I can just face this
slowness when using USB, if I don't use any USB everything is fine. If
switch all USB powertop scripts to "Bad" I also can use my system
reliably. This seems a bug in usb power management. Could you please
verify if this is the same that I'm facing here?

One extra thing, could you confirm that you face this behaviour even
with i915.enable_psr=0 so Daniel can accept this last patch?

Thank you very much,
Rodrigo.
_______________________________________________
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.

[Rodrigo Vivi]

Hi Matthew,

here is the patch I've mentioned on irc today:
http://cgit.freedesktop.org/~vivijim/drm-intel/commit/?h=psr_for_mjg59&id=83809492138f2395bfb12c19e6de916de64b9246

And I prepared this branch for now:
http://cgit.freedesktop.org/~vivijim/drm-intel/log/?h=psr_for_mjg59

I'm not sending yet the patches because I still face the missed screen
during boot that you had mentioned. As soon as I fixed it I'll submit
everything.

Thanks,
Rodrigo.


On Mon, Apr 20, 2015 at 7:43 AM, Vivi, Rodrigo <rodrigo.vivi@intel.com> wrote:
> On Sat, 2015-04-18 at 08:27 +0100, Matthew Garrett wrote:
>> On Mon, Apr 13, 2015 at 04:46:29PM -0700, Rodrigo Vivi wrote:
>> > Another questions,
>> >
>> > Are you using powertop --auto-tune?
>> >
>> > If so, can you please try to repdoruce X slowness issue on these 2 scenarios:
>> > 1. without doing the powertop auto-tune and psr enabled.
>>
>> Ah! Yes, this is the problem. If runtime PM is enabled on the i915 PCI
>> device (and the HDMI HDA device), things break. If it's disabled,
>> everything works fine. I hope that helps narrow it down!
>>
>
> Are you using external USB keyboard and mouse? I can just face this
> slowness when using USB, if I don't use any USB everything is fine. If
> switch all USB powertop scripts to "Bad" I also can use my system
> reliably. This seems a bug in usb power management. Could you please
> verify if this is the same that I'm facing here?
>
> One extra thing, could you confirm that you face this behaviour even
> with i915.enable_psr=0 so Daniel can accept this last patch?
>
> Thank you very much,
> Rodrigo.



-- 
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: PSR VLV: Add single frame update.

[Daniel Vetter]

On Fri, Apr 10, 2015 at 8:15 PM, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
>
>  /**
> + * intel_psr_single_frame_update - Single Frame Update
> + * @dev: DRM device
> + *
> + * Some platforms support a single frame update feature that is used to
> + * send and update only one frame on Remote Frame Buffer.
> + * So far it is only implemented for Valleyview and Cherryview because
> + * hardware requires this to be done before a page flip.
> + */
> +void intel_psr_single_frame_update(struct drm_device *dev)

While reading through frontbuffer code I realized that this function
here doesn't filter flip_prepare events according to the frontbuffer
bitmask. Which means we'll do a single-shot upload even when only
another CRTC changes. Rodrigo, can you please follow-up with a patch
to do that? Similar to how invalidate/flush only do something when
it's about a frontbuffer on the psr CRTC.

Thanks, Daniel
-- 
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