* [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2016-02-25 21:15 ` Rob Clark
0 siblings, 0 replies; 14+ 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] 14+ messages in thread
* [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2016-02-25 21:15 ` Rob Clark
0 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2016-02-25 21:15 UTC (permalink / raw)
To: dri-devel; +Cc: 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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ messages in thread
* [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-02-25 21:15 ` Rob Clark
(?)
@ 2016-02-25 21:15 ` Rob Clark
2016-02-29 17:52 ` Ville Syrjälä
2016-12-21 20:41 ` Imre Deak
-1 siblings, 2 replies; 14+ messages in thread
From: Rob Clark @ 2016-02-25 21:15 UTC (permalink / raw)
To: dri-devel
Add a new drm_debug bit for turning on DPCD logging, to aid debugging
with troublesome monitors.
v2: don't try to hexdump the universe if driver returns -errno, and
change the "too many retries" traces to DRM_ERROR()
v3: rename to more generic "AUX" instead of DP specific DPCD, add
DP_AUX_I2C_WRITE_STATUS_UPDATE
Signed-off-by: Rob Clark <robdclark@gmail.com>
---
drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
include/drm/drmP.h | 8 +++++-
2 files changed, 58 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
index df64ed1..3ef35fd 100644
--- a/drivers/gpu/drm/drm_dp_helper.c
+++ b/drivers/gpu/drm/drm_dp_helper.c
@@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
}
EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
+static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
+{
+ ssize_t ret;
+
+ DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
+ msg->request, msg->address, msg->size);
+
+ if (unlikely(drm_debug & DRM_UT_AUX)) {
+ switch (msg->request & ~DP_AUX_I2C_MOT) {
+ case DP_AUX_NATIVE_WRITE:
+ case DP_AUX_I2C_WRITE:
+ case DP_AUX_I2C_WRITE_STATUS_UPDATE:
+ print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
+ 16, 1, msg->buffer, msg->size, false);
+ break;
+ default:
+ break;
+ }
+ }
+
+ ret = aux->transfer(aux, msg);
+
+ DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
+
+ if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
+ switch (msg->request & ~DP_AUX_I2C_MOT) {
+ case DP_AUX_NATIVE_READ:
+ case DP_AUX_I2C_READ:
+ print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
+ 16, 1, msg->buffer, ret, false);
+ break;
+ default:
+ break;
+ }
+ }
+
+ return ret;
+}
+
#define AUX_RETRY_INTERVAL 500 /* us */
/**
@@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
*/
for (retry = 0; retry < 32; retry++) {
- err = aux->transfer(aux, &msg);
+ err = aux_transfer(aux, &msg);
if (err < 0) {
if (err == -EBUSY)
continue;
@@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
goto unlock;
}
-
switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
case DP_AUX_NATIVE_REPLY_ACK:
if (err < size)
@@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
goto unlock;
case DP_AUX_NATIVE_REPLY_NACK:
+ DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
err = -EIO;
goto unlock;
case DP_AUX_NATIVE_REPLY_DEFER:
+ DRM_DEBUG_AUX("native defer\n");
usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
break;
}
}
- DRM_DEBUG_KMS("too many retries, giving up\n");
+ DRM_ERROR("DPCD: too many retries, giving up!\n");
err = -EIO;
unlock:
@@ -549,12 +589,12 @@ 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++) {
- ret = aux->transfer(aux, msg);
+ ret = aux_transfer(aux, msg);
if (ret < 0) {
if (ret == -EBUSY)
continue;
- DRM_DEBUG_KMS("transaction failed: %d\n", ret);
+ DRM_DEBUG_AUX("transaction failed: %d\n", ret);
return ret;
}
@@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
break;
case DP_AUX_NATIVE_REPLY_NACK:
- DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
+ DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
return -EREMOTEIO;
case DP_AUX_NATIVE_REPLY_DEFER:
- DRM_DEBUG_KMS("native defer\n");
+ DRM_DEBUG_AUX("native defer\n");
/*
* We could check for I2C bit rate capabilities and if
* available adjust this interval. We could also be
@@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
return ret;
case DP_AUX_I2C_REPLY_NACK:
- DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
+ DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
aux->i2c_nack_count++;
return -EREMOTEIO;
case DP_AUX_I2C_REPLY_DEFER:
- DRM_DEBUG_KMS("I2C defer\n");
+ DRM_DEBUG_AUX("I2C defer\n");
/* DP Compliance Test 4.2.2.5 Requirement:
* Must have at least 7 retries for I2C defers on the
* transaction to pass this test
@@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
}
}
- DRM_DEBUG_KMS("too many retries, giving up\n");
+ DRM_ERROR("I2C: too many retries, giving up\n");
return -EREMOTEIO;
}
@@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
return err == 0 ? -EPROTO : err;
if (err < msg.size && err < ret) {
- DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
+ DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
msg.size, err);
ret = err;
}
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 3c8422c..cc524b5 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -117,7 +117,7 @@ struct dma_buf_attachment;
* drm.debug=0x2 will enable DRIVER messages
* drm.debug=0x3 will enable CORE and DRIVER messages
* ...
- * drm.debug=0x3f will enable all messages
+ * drm.debug=0x7f will enable all messages
*
* An interesting feature is that it's possible to enable verbose logging at
* run-time by echoing the debug value in its sysfs node:
@@ -129,6 +129,7 @@ struct dma_buf_attachment;
#define DRM_UT_PRIME 0x08
#define DRM_UT_ATOMIC 0x10
#define DRM_UT_VBL 0x20
+#define DRM_UT_AUX 0x40
extern __printf(2, 3)
void drm_ut_debug_printk(const char *function_name,
@@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
if (unlikely(drm_debug & DRM_UT_VBL)) \
drm_ut_debug_printk(__func__, fmt, ##args); \
} while (0)
+#define DRM_DEBUG_AUX(fmt, args...) \
+ do { \
+ if (unlikely(drm_debug & DRM_UT_AUX)) \
+ drm_ut_debug_printk(__func__, fmt, ##args); \
+ } while (0)
/*@}*/
--
2.5.0
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply related [flat|nested] 14+ 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
-1 siblings, 0 replies; 14+ 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] 14+ messages in thread
* Re: [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2016-02-29 16:46 ` Daniel Vetter
0 siblings, 0 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-02-29 16:46 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel, stable
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-02-25 21:15 ` [PATCH 2/2] drm/dp: add DPCD/AUX logging Rob Clark
@ 2016-02-29 17:52 ` Ville Syrjälä
2016-12-21 20:41 ` Imre Deak
1 sibling, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2016-02-29 17:52 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Thu, Feb 25, 2016 at 04:15:06PM -0500, Rob Clark wrote:
> Add a new drm_debug bit for turning on DPCD logging, to aid debugging
> with troublesome monitors.
>
> v2: don't try to hexdump the universe if driver returns -errno, and
> change the "too many retries" traces to DRM_ERROR()
> v3: rename to more generic "AUX" instead of DP specific DPCD, add
> DP_AUX_I2C_WRITE_STATUS_UPDATE
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
> ---
> drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
> include/drm/drmP.h | 8 +++++-
> 2 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index df64ed1..3ef35fd 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
> }
> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>
> +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> +{
> + ssize_t ret;
> +
> + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
> + msg->request, msg->address, msg->size);
Bunch of stuff doesn't seem to be indented quite right in this patch.
This being one of them.
> +
> + if (unlikely(drm_debug & DRM_UT_AUX)) {
> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> + case DP_AUX_NATIVE_WRITE:
> + case DP_AUX_I2C_WRITE:
> + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> + 16, 1, msg->buffer, msg->size, false);
That shouldn't say "DPCD", at least for i2c.
> + break;
> + default:
> + break;
> + }
> + }
> +
> + ret = aux->transfer(aux, msg);
> +
> + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
> +
> + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> + case DP_AUX_NATIVE_READ:
> + case DP_AUX_I2C_READ:
> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> + 16, 1, msg->buffer, ret, false);
ditto
Otherwise looks all right.
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> #define AUX_RETRY_INTERVAL 500 /* us */
>
> /**
> @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> */
> for (retry = 0; retry < 32; retry++) {
>
> - err = aux->transfer(aux, &msg);
> + err = aux_transfer(aux, &msg);
> if (err < 0) {
> if (err == -EBUSY)
> continue;
> @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> goto unlock;
> }
>
> -
> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> case DP_AUX_NATIVE_REPLY_ACK:
> if (err < size)
> @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> goto unlock;
>
> case DP_AUX_NATIVE_REPLY_NACK:
> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
> err = -EIO;
> goto unlock;
>
> case DP_AUX_NATIVE_REPLY_DEFER:
> + DRM_DEBUG_AUX("native defer\n");
> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> break;
> }
> }
>
> - DRM_DEBUG_KMS("too many retries, giving up\n");
> + DRM_ERROR("DPCD: too many retries, giving up!\n");
> err = -EIO;
>
> unlock:
> @@ -549,12 +589,12 @@ 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++) {
> - ret = aux->transfer(aux, msg);
> + ret = aux_transfer(aux, msg);
> if (ret < 0) {
> if (ret == -EBUSY)
> continue;
>
> - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
> + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
> return ret;
> }
>
> @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> break;
>
> case DP_AUX_NATIVE_REPLY_NACK:
> - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
> return -EREMOTEIO;
>
> case DP_AUX_NATIVE_REPLY_DEFER:
> - DRM_DEBUG_KMS("native defer\n");
> + DRM_DEBUG_AUX("native defer\n");
> /*
> * We could check for I2C bit rate capabilities and if
> * available adjust this interval. We could also be
> @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> return ret;
>
> case DP_AUX_I2C_REPLY_NACK:
> - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
> + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
> aux->i2c_nack_count++;
> return -EREMOTEIO;
>
> case DP_AUX_I2C_REPLY_DEFER:
> - DRM_DEBUG_KMS("I2C defer\n");
> + DRM_DEBUG_AUX("I2C defer\n");
> /* DP Compliance Test 4.2.2.5 Requirement:
> * Must have at least 7 retries for I2C defers on the
> * transaction to pass this test
> @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> }
> }
>
> - DRM_DEBUG_KMS("too many retries, giving up\n");
> + DRM_ERROR("I2C: too many retries, giving up\n");
> return -EREMOTEIO;
> }
>
> @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
> return err == 0 ? -EPROTO : err;
>
> if (err < msg.size && err < ret) {
> - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
> msg.size, err);
> ret = err;
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..cc524b5 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -117,7 +117,7 @@ struct dma_buf_attachment;
> * drm.debug=0x2 will enable DRIVER messages
> * drm.debug=0x3 will enable CORE and DRIVER messages
> * ...
> - * drm.debug=0x3f will enable all messages
> + * drm.debug=0x7f will enable all messages
> *
> * An interesting feature is that it's possible to enable verbose logging at
> * run-time by echoing the debug value in its sysfs node:
> @@ -129,6 +129,7 @@ struct dma_buf_attachment;
> #define DRM_UT_PRIME 0x08
> #define DRM_UT_ATOMIC 0x10
> #define DRM_UT_VBL 0x20
> +#define DRM_UT_AUX 0x40
>
> extern __printf(2, 3)
> void drm_ut_debug_printk(const char *function_name,
> @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
> if (unlikely(drm_debug & DRM_UT_VBL)) \
> drm_ut_debug_printk(__func__, fmt, ##args); \
> } while (0)
> +#define DRM_DEBUG_AUX(fmt, args...) \
> + do { \
> + if (unlikely(drm_debug & DRM_UT_AUX)) \
> + drm_ut_debug_printk(__func__, fmt, ##args); \
> + } while (0)
>
> /*@}*/
>
> --
> 2.5.0
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-02-25 21:15 ` [PATCH 2/2] drm/dp: add DPCD/AUX logging Rob Clark
2016-02-29 17:52 ` Ville Syrjälä
@ 2016-12-21 20:41 ` Imre Deak
2016-12-21 22:21 ` Rob Clark
1 sibling, 1 reply; 14+ messages in thread
From: Imre Deak @ 2016-12-21 20:41 UTC (permalink / raw)
To: Rob Clark, dri-devel
On Thu, 2016-02-25 at 16:15 -0500, Rob Clark wrote:
> Add a new drm_debug bit for turning on DPCD logging, to aid debugging
> with troublesome monitors.
>
> v2: don't try to hexdump the universe if driver returns -errno, and
> change the "too many retries" traces to DRM_ERROR()
> v3: rename to more generic "AUX" instead of DP specific DPCD, add
> DP_AUX_I2C_WRITE_STATUS_UPDATE
>
> Signed-off-by: Rob Clark <robdclark@gmail.com>
I used the rebased version of this after Ville pointed me to it for
remote debugging some odd adaptor behavior. It'd be quite useful imo, so:
Acked-by: Imre Deak <imre.deak@intel.com>
> ---
> drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
> include/drm/drmP.h | 8 +++++-
> 2 files changed, 58 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index df64ed1..3ef35fd 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
> }
> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>
> +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> +{
> + ssize_t ret;
> +
> + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
> + msg->request, msg->address, msg->size);
> +
> + if (unlikely(drm_debug & DRM_UT_AUX)) {
> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> + case DP_AUX_NATIVE_WRITE:
> + case DP_AUX_I2C_WRITE:
> + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> + 16, 1, msg->buffer, msg->size, false);
> + break;
> + default:
> + break;
> + }
> + }
> +
> + ret = aux->transfer(aux, msg);
> +
> + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
> +
> + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> + case DP_AUX_NATIVE_READ:
> + case DP_AUX_I2C_READ:
> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> + 16, 1, msg->buffer, ret, false);
> + break;
> + default:
> + break;
> + }
> + }
> +
> + return ret;
> +}
> +
> #define AUX_RETRY_INTERVAL 500 /* us */
>
> /**
> @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> */
> for (retry = 0; retry < 32; retry++) {
>
> - err = aux->transfer(aux, &msg);
> + err = aux_transfer(aux, &msg);
> if (err < 0) {
> if (err == -EBUSY)
> continue;
> @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> goto unlock;
> }
>
> -
> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> case DP_AUX_NATIVE_REPLY_ACK:
> if (err < size)
> @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> goto unlock;
>
> case DP_AUX_NATIVE_REPLY_NACK:
> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
> err = -EIO;
> goto unlock;
>
> case DP_AUX_NATIVE_REPLY_DEFER:
> + DRM_DEBUG_AUX("native defer\n");
> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> break;
> }
> }
>
> - DRM_DEBUG_KMS("too many retries, giving up\n");
> + DRM_ERROR("DPCD: too many retries, giving up!\n");
> err = -EIO;
>
> unlock:
> @@ -549,12 +589,12 @@ 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++) {
> - ret = aux->transfer(aux, msg);
> + ret = aux_transfer(aux, msg);
> if (ret < 0) {
> if (ret == -EBUSY)
> continue;
>
> - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
> + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
> return ret;
> }
>
> @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> break;
>
> case DP_AUX_NATIVE_REPLY_NACK:
> - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
> return -EREMOTEIO;
>
> case DP_AUX_NATIVE_REPLY_DEFER:
> - DRM_DEBUG_KMS("native defer\n");
> + DRM_DEBUG_AUX("native defer\n");
> /*
> * We could check for I2C bit rate capabilities and if
> * available adjust this interval. We could also be
> @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> return ret;
>
> case DP_AUX_I2C_REPLY_NACK:
> - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
> + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
> aux->i2c_nack_count++;
> return -EREMOTEIO;
>
> case DP_AUX_I2C_REPLY_DEFER:
> - DRM_DEBUG_KMS("I2C defer\n");
> + DRM_DEBUG_AUX("I2C defer\n");
> /* DP Compliance Test 4.2.2.5 Requirement:
> * Must have at least 7 retries for I2C defers on the
> * transaction to pass this test
> @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> }
> }
>
> - DRM_DEBUG_KMS("too many retries, giving up\n");
> + DRM_ERROR("I2C: too many retries, giving up\n");
> return -EREMOTEIO;
> }
>
> @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
> return err == 0 ? -EPROTO : err;
>
> if (err < msg.size && err < ret) {
> - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
> msg.size, err);
> ret = err;
> }
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index 3c8422c..cc524b5 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -117,7 +117,7 @@ struct dma_buf_attachment;
> * drm.debug=0x2 will enable DRIVER messages
> * drm.debug=0x3 will enable CORE and DRIVER messages
> * ...
> - * drm.debug=0x3f will enable all messages
> + * drm.debug=0x7f will enable all messages
> *
> * An interesting feature is that it's possible to enable verbose logging at
> * run-time by echoing the debug value in its sysfs node:
> @@ -129,6 +129,7 @@ struct dma_buf_attachment;
> #define DRM_UT_PRIME 0x08
> #define DRM_UT_ATOMIC 0x10
> #define DRM_UT_VBL 0x20
> +#define DRM_UT_AUX 0x40
>
> extern __printf(2, 3)
> void drm_ut_debug_printk(const char *function_name,
> @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
> if (unlikely(drm_debug & DRM_UT_VBL)) \
> drm_ut_debug_printk(__func__, fmt, ##args); \
> } while (0)
> +#define DRM_DEBUG_AUX(fmt, args...) \
> + do { \
> + if (unlikely(drm_debug & DRM_UT_AUX)) \
> + drm_ut_debug_printk(__func__, fmt, ##args); \
> + } while (0)
>
> /*@}*/
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-12-21 20:41 ` Imre Deak
@ 2016-12-21 22:21 ` Rob Clark
2016-12-22 7:02 ` Daniel Vetter
0 siblings, 1 reply; 14+ messages in thread
From: Rob Clark @ 2016-12-21 22:21 UTC (permalink / raw)
To: Imre Deak; +Cc: dri-devel
On Wed, Dec 21, 2016 at 3:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> On Thu, 2016-02-25 at 16:15 -0500, Rob Clark wrote:
>> Add a new drm_debug bit for turning on DPCD logging, to aid debugging
>> with troublesome monitors.
>>
>> v2: don't try to hexdump the universe if driver returns -errno, and
>> change the "too many retries" traces to DRM_ERROR()
>> v3: rename to more generic "AUX" instead of DP specific DPCD, add
>> DP_AUX_I2C_WRITE_STATUS_UPDATE
>>
>> Signed-off-by: Rob Clark <robdclark@gmail.com>
>
> I used the rebased version of this after Ville pointed me to it for
> remote debugging some odd adaptor behavior. It'd be quite useful imo, so:
> Acked-by: Imre Deak <imre.deak@intel.com>
tbh I completely forgot about this patch.. mind posting the rebased
version (or push to a git tree somewhere)? IIRC there were just some
minor cosmetic things to fix up, probably worth doing that before I
forget about it again..
BR,
-R
>
>> ---
>> drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
>> include/drm/drmP.h | 8 +++++-
>> 2 files changed, 58 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> index df64ed1..3ef35fd 100644
>> --- a/drivers/gpu/drm/drm_dp_helper.c
>> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
>> }
>> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>>
>> +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> +{
>> + ssize_t ret;
>> +
>> + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
>> + msg->request, msg->address, msg->size);
>> +
>> + if (unlikely(drm_debug & DRM_UT_AUX)) {
>> + switch (msg->request & ~DP_AUX_I2C_MOT) {
>> + case DP_AUX_NATIVE_WRITE:
>> + case DP_AUX_I2C_WRITE:
>> + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
>> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
>> + 16, 1, msg->buffer, msg->size, false);
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + ret = aux->transfer(aux, msg);
>> +
>> + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
>> +
>> + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
>> + switch (msg->request & ~DP_AUX_I2C_MOT) {
>> + case DP_AUX_NATIVE_READ:
>> + case DP_AUX_I2C_READ:
>> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
>> + 16, 1, msg->buffer, ret, false);
>> + break;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>> +
>> #define AUX_RETRY_INTERVAL 500 /* us */
>>
>> /**
>> @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> */
>> for (retry = 0; retry < 32; retry++) {
>>
>> - err = aux->transfer(aux, &msg);
>> + err = aux_transfer(aux, &msg);
>> if (err < 0) {
>> if (err == -EBUSY)
>> continue;
>> @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> goto unlock;
>> }
>>
>> -
>> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
>> case DP_AUX_NATIVE_REPLY_ACK:
>> if (err < size)
>> @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> goto unlock;
>>
>> case DP_AUX_NATIVE_REPLY_NACK:
>> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
>> err = -EIO;
>> goto unlock;
>>
>> case DP_AUX_NATIVE_REPLY_DEFER:
>> + DRM_DEBUG_AUX("native defer\n");
>> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
>> break;
>> }
>> }
>>
>> - DRM_DEBUG_KMS("too many retries, giving up\n");
>> + DRM_ERROR("DPCD: too many retries, giving up!\n");
>> err = -EIO;
>>
>> unlock:
>> @@ -549,12 +589,12 @@ 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++) {
>> - ret = aux->transfer(aux, msg);
>> + ret = aux_transfer(aux, msg);
>> if (ret < 0) {
>> if (ret == -EBUSY)
>> continue;
>>
>> - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
>> + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
>> return ret;
>> }
>>
>> @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> break;
>>
>> case DP_AUX_NATIVE_REPLY_NACK:
>> - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
>> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
>> return -EREMOTEIO;
>>
>> case DP_AUX_NATIVE_REPLY_DEFER:
>> - DRM_DEBUG_KMS("native defer\n");
>> + DRM_DEBUG_AUX("native defer\n");
>> /*
>> * We could check for I2C bit rate capabilities and if
>> * available adjust this interval. We could also be
>> @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> return ret;
>>
>> case DP_AUX_I2C_REPLY_NACK:
>> - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
>> + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
>> aux->i2c_nack_count++;
>> return -EREMOTEIO;
>>
>> case DP_AUX_I2C_REPLY_DEFER:
>> - DRM_DEBUG_KMS("I2C defer\n");
>> + DRM_DEBUG_AUX("I2C defer\n");
>> /* DP Compliance Test 4.2.2.5 Requirement:
>> * Must have at least 7 retries for I2C defers on the
>> * transaction to pass this test
>> @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> }
>> }
>>
>> - DRM_DEBUG_KMS("too many retries, giving up\n");
>> + DRM_ERROR("I2C: too many retries, giving up\n");
>> return -EREMOTEIO;
>> }
>>
>> @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
>> return err == 0 ? -EPROTO : err;
>>
>> if (err < msg.size && err < ret) {
>> - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
>> + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
>> msg.size, err);
>> ret = err;
>> }
>> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> index 3c8422c..cc524b5 100644
>> --- a/include/drm/drmP.h
>> +++ b/include/drm/drmP.h
>> @@ -117,7 +117,7 @@ struct dma_buf_attachment;
>> * drm.debug=0x2 will enable DRIVER messages
>> * drm.debug=0x3 will enable CORE and DRIVER messages
>> * ...
>> - * drm.debug=0x3f will enable all messages
>> + * drm.debug=0x7f will enable all messages
>> *
>> * An interesting feature is that it's possible to enable verbose logging at
>> * run-time by echoing the debug value in its sysfs node:
>> @@ -129,6 +129,7 @@ struct dma_buf_attachment;
>> #define DRM_UT_PRIME 0x08
>> #define DRM_UT_ATOMIC 0x10
>> #define DRM_UT_VBL 0x20
>> +#define DRM_UT_AUX 0x40
>>
>> extern __printf(2, 3)
>> void drm_ut_debug_printk(const char *function_name,
>> @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
>> if (unlikely(drm_debug & DRM_UT_VBL)) \
>> drm_ut_debug_printk(__func__, fmt, ##args); \
>> } while (0)
>> +#define DRM_DEBUG_AUX(fmt, args...) \
>> + do { \
>> + if (unlikely(drm_debug & DRM_UT_AUX)) \
>> + drm_ut_debug_printk(__func__, fmt, ##args); \
>> + } while (0)
>>
>> /*@}*/
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-12-21 22:21 ` Rob Clark
@ 2016-12-22 7:02 ` Daniel Vetter
2016-12-22 8:54 ` Imre Deak
2016-12-22 17:11 ` Rob Clark
0 siblings, 2 replies; 14+ messages in thread
From: Daniel Vetter @ 2016-12-22 7:02 UTC (permalink / raw)
To: Rob Clark; +Cc: dri-devel
On Wed, Dec 21, 2016 at 05:21:18PM -0500, Rob Clark wrote:
> On Wed, Dec 21, 2016 at 3:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> > On Thu, 2016-02-25 at 16:15 -0500, Rob Clark wrote:
> >> Add a new drm_debug bit for turning on DPCD logging, to aid debugging
> >> with troublesome monitors.
> >>
> >> v2: don't try to hexdump the universe if driver returns -errno, and
> >> change the "too many retries" traces to DRM_ERROR()
> >> v3: rename to more generic "AUX" instead of DP specific DPCD, add
> >> DP_AUX_I2C_WRITE_STATUS_UPDATE
> >>
> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
> >
> > I used the rebased version of this after Ville pointed me to it for
> > remote debugging some odd adaptor behavior. It'd be quite useful imo, so:
> > Acked-by: Imre Deak <imre.deak@intel.com>
>
> tbh I completely forgot about this patch.. mind posting the rebased
> version (or push to a git tree somewhere)? IIRC there were just some
> minor cosmetic things to fix up, probably worth doing that before I
> forget about it again..
There's patches for dynamic debugging floating around, I think that'd be
the more interesting approach than adding ever more magic bits that no one
understands. But besides that this makes sense.
-Daniel
>
> BR,
> -R
>
>
> >
> >> ---
> >> drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
> >> include/drm/drmP.h | 8 +++++-
> >> 2 files changed, 58 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> >> index df64ed1..3ef35fd 100644
> >> --- a/drivers/gpu/drm/drm_dp_helper.c
> >> +++ b/drivers/gpu/drm/drm_dp_helper.c
> >> @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
> >> }
> >> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
> >>
> >> +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >> +{
> >> + ssize_t ret;
> >> +
> >> + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
> >> + msg->request, msg->address, msg->size);
> >> +
> >> + if (unlikely(drm_debug & DRM_UT_AUX)) {
> >> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> >> + case DP_AUX_NATIVE_WRITE:
> >> + case DP_AUX_I2C_WRITE:
> >> + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
> >> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> >> + 16, 1, msg->buffer, msg->size, false);
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + }
> >> +
> >> + ret = aux->transfer(aux, msg);
> >> +
> >> + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
> >> +
> >> + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
> >> + switch (msg->request & ~DP_AUX_I2C_MOT) {
> >> + case DP_AUX_NATIVE_READ:
> >> + case DP_AUX_I2C_READ:
> >> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> >> + 16, 1, msg->buffer, ret, false);
> >> + break;
> >> + default:
> >> + break;
> >> + }
> >> + }
> >> +
> >> + return ret;
> >> +}
> >> +
> >> #define AUX_RETRY_INTERVAL 500 /* us */
> >>
> >> /**
> >> @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> >> */
> >> for (retry = 0; retry < 32; retry++) {
> >>
> >> - err = aux->transfer(aux, &msg);
> >> + err = aux_transfer(aux, &msg);
> >> if (err < 0) {
> >> if (err == -EBUSY)
> >> continue;
> >> @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> >> goto unlock;
> >> }
> >>
> >> -
> >> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> >> case DP_AUX_NATIVE_REPLY_ACK:
> >> if (err < size)
> >> @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> >> goto unlock;
> >>
> >> case DP_AUX_NATIVE_REPLY_NACK:
> >> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
> >> err = -EIO;
> >> goto unlock;
> >>
> >> case DP_AUX_NATIVE_REPLY_DEFER:
> >> + DRM_DEBUG_AUX("native defer\n");
> >> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> >> break;
> >> }
> >> }
> >>
> >> - DRM_DEBUG_KMS("too many retries, giving up\n");
> >> + DRM_ERROR("DPCD: too many retries, giving up!\n");
> >> err = -EIO;
> >>
> >> unlock:
> >> @@ -549,12 +589,12 @@ 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++) {
> >> - ret = aux->transfer(aux, msg);
> >> + ret = aux_transfer(aux, msg);
> >> if (ret < 0) {
> >> if (ret == -EBUSY)
> >> continue;
> >>
> >> - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
> >> + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
> >> return ret;
> >> }
> >>
> >> @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >> break;
> >>
> >> case DP_AUX_NATIVE_REPLY_NACK:
> >> - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
> >> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
> >> return -EREMOTEIO;
> >>
> >> case DP_AUX_NATIVE_REPLY_DEFER:
> >> - DRM_DEBUG_KMS("native defer\n");
> >> + DRM_DEBUG_AUX("native defer\n");
> >> /*
> >> * We could check for I2C bit rate capabilities and if
> >> * available adjust this interval. We could also be
> >> @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >> return ret;
> >>
> >> case DP_AUX_I2C_REPLY_NACK:
> >> - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
> >> + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
> >> aux->i2c_nack_count++;
> >> return -EREMOTEIO;
> >>
> >> case DP_AUX_I2C_REPLY_DEFER:
> >> - DRM_DEBUG_KMS("I2C defer\n");
> >> + DRM_DEBUG_AUX("I2C defer\n");
> >> /* DP Compliance Test 4.2.2.5 Requirement:
> >> * Must have at least 7 retries for I2C defers on the
> >> * transaction to pass this test
> >> @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> >> }
> >> }
> >>
> >> - DRM_DEBUG_KMS("too many retries, giving up\n");
> >> + DRM_ERROR("I2C: too many retries, giving up\n");
> >> return -EREMOTEIO;
> >> }
> >>
> >> @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
> >> return err == 0 ? -EPROTO : err;
> >>
> >> if (err < msg.size && err < ret) {
> >> - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> >> + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
> >> msg.size, err);
> >> ret = err;
> >> }
> >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> >> index 3c8422c..cc524b5 100644
> >> --- a/include/drm/drmP.h
> >> +++ b/include/drm/drmP.h
> >> @@ -117,7 +117,7 @@ struct dma_buf_attachment;
> >> * drm.debug=0x2 will enable DRIVER messages
> >> * drm.debug=0x3 will enable CORE and DRIVER messages
> >> * ...
> >> - * drm.debug=0x3f will enable all messages
> >> + * drm.debug=0x7f will enable all messages
> >> *
> >> * An interesting feature is that it's possible to enable verbose logging at
> >> * run-time by echoing the debug value in its sysfs node:
> >> @@ -129,6 +129,7 @@ struct dma_buf_attachment;
> >> #define DRM_UT_PRIME 0x08
> >> #define DRM_UT_ATOMIC 0x10
> >> #define DRM_UT_VBL 0x20
> >> +#define DRM_UT_AUX 0x40
> >>
> >> extern __printf(2, 3)
> >> void drm_ut_debug_printk(const char *function_name,
> >> @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
> >> if (unlikely(drm_debug & DRM_UT_VBL)) \
> >> drm_ut_debug_printk(__func__, fmt, ##args); \
> >> } while (0)
> >> +#define DRM_DEBUG_AUX(fmt, args...) \
> >> + do { \
> >> + if (unlikely(drm_debug & DRM_UT_AUX)) \
> >> + drm_ut_debug_printk(__func__, fmt, ##args); \
> >> + } while (0)
> >>
> >> /*@}*/
> >>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-12-22 7:02 ` Daniel Vetter
@ 2016-12-22 8:54 ` Imre Deak
2016-12-22 17:11 ` Rob Clark
1 sibling, 0 replies; 14+ messages in thread
From: Imre Deak @ 2016-12-22 8:54 UTC (permalink / raw)
To: Daniel Vetter, Rob Clark; +Cc: dri-devel
On Thu, 2016-12-22 at 08:02 +0100, Daniel Vetter wrote:
> On Wed, Dec 21, 2016 at 05:21:18PM -0500, Rob Clark wrote:
> > On Wed, Dec 21, 2016 at 3:41 PM, Imre Deak <imre.deak@intel.com> wrote:
> > > On Thu, 2016-02-25 at 16:15 -0500, Rob Clark wrote:
> > > > Add a new drm_debug bit for turning on DPCD logging, to aid debugging
> > > > with troublesome monitors.
> > > >
> > > > v2: don't try to hexdump the universe if driver returns -errno, and
> > > > change the "too many retries" traces to DRM_ERROR()
> > > > v3: rename to more generic "AUX" instead of DP specific DPCD, add
> > > > DP_AUX_I2C_WRITE_STATUS_UPDATE
> > > >
> > > > Signed-off-by: Rob Clark <robdclark@gmail.com>
> > >
> > > I used the rebased version of this after Ville pointed me to it for
> > > remote debugging some odd adaptor behavior. It'd be quite useful imo, so:
> > > Acked-by: Imre Deak <imre.deak@intel.com>
> >
> > tbh I completely forgot about this patch.. mind posting the rebased
> > version (or push to a git tree somewhere)? IIRC there were just some
> > minor cosmetic things to fix up, probably worth doing that before I
> > forget about it again..
>
> There's patches for dynamic debugging floating around, I think that'd be
> the more interesting approach than adding ever more magic bits that no one
> understands. But besides that this makes sense.
Ok, although it's better than not having any way to debug and it could be
converted to dyndbg with the reset. For reference:
https://github.com/ideak/linux/commits/aux-debug
--Imre
> -Daniel
>
> >
> > BR,
> > -R
> >
> >
> > >
> > > > ---
> > > > drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
> > > > include/drm/drmP.h | 8 +++++-
> > > > 2 files changed, 58 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > > index df64ed1..3ef35fd 100644
> > > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > > @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
> > > > }
> > > > EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
> > > >
> > > > +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > +{
> > > > + ssize_t ret;
> > > > +
> > > > + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
> > > > + msg->request, msg->address, msg->size);
> > > > +
> > > > + if (unlikely(drm_debug & DRM_UT_AUX)) {
> > > > + switch (msg->request & ~DP_AUX_I2C_MOT) {
> > > > + case DP_AUX_NATIVE_WRITE:
> > > > + case DP_AUX_I2C_WRITE:
> > > > + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
> > > > + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> > > > + 16, 1, msg->buffer, msg->size, false);
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + ret = aux->transfer(aux, msg);
> > > > +
> > > > + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
> > > > +
> > > > + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
> > > > + switch (msg->request & ~DP_AUX_I2C_MOT) {
> > > > + case DP_AUX_NATIVE_READ:
> > > > + case DP_AUX_I2C_READ:
> > > > + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
> > > > + 16, 1, msg->buffer, ret, false);
> > > > + break;
> > > > + default:
> > > > + break;
> > > > + }
> > > > + }
> > > > +
> > > > + return ret;
> > > > +}
> > > > +
> > > > #define AUX_RETRY_INTERVAL 500 /* us */
> > > >
> > > > /**
> > > > @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> > > > */
> > > > for (retry = 0; retry < 32; retry++) {
> > > >
> > > > - err = aux->transfer(aux, &msg);
> > > > + err = aux_transfer(aux, &msg);
> > > > if (err < 0) {
> > > > if (err == -EBUSY)
> > > > continue;
> > > > @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> > > > goto unlock;
> > > > }
> > > >
> > > > -
> > > > switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
> > > > case DP_AUX_NATIVE_REPLY_ACK:
> > > > if (err < size)
> > > > @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
> > > > goto unlock;
> > > >
> > > > case DP_AUX_NATIVE_REPLY_NACK:
> > > > + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
> > > > err = -EIO;
> > > > goto unlock;
> > > >
> > > > case DP_AUX_NATIVE_REPLY_DEFER:
> > > > + DRM_DEBUG_AUX("native defer\n");
> > > > usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
> > > > break;
> > > > }
> > > > }
> > > >
> > > > - DRM_DEBUG_KMS("too many retries, giving up\n");
> > > > + DRM_ERROR("DPCD: too many retries, giving up!\n");
> > > > err = -EIO;
> > > >
> > > > unlock:
> > > > @@ -549,12 +589,12 @@ 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++) {
> > > > - ret = aux->transfer(aux, msg);
> > > > + ret = aux_transfer(aux, msg);
> > > > if (ret < 0) {
> > > > if (ret == -EBUSY)
> > > > continue;
> > > >
> > > > - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
> > > > + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
> > > > return ret;
> > > > }
> > > >
> > > > @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > break;
> > > >
> > > > case DP_AUX_NATIVE_REPLY_NACK:
> > > > - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
> > > > + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
> > > > return -EREMOTEIO;
> > > >
> > > > case DP_AUX_NATIVE_REPLY_DEFER:
> > > > - DRM_DEBUG_KMS("native defer\n");
> > > > + DRM_DEBUG_AUX("native defer\n");
> > > > /*
> > > > * We could check for I2C bit rate capabilities and if
> > > > * available adjust this interval. We could also be
> > > > @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > return ret;
> > > >
> > > > case DP_AUX_I2C_REPLY_NACK:
> > > > - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
> > > > + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
> > > > aux->i2c_nack_count++;
> > > > return -EREMOTEIO;
> > > >
> > > > case DP_AUX_I2C_REPLY_DEFER:
> > > > - DRM_DEBUG_KMS("I2C defer\n");
> > > > + DRM_DEBUG_AUX("I2C defer\n");
> > > > /* DP Compliance Test 4.2.2.5 Requirement:
> > > > * Must have at least 7 retries for I2C defers on the
> > > > * transaction to pass this test
> > > > @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
> > > > }
> > > > }
> > > >
> > > > - DRM_DEBUG_KMS("too many retries, giving up\n");
> > > > + DRM_ERROR("I2C: too many retries, giving up\n");
> > > > return -EREMOTEIO;
> > > > }
> > > >
> > > > @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
> > > > return err == 0 ? -EPROTO : err;
> > > >
> > > > if (err < msg.size && err < ret) {
> > > > - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
> > > > + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
> > > > msg.size, err);
> > > > ret = err;
> > > > }
> > > > diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> > > > index 3c8422c..cc524b5 100644
> > > > --- a/include/drm/drmP.h
> > > > +++ b/include/drm/drmP.h
> > > > @@ -117,7 +117,7 @@ struct dma_buf_attachment;
> > > > * drm.debug=0x2 will enable DRIVER messages
> > > > * drm.debug=0x3 will enable CORE and DRIVER messages
> > > > * ...
> > > > - * drm.debug=0x3f will enable all messages
> > > > + * drm.debug=0x7f will enable all messages
> > > > *
> > > > * An interesting feature is that it's possible to enable verbose logging at
> > > > * run-time by echoing the debug value in its sysfs node:
> > > > @@ -129,6 +129,7 @@ struct dma_buf_attachment;
> > > > #define DRM_UT_PRIME 0x08
> > > > #define DRM_UT_ATOMIC 0x10
> > > > #define DRM_UT_VBL 0x20
> > > > +#define DRM_UT_AUX 0x40
> > > >
> > > > extern __printf(2, 3)
> > > > void drm_ut_debug_printk(const char *function_name,
> > > > @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
> > > > if (unlikely(drm_debug & DRM_UT_VBL)) \
> > > > drm_ut_debug_printk(__func__, fmt, ##args); \
> > > > } while (0)
> > > > +#define DRM_DEBUG_AUX(fmt, args...) \
> > > > + do { \
> > > > + if (unlikely(drm_debug & DRM_UT_AUX)) \
> > > > + drm_ut_debug_printk(__func__, fmt, ##args); \
> > > > + } while (0)
> > > >
> > > > /*@}*/
> > > >
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH 2/2] drm/dp: add DPCD/AUX logging
2016-12-22 7:02 ` Daniel Vetter
2016-12-22 8:54 ` Imre Deak
@ 2016-12-22 17:11 ` Rob Clark
1 sibling, 0 replies; 14+ messages in thread
From: Rob Clark @ 2016-12-22 17:11 UTC (permalink / raw)
To: Daniel Vetter; +Cc: dri-devel
On Thu, Dec 22, 2016 at 2:02 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 21, 2016 at 05:21:18PM -0500, Rob Clark wrote:
>> On Wed, Dec 21, 2016 at 3:41 PM, Imre Deak <imre.deak@intel.com> wrote:
>> > On Thu, 2016-02-25 at 16:15 -0500, Rob Clark wrote:
>> >> Add a new drm_debug bit for turning on DPCD logging, to aid debugging
>> >> with troublesome monitors.
>> >>
>> >> v2: don't try to hexdump the universe if driver returns -errno, and
>> >> change the "too many retries" traces to DRM_ERROR()
>> >> v3: rename to more generic "AUX" instead of DP specific DPCD, add
>> >> DP_AUX_I2C_WRITE_STATUS_UPDATE
>> >>
>> >> Signed-off-by: Rob Clark <robdclark@gmail.com>
>> >
>> > I used the rebased version of this after Ville pointed me to it for
>> > remote debugging some odd adaptor behavior. It'd be quite useful imo, so:
>> > Acked-by: Imre Deak <imre.deak@intel.com>
>>
>> tbh I completely forgot about this patch.. mind posting the rebased
>> version (or push to a git tree somewhere)? IIRC there were just some
>> minor cosmetic things to fix up, probably worth doing that before I
>> forget about it again..
>
> There's patches for dynamic debugging floating around, I think that'd be
> the more interesting approach than adding ever more magic bits that no one
> understands. But besides that this makes sense.
could make sense.. and by all means if someone wants to take over this
patch and do something more clever, go for it..
I won't have access to a dp monitor for a couple weeks, so other than
minor cosmetic tweaks there isn't much I can do.. and I mostly don't
want this to fall through the cracks again.
BR,
-R
> -Daniel
>
>>
>> BR,
>> -R
>>
>>
>> >
>> >> ---
>> >> drivers/gpu/drm/drm_dp_helper.c | 62 +++++++++++++++++++++++++++++++++--------
>> >> include/drm/drmP.h | 8 +++++-
>> >> 2 files changed, 58 insertions(+), 12 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
>> >> index df64ed1..3ef35fd 100644
>> >> --- a/drivers/gpu/drm/drm_dp_helper.c
>> >> +++ b/drivers/gpu/drm/drm_dp_helper.c
>> >> @@ -160,6 +160,45 @@ int drm_dp_bw_code_to_link_rate(u8 link_bw)
>> >> }
>> >> EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate);
>> >>
>> >> +static ssize_t aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> +{
>> >> + ssize_t ret;
>> >> +
>> >> + DRM_DEBUG_AUX("%s: req=0x%02x, address=0x%05x, size=%zu\n", aux->name,
>> >> + msg->request, msg->address, msg->size);
>> >> +
>> >> + if (unlikely(drm_debug & DRM_UT_AUX)) {
>> >> + switch (msg->request & ~DP_AUX_I2C_MOT) {
>> >> + case DP_AUX_NATIVE_WRITE:
>> >> + case DP_AUX_I2C_WRITE:
>> >> + case DP_AUX_I2C_WRITE_STATUS_UPDATE:
>> >> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
>> >> + 16, 1, msg->buffer, msg->size, false);
>> >> + break;
>> >> + default:
>> >> + break;
>> >> + }
>> >> + }
>> >> +
>> >> + ret = aux->transfer(aux, msg);
>> >> +
>> >> + DRM_DEBUG_AUX("%s: reply=0x%02x, size=%zd\n", aux->name, msg->reply, ret);
>> >> +
>> >> + if (unlikely(drm_debug & DRM_UT_AUX) && (ret > 0)) {
>> >> + switch (msg->request & ~DP_AUX_I2C_MOT) {
>> >> + case DP_AUX_NATIVE_READ:
>> >> + case DP_AUX_I2C_READ:
>> >> + print_hex_dump(KERN_DEBUG, "DPCD: ", DUMP_PREFIX_OFFSET,
>> >> + 16, 1, msg->buffer, ret, false);
>> >> + break;
>> >> + default:
>> >> + break;
>> >> + }
>> >> + }
>> >> +
>> >> + return ret;
>> >> +}
>> >> +
>> >> #define AUX_RETRY_INTERVAL 500 /* us */
>> >>
>> >> /**
>> >> @@ -197,7 +236,7 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> */
>> >> for (retry = 0; retry < 32; retry++) {
>> >>
>> >> - err = aux->transfer(aux, &msg);
>> >> + err = aux_transfer(aux, &msg);
>> >> if (err < 0) {
>> >> if (err == -EBUSY)
>> >> continue;
>> >> @@ -205,7 +244,6 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> goto unlock;
>> >> }
>> >>
>> >> -
>> >> switch (msg.reply & DP_AUX_NATIVE_REPLY_MASK) {
>> >> case DP_AUX_NATIVE_REPLY_ACK:
>> >> if (err < size)
>> >> @@ -213,16 +251,18 @@ static int drm_dp_dpcd_access(struct drm_dp_aux *aux, u8 request,
>> >> goto unlock;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_NACK:
>> >> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", err, msg.size);
>> >> err = -EIO;
>> >> goto unlock;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_DEFER:
>> >> + DRM_DEBUG_AUX("native defer\n");
>> >> usleep_range(AUX_RETRY_INTERVAL, AUX_RETRY_INTERVAL + 100);
>> >> break;
>> >> }
>> >> }
>> >>
>> >> - DRM_DEBUG_KMS("too many retries, giving up\n");
>> >> + DRM_ERROR("DPCD: too many retries, giving up!\n");
>> >> err = -EIO;
>> >>
>> >> unlock:
>> >> @@ -549,12 +589,12 @@ 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++) {
>> >> - ret = aux->transfer(aux, msg);
>> >> + ret = aux_transfer(aux, msg);
>> >> if (ret < 0) {
>> >> if (ret == -EBUSY)
>> >> continue;
>> >>
>> >> - DRM_DEBUG_KMS("transaction failed: %d\n", ret);
>> >> + DRM_DEBUG_AUX("transaction failed: %d\n", ret);
>> >> return ret;
>> >> }
>> >>
>> >> @@ -568,11 +608,11 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> break;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_NACK:
>> >> - DRM_DEBUG_KMS("native nack (result=%d, size=%zu)\n", ret, msg->size);
>> >> + DRM_DEBUG_AUX("native nack (result=%d, size=%zu)\n", ret, msg->size);
>> >> return -EREMOTEIO;
>> >>
>> >> case DP_AUX_NATIVE_REPLY_DEFER:
>> >> - DRM_DEBUG_KMS("native defer\n");
>> >> + DRM_DEBUG_AUX("native defer\n");
>> >> /*
>> >> * We could check for I2C bit rate capabilities and if
>> >> * available adjust this interval. We could also be
>> >> @@ -601,12 +641,12 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> return ret;
>> >>
>> >> case DP_AUX_I2C_REPLY_NACK:
>> >> - DRM_DEBUG_KMS("I2C nack (result=%d, size=%zu\n", ret, msg->size);
>> >> + DRM_DEBUG_AUX("I2C nack (result=%d, size=%zu)\n", ret, msg->size);
>> >> aux->i2c_nack_count++;
>> >> return -EREMOTEIO;
>> >>
>> >> case DP_AUX_I2C_REPLY_DEFER:
>> >> - DRM_DEBUG_KMS("I2C defer\n");
>> >> + DRM_DEBUG_AUX("I2C defer\n");
>> >> /* DP Compliance Test 4.2.2.5 Requirement:
>> >> * Must have at least 7 retries for I2C defers on the
>> >> * transaction to pass this test
>> >> @@ -625,7 +665,7 @@ static int drm_dp_i2c_do_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)
>> >> }
>> >> }
>> >>
>> >> - DRM_DEBUG_KMS("too many retries, giving up\n");
>> >> + DRM_ERROR("I2C: too many retries, giving up\n");
>> >> return -EREMOTEIO;
>> >> }
>> >>
>> >> @@ -653,7 +693,7 @@ static int drm_dp_i2c_drain_msg(struct drm_dp_aux *aux, struct drm_dp_aux_msg *o
>> >> return err == 0 ? -EPROTO : err;
>> >>
>> >> if (err < msg.size && err < ret) {
>> >> - DRM_DEBUG_KMS("Partial I2C reply: requested %zu bytes got %d bytes\n",
>> >> + DRM_DEBUG_AUX("Partial I2C reply: requested %zu bytes got %d bytes\n",
>> >> msg.size, err);
>> >> ret = err;
>> >> }
>> >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
>> >> index 3c8422c..cc524b5 100644
>> >> --- a/include/drm/drmP.h
>> >> +++ b/include/drm/drmP.h
>> >> @@ -117,7 +117,7 @@ struct dma_buf_attachment;
>> >> * drm.debug=0x2 will enable DRIVER messages
>> >> * drm.debug=0x3 will enable CORE and DRIVER messages
>> >> * ...
>> >> - * drm.debug=0x3f will enable all messages
>> >> + * drm.debug=0x7f will enable all messages
>> >> *
>> >> * An interesting feature is that it's possible to enable verbose logging at
>> >> * run-time by echoing the debug value in its sysfs node:
>> >> @@ -129,6 +129,7 @@ struct dma_buf_attachment;
>> >> #define DRM_UT_PRIME 0x08
>> >> #define DRM_UT_ATOMIC 0x10
>> >> #define DRM_UT_VBL 0x20
>> >> +#define DRM_UT_AUX 0x40
>> >>
>> >> extern __printf(2, 3)
>> >> void drm_ut_debug_printk(const char *function_name,
>> >> @@ -226,6 +227,11 @@ void drm_err(const char *format, ...);
>> >> if (unlikely(drm_debug & DRM_UT_VBL)) \
>> >> drm_ut_debug_printk(__func__, fmt, ##args); \
>> >> } while (0)
>> >> +#define DRM_DEBUG_AUX(fmt, args...) \
>> >> + do { \
>> >> + if (unlikely(drm_debug & DRM_UT_AUX)) \
>> >> + drm_ut_debug_printk(__func__, fmt, ##args); \
>> >> + } while (0)
>> >>
>> >> /*@}*/
>> >>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [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, 0 replies; 14+ 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] 14+ messages in thread
* Re: [PATCH 1/2] drm/dp: move hw_mutex up the call stack
@ 2015-12-11 16:32 ` Ville Syrjälä
0 siblings, 0 replies; 14+ messages in thread
From: Ville Syrjälä @ 2015-12-11 16:32 UTC (permalink / raw)
To: Rob Clark; +Cc: Dave Wysochanski, stable, dri-devel
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [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; 14+ 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] 14+ messages in thread
end of thread, other threads:[~2016-12-22 17:11 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-25 21:15 [PATCH 1/2] drm/dp: move hw_mutex up the call stack Rob Clark
2016-02-25 21:15 ` Rob Clark
2016-02-25 21:15 ` [PATCH 2/2] drm/dp: add DPCD/AUX logging Rob Clark
2016-02-29 17:52 ` Ville Syrjälä
2016-12-21 20:41 ` Imre Deak
2016-12-21 22:21 ` Rob Clark
2016-12-22 7:02 ` Daniel Vetter
2016-12-22 8:54 ` Imre Deak
2016-12-22 17:11 ` Rob Clark
2016-02-29 16:46 ` [PATCH 1/2] drm/dp: move hw_mutex up the call stack Daniel Vetter
2016-02-29 16:46 ` Daniel Vetter
-- strict thread matches above, loose matches on Subject: below --
2015-12-09 21:20 Rob Clark
2015-12-11 16:32 ` Ville Syrjälä
2015-12-11 16:32 ` Ville Syrjälä
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.