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

Todd Previte
2014-10-11T00:18:50Z

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