| Message ID | 20180712172419.790-1-lyude@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 12, 2018 at 01:24:19PM -0400, Lyude Paul wrote: > This is something we've needed for a very long time now, as it makes > debugging issues with faulty MST hubs along with debugging issues > regarding us interfacing with hubs correctly vastly easier to debug. > Currently this can actually be done if you trace the i2c devices for DP > using ftrace but that's significantly less useful for a couple of > reasons: > > - Tracing the i2c devices through ftrace means all of the traces are > going to contain a lot of "garbage" output that we're sending over the > i2c line. Most of this garbage comes from retrying transactions, DRM's > helper library adding extra transactions to work around bad hubs, etc. > - Having a user set up ftrace so that they can provide debugging > information is a lot more difficult then being able to say "just boot > with drm.debug=0x100" > - We can potentially expand upon this tracing in the future to print > debugging information in regards to other DP transactions like MST > sideband transactions > > This is inspired by a patch Rob Clark sent to do this a long time back. > Neither of us could find the patch however, so we both assumed it would > probably just be easier to rewrite it anyway. > > Cc: Rob Clark <robdclark@gmail.com> > Signed-off-by: Lyude Paul <lyude@redhat.com> > --- > drivers/gpu/drm/drm_dp_helper.c | 34 ++++++++++++++++++++++++++++----- > drivers/gpu/drm/drm_drv.c | 15 ++++++++------- > include/drm/drm_print.h | 6 ++++++ > 3 files changed, 43 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c > index a7ba602a43a8..6fa5a59aa17c 100644 > --- a/drivers/gpu/drm/drm_dp_helper.c > +++ b/drivers/gpu/drm/drm_dp_helper.c > @@ -185,6 +185,21 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); > > #define AUX_RETRY_INTERVAL 500 /* us */ > > +static inline void > +drm_dp_dump_access(const struct drm_dp_aux *aux, > + u8 request, uint offset, void *buffer, int ret) > +{ > + const char *arrow = request == DP_AUX_NATIVE_READ ? "->" : "<-"; > + > + if (ret > 0) > + drm_dbg(DRM_UT_DP, "%s: 0x%08x AUX %s (ret=%3d) %*ph%s\n", > + aux->name, offset, arrow, ret, min(ret, 64), > + buffer, ret > 64 ? "..." : ""); Afaik dp spec limits transfers to 20 bytes max. We should probably update the docs about this in drm_dp_aux_msg. i915 definitely throws a E2BIG at you if you try something else. Similar with addresses, they're only 20 bits afaiui. Again should probably update the docs of drm_dp_aux_msg. Otherwise lgtm and I guess we can figure out how to report i2c stuff at a different time, or delegate that to the i2c core. With the pretty printing adjusted to follow the dp limits: Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> btw, want drm-misc commit rights for stuff like this? If yes just ask seanpaul/mlankhorst/padovan on irc for ack and then poke daniels or another fdo admin. Cheers, Daniel > + else > + drm_dbg(DRM_UT_DP, "%s: 0x%08x AUX %s (ret=%3d)\n", > + aux->name, offset, arrow, ret); > +} > + > /** > * DOC: dp helpers > * > @@ -288,10 +303,14 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, > ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, > 1); > if (ret != 1) > - return ret; > + goto out; > > - return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer, > - size); > + ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer, > + size); > + > +out: > + drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, buffer, ret); > + return ret; > } > EXPORT_SYMBOL(drm_dp_dpcd_read); > > @@ -312,8 +331,13 @@ EXPORT_SYMBOL(drm_dp_dpcd_read); > ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset, > void *buffer, size_t size) > { > - return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, > - size); > + int ret; > + > + > + ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, > + size); > + drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, ret); > + return ret; > } > EXPORT_SYMBOL(drm_dp_dpcd_write); > > diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c > index 7af748ed1c58..e555eb5de275 100644 > --- a/drivers/gpu/drm/drm_drv.c > +++ b/drivers/gpu/drm/drm_drv.c > @@ -53,13 +53,14 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl"); > MODULE_DESCRIPTION("DRM shared core routines"); > MODULE_LICENSE("GPL and additional rights"); > MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n" > -"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n" > -"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n" > -"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n" > -"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n" > -"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n" > -"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" > -"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)"); > +"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n" > +"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n" > +"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n" > +"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n" > +"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n" > +"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" > +"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n" > +"\t\tBit 8 (0x100) will enable DP messages (displayport code)"); > module_param_named(debug, drm_debug, int, 0600); > > static DEFINE_SPINLOCK(drm_minor_lock); > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index e1a46e9991cc..767c90b654c5 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -195,6 +195,7 @@ static inline struct drm_printer drm_debug_printer(const char *prefix) > #define DRM_UT_VBL 0x20 > #define DRM_UT_STATE 0x40 > #define DRM_UT_LEASE 0x80 > +#define DRM_UT_DP 0x100 > > __printf(3, 4) > void drm_dev_printk(const struct device *dev, const char *level, > @@ -307,6 +308,11 @@ void drm_err(const char *format, ...); > #define DRM_DEBUG_LEASE(fmt, ...) \ > drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__) > > +#define DRM_DEV_DEBUG_DP(dev, fmt, ...) \ > + drm_dev_dbg(dev, DRM_UT_DP, fmt, ## __VA_ARGS__) > +#define DRM_DEBUG_DP(dev, fmt, ...) \ > + drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__) > + > #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, category, fmt, ...) \ > ({ \ > static DEFINE_RATELIMIT_STATE(_rs, \ > -- > 2.17.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index a7ba602a43a8..6fa5a59aa17c 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -185,6 +185,21 @@ EXPORT_SYMBOL(drm_dp_bw_code_to_link_rate); #define AUX_RETRY_INTERVAL 500 /* us */ +static inline void +drm_dp_dump_access(const struct drm_dp_aux *aux, + u8 request, uint offset, void *buffer, int ret) +{ + const char *arrow = request == DP_AUX_NATIVE_READ ? "->" : "<-"; + + if (ret > 0) + drm_dbg(DRM_UT_DP, "%s: 0x%08x AUX %s (ret=%3d) %*ph%s\n", + aux->name, offset, arrow, ret, min(ret, 64), + buffer, ret > 64 ? "..." : ""); + else + drm_dbg(DRM_UT_DP, "%s: 0x%08x AUX %s (ret=%3d)\n", + aux->name, offset, arrow, ret); +} + /** * DOC: dp helpers * @@ -288,10 +303,14 @@ ssize_t drm_dp_dpcd_read(struct drm_dp_aux *aux, unsigned int offset, ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, DP_DPCD_REV, buffer, 1); if (ret != 1) - return ret; + goto out; - return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer, - size); + ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_READ, offset, buffer, + size); + +out: + drm_dp_dump_access(aux, DP_AUX_NATIVE_READ, offset, buffer, ret); + return ret; } EXPORT_SYMBOL(drm_dp_dpcd_read); @@ -312,8 +331,13 @@ EXPORT_SYMBOL(drm_dp_dpcd_read); ssize_t drm_dp_dpcd_write(struct drm_dp_aux *aux, unsigned int offset, void *buffer, size_t size) { - return drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, - size); + int ret; + + + ret = drm_dp_dpcd_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, + size); + drm_dp_dump_access(aux, DP_AUX_NATIVE_WRITE, offset, buffer, ret); + return ret; } EXPORT_SYMBOL(drm_dp_dpcd_write); diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c index 7af748ed1c58..e555eb5de275 100644 --- a/drivers/gpu/drm/drm_drv.c +++ b/drivers/gpu/drm/drm_drv.c @@ -53,13 +53,14 @@ MODULE_AUTHOR("Gareth Hughes, Leif Delgass, José Fonseca, Jon Smirl"); MODULE_DESCRIPTION("DRM shared core routines"); MODULE_LICENSE("GPL and additional rights"); MODULE_PARM_DESC(debug, "Enable debug output, where each bit enables a debug category.\n" -"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n" -"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n" -"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n" -"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n" -"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n" -"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" -"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)"); +"\t\tBit 0 (0x01) will enable CORE messages (drm core code)\n" +"\t\tBit 1 (0x02) will enable DRIVER messages (drm controller code)\n" +"\t\tBit 2 (0x04) will enable KMS messages (modesetting code)\n" +"\t\tBit 3 (0x08) will enable PRIME messages (prime code)\n" +"\t\tBit 4 (0x10) will enable ATOMIC messages (atomic code)\n" +"\t\tBit 5 (0x20) will enable VBL messages (vblank code)\n" +"\t\tBit 7 (0x80) will enable LEASE messages (leasing code)\n" +"\t\tBit 8 (0x100) will enable DP messages (displayport code)"); module_param_named(debug, drm_debug, int, 0600); static DEFINE_SPINLOCK(drm_minor_lock); diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h index e1a46e9991cc..767c90b654c5 100644 --- a/include/drm/drm_print.h +++ b/include/drm/drm_print.h @@ -195,6 +195,7 @@ static inline struct drm_printer drm_debug_printer(const char *prefix) #define DRM_UT_VBL 0x20 #define DRM_UT_STATE 0x40 #define DRM_UT_LEASE 0x80 +#define DRM_UT_DP 0x100 __printf(3, 4) void drm_dev_printk(const struct device *dev, const char *level, @@ -307,6 +308,11 @@ void drm_err(const char *format, ...); #define DRM_DEBUG_LEASE(fmt, ...) \ drm_dbg(DRM_UT_LEASE, fmt, ##__VA_ARGS__) +#define DRM_DEV_DEBUG_DP(dev, fmt, ...) \ + drm_dev_dbg(dev, DRM_UT_DP, fmt, ## __VA_ARGS__) +#define DRM_DEBUG_DP(dev, fmt, ...) \ + drm_dbg(DRM_UT_DP, fmt, ## __VA_ARGS__) + #define _DRM_DEV_DEFINE_DEBUG_RATELIMITED(dev, category, fmt, ...) \ ({ \ static DEFINE_RATELIMIT_STATE(_rs, \
This is something we've needed for a very long time now, as it makes debugging issues with faulty MST hubs along with debugging issues regarding us interfacing with hubs correctly vastly easier to debug. Currently this can actually be done if you trace the i2c devices for DP using ftrace but that's significantly less useful for a couple of reasons: - Tracing the i2c devices through ftrace means all of the traces are going to contain a lot of "garbage" output that we're sending over the i2c line. Most of this garbage comes from retrying transactions, DRM's helper library adding extra transactions to work around bad hubs, etc. - Having a user set up ftrace so that they can provide debugging information is a lot more difficult then being able to say "just boot with drm.debug=0x100" - We can potentially expand upon this tracing in the future to print debugging information in regards to other DP transactions like MST sideband transactions This is inspired by a patch Rob Clark sent to do this a long time back. Neither of us could find the patch however, so we both assumed it would probably just be easier to rewrite it anyway. Cc: Rob Clark <robdclark@gmail.com> Signed-off-by: Lyude Paul <lyude@redhat.com> --- drivers/gpu/drm/drm_dp_helper.c | 34 ++++++++++++++++++++++++++++----- drivers/gpu/drm/drm_drv.c | 15 ++++++++------- include/drm/drm_print.h | 6 ++++++ 3 files changed, 43 insertions(+), 12 deletions(-)