| Message ID | 1456434906-29600-2-git-send-email-robdclark@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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) > > /*@}*/ >
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) >> >> /*@}*/ >>
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
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 > >
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
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) /*@}*/
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(-)