[Intel-gfx] [PATCH] drm/i915: Add extra plane information in debugfs.

[Maarten Lankhorst]

Op 23-10-15 om 16:24 schreef Robert Fekete:
> Extends i915_display_info so that for each active crtc also print
> all planes associated with the pipe. This patch shows information
> about each plane wrt format, size, position, rotation, and scaling.
> This is very useful when debugging user space compositors that try
> to utilize several planes for a commit.
>
> Signed-off-by: Robert Fekete <robert.fekete at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 124 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index eca94d0e4d99..6234f7293dc6 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2950,6 +2950,122 @@ static bool cursor_position(struct drm_device *dev, int pipe, int *x, int *y)
>  	return cursor_active(dev, pipe);
>  }
>  
> +static const char *plane_type(enum drm_plane_type type)
> +{
> +	switch (type) {
> +	case DRM_PLANE_TYPE_OVERLAY:
> +		return "OVL";
> +	case DRM_PLANE_TYPE_PRIMARY:
> +		return "PRI";
> +	case DRM_PLANE_TYPE_CURSOR:
> +		return "CUR";
> +	default:
> +		MISSING_CASE(type);
> +		return "unknown";
If you remove the default case you'll get a compiler warning when a new plane type is added.
> +	}
> +}
> +
> +static const char *plane_format(uint32_t format)
> +{
> +	static char pixel_format_string[5];
> +
> +	/* fourcc string encoding to string */
> +	pixel_format_string[0] = format & 0xff;
> +	pixel_format_string[1] = (format >> 8) & 0xff;
> +	pixel_format_string[2] = (format >> 16) & 0xff;
> +	pixel_format_string[3] = (format >> 24) & 0xff;
> +	pixel_format_string[4] = '\0';
> +
> +	return pixel_format_string;
> +}
Surely drm core would have a function for something like this?

A little grepping reveals drm_get_format_name.
> +static const char *plane_rotation(unsigned int rotation)
> +{
> +	switch (rotation) {
> +	case DRM_ROTATE_0:
> +		return "0";
> +	case DRM_ROTATE_90:
> +		return "90";
> +	case DRM_ROTATE_180:
> +		return "180";
> +	case DRM_ROTATE_270:
> +		return "270";
> +	case DRM_REFLECT_X:
> +		return "FLIP X";
> +	case DRM_REFLECT_Y:
> +		return "FLIP Y";
> +	default:
> +		MISSING_CASE(rotation);
> +		return "unknown";
> +	}
> +}
I'm not sure that this is correct, and doing rotation = 180 + REFLECT_X + REFLECT_Y is allowed.

This would result in a plane with a normal orientation, but that would trigger a MISSING_CASE.
> +
> +static void intel_plane_info(struct seq_file *m, struct intel_crtc *intel_crtc)
> +{
> +	struct drm_info_node *node = m->private;
> +	struct drm_device *dev = node->minor->dev;
> +	struct intel_plane *intel_plane;
> +
> +	for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) {
> +		struct drm_plane_state *state;
> +		struct drm_plane *plane = &intel_plane->base;
> +		uint32_t fb_format = 0;
> +
> +		if (!plane->state) {
> +			seq_puts(m, "plane->state is NULL!\n");
> +			continue;
> +		}
> +
> +		state = plane->state;
> +
> +		if (state->fb) {
> +			/* plane not active */
> +			fb_format = state->fb->pixel_format;
> +		}
> +
> +		seq_printf(m, "\t--Plane id %d: type=%s, crtc_pos=%4dx%4d, crtc_size=%4dx%4d, src_pos=%d.%dx%d.%d, src_size=%d.%dx%d.%d, format=%s, rotation=%s\n",
> +			   plane->base.id,
> +			   plane_type(intel_plane->base.type),
> +			   state->crtc_x, state->crtc_y,
> +			   state->crtc_w, state->crtc_h,
> +			   (state->src_x >> 16), (state->src_x & 0x00ff),
> +			   (state->src_y >> 16), (state->src_y & 0x00ff),
> +			   (state->src_w >> 16), (state->src_w & 0x00ff),
> +			   (state->src_h >> 16), (state->src_h & 0x00ff),
> +			   fb_format ? plane_format(fb_format) : "N/A",
> +			   plane_rotation(state->rotation));
> +	}
> +}
> +
> +static void intel_scaler_info(struct seq_file *m, struct intel_crtc *intel_crtc)
> +{
> +	struct intel_crtc_state *pipe_config;
> +	int num_scalers = intel_crtc->num_scalers;
> +	int i;
> +
> +	pipe_config = to_intel_crtc_state(intel_crtc->base.state);
> +
> +	/* Not all platformas have a scaler */
> +	if (num_scalers) {
> +		seq_printf(m, "\tnum_scalers=%d, scaler_users=%d scaler_id=%d",
> +			   num_scalers,
> +			   pipe_config->scaler_state.scaler_users,
> +			   pipe_config->scaler_state.scaler_id);
scaler_users is a bitmask and should use %x.
> +		for (i = 0; i < SKL_NUM_SCALERS; i++) {
> +			struct intel_scaler *sc =
> +					&pipe_config->scaler_state.scalers[i];
> +
> +			seq_printf(m, ", scalers[%d]: use=%d, mode=%d",
mode=%x again because of being a bit mask.
> +				   i, sc->in_use, sc->mode);
yesno(sc->in_use);
> +		}
> +		seq_puts(m, "\n");
> +	} else {
> +		seq_puts(m, "\tNo scalers available on this platform\n");
> +	}
> +}
> +
>  static int i915_display_info(struct seq_file *m, void *unused)
>  {
>  	struct drm_info_node *node = m->private;
> @@ -2969,10 +3085,12 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  
>  		pipe_config = to_intel_crtc_state(crtc->base.state);
>  
> -		seq_printf(m, "CRTC %d: pipe: %c, active=%s (size=%dx%d)\n",
> +		seq_printf(m, "CRTC %d: pipe: %c, active=%s, (size=%dx%d), dither=%s, bpp=%d\n",
>  			   crtc->base.base.id, pipe_name(crtc->pipe),
>  			   yesno(pipe_config->base.active),
> -			   pipe_config->pipe_src_w, pipe_config->pipe_src_h);
> +			   pipe_config->pipe_src_w, pipe_config->pipe_src_h,
> +			   yesno(pipe_config->dither), pipe_config->pipe_bpp);
> +
>  		if (pipe_config->base.active) {
>  			intel_crtc_info(m, crtc);
>  
> @@ -2982,6 +3100,8 @@ static int i915_display_info(struct seq_file *m, void *unused)
>  				   x, y, crtc->base.cursor->state->crtc_w,
>  				   crtc->base.cursor->state->crtc_h,
>  				   crtc->cursor_addr, yesno(active));
> +			intel_scaler_info(m, crtc);
> +			intel_plane_info(m, crtc);
>  		}
>  
>  		seq_printf(m, "\tunderrun reporting: cpu=%s pch=%s \n",