[PATCH 0/3] Only initialize detected DDI buffers (v2)

[PATCH 0/3] Only initialize detected DDI buffers (v2)

[Damien Lespiau]

Changes since the previous patch:
  http://lists.freedesktop.org/archives/intel-gfx/2014-August/050058.html

  - patch 1 is a bonus patch adding one more of those for_each() macro
  - patch 2 limits the DDI buffer initialization to the detected DDIs
  - patch 3 is the v2 of the previous patch with Paulo's suggestion implemented

-- 
Damien

Damien Lespiau (3):
  drm/i915: Introduce a for_each_intel_encoder() macro
  drm/i915: Iterate through the initialized DDIs to prepare their buffers
  drm/i915: Don't write the HDMI buffer translation entry when not needed

 drivers/gpu/drm/i915/i915_debugfs.c  |  3 +-
 drivers/gpu/drm/i915/i915_drv.h      | 10 ++++++
 drivers/gpu/drm/i915/i915_irq.c      |  8 ++---
 drivers/gpu/drm/i915/intel_ddi.c     | 25 ++++++++++++---
 drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++------------------------
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  3 +-
 7 files changed, 57 insertions(+), 53 deletions(-)

-- 
1.8.3.1

[PATCH 1/3] drm/i915: Introduce a for_each_intel_encoder() macro

[Damien Lespiau]

Following the established idom, let's provide a macro to iterate through
the encoders.

spatch helps, once more, for the substitution:

  @@
  iterator name list_for_each_entry;
  iterator name for_each_intel_encoder;
  struct intel_encoder * encoder;
  struct drm_device * dev;
  @@
  -list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
  +for_each_intel_encoder(dev, encoder) {
    ...
  }

I also modified a few call sites by hand where a pointer to mode_config
was directly used (to avoid overflowing 80 chars).

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  3 +-
 drivers/gpu/drm/i915/i915_drv.h      |  5 +++
 drivers/gpu/drm/i915/i915_irq.c      |  8 ++---
 drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++------------------------
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  3 +-
 6 files changed, 31 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index aea1a81..330caa1 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2708,8 +2708,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
 	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
 
 	drm_modeset_lock_all(dev);
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (!encoder->base.crtc)
 			continue;
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2e5f41c..dcf318b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -171,6 +171,11 @@ enum hpd_pin {
 #define for_each_intel_crtc(dev, intel_crtc) \
 	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
 
+#define for_each_intel_encoder(dev, intel_encoder) 		\
+	list_for_each_entry(intel_encoder,			\
+			    &dev->mode_config.encoder_list,	\
+			    base.head)
+
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		if ((intel_encoder)->base.crtc == (__crtc))
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 98abc22..db577c7 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -3513,18 +3513,17 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
 static void ibx_hpd_irq_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *intel_encoder;
 	u32 hotplug_irqs, hotplug, enabled_irqs = 0;
 
 	if (HAS_PCH_IBX(dev)) {
 		hotplug_irqs = SDE_HOTPLUG_MASK;
-		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		for_each_intel_encoder(dev, intel_encoder)
 			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
 				enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
 	} else {
 		hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
-		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		for_each_intel_encoder(dev, intel_encoder)
 			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
 				enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
 	}
@@ -4443,7 +4442,6 @@ static int i965_irq_postinstall(struct drm_device *dev)
 static void i915_hpd_irq_setup(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *intel_encoder;
 	u32 hotplug_en;
 
@@ -4454,7 +4452,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
 		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
 		/* Note HDMI and DP share hotplug bits */
 		/* enable bits are the same for all generations */
-		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
+		for_each_intel_encoder(dev, intel_encoder)
 			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
 				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
 		/* Programming the CRT detection parameters tends
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 883af0b..dd5dcfc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6391,7 +6391,6 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
 static void ironlake_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *encoder;
 	u32 val, final;
 	bool has_lvds = false;
@@ -6401,8 +6400,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
 	bool can_ssc = false;
 
 	/* We need to take the global config into account */
-	list_for_each_entry(encoder, &mode_config->encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			has_panel = true;
@@ -6709,11 +6707,10 @@ static void lpt_disable_clkout_dp(struct drm_device *dev)
 
 static void lpt_init_pch_refclk(struct drm_device *dev)
 {
-	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct intel_encoder *encoder;
 	bool has_vga = false;
 
-	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_ANALOG:
 			has_vga = true;
@@ -9872,8 +9869,7 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
 			to_intel_encoder(connector->base.encoder);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		encoder->new_crtc =
 			to_intel_crtc(encoder->base.crtc);
 	}
@@ -9904,8 +9900,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
 		connector->base.encoder = &connector->new_encoder->base;
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		encoder->base.crtc = &encoder->new_crtc->base;
 	}
 
@@ -10066,8 +10061,7 @@ static bool check_single_encoder_cloning(struct intel_crtc *crtc,
 	struct drm_device *dev = crtc->base.dev;
 	struct intel_encoder *source_encoder;
 
-	list_for_each_entry(source_encoder,
-			    &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, source_encoder) {
 		if (source_encoder->new_crtc != crtc)
 			continue;
 
@@ -10083,8 +10077,7 @@ static bool check_encoder_cloning(struct intel_crtc *crtc)
 	struct drm_device *dev = crtc->base.dev;
 	struct intel_encoder *encoder;
 
-	list_for_each_entry(encoder,
-			    &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (encoder->new_crtc != crtc)
 			continue;
 
@@ -10168,8 +10161,7 @@ encoder_retry:
 	 * adjust it according to limitations or connector properties, and also
 	 * a chance to reject the mode entirely.
 	 */
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 
 		if (&encoder->new_crtc->base != crtc)
 			continue;
@@ -10247,8 +10239,7 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
 				1 << connector->new_encoder->new_crtc->pipe;
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (encoder->base.crtc == &encoder->new_crtc->base)
 			continue;
 
@@ -10322,8 +10313,7 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
 	struct intel_crtc *intel_crtc;
 	struct drm_connector *connector;
 
-	list_for_each_entry(intel_encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, intel_encoder) {
 		if (!intel_encoder->base.crtc)
 			continue;
 
@@ -10563,8 +10553,7 @@ check_encoder_state(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_connector *connector;
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		bool enabled = false;
 		bool active = false;
 		enum pipe pipe, tracked_pipe;
@@ -10643,8 +10632,7 @@ check_crtc_state(struct drm_device *dev)
 		WARN(crtc->active && !crtc->base.enabled,
 		     "active crtc, but not enabled in sw tracking\n");
 
-		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-				    base.head) {
+		for_each_intel_encoder(dev, encoder) {
 			if (encoder->base.crtc != &crtc->base)
 				continue;
 			enabled = true;
@@ -10666,8 +10654,7 @@ check_crtc_state(struct drm_device *dev)
 		if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
 			active = crtc->active;
 
-		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-				    base.head) {
+		for_each_intel_encoder(dev, encoder) {
 			enum pipe pipe;
 			if (encoder->base.crtc != &crtc->base)
 				continue;
@@ -11035,7 +11022,7 @@ static void intel_set_config_restore_state(struct drm_device *dev,
 	}
 
 	count = 0;
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		encoder->new_crtc =
 			to_intel_crtc(config->save_encoder_crtcs[count++]);
 	}
@@ -11194,8 +11181,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	}
 
 	/* Check for any encoders that needs to be disabled. */
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		int num_connectors = 0;
 		list_for_each_entry(connector,
 				    &dev->mode_config.connector_list,
@@ -11228,9 +11214,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	for_each_intel_crtc(dev, crtc) {
 		crtc->new_enabled = false;
 
-		list_for_each_entry(encoder,
-				    &dev->mode_config.encoder_list,
-				    base.head) {
+		for_each_intel_encoder(dev, encoder) {
 			if (encoder->new_crtc == crtc) {
 				crtc->new_enabled = true;
 				break;
@@ -11267,7 +11251,7 @@ static void disable_crtc_nofb(struct intel_crtc *crtc)
 			connector->new_encoder = NULL;
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (encoder->new_crtc == crtc)
 			encoder->new_crtc = NULL;
 	}
@@ -11899,8 +11883,7 @@ static int intel_encoder_clones(struct intel_encoder *encoder)
 	int index_mask = 0;
 	int entry = 0;
 
-	list_for_each_entry(source_encoder,
-			    &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, source_encoder) {
 		if (encoders_cloneable(encoder, source_encoder))
 			index_mask |= (1 << entry);
 
@@ -12089,7 +12072,7 @@ static void intel_setup_outputs(struct drm_device *dev)
 
 	intel_edp_psr_init(dev);
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		encoder->base.possible_crtcs = encoder->crtc_mask;
 		encoder->base.possible_clones =
 			intel_encoder_clones(encoder);
@@ -12983,8 +12966,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
 	}
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		pipe = 0;
 
 		if (encoder->get_hw_state(encoder, &pipe)) {
@@ -13048,8 +13030,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
 	}
 
 	/* HW state is read out, now we need to sanitize this mess. */
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		intel_sanitize_encoder(encoder);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 5f47d35..9169786 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -885,7 +885,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc *crtc)
 	if (HAS_GMCH_DISPLAY(dev))
 		return false;
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (encoder->new_crtc != crtc)
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 881361c..1987491 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -823,8 +823,7 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
 	struct intel_encoder *encoder;
 	struct intel_lvds_encoder *lvds_encoder;
 
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
-			    base.head) {
+	for_each_intel_encoder(dev, encoder) {
 		if (encoder->type == INTEL_OUTPUT_LVDS) {
 			lvds_encoder = to_lvds_encoder(&encoder->base);
 
-- 
1.8.3.1

[PATCH 2/3] drm/i915: Iterate through the initialized DDIs to prepare their buffers

[Damien Lespiau]

Not every DDIs is necessarily connected can be strapped off and, in the
future, we'll have platforms with a different number of default DDI
ports. So, let's only call intel_prepare_ddi_buffers() on DDI ports that
are actually detected.

We also use the opportunity to give a struct intel_digital_port to
intel_prepare_ddi_buffers() as we'll need it in a following patch to
query if the port supports HMDI or not.

On my HSW machine this removes the initialization of a couple of
(unused) DDIs.

Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  5 +++++
 drivers/gpu/drm/i915/intel_ddi.c | 16 ++++++++++++----
 2 files changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dcf318b..701aae0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -176,6 +176,11 @@ enum hpd_pin {
 			    &dev->mode_config.encoder_list,	\
 			    base.head)
 
+#define for_each_digital_port(dev, digital_port)		\
+	list_for_each_entry(digital_port,			\
+			    &dev->mode_config.encoder_list,	\
+			    base.base.head)
+
 #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
 	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
 		if ((intel_encoder)->base.crtc == (__crtc))
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ca1f9a8..fcbddd9 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -152,10 +152,12 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
  * in either FDI or DP modes only, as HDMI connections will work with both
  * of those
  */
-static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
+static void intel_prepare_ddi_buffers(struct drm_device *dev,
+				      struct intel_digital_port *intel_dig_port)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 reg;
+	int port = intel_dig_port->port;
 	int i, n_hdmi_entries, hdmi_800mV_0dB;
 	int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
 	const u32 *ddi_translations_fdi;
@@ -232,13 +234,19 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
  */
 void intel_prepare_ddi(struct drm_device *dev)
 {
-	int port;
+	struct intel_digital_port *intel_dig_port;
+	bool visited[I915_MAX_PORTS] = { 0, };
 
 	if (!HAS_DDI(dev))
 		return;
 
-	for (port = PORT_A; port <= PORT_E; port++)
-		intel_prepare_ddi_buffers(dev, port);
+	for_each_digital_port(dev, intel_dig_port) {
+		if (visited[intel_dig_port->port])
+			continue;
+
+		intel_prepare_ddi_buffers(dev, intel_dig_port);
+		visited[intel_dig_port->port] = true;
+	}
 }
 
 static const long hsw_ddi_buf_ctl_values[] = {
-- 
1.8.3.1

[PATCH 3/3] drm/i915: Don't write the HDMI buffer translation entry when not needed

[Damien Lespiau]

We don't actually need to write the HDMI entry on DDIs that have no
chance to be used as HDMI ports.

While this patch shouldn't change the current behaviour, it makes
further enabling work easier as we'll have an eDP table filling the full
10 entries.

v2: Rely on the logic from intel_ddi_init() to figure out if the DDI port
    supports HDMI or not (Paulo).

Suggested-by: Satheeshakrishna M <satheeshakrishna.m@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index fcbddd9..3c6928e 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -145,6 +145,12 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
 	}
 }
 
+static bool
+intel_dig_port_supports_hdmi(const struct intel_digital_port *intel_dig_port)
+{
+	return intel_dig_port->hdmi.hdmi_reg;
+}
+
 /*
  * Starting with Haswell, DDI port buffers must be programmed with correct
  * values in advance. The buffer values are different for FDI and DP modes,
@@ -217,6 +223,9 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev,
 		reg += 4;
 	}
 
+	if (!intel_dig_port_supports_hdmi(intel_dig_port))
+		return;
+
 	/* Choose a good default if VBT is badly populated */
 	if (hdmi_level == HDMI_LEVEL_SHIFT_UNKNOWN ||
 	    hdmi_level >= n_hdmi_entries)
-- 
1.8.3.1

Re: [PATCH 1/3] drm/i915: Introduce a for_each_intel_encoder() macro

[Daniel Vetter]

On Tue, Aug 05, 2014 at 11:29:37AM +0100, Damien Lespiau wrote:
> Following the established idom, let's provide a macro to iterate through
> the encoders.
> 
> spatch helps, once more, for the substitution:
> 
>   @@
>   iterator name list_for_each_entry;
>   iterator name for_each_intel_encoder;
>   struct intel_encoder * encoder;
>   struct drm_device * dev;
>   @@
>   -list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
>   +for_each_intel_encoder(dev, encoder) {
>     ...
>   }
> 
> I also modified a few call sites by hand where a pointer to mode_config
> was directly used (to avoid overflowing 80 chars).
> 
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c  |  3 +-
>  drivers/gpu/drm/i915/i915_drv.h      |  5 +++
>  drivers/gpu/drm/i915/i915_irq.c      |  8 ++---
>  drivers/gpu/drm/i915/intel_display.c | 59 ++++++++++++------------------------
>  drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
>  drivers/gpu/drm/i915/intel_lvds.c    |  3 +-
>  6 files changed, 31 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index aea1a81..330caa1 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -2708,8 +2708,7 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
>  	*source = INTEL_PIPE_CRC_SOURCE_PIPE;
>  
>  	drm_modeset_lock_all(dev);
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (!encoder->base.crtc)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2e5f41c..dcf318b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -171,6 +171,11 @@ enum hpd_pin {
>  #define for_each_intel_crtc(dev, intel_crtc) \
>  	list_for_each_entry(intel_crtc, &dev->mode_config.crtc_list, base.head)
>  
> +#define for_each_intel_encoder(dev, intel_encoder) 		\

Remove the spuriuos space checkpatch spotted above

> +	list_for_each_entry(intel_encoder,			\
> +			    &dev->mode_config.encoder_list,	\

... and wrapped (dev) correctly to avoid surprises.

Queued for -next, thanks for the patch.
-Daniel

> +			    base.head)
> +
>  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
>  	list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
>  		if ((intel_encoder)->base.crtc == (__crtc))
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 98abc22..db577c7 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -3513,18 +3513,17 @@ static void cherryview_irq_preinstall(struct drm_device *dev)
>  static void ibx_hpd_irq_setup(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *intel_encoder;
>  	u32 hotplug_irqs, hotplug, enabled_irqs = 0;
>  
>  	if (HAS_PCH_IBX(dev)) {
>  		hotplug_irqs = SDE_HOTPLUG_MASK;
> -		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		for_each_intel_encoder(dev, intel_encoder)
>  			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
>  				enabled_irqs |= hpd_ibx[intel_encoder->hpd_pin];
>  	} else {
>  		hotplug_irqs = SDE_HOTPLUG_MASK_CPT;
> -		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		for_each_intel_encoder(dev, intel_encoder)
>  			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
>  				enabled_irqs |= hpd_cpt[intel_encoder->hpd_pin];
>  	}
> @@ -4443,7 +4442,6 @@ static int i965_irq_postinstall(struct drm_device *dev)
>  static void i915_hpd_irq_setup(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *intel_encoder;
>  	u32 hotplug_en;
>  
> @@ -4454,7 +4452,7 @@ static void i915_hpd_irq_setup(struct drm_device *dev)
>  		hotplug_en &= ~HOTPLUG_INT_EN_MASK;
>  		/* Note HDMI and DP share hotplug bits */
>  		/* enable bits are the same for all generations */
> -		list_for_each_entry(intel_encoder, &mode_config->encoder_list, base.head)
> +		for_each_intel_encoder(dev, intel_encoder)
>  			if (dev_priv->hpd_stats[intel_encoder->hpd_pin].hpd_mark == HPD_ENABLED)
>  				hotplug_en |= hpd_mask_i915[intel_encoder->hpd_pin];
>  		/* Programming the CRT detection parameters tends
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 883af0b..dd5dcfc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6391,7 +6391,6 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc,
>  static void ironlake_init_pch_refclk(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *encoder;
>  	u32 val, final;
>  	bool has_lvds = false;
> @@ -6401,8 +6400,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
>  	bool can_ssc = false;
>  
>  	/* We need to take the global config into account */
> -	list_for_each_entry(encoder, &mode_config->encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_LVDS:
>  			has_panel = true;
> @@ -6709,11 +6707,10 @@ static void lpt_disable_clkout_dp(struct drm_device *dev)
>  
>  static void lpt_init_pch_refclk(struct drm_device *dev)
>  {
> -	struct drm_mode_config *mode_config = &dev->mode_config;
>  	struct intel_encoder *encoder;
>  	bool has_vga = false;
>  
> -	list_for_each_entry(encoder, &mode_config->encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		switch (encoder->type) {
>  		case INTEL_OUTPUT_ANALOG:
>  			has_vga = true;
> @@ -9872,8 +9869,7 @@ static void intel_modeset_update_staged_output_state(struct drm_device *dev)
>  			to_intel_encoder(connector->base.encoder);
>  	}
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		encoder->new_crtc =
>  			to_intel_crtc(encoder->base.crtc);
>  	}
> @@ -9904,8 +9900,7 @@ static void intel_modeset_commit_output_state(struct drm_device *dev)
>  		connector->base.encoder = &connector->new_encoder->base;
>  	}
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		encoder->base.crtc = &encoder->new_crtc->base;
>  	}
>  
> @@ -10066,8 +10061,7 @@ static bool check_single_encoder_cloning(struct intel_crtc *crtc,
>  	struct drm_device *dev = crtc->base.dev;
>  	struct intel_encoder *source_encoder;
>  
> -	list_for_each_entry(source_encoder,
> -			    &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, source_encoder) {
>  		if (source_encoder->new_crtc != crtc)
>  			continue;
>  
> @@ -10083,8 +10077,7 @@ static bool check_encoder_cloning(struct intel_crtc *crtc)
>  	struct drm_device *dev = crtc->base.dev;
>  	struct intel_encoder *encoder;
>  
> -	list_for_each_entry(encoder,
> -			    &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (encoder->new_crtc != crtc)
>  			continue;
>  
> @@ -10168,8 +10161,7 @@ encoder_retry:
>  	 * adjust it according to limitations or connector properties, and also
>  	 * a chance to reject the mode entirely.
>  	 */
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  
>  		if (&encoder->new_crtc->base != crtc)
>  			continue;
> @@ -10247,8 +10239,7 @@ intel_modeset_affected_pipes(struct drm_crtc *crtc, unsigned *modeset_pipes,
>  				1 << connector->new_encoder->new_crtc->pipe;
>  	}
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (encoder->base.crtc == &encoder->new_crtc->base)
>  			continue;
>  
> @@ -10322,8 +10313,7 @@ intel_modeset_update_state(struct drm_device *dev, unsigned prepare_pipes)
>  	struct intel_crtc *intel_crtc;
>  	struct drm_connector *connector;
>  
> -	list_for_each_entry(intel_encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, intel_encoder) {
>  		if (!intel_encoder->base.crtc)
>  			continue;
>  
> @@ -10563,8 +10553,7 @@ check_encoder_state(struct drm_device *dev)
>  	struct intel_encoder *encoder;
>  	struct intel_connector *connector;
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		bool enabled = false;
>  		bool active = false;
>  		enum pipe pipe, tracked_pipe;
> @@ -10643,8 +10632,7 @@ check_crtc_state(struct drm_device *dev)
>  		WARN(crtc->active && !crtc->base.enabled,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
> -		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -				    base.head) {
> +		for_each_intel_encoder(dev, encoder) {
>  			if (encoder->base.crtc != &crtc->base)
>  				continue;
>  			enabled = true;
> @@ -10666,8 +10654,7 @@ check_crtc_state(struct drm_device *dev)
>  		if (crtc->pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE)
>  			active = crtc->active;
>  
> -		list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -				    base.head) {
> +		for_each_intel_encoder(dev, encoder) {
>  			enum pipe pipe;
>  			if (encoder->base.crtc != &crtc->base)
>  				continue;
> @@ -11035,7 +11022,7 @@ static void intel_set_config_restore_state(struct drm_device *dev,
>  	}
>  
>  	count = 0;
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		encoder->new_crtc =
>  			to_intel_crtc(config->save_encoder_crtcs[count++]);
>  	}
> @@ -11194,8 +11181,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	}
>  
>  	/* Check for any encoders that needs to be disabled. */
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		int num_connectors = 0;
>  		list_for_each_entry(connector,
>  				    &dev->mode_config.connector_list,
> @@ -11228,9 +11214,7 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	for_each_intel_crtc(dev, crtc) {
>  		crtc->new_enabled = false;
>  
> -		list_for_each_entry(encoder,
> -				    &dev->mode_config.encoder_list,
> -				    base.head) {
> +		for_each_intel_encoder(dev, encoder) {
>  			if (encoder->new_crtc == crtc) {
>  				crtc->new_enabled = true;
>  				break;
> @@ -11267,7 +11251,7 @@ static void disable_crtc_nofb(struct intel_crtc *crtc)
>  			connector->new_encoder = NULL;
>  	}
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (encoder->new_crtc == crtc)
>  			encoder->new_crtc = NULL;
>  	}
> @@ -11899,8 +11883,7 @@ static int intel_encoder_clones(struct intel_encoder *encoder)
>  	int index_mask = 0;
>  	int entry = 0;
>  
> -	list_for_each_entry(source_encoder,
> -			    &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, source_encoder) {
>  		if (encoders_cloneable(encoder, source_encoder))
>  			index_mask |= (1 << entry);
>  
> @@ -12089,7 +12072,7 @@ static void intel_setup_outputs(struct drm_device *dev)
>  
>  	intel_edp_psr_init(dev);
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		encoder->base.possible_crtcs = encoder->crtc_mask;
>  		encoder->base.possible_clones =
>  			intel_encoder_clones(encoder);
> @@ -12983,8 +12966,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
>  			intel_display_power_get(dev_priv, POWER_DOMAIN_PLLS);
>  	}
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		pipe = 0;
>  
>  		if (encoder->get_hw_state(encoder, &pipe)) {
> @@ -13048,8 +13030,7 @@ void intel_modeset_setup_hw_state(struct drm_device *dev,
>  	}
>  
>  	/* HW state is read out, now we need to sanitize this mess. */
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		intel_sanitize_encoder(encoder);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 5f47d35..9169786 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -885,7 +885,7 @@ static bool hdmi_12bpc_possible(struct intel_crtc *crtc)
>  	if (HAS_GMCH_DISPLAY(dev))
>  		return false;
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (encoder->new_crtc != crtc)
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 881361c..1987491 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -823,8 +823,7 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
>  	struct intel_encoder *encoder;
>  	struct intel_lvds_encoder *lvds_encoder;
>  
> -	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> -			    base.head) {
> +	for_each_intel_encoder(dev, encoder) {
>  		if (encoder->type == INTEL_OUTPUT_LVDS) {
>  			lvds_encoder = to_lvds_encoder(&encoder->base);
>  
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/3] drm/i915: Iterate through the initialized DDIs to prepare their buffers

[Paulo Zanoni]

2014-08-05 7:29 GMT-03:00 Damien Lespiau <damien.lespiau@intel.com>:
> Not every DDIs is necessarily connected can be strapped off and, in the
> future, we'll have platforms with a different number of default DDI
> ports. So, let's only call intel_prepare_ddi_buffers() on DDI ports that
> are actually detected.
>
> We also use the opportunity to give a struct intel_digital_port to
> intel_prepare_ddi_buffers() as we'll need it in a following patch to
> query if the port supports HMDI or not.
>
> On my HSW machine this removes the initialization of a couple of
> (unused) DDIs.
>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  5 +++++
>  drivers/gpu/drm/i915/intel_ddi.c | 16 ++++++++++++----
>  2 files changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index dcf318b..701aae0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -176,6 +176,11 @@ enum hpd_pin {
>                             &dev->mode_config.encoder_list,     \
>                             base.head)
>
> +#define for_each_digital_port(dev, digital_port)               \
> +       list_for_each_entry(digital_port,                       \
> +                           &dev->mode_config.encoder_list,     \
> +                           base.base.head)

We can't really assume that every encoder is intel_digital_port since
we still have the CRT encoder on HSW/BDW.

And we can't run this code just for the dig_ports since CRT needs it too.


> +
>  #define for_each_encoder_on_crtc(dev, __crtc, intel_encoder) \
>         list_for_each_entry((intel_encoder), &(dev)->mode_config.encoder_list, base.head) \
>                 if ((intel_encoder)->base.crtc == (__crtc))
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index ca1f9a8..fcbddd9 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -152,10 +152,12 @@ enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder)
>   * in either FDI or DP modes only, as HDMI connections will work with both
>   * of those
>   */
> -static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
> +static void intel_prepare_ddi_buffers(struct drm_device *dev,
> +                                     struct intel_digital_port *intel_dig_port)
>  {
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 reg;
> +       int port = intel_dig_port->port;
>         int i, n_hdmi_entries, hdmi_800mV_0dB;
>         int hdmi_level = dev_priv->vbt.ddi_port_info[port].hdmi_level_shift;
>         const u32 *ddi_translations_fdi;
> @@ -232,13 +234,19 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port)
>   */
>  void intel_prepare_ddi(struct drm_device *dev)
>  {
> -       int port;
> +       struct intel_digital_port *intel_dig_port;
> +       bool visited[I915_MAX_PORTS] = { 0, };
>
>         if (!HAS_DDI(dev))
>                 return;
>
> -       for (port = PORT_A; port <= PORT_E; port++)
> -               intel_prepare_ddi_buffers(dev, port);
> +       for_each_digital_port(dev, intel_dig_port) {
> +               if (visited[intel_dig_port->port])
> +                       continue;
> +
> +               intel_prepare_ddi_buffers(dev, intel_dig_port);
> +               visited[intel_dig_port->port] = true;
> +       }

A comment on why we need the "visited" array is much appreciated,
because it appears to be useless for the code reader. Why is it here?
Will we ever have more than one encoder per port?


>  }
>
>  static const long hsw_ddi_buf_ctl_values[] = {
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/3] drm/i915: Iterate through the initialized DDIs to prepare their buffers

[Damien Lespiau]

On Thu, Aug 07, 2014 at 11:17:35AM -0300, Paulo Zanoni wrote:
> > +#define for_each_digital_port(dev, digital_port)               \
> > +       list_for_each_entry(digital_port,                       \
> > +                           &dev->mode_config.encoder_list,     \
> > +                           base.base.head)
> 
> We can't really assume that every encoder is intel_digital_port since
> we still have the CRT encoder on HSW/BDW.
> 
> And we can't run this code just for the dig_ports since CRT needs it too.

Ah, missed that, of course...

> > +       for_each_digital_port(dev, intel_dig_port) {
> > +               if (visited[intel_dig_port->port])
> > +                       continue;
> > +
> > +               intel_prepare_ddi_buffers(dev, intel_dig_port);
> > +               visited[intel_dig_port->port] = true;
> > +       }
> 
> A comment on why we need the "visited" array is much appreciated,
> because it appears to be useless for the code reader. Why is it here?
> Will we ever have more than one encoder per port?

Because of the MST "fake" encoder.

-- 
Damien