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

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

[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@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";
+	}
+}
+
+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;
+}
+
+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";
+	}
+}
+
+
+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);
+
+		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",
+				   i, sc->in_use, sc->mode);
+		}
+		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",
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Chris Wilson]

On Fri, Oct 23, 2015 at 04:24:25PM +0200, Robert Fekete wrote:
> +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";
> +	}

Note you can have DRM_ROTATE_foo | DRM_REFLECT_bar.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Fekete, Robert]

On Friday, October 23, 2015 4:27 PM, Chris Wilson wrote:
> On Fri, Oct 23, 2015 at 04:24:25PM +0200, Robert Fekete wrote:
> > +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";
> > +	}
> 
> Note you can have DRM_ROTATE_foo | DRM_REFLECT_bar.
> -Chris

Yepp, I'll rewrite this one. Thanks.

> --
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
----------------------------------------------------------------------
Intel Sweden AB
Registered Office: Isafjordsgatan 30B, 164 40 Kista, Stockholm, Sweden
Registration Number: 556189-6027

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Ville Syrjälä]

On Fri, Oct 23, 2015 at 04:24:25PM +0200, Robert Fekete wrote:
> 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@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";
> +	}
> +}
> +
> +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;
> +}

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";

It's a bitmask so that doesn't work.

> +	default:
> +		MISSING_CASE(rotation);
> +		return "unknown";
> +	}
> +}
> +
> +
> +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),

0x00ff is wrong. But even 0xffff wouldn't generate any kind of human
readable results. See drm_rect_debug_print(). Maybe massage that into a
form that gives you the result as a string.

Also would probably be good to print also the clipped coordinates.

> +			   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);
> +
> +		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",
> +				   i, sc->in_use, sc->mode);
> +		}
> +		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",
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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",

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Robert Fekete]

Hi, thanks for reviewing Maarten.
See comments inline...

On mån, 2015-10-26 at 09:23 +0100, Maarten Lankhorst wrote:
> 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@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.
I see, smart way for anyone modifying the drm_plane_type enum to see
whom it may affect. I'll do that adding a comment why I omit the
default: case
> > +	}
> > +}
> > +
> > +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.

Yepp there is :-). Shame on me missing that. I'll use that one instead.

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

Yepp I misunderstood the value of DRM_ROTATE_xxx. It is a bit-position
intended to be used with BIT() macro and bitops. I'll rewrite this
function into printing the bits set to see if weird values may enter
into the rotation value. Above version is indeed wrong. Thanks for
noticing.


Although I may need an explanation in what is a valid rotation value.
This is how I get it.

DRM_ROTATE_0:    00000001 (1 << 0)
DRM_ROTATE_90:   00000010 (1 << 1)
DRM_ROTATE_180:  00000100 (1 << 2)
DRM_ROTATE_270:  00001000 (1 << 3)
DRM_REFLECT_X:   00010000 (1 << 4)
DRM_REFLECT_Y:   00100000 (1 << 5)

DRM_REFLECT_MASK 11110000
DRM_ROTATE_MASK  00001111

I assume 00010010 to be a valid rotation (ROTATE_90 and a REFLECT_X)
OTOH 00000110 (ROTATE_90 and ROTATE_180) is wrong right? But it could
also be interpreted as ROTATE_270 if it is an accumulative rotation.
Also with this notion you will have two different rotation values for
the same actual rotation value. 

Also what confuses me with this setup (IIUC) is that different values
will in fact ultimately mean the same thing.
00110001 (FLIP X and Y and ROTATE_0) is rot_180
00000100 (ROTATE_180)

00110100 (FLIP X and Y and ROTATE_180) is rot_0
00000001 (ROTATE_0)
plus some more examples

Regarding if rotation is CW or CCW? I cant figure that out in drm_crtc.h
where it is defined. OTOH in intel_display.c I can see a comment that
DRM_ROTATE_ is indeed CCW in order to be compatible with XRandr. I guess
that is one way to documentat things, or would perhaps a clockwise bit
be useful
DRM_ROTATE_CW:   01000000 (1 << 6)
AND a comment that default is CCW. Or at least a comment in drm_crtc.h
like: /* DRM_ROTATE_ is counter clockwise */

Finally for my better understanding....
If you for example do a REFLECT_X and a ROT_270 the order in which it is
applied matters. Which order is default? quite important for user-space
to know when manipulating these bits. I can't find any help in libdrm
either.
so what I mean.
(First REFLECT_X then ROTATE_270) != (First ROTATE_270 then REFLECT_X)
 

> > +
> > +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.
Ack!
> > +		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.
Ack!
> > +				   i, sc->in_use, sc->mode);
> yesno(sc->in_use);
Ack!
> > +		}
> > +		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",
> 

I'll send out a V2.

-- 
BR
/Robert Fekete
Intel Open Source Technology Center

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Ville Syrjälä]

On Mon, Oct 26, 2015 at 03:56:33PM +0100, Robert Fekete wrote:
> Hi, thanks for reviewing Maarten.
> See comments inline...
> 
> On mån, 2015-10-26 at 09:23 +0100, Maarten Lankhorst wrote:
> > 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@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.
> I see, smart way for anyone modifying the drm_plane_type enum to see
> whom it may affect. I'll do that adding a comment why I omit the
> default: case
> > > +	}
> > > +}
> > > +
> > > +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.
> 
> Yepp there is :-). Shame on me missing that. I'll use that one instead.
> 
> > > +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.
> 
> Yepp I misunderstood the value of DRM_ROTATE_xxx. It is a bit-position
> intended to be used with BIT() macro and bitops. I'll rewrite this
> function into printing the bits set to see if weird values may enter
> into the rotation value. Above version is indeed wrong. Thanks for
> noticing.
> 
> 
> Although I may need an explanation in what is a valid rotation value.
> This is how I get it.
> 
> DRM_ROTATE_0:    00000001 (1 << 0)
> DRM_ROTATE_90:   00000010 (1 << 1)
> DRM_ROTATE_180:  00000100 (1 << 2)
> DRM_ROTATE_270:  00001000 (1 << 3)
> DRM_REFLECT_X:   00010000 (1 << 4)
> DRM_REFLECT_Y:   00100000 (1 << 5)
> 
> DRM_REFLECT_MASK 11110000
> DRM_ROTATE_MASK  00001111
> 
> I assume 00010010 to be a valid rotation (ROTATE_90 and a REFLECT_X)
> OTOH 00000110 (ROTATE_90 and ROTATE_180) is wrong right?

Yes, only one angle can be specified.

> But it could
> also be interpreted as ROTATE_270 if it is an accumulative rotation.
> Also with this notion you will have two different rotation values for
> the same actual rotation value. 
> 
> Also what confuses me with this setup (IIUC) is that different values
> will in fact ultimately mean the same thing.
> 00110001 (FLIP X and Y and ROTATE_0) is rot_180
> 00000100 (ROTATE_180)
> 
> 00110100 (FLIP X and Y and ROTATE_180) is rot_0
> 00000001 (ROTATE_0)
> plus some more examples

Yes, all those are valid. drm_rotation_simplify() tries to use such
identities to eliminate unsupported angles/reflections, but currently
it's only meant as a helper for drivers.

> 
> Regarding if rotation is CW or CCW? I cant figure that out in drm_crtc.h
> where it is defined. OTOH in intel_display.c I can see a comment that
> DRM_ROTATE_ is indeed CCW in order to be compatible with XRandr. I guess
> that is one way to documentat things, or would perhaps a clockwise bit
> be useful
> DRM_ROTATE_CW:   01000000 (1 << 6)
> AND a comment that default is CCW. Or at least a comment in drm_crtc.h
> like: /* DRM_ROTATE_ is counter clockwise */
> 
> Finally for my better understanding....
> If you for example do a REFLECT_X and a ROT_270 the order in which it is
> applied matters. Which order is default? quite important for user-space
> to know when manipulating these bits. I can't find any help in libdrm
> either.
> so what I mean.
> (First REFLECT_X then ROTATE_270) != (First ROTATE_270 then REFLECT_X)

The rotation property documentation has the details, but repeating it
next to the DRM_ROTATE bits probably wouldn't hurt.

>  
> 
> > > +
> > > +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.
> Ack!
> > > +		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.
> Ack!
> > > +				   i, sc->in_use, sc->mode);
> > yesno(sc->in_use);
> Ack!
> > > +		}
> > > +		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",
> > 
> 
> I'll send out a V2.
> 
> -- 
> BR
> /Robert Fekete
> Intel Open Source Technology Center
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Add extra plane information in debugfs.

[Robert Fekete]

On mån, 2015-10-26 at 17:40 +0200, Ville Syrjälä wrote:
> On Mon, Oct 26, 2015 at 03:56:33PM +0100, Robert Fekete wrote:
> > Hi, thanks for reviewing Maarten.
> > See comments inline...
> > 
> > On mån, 2015-10-26 at 09:23 +0100, Maarten Lankhorst wrote:
> > > 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@linux.intel.com>
...

> > > 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.
> > 
> > Yepp I misunderstood the value of DRM_ROTATE_xxx. It is a bit-position
> > intended to be used with BIT() macro and bitops. I'll rewrite this
> > function into printing the bits set to see if weird values may enter
> > into the rotation value. Above version is indeed wrong. Thanks for
> > noticing.
> > 
> > 
> > Although I may need an explanation in what is a valid rotation value.
> > This is how I get it.
> > 
> > DRM_ROTATE_0:    00000001 (1 << 0)
> > DRM_ROTATE_90:   00000010 (1 << 1)
> > DRM_ROTATE_180:  00000100 (1 << 2)
> > DRM_ROTATE_270:  00001000 (1 << 3)
> > DRM_REFLECT_X:   00010000 (1 << 4)
> > DRM_REFLECT_Y:   00100000 (1 << 5)
> > 
> > DRM_REFLECT_MASK 11110000
> > DRM_ROTATE_MASK  00001111
> > 
> > I assume 00010010 to be a valid rotation (ROTATE_90 and a REFLECT_X)
> > OTOH 00000110 (ROTATE_90 and ROTATE_180) is wrong right?
> 
> Yes, only one angle can be specified.

OK,

> > But it could
> > also be interpreted as ROTATE_270 if it is an accumulative rotation.
> > Also with this notion you will have two different rotation values for
> > the same actual rotation value. 
> > 
> > Also what confuses me with this setup (IIUC) is that different values
> > will in fact ultimately mean the same thing.
> > 00110001 (FLIP X and Y and ROTATE_0) is rot_180
> > 00000100 (ROTATE_180)
> > 
> > 00110100 (FLIP X and Y and ROTATE_180) is rot_0
> > 00000001 (ROTATE_0)
> > plus some more examples
> 
> Yes, all those are valid. drm_rotation_simplify() tries to use such
> identities to eliminate unsupported angles/reflections, but currently
> it's only meant as a helper for drivers.

Ok,

> > 
> > Regarding if rotation is CW or CCW? I cant figure that out in drm_crtc.h
> > where it is defined. OTOH in intel_display.c I can see a comment that
> > DRM_ROTATE_ is indeed CCW in order to be compatible with XRandr. I guess
> > that is one way to documentat things, or would perhaps a clockwise bit
> > be useful
> > DRM_ROTATE_CW:   01000000 (1 << 6)
> > AND a comment that default is CCW. Or at least a comment in drm_crtc.h
> > like: /* DRM_ROTATE_ is counter clockwise */
> > 
> > Finally for my better understanding....
> > If you for example do a REFLECT_X and a ROT_270 the order in which it is
> > applied matters. Which order is default? quite important for user-space
> > to know when manipulating these bits. I can't find any help in libdrm
> > either.
> > so what I mean.
> > (First REFLECT_X then ROTATE_270) != (First ROTATE_270 then REFLECT_X)
> 
> The rotation property documentation has the details, but repeating it
> next to the DRM_ROTATE bits probably wouldn't hurt.

Yepp, saw that under drm-kms-properties now. 

-- 
BR
/Robert Fekete
Intel Open Source Technology Center

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx