Re: [RFC PATCH 1/7] drm/i915/dp: clean up cargo culted intel_dp_aux_native_read_retry() usage

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Feb 04, 2014 at 03:40:44PM +0200, Jani Nikula wrote:
> intel_dp_aux_native_read_retry() is only needed when the sink might be
> asleep. Use the regular read without retries otherwise.

I guess I should repeat here what I mentioned to Jani:

The DP spec seems to indicate that AUX transactions should be performed
only when the sink is in on/standby states. When it's in the sleep state,
it may only monitor the AUX channel enough to detect something's happening.
When it detects the AUX activity, it may take up to 1ms (20ms for
"embedded connections") to respond. The state machine diagram shows
that the only valid AUX transaction here is a write of 1h to the 600h
register to wake the sink up to standby state. After that further AUX
transfers can be peformed normally. And once the source is done with
the AUX transfers, it can either proceed w/ link training and move the
sink to the on state, or it may put it back to sleep.

Currently we're doing all kinds of AUX transfers while the sink is still
in the sleep state. This doesn't appear valid according to the spec, and
even if it were, I think we'd need to use the _retry variant pretty much
always since there would no guarantee that the sink wouldn't put the AUX
channel back into the monitor-only mode between transfers.

So it seems we'd need to keep better track of the sink power state while
doing AUX transfers.

> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index f1ef3d482c87..19ff1b161ffb 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2871,9 +2871,9 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> -		intel_dp_aux_native_read_retry(intel_dp, DP_PSR_SUPPORT,
> -					       intel_dp->psr_dpcd,
> -					       sizeof(intel_dp->psr_dpcd));
> +		intel_dp_aux_native_read(intel_dp, DP_PSR_SUPPORT,
> +					 intel_dp->psr_dpcd,
> +					 sizeof(intel_dp->psr_dpcd));
>  		if (intel_dp->psr_dpcd[0] & DP_PSR_IS_SUPPORTED) {
>  			dev_priv->psr.sink_support = true;
>  			DRM_DEBUG_KMS("Detected EDP PSR Panel.\n");
> @@ -2895,9 +2895,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0x10)
>  		return true; /* no per-port downstream info */
>  
> -	if (intel_dp_aux_native_read_retry(intel_dp, DP_DOWNSTREAM_PORT_0,
> -					   intel_dp->downstream_ports,
> -					   DP_MAX_DOWNSTREAM_PORTS) == 0)
> +	if (intel_dp_aux_native_read(intel_dp, DP_DOWNSTREAM_PORT_0,
> +				     intel_dp->downstream_ports,
> +				     DP_MAX_DOWNSTREAM_PORTS) !=
> +	    DP_MAX_DOWNSTREAM_PORTS)
>  		return false; /* downstream port status fetch failed */
>  
>  	return true;
> @@ -2913,11 +2914,11 @@ intel_dp_probe_oui(struct intel_dp *intel_dp)
>  
>  	edp_panel_vdd_on(intel_dp);
>  
> -	if (intel_dp_aux_native_read_retry(intel_dp, DP_SINK_OUI, buf, 3))
> +	if (intel_dp_aux_native_read(intel_dp, DP_SINK_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Sink OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> -	if (intel_dp_aux_native_read_retry(intel_dp, DP_BRANCH_OUI, buf, 3))
> +	if (intel_dp_aux_native_read(intel_dp, DP_BRANCH_OUI, buf, 3) == 3)
>  		DRM_DEBUG_KMS("Branch OUI: %02hx%02hx%02hx\n",
>  			      buf[0], buf[1], buf[2]);
>  
> @@ -2953,18 +2954,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
>  	return 0;
>  }
>  
> -static bool
> +static inline bool
>  intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
>  {
> -	int ret;
> -
> -	ret = intel_dp_aux_native_read_retry(intel_dp,
> -					     DP_DEVICE_SERVICE_IRQ_VECTOR,
> -					     sink_irq_vector, 1);
> -	if (!ret)
> -		return false;
> -
> -	return true;
> +	return intel_dp_aux_native_read(intel_dp, DP_DEVICE_SERVICE_IRQ_VECTOR,
> +					sink_irq_vector, 1) == 1;
>  }
>  
>  static void
> @@ -3047,8 +3041,7 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>  		uint8_t reg;
> -		if (!intel_dp_aux_native_read_retry(intel_dp, DP_SINK_COUNT,
> -						    &reg, 1))
> +		if (intel_dp_aux_native_read(intel_dp, DP_SINK_COUNT, &reg, 1) != 1)
>  			return connector_status_unknown;
>  		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>  					      : connector_status_disconnected;
> -- 
> 1.7.9.5

-- 
Ville Syrjälä
Intel OTC