Re: [PATCH 1/2] drm/dp: move hw_mutex up the call stack

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Feb 25, 2016 at 04:15:05PM -0500, Rob Clark wrote:
> 1) don't let other threads trying to bang on aux channel interrupt the
> defer timeout/logic
> 2) don't let other threads interrupt the i2c over aux logic
> 
> Technically, according to people who actually have the DP spec, this
> should not be required.  In practice, it makes some troublesome Dell
> monitor (and perhaps others) work, so probably a case of "It's compliant
> if it works with windows" on the hw vendor's part..
> 
> v2: rebased to come before DPCD/AUX logging patch for easier backport
> to stable branches.
> 
> Reported-by: Dave Wysochanski <dwysocha@redhat.com>
> Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1274157
> Cc: stable@vger.kernel.org
> Signed-off-by: Rob Clark <robdclark@gmail.com>

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Dave, can you pls pick up?

Thanks, Daniel

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 27 +++++++++++++++++----------
>  1 file changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 7d58f59..df64ed1 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -179,7 +179,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  {
>  	struct drm_dp_aux_msg msg;
>  	unsigned int retry;
> -	int err;
> +	int err = 0;
>  
>  	memset(&msg, 0, sizeof(msg));
>  	msg.address = offset;
> @@ -187,6 +187,8 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  	msg.buffer = buffer;
>  	msg.size = size;
>  
> +	mutex_lock(&aux->hw_mutex);
> +
>  	/*
>  	 * The specification doesn't give any recommendation on how often to
>  	 * retry native transactions. We used to retry 7 times like for
> @@ -195,25 +197,24 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  	 */
>  	for (retry = 0; retry < 32; retry++) {
>  
> -		mutex_lock(&aux->hw_mutex);
>  		err = aux->transfer(aux, &msg);
> -		mutex_unlock(&aux->hw_mutex);
>  		if (err < 0) {
>  			if (err == -EBUSY)
>  				continue;
>  
> -			return err;
> +			goto unlock;
>  		}
>  
>  
>  		switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
>  		case DP_AUX_NATIVE_REPLY_ACK:
>  			if (err < size)
> -				return -EPROTO;
> -			return err;
> +				err = -EPROTO;
> +			goto unlock;
>  
>  		case DP_AUX_NATIVE_REPLY_NACK:
> -			return -EIO;
> +			err = -EIO;
> +			goto unlock;
>  
>  		case DP_AUX_NATIVE_REPLY_DEFER:
>  			usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> @@ -222,7 +223,11 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>  	}
>  
>  	DRM_DEBUG_KMS("too many retries, giving up\n");
> -	return -EIO;
> +	err = -EIO;
> +
> +unlock:
> +	mutex_unlock(&aux->hw_mutex);
> +	return err;
>  }
>  
>  /**
> @@ -544,9 +549,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>  	int max_retries = max(7, drm_dp_i2c_retry_count(msg, dp_aux_i2c_speed_khz));
>  
>  	for (retry = 0, defer_i2c = 0; retry < (max_retries + defer_i2c); retry++) {
> -		mutex_lock(&aux->hw_mutex);
>  		ret = aux->transfer(aux, msg);
> -		mutex_unlock(&aux->hw_mutex);
>  		if (ret < 0) {
>  			if (ret == -EBUSY)
>  				continue;
> @@ -685,6 +688,8 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  
>  	memset(&msg, 0, sizeof(msg));
>  
> +	mutex_lock(&aux->hw_mutex);
> +
>  	for (i = 0; i < num; i++) {
>  		msg.address = msgs[i].addr;
>  		drm_dp_i2c_msg_set_request(&msg, &msgs[i]);
> @@ -739,6 +744,8 @@ static int drm_dp_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs,
>  	msg.size = 0;
>  	(void)drm_dp_i2c_do_msg(aux, &msg);
>  
> +	mutex_unlock(&aux->hw_mutex);
> +
>  	return err;
>  }
>  
> -- 
> 2.5.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch