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@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
