diff mbox

[2/2] drm/i915: add a display info file to debugfs

Message ID 1390262100-13526-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive)
State New, archived
Headers show

Commit Message

Jesse Barnes Jan. 20, 2014, 11:55 p.m. UTC 
Can be expanded up on to include all sorts of things (HDMI infoframe
data, more DP status, etc).  Should be useful for bug reports to get a
baseline on the display config and info.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 155 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_drv.h     |   4 +
 2 files changed, 159 insertions(+)

Comments

Rodrigo Vivi Jan. 30, 2014, 9:58 p.m. UTC | #1 
On Mon, Jan 20, 2014 at 9:55 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> Can be expanded up on to include all sorts of things (HDMI infoframe
> data, more DP status, etc).  Should be useful for bug reports to get a
> baseline on the display config and info.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 155 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h     |   4 +
>  2 files changed, 159 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 75a489e..20ae7ed 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1992,6 +1992,160 @@ static int i915_power_domain_info(struct seq_file *m, void *unused)
>         return 0;
>  }
>
> +static void intel_encoder_info(struct seq_file *m,
> +                              struct intel_crtc *intel_crtc,
> +                              struct intel_encoder *intel_encoder)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_crtc *crtc = &intel_crtc->base;
> +       struct intel_connector *intel_connector;
> +       struct drm_encoder *encoder;
> +
> +       encoder = &intel_encoder->base;
> +       seq_printf(m, "\tencoder %d: type: %s, connectors:\n",
> +                  encoder->base.id, drm_get_encoder_name(encoder));
> +       for_each_connector_on_encoder(dev, encoder, intel_connector) {
> +               struct drm_connector *connector = &intel_connector->base;
> +               seq_printf(m, "\t\tconnector %d: type: %s, status: %s",
> +                          connector->base.id,
> +                          drm_get_connector_name(connector),
> +                          drm_get_connector_status_name(connector->status));
> +               if (connector->status == connector_status_connected) {
> +                       struct drm_display_mode *mode = &crtc->mode;
> +                       seq_printf(m, ", mode:\n");
> +                       seq_printf(m, "\t\t%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);
> +               } else {
> +                       seq_printf(m, "\n");

for cases like this shouldn't we use seq_put instead of seq_printf?

> +               }
> +       }
> +}
> +
> +static void intel_crtc_info(struct seq_file *m, struct intel_crtc *intel_crtc)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_crtc *crtc = &intel_crtc->base;
> +       struct intel_encoder *intel_encoder;
> +
> +       seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n",
> +                  crtc->fb->base.id, crtc->x, crtc->y,
> +                  crtc->fb->width, crtc->fb->height);
> +       for_each_encoder_on_crtc(dev, crtc, intel_encoder)
> +               intel_encoder_info(m, intel_crtc, intel_encoder);
> +}
> +
> +static void intel_panel_info(struct seq_file *m, struct intel_panel *panel)
> +{
> +       struct drm_display_mode *mode = panel->fixed_mode;
> +
> +       seq_printf(m, "\tfixed mode:\n");
> +       seq_printf(m, "\t\t%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);
> +}

I would prefer a more bloated info, with variable_name =
vriable_value... I know this is bloated, but I'm also think on our
lazyness when reading bug reports ;)

> +
> +static void intel_dp_info(struct seq_file *m,
> +                         struct intel_connector *intel_connector)
> +{
> +       struct intel_encoder *intel_encoder = intel_connector->encoder;
> +       struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
> +
> +       seq_printf(m, "\tDPCD rev: %x\n", intel_dp->dpcd[DP_DPCD_REV]);
> +       seq_printf(m, "\taudio support: %s\n", intel_dp->has_audio ? "yes" :
> +                  "no");
> +       if (intel_encoder->type == INTEL_OUTPUT_EDP)
> +               intel_panel_info(m, &intel_connector->panel);
> +}
> +
> +static void intel_hdmi_info(struct seq_file *m,
> +                           struct intel_connector *intel_connector)
> +{
> +       struct intel_encoder *intel_encoder = intel_connector->encoder;
> +       struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
> +
> +       seq_printf(m, "\taudio support: %s\n", intel_hdmi->has_audio ? "yes" :
> +                  "no");
> +}
> +
> +static void intel_lvds_info(struct seq_file *m,
> +                           struct intel_connector *intel_connector)
> +{
> +       intel_panel_info(m, &intel_connector->panel);
> +}
> +
> +static void intel_connector_info(struct seq_file *m,
> +                                struct drm_connector *connector)
> +{
> +       struct intel_connector *intel_connector = to_intel_connector(connector);
> +       struct intel_encoder *intel_encoder = intel_connector->encoder;
> +
> +       seq_printf(m, "connector %d: type %s, status: %s\n",
> +                  connector->base.id, drm_get_connector_name(connector),
> +                  drm_get_connector_status_name(connector->status));
> +       if (connector->status == connector_status_connected) {
> +               seq_printf(m, "\tname: %s\n", connector->display_info.name);
> +               seq_printf(m, "\tphysical dimensions: %dx%dmm\n",
> +                          connector->display_info.width_mm,
> +                          connector->display_info.height_mm);
> +               seq_printf(m, "\tsubpixel order: %s\n",
> +                          drm_get_subpixel_order_name(connector->display_info.subpixel_order));
> +               seq_printf(m, "\tCEA rev: %d\n",
> +                          connector->display_info.cea_rev);
> +       }
> +       if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
> +           intel_encoder->type == INTEL_OUTPUT_EDP)
> +               intel_dp_info(m, intel_connector);
> +       else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
> +               intel_hdmi_info(m, intel_connector);
> +       else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
> +               intel_lvds_info(m, intel_connector);
> +
> +}
> +
> +static int i915_display_info(struct seq_file *m, void *unused)
> +{
> +       struct drm_info_node *node = (struct drm_info_node *) m->private;
> +       struct drm_device *dev = node->minor->dev;
> +       struct drm_crtc *crtc;
> +       struct drm_connector *connector;
> +
> +       drm_modeset_lock_all(dev);
> +       seq_printf(m, "CRTC info\n");
> +       seq_printf(m, "---------\n");
> +       list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> +               struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> +               seq_printf(m, "CRTC %d: pipe: %c, active: %s\n",
> +                          crtc->base.id, pipe_name(intel_crtc->pipe),
> +                          intel_crtc->active ? "yes" : "no");
> +               if (intel_crtc->active)
> +                       intel_crtc_info(m, intel_crtc);
> +       }
> +
> +       seq_printf(m, "\n");
> +       seq_printf(m, "Connector info\n");
> +       seq_printf(m, "--------------\n");
> +       list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +               intel_connector_info(m, connector);
> +       }
> +       drm_modeset_unlock_all(dev);
> +
> +       return 0;
> +}
> +
>  struct pipe_crc_info {
>         const char *name;
>         struct drm_device *dev;
> @@ -3235,6 +3389,7 @@ static const struct drm_info_list i915_debugfs_list[] = {
>         {"i915_energy_uJ", i915_energy_uJ, 0},
>         {"i915_pc8_status", i915_pc8_status, 0},
>         {"i915_power_domain_info", i915_power_domain_info, 0},
> +       {"i915_display_info", i915_display_info, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index cc8afff..741d14e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -162,6 +162,10 @@ enum hpd_pin {
>         list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
>                 if ((intel_encoder)->base.crtc == (__crtc))
>
> +#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
> +       list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
> +               if ((intel_connector)->base.encoder == (__encoder))

are all of this parenthesis needed?

> +
>  struct drm_i915_private;
>
>  enum intel_dpll_id {
> --
> 1.8.3.2
>

with or without my bikesheds, feel free to use:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>


> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Jesse Barnes Feb. 5, 2014, 3:01 p.m. UTC | #2 
On Thu, 30 Jan 2014 19:58:43 -0200
Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
> > +                       seq_printf(m, ", mode:\n");
> > +                       seq_printf(m, "\t\t%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);
> > +               } else {
> > +                       seq_printf(m, "\n");
> 
> for cases like this shouldn't we use seq_put instead of seq_printf?

Yeah I guess that's a bit simpler, I'll switch it over.

> > +       seq_printf(m, "\tfixed mode:\n");
> > +       seq_printf(m, "\t\t%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);
> > +}
> 
> I would prefer a more bloated info, with variable_name =
> vriable_value... I know this is bloated, but I'm also think on our
> lazyness when reading bug reports ;)

Yeah I always have to look up the field names too, I'll annotate this
just to make things extra easy for us. :)

> > +#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
> > +       list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
> > +               if ((intel_connector)->base.encoder == (__encoder))
> 
> are all of this parenthesis needed?

I think that's the minimal amount, yeah.  Anything passed in to the
macro needs to have parens around it just in case it's an expression
that would cause trouble when expanded into the code.

Thanks for the review, I'll re-post shortly.

Jesse
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 75a489e..20ae7ed 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1992,6 +1992,160 @@  static int i915_power_domain_info(struct seq_file *m, void *unused)
 	return 0;
 }
 
+static void intel_encoder_info(struct seq_file *m,
+			       struct intel_crtc *intel_crtc,
+			       struct intel_encoder *intel_encoder)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct intel_connector *intel_connector;
+	struct drm_encoder *encoder;
+
+	encoder = &intel_encoder->base;
+	seq_printf(m, "\tencoder %d: type: %s, connectors:\n",
+		   encoder->base.id, drm_get_encoder_name(encoder));
+	for_each_connector_on_encoder(dev, encoder, intel_connector) {
+		struct drm_connector *connector = &intel_connector->base;
+		seq_printf(m, "\t\tconnector %d: type: %s, status: %s",
+			   connector->base.id,
+			   drm_get_connector_name(connector),
+			   drm_get_connector_status_name(connector->status));
+		if (connector->status == connector_status_connected) {
+			struct drm_display_mode *mode = &crtc->mode;
+			seq_printf(m, ", mode:\n");
+			seq_printf(m, "\t\t%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);
+		} else {
+			seq_printf(m, "\n");
+		}
+	}
+}
+
+static void intel_crtc_info(struct seq_file *m, struct intel_crtc *intel_crtc)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_crtc *crtc = &intel_crtc->base;
+	struct intel_encoder *intel_encoder;
+
+	seq_printf(m, "\tfb: %d, pos: %dx%d, size: %dx%d\n",
+		   crtc->fb->base.id, crtc->x, crtc->y,
+		   crtc->fb->width, crtc->fb->height);
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
+		intel_encoder_info(m, intel_crtc, intel_encoder);
+}
+
+static void intel_panel_info(struct seq_file *m, struct intel_panel *panel)
+{
+	struct drm_display_mode *mode = panel->fixed_mode;
+
+	seq_printf(m, "\tfixed mode:\n");
+	seq_printf(m, "\t\t%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 void intel_dp_info(struct seq_file *m,
+			  struct intel_connector *intel_connector)
+{
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	struct intel_dp *intel_dp = enc_to_intel_dp(&intel_encoder->base);
+
+	seq_printf(m, "\tDPCD rev: %x\n", intel_dp->dpcd[DP_DPCD_REV]);
+	seq_printf(m, "\taudio support: %s\n", intel_dp->has_audio ? "yes" :
+		   "no");
+	if (intel_encoder->type == INTEL_OUTPUT_EDP)
+		intel_panel_info(m, &intel_connector->panel);
+}
+
+static void intel_hdmi_info(struct seq_file *m,
+			    struct intel_connector *intel_connector)
+{
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
+
+	seq_printf(m, "\taudio support: %s\n", intel_hdmi->has_audio ? "yes" :
+		   "no");
+}
+
+static void intel_lvds_info(struct seq_file *m,
+			    struct intel_connector *intel_connector)
+{
+	intel_panel_info(m, &intel_connector->panel);
+}
+
+static void intel_connector_info(struct seq_file *m,
+				 struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_encoder *intel_encoder = intel_connector->encoder;
+
+	seq_printf(m, "connector %d: type %s, status: %s\n",
+		   connector->base.id, drm_get_connector_name(connector),
+		   drm_get_connector_status_name(connector->status));
+	if (connector->status == connector_status_connected) {
+		seq_printf(m, "\tname: %s\n", connector->display_info.name);
+		seq_printf(m, "\tphysical dimensions: %dx%dmm\n",
+			   connector->display_info.width_mm,
+			   connector->display_info.height_mm);
+		seq_printf(m, "\tsubpixel order: %s\n",
+			   drm_get_subpixel_order_name(connector->display_info.subpixel_order));
+		seq_printf(m, "\tCEA rev: %d\n",
+			   connector->display_info.cea_rev);
+	}
+	if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT ||
+	    intel_encoder->type == INTEL_OUTPUT_EDP)
+		intel_dp_info(m, intel_connector);
+	else if (intel_encoder->type == INTEL_OUTPUT_HDMI)
+		intel_hdmi_info(m, intel_connector);
+	else if (intel_encoder->type == INTEL_OUTPUT_LVDS)
+		intel_lvds_info(m, intel_connector);
+
+}
+
+static int i915_display_info(struct seq_file *m, void *unused)
+{
+	struct drm_info_node *node = (struct drm_info_node *) m->private;
+	struct drm_device *dev = node->minor->dev;
+	struct drm_crtc *crtc;
+	struct drm_connector *connector;
+
+	drm_modeset_lock_all(dev);
+	seq_printf(m, "CRTC info\n");
+	seq_printf(m, "---------\n");
+	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+		struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+		seq_printf(m, "CRTC %d: pipe: %c, active: %s\n",
+			   crtc->base.id, pipe_name(intel_crtc->pipe),
+			   intel_crtc->active ? "yes" : "no");
+		if (intel_crtc->active)
+			intel_crtc_info(m, intel_crtc);
+	}
+
+	seq_printf(m, "\n");
+	seq_printf(m, "Connector info\n");
+	seq_printf(m, "--------------\n");
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		intel_connector_info(m, connector);
+	}
+	drm_modeset_unlock_all(dev);
+
+	return 0;
+}
+
 struct pipe_crc_info {
 	const char *name;
 	struct drm_device *dev;
@@ -3235,6 +3389,7 @@  static const struct drm_info_list i915_debugfs_list[] = {
 	{"i915_energy_uJ", i915_energy_uJ, 0},
 	{"i915_pc8_status", i915_pc8_status, 0},
 	{"i915_power_domain_info", i915_power_domain_info, 0},
+	{"i915_display_info", i915_display_info, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cc8afff..741d14e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -162,6 +162,10 @@  enum hpd_pin {
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		if ((intel_encoder)->base.crtc == (__crtc))
 
+#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
+	list_for_each_entry((intel_connector), &(dev)->mode_config.connector_list, base.head) \
+		if ((intel_connector)->base.encoder == (__encoder))
+
 struct drm_i915_private;
 
 enum intel_dpll_id {