[Intel-gfx] [PATCH 2/7] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic

[R, Durgadoss]

>-----Original Message-----
>From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Rodrigo Vivi
>Sent: Saturday, February 28, 2015 6:56 AM
>To: intel-gfx at lists.freedesktop.org
>Cc: Runyan, Arthur J; Vivi, Rodrigo
>Subject: [Intel-gfx] [PATCH 2/7] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic
>
>Since the begining there is a missunderstanding on the meaning of this
>dpcd bit.
>This bit should'n indicate wheter to use link standby or not, but just

Shouldn't , whether

>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 to do exit HW tracking is still
>fully responsible for this exit logic with DP training or not.

I think it will be better to rephrase this as something like below:
"Even with help of frontbuffer tracking, HW is still fully responsible for
PSR exit logic with/without DP training"

>
>Cc: Arthur Runyan <arthur.j.runyan at intel.com>
>Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>---
> drivers/gpu/drm/i915/intel_psr.c | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>index 2e6831d..6c8e9e0 100644
>--- a/drivers/gpu/drm/i915/intel_psr.c
>+++ b/drivers/gpu/drm/i915/intel_psr.c
>@@ -242,8 +242,10 @@ 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) {

Shouldn't this 'if' check also include if (psr.link_standby) ?
Otherwise, even for link_disable cases we are configuring to 'skip
TPs'. Will this work ?

I see that in the next patch, you are removing this whole link_standby
thing. So, would leave it up to you to either add this check or not.

Thanks,
Durga

> 		val |= EDP_PSR_TP2_TP3_TIME_0us;
> 		val |= EDP_PSR_TP1_TIME_0us;
> 		val |= EDP_PSR_SKIP_AUX_EXIT;
>@@ -354,8 +356,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;
>--
>1.9.3
>
>_______________________________________________
>Intel-gfx mailing list
>Intel-gfx at lists.freedesktop.org
>http://lists.freedesktop.org/mailman/listinfo/intel-gfx