* [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2015-12-09 21:20 Rob Clark
2015-12-11 16:32 ` Ville Syrjälä
0 siblings, 1 reply; 4+ messages in thread
From: Rob Clark @ 2015-12-09 21:20 UTC (permalink / raw)
To: dri-devel; +Cc: Dave Wysochanski, Rob Clark, stable
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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] drm/dp: move hw_mutex up the call stack
2015-12-09 21:20 [PATCH 1/2] drm/dp: move hw_mutex up the call stack Rob Clark
@ 2015-12-11 16:32 ` Ville Syrjälä
0 siblings, 0 replies; 4+ messages in thread
From: Ville Syrjälä @ 2015-12-11 16:32 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel, Dave Wysochanski, stable
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2016-02-25 21:15 Rob Clark
2016-02-29 16:46 ` Daniel Vetter
0 siblings, 1 reply; 4+ messages in thread
From: Rob Clark @ 2016-02-25 21:15 UTC (permalink / raw)
To: dri-devel
Cc: Jani Nikula, Ville Syrjälä, Daniel Vetter, Rob Clark,
stable
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>
---
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
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH 1/2] drm/dp: move hw_mutex up the call stack
2016-02-25 21:15 Rob Clark
@ 2016-02-29 16:46 ` Daniel Vetter
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Vetter @ 2016-02-29 16:46 UTC (permalink / raw)
To: Rob Clark
Cc: dri-devel, Jani Nikula, Ville Syrjälä, Daniel Vetter,
stable, Dave Airlie
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
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-02-29 16:45 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-12-09 21:20 [PATCH 1/2] drm/dp: move hw_mutex up the call stack Rob Clark
2015-12-11 16:32 ` Ville Syrjälä
-- strict thread matches above, loose matches on Subject: below --
2016-02-25 21:15 Rob Clark
2016-02-29 16:46 ` Daniel Vetter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).