From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jindal, Sonika" Subject: Re: [PATCH 2/7] drm/i915/psr: Try to program link training times correctly Date: Thu, 19 May 2016 16:20:02 +0530 Message-ID: <573D9A5A.4010004@intel.com> References: <1463590036-17824-1-git-send-email-daniel.vetter@ffwll.ch> <1463590036-17824-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0119744276==" Return-path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by gabe.freedesktop.org (Postfix) with ESMTP id C83F06E09E for ; Thu, 19 May 2016 10:50:07 +0000 (UTC) In-Reply-To: <1463590036-17824-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter , Intel Graphics Development Cc: stable@vger.kernel.org, "Pandiyan, Dhinakaran" , Rodrigo Vivi , Daniel Vetter List-Id: intel-gfx@lists.freedesktop.org This is a multi-part message in MIME format. --===============0119744276== Content-Type: multipart/alternative; boundary="------------060705030206060909020908" This is a multi-part message in MIME format. --------------060705030206060909020908 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit On 5/18/2016 10:17 PM, Daniel Vetter wrote: > Oops. Hw default for programming these fields to 0 is "skip link > training". Display won't take that too well usually. But we were defaulting it to value 0, which means 500us for both TP1 and TP2 or TP3 time. I dont think it means skip link training. This is just to set the time for the patterns. Skip aux handshake can happen if bit 12 of SRD_CTL is set. Does this solution help in fixing the bug mentioned here? > > v2: Unbotch the math a bit. > > v3: Drop debug hunk. > > Tested-by: Lyude > Cc: Lyude > Cc: stable@vger.kernel.org > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95176 > Cc: Rodrigo Vivi > Cc: Sonika Jindal > Cc: Durgadoss R > Cc: "Pandiyan, Dhinakaran" > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++++++++++++++++------ > 1 file changed, 47 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c > index c3abae4bc596..a788d1e9589b 100644 > --- a/drivers/gpu/drm/i915/intel_psr.c > +++ b/drivers/gpu/drm/i915/intel_psr.c > @@ -280,7 +280,10 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) > * with the 5 or 6 idle patterns. > */ > uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames); > - uint32_t val = 0x0; > + uint32_t val = EDP_PSR_ENABLE; > + > + val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT; > + val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT; > > if (IS_HASWELL(dev)) > val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; > @@ -288,14 +291,50 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) > if (dev_priv->psr.link_standby) > val |= EDP_PSR_LINK_STANDBY; > > - I915_WRITE(EDP_PSR_CTL, val | > - max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT | > - idle_frames << EDP_PSR_IDLE_FRAME_SHIFT | > - EDP_PSR_ENABLE); > + if (dev_priv->vbt.psr.tp1_wakeup_time > 5) > + val |= EDP_PSR_TP1_TIME_2500us; > + else if (dev_priv->vbt.psr.tp1_wakeup_time > 1) > + val |= EDP_PSR_TP1_TIME_500us; > + else if (dev_priv->vbt.psr.tp1_wakeup_time > 0) > + val |= EDP_PSR_TP1_TIME_100us; > + else > + val |= EDP_PSR_TP1_TIME_0us; > + > + if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5) > + val |= EDP_PSR_TP2_TP3_TIME_2500us; > + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1) > + val |= EDP_PSR_TP2_TP3_TIME_500us; > + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0) > + val |= EDP_PSR_TP2_TP3_TIME_100us; > + else > + val |= EDP_PSR_TP2_TP3_TIME_0us; > + > + if (intel_dp_source_supports_hbr2(intel_dp) && > + drm_dp_tps3_supported(intel_dp->dpcd)) > + val |= EDP_PSR_TP1_TP3_SEL; > + else > + val |= EDP_PSR_TP1_TP2_SEL; > + > + I915_WRITE(EDP_PSR_CTL, val); > + > + if (!dev_priv->psr.psr2_support) > + return; > + > + /* FIXME: selective update is probably totally broken because it doesn't > + * mesh at all with our frontbuffer tracking. And the hw alone isn't > + * good enough. */ > + val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE; > + > + if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5) > + val |= EDP_PSR2_TP2_TIME_2500; > + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1) > + val |= EDP_PSR2_TP2_TIME_500; > + else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0) > + val |= EDP_PSR2_TP2_TIME_100; > + else > + val |= EDP_PSR2_TP2_TIME_50; > > - if (dev_priv->psr.psr2_support) > - I915_WRITE(EDP_PSR2_CTL, EDP_PSR2_ENABLE | > - EDP_SU_TRACK_ENABLE | EDP_PSR2_TP2_TIME_100); > + I915_WRITE(EDP_PSR2_CTL, val); > } > > static bool intel_psr_match_conditions(struct intel_dp *intel_dp) --------------060705030206060909020908 Content-Type: text/html; charset=windows-1252 Content-Transfer-Encoding: 7bit

On 5/18/2016 10:17 PM, Daniel Vetter wrote:
Oops. Hw default for programming these fields to 0 is "skip link
training". Display won't take that too well usually.
But we were defaulting it to value 0, which means 500us for both TP1 and TP2 or TP3 time.
I dont think it means skip link training. This is just to set the time for the patterns.
Skip aux handshake can happen if bit 12 of SRD_CTL is set.

Does this solution help in fixing the bug mentioned here?

 

v2: Unbotch the math a bit.

v3: Drop debug hunk.

Tested-by: Lyude <cpaul@redhat.com>
Cc: Lyude <cpaul@redhat.com>
Cc: stable@vger.kernel.org
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=95176
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: Sonika Jindal <sonika.jindal@intel.com>
Cc: Durgadoss R <durgadoss.r@intel.com>
Cc: "Pandiyan, Dhinakaran" <dhinakaran.pandiyan@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++++++++++++++++------
 1 file changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index c3abae4bc596..a788d1e9589b 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -280,7 +280,10 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	 * with the 5 or 6 idle patterns.
 	 */
 	uint32_t idle_frames = max(6, dev_priv->vbt.psr.idle_frames);
-	uint32_t val = 0x0;
+	uint32_t val = EDP_PSR_ENABLE;
+
+	val |= max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT;
+	val |= idle_frames << EDP_PSR_IDLE_FRAME_SHIFT;
 
 	if (IS_HASWELL(dev))
 		val |= EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
@@ -288,14 +291,50 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp)
 	if (dev_priv->psr.link_standby)
 		val |= EDP_PSR_LINK_STANDBY;
 
-	I915_WRITE(EDP_PSR_CTL, val |
-		   max_sleep_time << EDP_PSR_MAX_SLEEP_TIME_SHIFT |
-		   idle_frames << EDP_PSR_IDLE_FRAME_SHIFT |
-		   EDP_PSR_ENABLE);
+	if (dev_priv->vbt.psr.tp1_wakeup_time > 5)
+		val |= EDP_PSR_TP1_TIME_2500us;
+	else if (dev_priv->vbt.psr.tp1_wakeup_time > 1)
+		val |= EDP_PSR_TP1_TIME_500us;
+	else if (dev_priv->vbt.psr.tp1_wakeup_time > 0)
+		val |= EDP_PSR_TP1_TIME_100us;
+	else
+		val |= EDP_PSR_TP1_TIME_0us;
+
+	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
+		val |= EDP_PSR_TP2_TP3_TIME_2500us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
+		val |= EDP_PSR_TP2_TP3_TIME_500us;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
+		val |= EDP_PSR_TP2_TP3_TIME_100us;
+	else
+		val |= EDP_PSR_TP2_TP3_TIME_0us;
+
+	if (intel_dp_source_supports_hbr2(intel_dp) &&
+	    drm_dp_tps3_supported(intel_dp->dpcd))
+		val |= EDP_PSR_TP1_TP3_SEL;
+	else
+		val |= EDP_PSR_TP1_TP2_SEL;
+
+	I915_WRITE(EDP_PSR_CTL, val);
+
+	if (!dev_priv->psr.psr2_support)
+		return;
+
+	/* FIXME: selective update is probably totally broken because it doesn't
+	 * mesh at all with our frontbuffer tracking. And the hw alone isn't
+	 * good enough. */
+	val = EDP_PSR2_ENABLE | EDP_SU_TRACK_ENABLE;
+
+	if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 5)
+		val |= EDP_PSR2_TP2_TIME_2500;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 1)
+		val |= EDP_PSR2_TP2_TIME_500;
+	else if (dev_priv->vbt.psr.tp2_tp3_wakeup_time > 0)
+		val |= EDP_PSR2_TP2_TIME_100;
+	else
+		val |= EDP_PSR2_TP2_TIME_50;
 
-	if (dev_priv->psr.psr2_support)
-		I915_WRITE(EDP_PSR2_CTL, EDP_PSR2_ENABLE |
-				EDP_SU_TRACK_ENABLE | EDP_PSR2_TP2_TIME_100);
+	I915_WRITE(EDP_PSR2_CTL, val);
 }
 
 static bool intel_psr_match_conditions(struct intel_dp *intel_dp)

--------------060705030206060909020908-- --===============0119744276== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4 IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== --===============0119744276==--