Re: [RFC] drm: add atomic state printing

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

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.

I wonder if we want to dump out more about the fb here too? size+fmt
perhaps?

> +	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.

> +	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?

> +}
>  
>  /*@}*/
>  
> @@ -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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel