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