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

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

On Wed, Dec 09, 2015 at 04:20:09PM -0500, Rob Clark wrote:

Something small missing here perhaps. The patch subject is not part
of the commit message body.

> 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..
> 
> 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>

Real world wins.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/drm_dp_helper.c | 29 +++++++++++++++++++----------
>  1 file changed, 19 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index 9535c5b..76e08dc 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -178,7 +178,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;
> @@ -186,6 +186,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
> @@ -194,25 +196,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);
> @@ -221,7 +222,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;
>  }
>  
>  /**
> @@ -542,10 +547,10 @@ 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));
>  
> +	WARN_ON(!mutex_is_locked(&aux->hw_mutex));
> +
>  	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;
> @@ -684,6 +689,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]);
> @@ -738,6 +745,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
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel