Re: [RFC] drm: add atomic state printing

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 13, 2016 at 04:14:04PM -0400, Rob Clark wrote:
> On Thu, Oct 13, 2016 at 2:24 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Oct 13, 2016 at 01:16:29PM -0400, Rob Clark wrote:
> >> Sometimes we just want to see the atomic state, without getting so many
> >> other debug traces.  So add a new drm.debug bit for that.
> >>
> >> Note: it would be nice to put the helpers for printing plane/crtc state
> >> next to plane/crtc state structs.. so someone adding new stuff to the
> >> state structs is more likely to remember to update the corresponding
> >> print_state() fxn.  But the header include order for that doesn't really
> >> work out.
> >>
> >> Also, just including the corresponding mdp bits as an example.  Dumps
> >> out something like:
> >>
> >> [drm] plane[24]: RGB0
> >> [drm]         crtc=crtc-0
> >> [drm]         fb=35
> >> [drm]         crtc-pos=[0,0, 720x400]
> >> [drm]         src-pos=[0,0, 720x400]
> >> [drm]         rotation=0
> >> [drm]         premultiplied=0
> >> [drm]         zpos=1
> >> [drm]         alpha=255
> >> [drm]         stage=STAGE0
> >> [drm]         mode_changed=1
> >> [drm]         pending=0
> >> [drm] crtc[27]: crtc-0
> >> [drm]         enable=1
> >> [drm]         active=0
> >> [drm]         planes_changed=1
> >> [drm]         mode_changed=1
> >> [drm]         active_changed=0
> >> [drm]         connectors_changed=1
> >> [drm]         color_mgmt_changed=0
> >> [drm]         plane_mask=1
> >> [drm]         connector_mask=1
> >> [drm]         encoder_mask=1
> >> [drm]         mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 1089 1125 0x48 0x5
> >> ---
> >>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c  |  3 ++
> >>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h   | 28 ++++++++++++++++++
> >>  drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c |  4 ++-
> >>  include/drm/drmP.h                        | 47 ++++++++++++++++++++++++++++++-
> >>  4 files changed, 80 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> >> index e42f62d..da84179 100644
> >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c
> >> @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc,
> >>
> >>       DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event);
> >>
> >> +     DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name);
> >> +     drm_print_crtc_state(crtc->state);
> >> +
> >>       WARN_ON(mdp5_crtc->event);
> >>
> >>       spin_lock_irqsave(&dev->event_lock, flags);
> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> >> index e4b3fb3..466acbc 100644
> >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h
> >> @@ -92,6 +92,22 @@ struct mdp5_plane_state {
> >>  #define to_mdp5_plane_state(x) \
> >>               container_of(x, struct mdp5_plane_state, base)
> >>
> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage);
> >> +
> >> +static inline void
> >> +mdp5_print_plane_state(const struct mdp5_plane_state *state)
> >> +{
> >> +     const struct drm_plane *plane = state->base.plane;
> >> +     DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name);
> >> +     drm_print_plane_state(&state->base);
> >> +     DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied);
> >> +     DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos);
> >> +     DRM_DEBUG_STATE("\talpha=%u\n", state->alpha);
> >> +     DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage));
> >> +     DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed);
> >> +     DRM_DEBUG_STATE("\tpending=%u\n", state->pending);
> >> +}
> >> +
> >>  enum mdp5_intf_mode {
> >>       MDP5_INTF_MODE_NONE = 0,
> >>
> >> @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, u32 reg)
> >>       return msm_readl(mdp5_kms->mmio + reg);
> >>  }
> >>
> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage)
> >> +{
> >> +     static const char *names[] = {
> >> +#define NAME(n) [n] = #n
> >> +             NAME(STAGE_UNUSED), NAME(STAGE_BASE),
> >> +             NAME(STAGE0), NAME(STAGE1), NAME(STAGE2),
> >> +             NAME(STAGE3), NAME(STAGE4), NAME(STAGE6),
> >> +#undef NAME
> >> +     };
> >> +     return names[stage];
> >> +}
> >> +
> >>  static inline const char *pipe2name(enum mdp5_pipe pipe)
> >>  {
> >>       static const char *names[] = {
> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> index 432c098..df301df 100644
> >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c
> >> @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane,
> >>
> >>       DBG("%s: update", mdp5_plane->name);
> >>
> >> +     mdp5_print_plane_state(to_mdp5_plane_state(state));
> >> +
> >>       if (!plane_enabled(state)) {
> >>               to_mdp5_plane_state(state)->pending = true;
> >>       } else if (to_mdp5_plane_state(state)->mode_changed) {
> >> @@ -904,7 +906,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev,
> >>       type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY;
> >>       ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs,
> >>                                mdp5_plane->formats, mdp5_plane->nformats,
> >> -                              type, NULL);
> >> +                              type, "%s", mdp5_plane->name);
> >>       if (ret)
> >>               goto fail;
> >>
> >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> >> index 28d341a..0ee0aa4 100644
> >> --- a/include/drm/drmP.h
> >> +++ b/include/drm/drmP.h
> >> @@ -133,6 +133,7 @@ struct dma_buf_attachment;
> >>  #define DRM_UT_PRIME         0x08
> >>  #define DRM_UT_ATOMIC                0x10
> >>  #define DRM_UT_VBL           0x20
> >> +#define DRM_UT_STATE         0x40
> >>
> >>  extern __printf(2, 3)
> >>  void drm_ut_debug_printk(const char *function_name,
> >> @@ -193,6 +194,8 @@ void drm_err(const char *format, ...);
> >>  #define DRM_INFO_ONCE(fmt, ...)                              \
> >>       printk_once(KERN_INFO "[" DRM_NAME "] " fmt, ##__VA_ARGS__)
> >>
> >> +extern unsigned int drm_debug;
> >> +
> >>  /**
> >>   * Debug output.
> >>   *
> >> @@ -230,6 +233,49 @@ 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_STATE(fmt, args...)                                        \
> >> +     do {                                                            \
> >> +             if (unlikely(drm_debug & DRM_UT_STATE))                 \
> >> +                     DRM_INFO(fmt, ##args);                          \
> >> +     } while (0)
> >> +
> >> +
> >> +static inline void
> >> +drm_print_crtc_state(const struct drm_crtc_state *state)
> >> +{
> >> +     const struct drm_display_mode *mode = &state->mode;
> >> +
> >> +     DRM_DEBUG_STATE("\tenable=%d\n", state->enable);
> >> +     DRM_DEBUG_STATE("\tactive=%d\n", state->active);
> >> +     DRM_DEBUG_STATE("\tplanes_changed=%d\n", state->planes_changed);
> >> +     DRM_DEBUG_STATE("\tmode_changed=%d\n", state->mode_changed);
> >> +     DRM_DEBUG_STATE("\tactive_changed=%d\n", state->active_changed);
> >> +     DRM_DEBUG_STATE("\tconnectors_changed=%d\n", state->connectors_changed);
> >> +     DRM_DEBUG_STATE("\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed);
> >> +     DRM_DEBUG_STATE("\tplane_mask=%x\n", state->plane_mask);
> >> +     DRM_DEBUG_STATE("\tconnector_mask=%x\n", state->connector_mask);
> >> +     DRM_DEBUG_STATE("\tencoder_mask=%x\n", state->encoder_mask);
> >> +     DRM_DEBUG_STATE("\tmode: %d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x 0x%x\n",
> >> +                     mode->base.id, mode->name, mode->vrefresh, mode->clock,
> >> +                     mode->hdisplay, mode->hsync_start,
> >> +                     mode->hsync_end, mode->htotal,
> >> +                     mode->vdisplay, mode->vsync_start,
> >> +                     mode->vsync_end, mode->vtotal, mode->type, mode->flags);
> >> +}
> >> +
> >> +static inline void
> >> +drm_print_plane_state(const struct drm_plane_state *state)
> >> +{
> >> +     DRM_DEBUG_STATE("\tcrtc=%s\n", state->crtc ? state->crtc->name : "(null)");
> >> +     DRM_DEBUG_STATE("\tfb=%d\n", state->fb ? state->fb->base.id : -1);
> >
> > -1 doesn't look like a legit thing. 0 would mean "not there" I think.
> 
> hmm, yeah, I guess zero isn't a valid mode-object id..
> 
> > I wonder if we want to dump out more about the fb here too? size+fmt
> > perhaps?
> 
> yeah, sounds reasonable
> 
> >> +     DRM_DEBUG_STATE("\tcrtc-pos=[%d,%d, %ux%u]\n",
> >> +                     state->crtc_x, state->crtc_y,
> >> +                     state->crtc_w, state->crtc_h);
> >> +     DRM_DEBUG_STATE("\tsrc-pos=[%u,%u, %ux%u]\n",
> >> +                     state->src_x >> 16,  state->src_y >> 16,
> >> +                     state->src_w >> 16,  state->src_h >> 16);
> >
> > These should be printed with the fractional part included. Also your
> > "[...]" thing doesn't match the format I've tried to use elsewhere for
> > these things. Maybe yank the stuff from the drm_rect_print() thing
> > into a few macros that can be used to print them easily anywhere.
> > Something like
> >
> > #define FMT_FIXED_16_16 "...."
> > #define something(x) ...
> > printk("blah " FMT_FIXED_16_16 " bleh\n", something(state->src_x));
> >
> > maybe?
> >
> > Maybe we should have something like that for printing out modes too?
> > In case we want to mix up mode dumps with other output on the same
> > line.
> 
> yeah, I basically cut/paste from drm_mode_debug_printmodeline().. so
> maybe some fmt and "splitter" macros makes sense..
> 
> >> +     DRM_DEBUG_STATE("\trotation=%x\n", state->rotation);
> >
> > Should we dump out all the derived state too? The problem is of course
> > that it might not be up to date yet, so when exactly you print it would
> > affect whether you stale data or not. Maybe dump it only based on some
> > bool function argument?
> 
> not sure what you meant by derived state?

Everything not directly coming from the user. Eg. src/dst rects,
normalized_zpos, etc.

> Although I guess we could
> translate the rotation to strings..
> 
> btw, I'm slightly bummed that no one has invented a way to make
> seq_file that goes to log.. re-using same dump_state() fxns for both
> debugfs and printk would be nice.  (And perhaps be enough of an
> argument to add (*print_state)() fxn ptrs into
> drm_{plane,crtc}_funcs..)
> 
> BR,
> -R
> 
> >> +}
> >>
> >>  /*@}*/
> >>
> >> @@ -988,7 +1034,6 @@ static inline wait_queue_head_t *drm_crtc_vblank_waitqueue(struct drm_crtc *crtc
> >>  /* drm_drv.c */
> >>  void drm_put_dev(struct drm_device *dev);
> >>  void drm_unplug_dev(struct drm_device *dev);
> >> -extern unsigned int drm_debug;
> >>
> >>                               /* Debugfs support */
> >>  #if defined(CONFIG_DEBUG_FS)
> >> --
> >> 2.7.4
> >>
> >> _______________________________________________
> >> dri-devel mailing list
> >> dri-devel@lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel