Re: [PATCH] drm/i915: Change order of operations for VLV/CHV to not train DP link before PHYs are ready

[Todd Previte <tprevite@gmail.com>]

On 10/10/14 1:04 AM, Jani Nikula wrote:
> On Thu, 09 Oct 2014, Todd Previte<tprevite@gmail.com>  wrote:
>> Reorder the function calls in chv/vlv_pre_enable_dp() such that link training is not initiated
>> before the PHYs come up out of reset. Also check the status of vlv_wait_port_ready() and
>> only attempt to train if the PHYs are actually running.
>>
>> The specification lists the wait for the PHYs as one of the final steps in enabling
>> the Displayport hardware for use.  While the PHYs are in reset, no communication is p
>> ossible across the link. Attempting to train the link while the PHYs are in reset will
>> result in link training failure with one or more WARN() in the logs. Moving the
>> intel_enable_dp() function after vlv_wait_port_ready() and only when the PHYs are ready
>> helps ensure reliable operation of the Displayport link.
> There's been some back and forth about the mode set sequence ordering in
> the past. There's also some disagreement between some specs about
> this. The published display spec at 01.org says,
>
> • Enable ports
> • Wait for DPIO phystatus ready in 6014
> • Enable pipe A/B
> • Enable planes (VGA or hires)
>
> This is in line with the current implementation. However, there's no
> details about the link training - perhaps you could try waiting for DPIO
> phystatus ready right after intel_dp_enable_port() but before link
> training?
>
> BR,
> Jani.
>
>
> PS. Please wrap the commit messages at, say, 72 characters. Below 80
> anyway.
Thanks for the feedback, Jani. That might be a good option. That 
essentially moves the wait_port_ready into the intel_enable_dp() 
function, so it would need a platform check there as well. Not a big 
deal, really. Alternatively,  it looks like intel_dp_enable_port() can 
be moved out of intel_enable_dp() and placed right before the call to 
vlv_wait_port_ready(). There's only a few places where intel_enable_dp() 
is called, it shouldn't be too much of an issue to pull 
intel_dp_enable_port() out and place it where necessary. I'll post the 
updated patch as soon as it's ready.

I'll amend the commit message and wrap it under 80 chars, too.

-T

>> Signed-off-by: Todd Previte<tprevite@gmail.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |  8 ++++++--
>>   drivers/gpu/drm/i915/intel_dp.c      | 10 ++++------
>>   drivers/gpu/drm/i915/intel_drv.h     |  2 +-
>>   3 files changed, 11 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index c51d950..4b280c1 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -1723,7 +1723,7 @@ static void chv_disable_pll(struct drm_i915_private *dev_priv, enum pipe pipe)
>>   	mutex_unlock(&dev_priv->dpio_lock);
>>   }
>>   
>> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>> +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>>   		struct intel_digital_port *dport)
>>   {
>>   	u32 port_mask;
>> @@ -1746,9 +1746,13 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>>   		BUG();
>>   	}
>>   
>> -	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000))
>> +	if (wait_for((I915_READ(dpll_reg) & port_mask) == 0, 1000)) {
>>   		WARN(1, "timed out waiting for port %c ready: 0x%08x\n",
>>   		     port_name(dport->port), I915_READ(dpll_reg));
>> +		return -EIO;
>> +	}
>> +
>> +	return 0;
>>   }
>>   
>>   static void intel_prepare_shared_dpll(struct intel_crtc *crtc)
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index a8352c4..ada8b07 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -2705,9 +2705,8 @@ static void vlv_pre_enable_dp(struct intel_encoder *encoder)
>>   		pps_unlock(intel_dp);
>>   	}
>>   
>> -	intel_enable_dp(encoder);
>> -
>> -	vlv_wait_port_ready(dev_priv, dport);
>> +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
>> +	    intel_enable_dp(encoder);
>>   }
>>   
>>   static void vlv_dp_pre_pll_enable(struct intel_encoder *encoder)
>> @@ -2805,9 +2804,8 @@ static void chv_pre_enable_dp(struct intel_encoder *encoder)
>>   		pps_unlock(intel_dp);
>>   	}
>>   
>> -	intel_enable_dp(encoder);
>> -
>> -	vlv_wait_port_ready(dev_priv, dport);
>> +	if (vlv_wait_port_ready(dev_priv, dport) == 0)
>> +	    intel_enable_dp(encoder);
>>   }
>>   
>>   static void chv_dp_pre_pll_enable(struct intel_encoder *encoder)
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index dc80444..2ff2c8c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -876,7 +876,7 @@ intel_wait_for_vblank(struct drm_device *dev, int pipe)
>>   	drm_wait_one_vblank(dev, pipe);
>>   }
>>   int ironlake_get_lanes_required(int target_clock, int link_bw, int bpp);
>> -void vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>> +int vlv_wait_port_ready(struct drm_i915_private *dev_priv,
>>   			 struct intel_digital_port *dport);
>>   bool intel_get_load_detect_pipe(struct drm_connector *connector,
>>   				struct drm_display_mode *mode,
>> -- 
>> 1.9.1
>>
>> _______________________________________________
>> 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