On Thu, Oct 13, 2016 at 01:38:30PM -0400, Sean Paul wrote:
> On Thu, Oct 13, 2016 at 1:16 PM, Rob Clark <robdclark@xxxxxxxxx> 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.
Well people might want to print it out from various places during
debugging, so exposing the function at least would be a good help so
that people don't have to reinvent the same thing every time.
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> _______________________________________________
> dri-devel mailing list
> dri-devel@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
