[PATCH 1/2] drm: expose subpixel order name routine

[PATCH 1/2] drm: expose subpixel order name routine

[Jesse Barnes]

Just like we have for connector type etc.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_crtc.c | 20 ++++++++++++++++++++
 include/drm/drm_crtc.h     |  1 +
 2 files changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 266a01d..3982dd9 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -215,6 +215,16 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] =
 	{ DRM_MODE_ENCODER_DSI, "DSI" },
 };
 
+static const struct drm_prop_enum_list drm_subpixel_enum_list[] =
+{
+	{ SubPixelUnknown, "Unknown" },
+	{ SubPixelHorizontalRGB, "Horizontal RGB" },
+	{ SubPixelHorizontalBGR, "Horizontal BGR" },
+	{ SubPixelVerticalRGB, "Vertical RGB" },
+	{ SubPixelVerticalBGR, "Vertical BGR" },
+	{ SubPixelNone, "None" },
+};
+
 void drm_connector_ida_init(void)
 {
 	int i;
@@ -264,6 +274,16 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
 }
 EXPORT_SYMBOL(drm_get_connector_status_name);
 
+const char *drm_get_subpixel_order_name(enum subpixel_order order)
+{
+	static char buf[32];
+
+	snprintf(buf, 32, "%s", drm_subpixel_enum_list[order].name);
+
+	return buf;
+}
+EXPORT_SYMBOL(drm_get_subpixel_order_name);
+
 static char printable_char(int c)
 {
 	return isascii(c) && isprint(c) ? c : '?';
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f32c5cd..271373f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -963,6 +963,7 @@ extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
 extern const char *drm_get_connector_name(const struct drm_connector *connector);
 extern const char *drm_get_connector_status_name(enum drm_connector_status status);
+extern const char *drm_get_subpixel_order_name(enum subpixel_order order);
 extern const char *drm_get_dpms_name(int val);
 extern const char *drm_get_dvi_i_subconnector_name(int val);
 extern const char *drm_get_dvi_i_select_name(int val);
-- 
1.8.3.2

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

[Jesse Barnes]

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");
+		}
+	}
+}
+
+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 {
-- 
1.8.3.2

Re: [PATCH 1/2] drm: expose subpixel order name routine

[Chris Wilson]

On Mon, Jan 20, 2014 at 03:54:59PM -0800, Jesse Barnes wrote:
> Just like we have for connector type etc.

Then we might as well take a similar defensive approach if we want to
expand the number of contexts we call it from.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm: expose subpixel order name routine

[Jesse Barnes]

On Tue, 21 Jan 2014 09:05:04 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Mon, Jan 20, 2014 at 03:54:59PM -0800, Jesse Barnes wrote:
> > Just like we have for connector type etc.
> 
> Then we might as well take a similar defensive approach if we want to
> expand the number of contexts we call it from.

Since I'm not printing an identifier number into the string I can just
return the one from the array, if that's what you mean.  Fixed.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH] drm: expose subpixel order name routine v2

[Jesse Barnes]

Just like we have for connector type etc.

v2: just return the string directly to avoid repeating the mistakes of
    the past (Chris)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/drm_crtc.c | 16 ++++++++++++++++
 include/drm/drm_crtc.h     |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 266a01d..65f2937 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -215,6 +215,16 @@ static const struct drm_prop_enum_list drm_encoder_enum_list[] =
 	{ DRM_MODE_ENCODER_DSI, "DSI" },
 };
 
+static const struct drm_prop_enum_list drm_subpixel_enum_list[] =
+{
+	{ SubPixelUnknown, "Unknown" },
+	{ SubPixelHorizontalRGB, "Horizontal RGB" },
+	{ SubPixelHorizontalBGR, "Horizontal BGR" },
+	{ SubPixelVerticalRGB, "Vertical RGB" },
+	{ SubPixelVerticalBGR, "Vertical BGR" },
+	{ SubPixelNone, "None" },
+};
+
 void drm_connector_ida_init(void)
 {
 	int i;
@@ -264,6 +274,12 @@ const char *drm_get_connector_status_name(enum drm_connector_status status)
 }
 EXPORT_SYMBOL(drm_get_connector_status_name);
 
+const char *drm_get_subpixel_order_name(enum subpixel_order order)
+{
+	return drm_subpixel_enum_list[order].name;
+}
+EXPORT_SYMBOL(drm_get_subpixel_order_name);
+
 static char printable_char(int c)
 {
 	return isascii(c) && isprint(c) ? c : '?';
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index f32c5cd..271373f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -963,6 +963,7 @@ extern void drm_encoder_cleanup(struct drm_encoder *encoder);
 
 extern const char *drm_get_connector_name(const struct drm_connector *connector);
 extern const char *drm_get_connector_status_name(enum drm_connector_status status);
+extern const char *drm_get_subpixel_order_name(enum subpixel_order order);
 extern const char *drm_get_dpms_name(int val);
 extern const char *drm_get_dvi_i_subconnector_name(int val);
 extern const char *drm_get_dvi_i_select_name(int val);
-- 
1.8.3.2

Re: [PATCH 1/2] drm: expose subpixel order name routine

[Chris Wilson]

On Tue, Jan 21, 2014 at 12:46:08PM -0800, Jesse Barnes wrote:
> On Tue, 21 Jan 2014 09:05:04 +0000
> Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > On Mon, Jan 20, 2014 at 03:54:59PM -0800, Jesse Barnes wrote:
> > > Just like we have for connector type etc.
> > 
> > Then we might as well take a similar defensive approach if we want to
> > expand the number of contexts we call it from.
> 
> Since I'm not printing an identifier number into the string I can just
> return the one from the array, if that's what you mean.  Fixed.

Heh, sounds useful as well. :)

I was thinking of the style encouraged for intel_output_name(), viz

  const char *intel_subpixel_order_to_string(enum subpixel_order order) /* or whatever it was called */
  {
	static const char *names[] = {
		[SubPixelUnknown] = "unknown",
	};

	if ((unsigned)order >= ARRAY_SIZE(names) || !names[order])
		return "unknown"
	
	return names[order];
  }

If we were consistent in using that approach, the code is robust against
any changes or bad input, without being too cumbersome.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Rodrigo Vivi]

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



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br

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

[Jesse Barnes]

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