[RFC] drm: add atomic state printing

Rob Clark robdclark at gmail.com
Thu Oct 13 17:51:20 UTC 2016


On Thu, Oct 13, 2016 at 1:38 PM, Sean Paul <seanpaul at chromium.org> wrote:
> On Thu, Oct 13, 2016 at 1:16 PM, Rob Clark <robdclark at gmail.com> 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));
>> +
>
> I feel like if we add this, we should create a helper
> drm_atomic_helper_print_state() (or w/e) to dump all of the state at
> once (on flush), instead of sprinkling calls throughout the driver. So
> I guess that would require the new helper call + optional helper_funcs
> for driver-specific crtc/plane fields.
>
> Perhaps that's too involved, but IMO it seems like something that'd be
> easier to gain traction across drivers.
>

yeah, sprinkling it in driver (at least for now) was basically just to
avoid having to add new print_state() hooks..  I guess hooks could be
made optional, and just call the drm_print_xyz_state() helper directly
to print core state if driver doesn't override.  For now didn't want
to make it too fancy in case it turned out to be throw-away debug code
for debugging $current_problem ;-)

BR,
-R

> Sean
>
>
>>         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);
>> +       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);
>> +       DRM_DEBUG_STATE("\trotation=%x\n", state->rotation);
>> +}
>>
>>  /*@}*/
>>
>> @@ -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 at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel


More information about the dri-devel mailing list