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? 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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
