Re: [PATCH] drm/dp_helper: Add DP aux channel tracing

From: Daniel Vetter
Date: Thu Jul 12 2018 - 16:01:38 EST

Next message: Mimi Zohar: "Re: [PATCH v5 7/8] ima: based on policy warn about loading firmware (pre-allocated buffer)"
Previous message: Thomas Gleixner: "Re: [PATCH v2 1/4] x86/split_lock: Enumerate #AC exception for split locked access feature"
In reply to: Lyude Paul: "[PATCH] drm/dp_helper: Add DP aux channel tracing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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@xxxxxxxxx>
> Signed-off-by: Lyude Paul <lyude@xxxxxxxxxx>
> ---
> 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@xxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Next message: Mimi Zohar: "Re: [PATCH v5 7/8] ima: based on policy warn about loading firmware (pre-allocated buffer)"
Previous message: Thomas Gleixner: "Re: [PATCH v2 1/4] x86/split_lock: Enumerate #AC exception for split locked access feature"
In reply to: Lyude Paul: "[PATCH] drm/dp_helper: Add DP aux channel tracing"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]