drm/i915: introduce pipe_config->ddi_personality

From: Paulo Zanoni <[email protected]>

Because I got annoyed that I had to document what values "int ddi_personality" is supposed to hold.

A good side-effect of this change is that now the compilers can do some additional checks on our code, which may prevent some bugs in the future. A bad side-effect of this change is that now the compilers do some additional checks on our code and complain when a switch statement doesn't check for all possible values, so we need to add "default" cases to all those switches. Hopefully, this may help preventing confusions against DRM_MODE_CONNECTOR_* and DRM_MODE_ENCODER_*.

I guess that just by looking at the patch, some people will think this change is not worth its benefits. In this case, I don't really mind dropping the patch.

Also, there's probably still a few more places where we can s/int/enum intel_output_type/, but we can change that later, when we spot the places.

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/i915_debugfs.c  |  2 ++
 drivers/gpu/drm/i915/intel_display.c | 10 ++++++++++
 drivers/gpu/drm/i915/intel_drv.h     | 31 ++++++++++++++++---------------
 3 files changed, 28 insertions(+), 15 deletions(-)

drivers/gpu/drm/i915/i915_debugfs.c +2 -0

index 61ea8da..a79f83c 100644
@@ -2971,6 +2971,8 @@ static int i9xx_pipe_crc_auto_source(struct drm_device *dev, enum pipe pipe,
 				break;
 			}
 			break;
+		default:
+			break;
 		}
 	}
 	drm_modeset_unlock_all(dev);

drivers/gpu/drm/i915/intel_display.c +10 -0

index 16750c4..83b8771 100644
@@ -6269,6 +6269,8 @@ static int i9xx_crtc_mode_set(struct intel_crtc *crtc,
 		case INTEL_OUTPUT_DSI:
 			is_dsi = true;
 			break;
+		default:
+			break;
 		}
 
 		num_connectors++;

@@ -6597,6 +6599,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
 			if (enc_to_dig_port(&encoder->base)->port == PORT_A)
 				has_cpu_edp = true;
 			break;
+		default:
+			break;
 		}
 	}
 

@@ -6901,6 +6905,8 @@ static void lpt_init_pch_refclk(struct drm_device *dev)
 		case INTEL_OUTPUT_ANALOG:
 			has_vga = true;
 			break;
+		default:
+			break;
 		}
 	}
 

@@ -6934,6 +6940,8 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
 			break;
+		default:
+			break;
 		}
 		num_connectors++;
 	}

@@ -7188,6 +7196,8 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		case INTEL_OUTPUT_HDMI:
 			is_sdvo = true;
 			break;
+		default:
+			break;
 		}
 
 		num_connectors++;

drivers/gpu/drm/i915/intel_drv.h +16 -15

index bf3267c..deb1bc6 100644
@@ -94,18 +94,20 @@
 
 /* these are outputs from the chip - integrated only
    external chips are via DVO or SDVO output */
-#define INTEL_OUTPUT_UNUSED 0
-#define INTEL_OUTPUT_ANALOG 1
-#define INTEL_OUTPUT_DVO 2
-#define INTEL_OUTPUT_SDVO 3
-#define INTEL_OUTPUT_LVDS 4
-#define INTEL_OUTPUT_TVOUT 5
-#define INTEL_OUTPUT_HDMI 6
-#define INTEL_OUTPUT_DISPLAYPORT 7
-#define INTEL_OUTPUT_EDP 8
-#define INTEL_OUTPUT_DSI 9
-#define INTEL_OUTPUT_UNKNOWN 10
-#define INTEL_OUTPUT_DP_MST 11
+enum intel_output_type {
+	INTEL_OUTPUT_UNUSED = 0,
+	INTEL_OUTPUT_ANALOG = 1,
+	INTEL_OUTPUT_DVO = 2,
+	INTEL_OUTPUT_SDVO = 3,
+	INTEL_OUTPUT_LVDS = 4,
+	INTEL_OUTPUT_TVOUT = 5,
+	INTEL_OUTPUT_HDMI = 6,
+	INTEL_OUTPUT_DISPLAYPORT = 7,
+	INTEL_OUTPUT_EDP = 8,
+	INTEL_OUTPUT_DSI = 9,
+	INTEL_OUTPUT_UNKNOWN = 10,
+	INTEL_OUTPUT_DP_MST = 11,
+};
 
 #define INTEL_DVO_CHIP_NONE 0
 #define INTEL_DVO_CHIP_LVDS 1

@@ -136,7 +138,7 @@ struct intel_encoder {
 	 */
 	struct intel_crtc *new_crtc;
 
-	int type;
+	enum intel_output_type type;
 	unsigned int cloneable;
 	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);

@@ -387,8 +389,7 @@ struct intel_crtc_config {
 	bool dp_encoder_is_mst;
 	int pbn;
 
-	/* This should be INTEL_OUTPUT_*. */
-	int ddi_personality;
+	enum intel_output_type ddi_personality;
 };
 
 struct intel_pipe_wm {

Signature · 6 lines

2.1.1

_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

From: Paulo Zanoni <[email protected]>

On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP). If no connector is connected, we consider the encoder type to be INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set modes on disconnected connectors, so when we try to set a mode on an INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up printing a WARN. So on this patch, we introduce pipe_config->ddi_personality to help with that.

When the user space sets a mode on a connector, we check the connector type and match it against intel_encoder->type and set the DDI personality accordingly. Then, after the compute config stage is over, we properly assign the personality to intel_encoder->type.

This patch was previously called "drm/i915: Set the digital port encoder personality during modeset".

References: http://patchwork.freedesktop.org/patch/17838/
Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Credits-to: Damien Lespiau <[email protected]> (for the previous versions of the patch).

Cc Damien Lespiau <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 3 files changed, 100 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +52 -0

index cb5367c..5cdc2f4 100644
@@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
 	intel_dp_encoder_destroy(encoder);
 }
 
+static bool intel_ddi_set_personality(struct intel_encoder *encoder,
+				      struct intel_crtc_config *pipe_config)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct intel_connector *connector;
+	int connectors = 0;
+	enum port port = intel_ddi_get_encoder_port(encoder);
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+
+		if (connector->new_encoder != encoder)
+			continue;
+
+		connectors++;
+		if (WARN_ON(connectors > 1))
+			return false;
+
+		switch (connector->base.connector_type) {
+		case DRM_MODE_CONNECTOR_HDMIA:
+		case DRM_MODE_CONNECTOR_HDMIB:
+			pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
+			break;
+		case DRM_MODE_CONNECTOR_DisplayPort:
+			pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
+			break;
+		case DRM_MODE_CONNECTOR_eDP:
+			pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
+			break;
+		default:
+			WARN(1, "DRM connector type %d\n",
+			     connector->base.connector_type);
+			return false;
+		}
+
+		if (encoder->type == pipe_config->ddi_personality)
+			continue;
+
+		/* We expect eDP to always have encoder->type correctly set, so
+		 * it shouldn't reach this point. */
+		if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
+			DRM_ERROR("DDI %c, type %s marked as eDP\n",
+				   port_name(port),
+				   intel_output_name(encoder->type));
+			return false;
+		}
+
+		/*
+		 * We can't change the DDI type if we already have a connected
+		 * device on this port. The first time a DDI is used though
+		 * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+		 * connector to be connected (either trought the kernel command

drivers/gpu/drm/i915/intel_ddi.c +32 -3

+
+		DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
+			      port_name(port),
+			      intel_output_name(pipe_config->ddi_personality));
+	}
+
+	return true;
+
+}
+
+void intel_ddi_commit_personality(struct intel_crtc *crtc)
+{
+	struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
+
+	switch (encoder->type) {
+	case INTEL_OUTPUT_HDMI:
+	case INTEL_OUTPUT_DISPLAYPORT:
+	case INTEL_OUTPUT_EDP:
+	case INTEL_OUTPUT_UNKNOWN:
+		encoder->type = crtc->config.ddi_personality;
+		break;
+	case INTEL_OUTPUT_ANALOG:
+		break;
+	default:
+		WARN(1, "Output type %s\n", intel_output_name(encoder->type));
+		break;
+	}
+}
+
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
 				     struct intel_crtc_config *pipe_config)
 {
-	int type = encoder->type;
 	int port = intel_ddi_get_encoder_port(encoder);
 
-	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+	if (!intel_ddi_set_personality(encoder, pipe_config))
+		return false;
 
 	if (port == PORT_A)
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-	if (type == INTEL_OUTPUT_HDMI)
+	if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
 		return intel_dp_compute_config(encoder, pipe_config);

drivers/gpu/drm/i915/intel_display.c +0 -0

index b5dbc88..16750c4 100644

drivers/gpu/drm/i915/intel_display.c +1 -0

+
 	if (!intel_ddi_pll_select(crtc))
 		return -EINVAL;
 

drivers/gpu/drm/i915/intel_drv.h +4 -0

index 5ab813c..bf3267c 100644
@@ -386,6 +386,9 @@ struct intel_crtc_config {
 
 	bool dp_encoder_is_mst;
 	int pbn;
+
+	/* This should be INTEL_OUTPUT_*. */
+	int ddi_personality;
 };
 
 struct intel_pipe_wm {

@@ -817,6 +820,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
 			 struct intel_crtc_config *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
+void intel_ddi_commit_personality(struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,

Signature · 1 line

2.1.1

Quoted 99 lines

From: Paulo Zanoni <[email protected]>

On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP). If no connector is connected, we consider the encoder type to be INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set modes on disconnected connectors, so when we try to set a mode on an INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up printing a WARN. So on this patch, we introduce pipe_config->ddi_personality to help with that.

When the user space sets a mode on a connector, we check the connector type and match it against intel_encoder->type and set the DDI personality accordingly. Then, after the compute config stage is over, we properly assign the personality to intel_encoder->type.

This patch was previously called "drm/i915: Set the digital port encoder personality during modeset".

References: http://patchwork.freedesktop.org/patch/17838/
Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Credits-to: Damien Lespiau <[email protected]> (for the previous versions of the patch).

Cc Damien Lespiau <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 3 files changed, 100 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +59 -0

index cb5367c..5cdc2f4 100644
@@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
      intel_dp_encoder_destroy(encoder);
 }
 
+static bool intel_ddi_set_personality(struct intel_encoder *encoder,
+                                   struct intel_crtc_config *pipe_config)
+{
+     struct drm_device *dev = encoder->base.dev;
+     struct intel_connector *connector;
+     int connectors = 0;
+     enum port port = intel_ddi_get_encoder_port(encoder);
+
+     list_for_each_entry(connector, &dev->mode_config.connector_list,
+                         base.head) {
+
+             if (connector->new_encoder != encoder)
+                     continue;
+
+             connectors++;
+             if (WARN_ON(connectors > 1))
+                     return false;
+
+             switch (connector->base.connector_type) {
+             case DRM_MODE_CONNECTOR_HDMIA:
+             case DRM_MODE_CONNECTOR_HDMIB:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
+                     break;
+             case DRM_MODE_CONNECTOR_DisplayPort:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
+                     break;
+             case DRM_MODE_CONNECTOR_eDP:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
+                     break;
+             default:
+                     WARN(1, "DRM connector type %d\n",
+                          connector->base.connector_type);
+                     return false;
+             }
+
+             if (encoder->type == pipe_config->ddi_personality)
+                     continue;
+
+             /* We expect eDP to always have encoder->type correctly set, so
+              * it shouldn't reach this point. */
+             if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
+                     DRM_ERROR("DDI %c, type %s marked as eDP\n",
+                                port_name(port),
+                                intel_output_name(encoder->type));
+                     return false;
+             }
+
+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command
+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 3 lines

drivers/gpu/drm/i915/intel_ddi.c +3 -0

+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Quoted 53 lines

drivers/gpu/drm/i915/intel_ddi.c +32 -3

+
+             DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
+                           port_name(port),
+                           intel_output_name(pipe_config->ddi_personality));
+     }
+
+     return true;
+
+}
+
+void intel_ddi_commit_personality(struct intel_crtc *crtc)
+{
+     struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
+
+     switch (encoder->type) {
+     case INTEL_OUTPUT_HDMI:
+     case INTEL_OUTPUT_DISPLAYPORT:
+     case INTEL_OUTPUT_EDP:
+     case INTEL_OUTPUT_UNKNOWN:
+             encoder->type = crtc->config.ddi_personality;
+             break;
+     case INTEL_OUTPUT_ANALOG:
+             break;
+     default:
+             WARN(1, "Output type %s\n", intel_output_name(encoder->type));
+             break;
+     }
+}
+
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
                                   struct intel_crtc_config *pipe_config)
 {
-     int type = encoder->type;
      int port = intel_ddi_get_encoder_port(encoder);
 
-     WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+     if (!intel_ddi_set_personality(encoder, pipe_config))
+             return false;
 
      if (port == PORT_A)
              pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-     if (type == INTEL_OUTPUT_HDMI)
+     if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
              return intel_hdmi_compute_config(encoder, pipe_config);
      else
              return intel_dp_compute_config(encoder, pipe_config);

drivers/gpu/drm/i915/intel_display.c +0 -0

index b5dbc88..16750c4 100644

drivers/gpu/drm/i915/intel_display.c static int haswell_crtc_mode_set(struct intel_crtc *crtc, · line 7813

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -7810,6 +7810,8 @@ static int haswell_crtc_mode_set(struct intel_crtc *crtc,
                               int x, int y,
                               struct drm_framebuffer *fb)
 {
+     intel_ddi_commit_personality(crtc);

This will conflict with Ander's in-flight patches to completely remove the modeset callback. But I'm not really sure

Also I'm not sure whether we should keep updating encoder->type now that we have ddi_personality - tracking the same state in two places usually leads to bugs. Imo it's better to switch all existing encoder->type checks in the ddi code over to check config->ddi_personality. We might need a prep patch to also set the ddi_personality to FDI for the vga output on hsw. Thinking about this, a separate enum might look pretty for this. Or just match the bitfield enum of the register.

Also I think we should have state readout support for ddi_personality just for paranoia.

-Daniel

Quoted 27 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

+
      if (!intel_ddi_pll_select(crtc))
              return -EINVAL;
 

drivers/gpu/drm/i915/intel_drv.h +4 -1

index 5ab813c..bf3267c 100644
@@ -386,6 +386,9 @@ struct intel_crtc_config {
 
      bool dp_encoder_is_mst;
      int pbn;
+
+     /* This should be INTEL_OUTPUT_*. */
+     int ddi_personality;
 };
 
 struct intel_pipe_wm {

@@ -817,6 +820,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
                       struct intel_crtc_config *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
+void intel_ddi_commit_personality(struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
--

Quoted 2 lines

2.1.1

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

2014-10-28 5:49 GMT-02:00 Daniel Vetter <[email protected]>:

Quoted 100 lines · Paulo Zanoni

Quoted 99 lines

From: Paulo Zanoni <[email protected]>

On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP). If no connector is connected, we consider the encoder type to be INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set modes on disconnected connectors, so when we try to set a mode on an INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up printing a WARN. So on this patch, we introduce pipe_config->ddi_personality to help with that.

When the user space sets a mode on a connector, we check the connector type and match it against intel_encoder->type and set the DDI personality accordingly. Then, after the compute config stage is over, we properly assign the personality to intel_encoder->type.

This patch was previously called "drm/i915: Set the digital port encoder personality during modeset".

References: http://patchwork.freedesktop.org/patch/17838/
Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Credits-to: Damien Lespiau <[email protected]> (for the previous versions of the patch).

Cc Damien Lespiau <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 3 files changed, 100 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +59 -0

index cb5367c..5cdc2f4 100644
@@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
      intel_dp_encoder_destroy(encoder);
 }
 
+static bool intel_ddi_set_personality(struct intel_encoder *encoder,
+                                   struct intel_crtc_config *pipe_config)
+{
+     struct drm_device *dev = encoder->base.dev;
+     struct intel_connector *connector;
+     int connectors = 0;
+     enum port port = intel_ddi_get_encoder_port(encoder);
+
+     list_for_each_entry(connector, &dev->mode_config.connector_list,
+                         base.head) {
+
+             if (connector->new_encoder != encoder)
+                     continue;
+
+             connectors++;
+             if (WARN_ON(connectors > 1))
+                     return false;
+
+             switch (connector->base.connector_type) {
+             case DRM_MODE_CONNECTOR_HDMIA:
+             case DRM_MODE_CONNECTOR_HDMIB:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
+                     break;
+             case DRM_MODE_CONNECTOR_DisplayPort:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
+                     break;
+             case DRM_MODE_CONNECTOR_eDP:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
+                     break;
+             default:
+                     WARN(1, "DRM connector type %d\n",
+                          connector->base.connector_type);
+                     return false;
+             }
+
+             if (encoder->type == pipe_config->ddi_personality)
+                     continue;
+
+             /* We expect eDP to always have encoder->type correctly set, so
+              * it shouldn't reach this point. */
+             if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
+                     DRM_ERROR("DDI %c, type %s marked as eDP\n",
+                                port_name(port),
+                                intel_output_name(encoder->type));
+                     return false;
+             }
+
+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command
+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),

Quoted 8 lines

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 3 lines

drivers/gpu/drm/i915/intel_ddi.c +3 -0

+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Which patch? Please tell me the email subject, or patchwork link.

Quoted 61 lines

Quoted 53 lines

drivers/gpu/drm/i915/intel_ddi.c +32 -3

+
+             DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
+                           port_name(port),
+                           intel_output_name(pipe_config->ddi_personality));
+     }
+
+     return true;
+
+}
+
+void intel_ddi_commit_personality(struct intel_crtc *crtc)
+{
+     struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
+
+     switch (encoder->type) {
+     case INTEL_OUTPUT_HDMI:
+     case INTEL_OUTPUT_DISPLAYPORT:
+     case INTEL_OUTPUT_EDP:
+     case INTEL_OUTPUT_UNKNOWN:
+             encoder->type = crtc->config.ddi_personality;
+             break;
+     case INTEL_OUTPUT_ANALOG:
+             break;
+     default:
+             WARN(1, "Output type %s\n", intel_output_name(encoder->type));
+             break;
+     }
+}
+
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
                                   struct intel_crtc_config *pipe_config)
 {
-     int type = encoder->type;
      int port = intel_ddi_get_encoder_port(encoder);
 
-     WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+     if (!intel_ddi_set_personality(encoder, pipe_config))
+             return false;
 
      if (port == PORT_A)
              pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-     if (type == INTEL_OUTPUT_HDMI)
+     if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
              return intel_hdmi_compute_config(encoder, pipe_config);
      else
              return intel_dp_compute_config(encoder, pipe_config);

drivers/gpu/drm/i915/intel_display.c +0 -0

index b5dbc88..16750c4 100644

drivers/gpu/drm/i915/intel_display.c static int haswell_crtc_mode_set(struct intel_crtc *crtc, · line 7813

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -7810,6 +7810,8 @@ static int haswell_crtc_mode_set(struct intel_crtc *crtc,
                               int x, int y,
                               struct drm_framebuffer *fb)
 {
+     intel_ddi_commit_personality(crtc);

This will conflict with Ander's in-flight patches to completely remove the modeset callback. But I'm not really sure

Quoted 8 lines

Also I'm not sure whether we should keep updating encoder->type now that we have ddi_personality - tracking the same state in two places usually leads to bugs. Imo it's better to switch all existing encoder->type checks in the ddi code over to check config->ddi_personality. We might need a prep patch to also set the ddi_personality to FDI for the vga output on hsw. Thinking about this, a separate enum might look pretty for this. Or just match the bitfield enum of the register.

The problem is that intel_display.c also has checks for encoder types... I'm not 100% sure, but I think we just can't go with just ddi_personality.

Quoted 4 lines

Also I think we should have state readout support for ddi_personality just for paranoia.

And how exactly would you implement that? Doesn't make too much sense for me...

Quoted 99 lines

From: Paulo Zanoni <[email protected]>

On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP). If no connector is connected, we consider the encoder type to be INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set modes on disconnected connectors, so when we try to set a mode on an INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up printing a WARN. So on this patch, we introduce pipe_config->ddi_personality to help with that.

When the user space sets a mode on a connector, we check the connector type and match it against intel_encoder->type and set the DDI personality accordingly. Then, after the compute config stage is over, we properly assign the personality to intel_encoder->type.

This patch was previously called "drm/i915: Set the digital port encoder personality during modeset".

References: http://patchwork.freedesktop.org/patch/17838/
Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Credits-to: Damien Lespiau <[email protected]> (for the previous versions of the patch).

Cc Damien Lespiau <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 3 files changed, 100 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +59 -0

index cb5367c..5cdc2f4 100644
@@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
      intel_dp_encoder_destroy(encoder);
 }
 
+static bool intel_ddi_set_personality(struct intel_encoder *encoder,
+                                   struct intel_crtc_config *pipe_config)
+{
+     struct drm_device *dev = encoder->base.dev;
+     struct intel_connector *connector;
+     int connectors = 0;
+     enum port port = intel_ddi_get_encoder_port(encoder);
+
+     list_for_each_entry(connector, &dev->mode_config.connector_list,
+                         base.head) {
+
+             if (connector->new_encoder != encoder)
+                     continue;
+
+             connectors++;
+             if (WARN_ON(connectors > 1))
+                     return false;
+
+             switch (connector->base.connector_type) {
+             case DRM_MODE_CONNECTOR_HDMIA:
+             case DRM_MODE_CONNECTOR_HDMIB:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
+                     break;
+             case DRM_MODE_CONNECTOR_DisplayPort:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
+                     break;
+             case DRM_MODE_CONNECTOR_eDP:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
+                     break;
+             default:
+                     WARN(1, "DRM connector type %d\n",
+                          connector->base.connector_type);
+                     return false;
+             }
+
+             if (encoder->type == pipe_config->ddi_personality)
+                     continue;
+
+             /* We expect eDP to always have encoder->type correctly set, so
+              * it shouldn't reach this point. */
+             if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
+                     DRM_ERROR("DDI %c, type %s marked as eDP\n",
+                                port_name(port),
+                                intel_output_name(encoder->type));
+                     return false;
+             }
+
+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command
+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 3 lines

drivers/gpu/drm/i915/intel_ddi.c +3 -0

+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Quoted 53 lines

drivers/gpu/drm/i915/intel_ddi.c +32 -3

+
+             DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
+                           port_name(port),
+                           intel_output_name(pipe_config->ddi_personality));
+     }
+
+     return true;
+
+}
+
+void intel_ddi_commit_personality(struct intel_crtc *crtc)
+{
+     struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
+
+     switch (encoder->type) {
+     case INTEL_OUTPUT_HDMI:
+     case INTEL_OUTPUT_DISPLAYPORT:
+     case INTEL_OUTPUT_EDP:
+     case INTEL_OUTPUT_UNKNOWN:
+             encoder->type = crtc->config.ddi_personality;
+             break;
+     case INTEL_OUTPUT_ANALOG:
+             break;
+     default:
+             WARN(1, "Output type %s\n", intel_output_name(encoder->type));
+             break;
+     }
+}
+
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
                                   struct intel_crtc_config *pipe_config)
 {
-     int type = encoder->type;
      int port = intel_ddi_get_encoder_port(encoder);
 
-     WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+     if (!intel_ddi_set_personality(encoder, pipe_config))
+             return false;
 
      if (port == PORT_A)
              pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-     if (type == INTEL_OUTPUT_HDMI)
+     if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
              return intel_hdmi_compute_config(encoder, pipe_config);
      else
              return intel_dp_compute_config(encoder, pipe_config);

drivers/gpu/drm/i915/intel_display.c +0 -0

index b5dbc88..16750c4 100644

drivers/gpu/drm/i915/intel_display.c static int haswell_crtc_mode_set(struct intel_crtc *crtc, · line 7813

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -7810,6 +7810,8 @@ static int haswell_crtc_mode_set(struct intel_crtc *crtc,
                               int x, int y,
                               struct drm_framebuffer *fb)
 {
+     intel_ddi_commit_personality(crtc);

This will conflict with Ander's in-flight patches to completely remove the modeset callback. But I'm not really sure

Also I'm not sure whether we should keep updating encoder->type now that we have ddi_personality - tracking the same state in two places usually leads to bugs. Imo it's better to switch all existing encoder->type checks in the ddi code over to check config->ddi_personality. We might need a prep patch to also set the ddi_personality to FDI for the vga output on hsw. Thinking about this, a separate enum might look pretty for this. Or just match the bitfield enum of the register.

Also I think we should have state readout support for ddi_personality just for paranoia.
-Daniel

Quoted 27 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

+
      if (!intel_ddi_pll_select(crtc))
              return -EINVAL;
 

drivers/gpu/drm/i915/intel_drv.h +4 -1

index 5ab813c..bf3267c 100644
@@ -386,6 +386,9 @@ struct intel_crtc_config {
 
      bool dp_encoder_is_mst;
      int pbn;
+
+     /* This should be INTEL_OUTPUT_*. */
+     int ddi_personality;
 };
 
 struct intel_pipe_wm {

@@ -817,6 +820,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
                       struct intel_crtc_config *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
+void intel_ddi_commit_personality(struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
--

Quoted 2 lines

2.1.1

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

2014-10-28 5:49 GMT-02:00 Daniel Vetter <[email protected]>:

Quoted 100 lines · Paulo Zanoni

Quoted 99 lines

From: Paulo Zanoni <[email protected]>

On HSW+, one encoder (DDI) can have multiple connectors (HDMI and DP). If no connector is connected, we consider the encoder type to be INTEL_OUTPUT_UNKNOWN. The problem is that we allow user space to set modes on disconnected connectors, so when we try to set a mode on an INTEL_OUTPUT_UNKNOWN connector, we don't know what to do and end up printing a WARN. So on this patch, we introduce pipe_config->ddi_personality to help with that.

When the user space sets a mode on a connector, we check the connector type and match it against intel_encoder->type and set the DDI personality accordingly. Then, after the compute config stage is over, we properly assign the personality to intel_encoder->type.

This patch was previously called "drm/i915: Set the digital port encoder personality during modeset".

References: http://patchwork.freedesktop.org/patch/17838/
Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Credits-to: Damien Lespiau <[email protected]> (for the previous versions of the patch).

Cc Damien Lespiau <[email protected]>

Cc Daniel Vetter <[email protected]>

Signed-off-by Paulo Zanoni <[email protected]>

---
 drivers/gpu/drm/i915/intel_ddi.c     | 97 ++++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 +
 drivers/gpu/drm/i915/intel_drv.h     |  4 ++
 3 files changed, 100 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_ddi.c +59 -0

index cb5367c..5cdc2f4 100644
@@ -1557,18 +1557,109 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
      intel_dp_encoder_destroy(encoder);
 }
 
+static bool intel_ddi_set_personality(struct intel_encoder *encoder,
+                                   struct intel_crtc_config *pipe_config)
+{
+     struct drm_device *dev = encoder->base.dev;
+     struct intel_connector *connector;
+     int connectors = 0;
+     enum port port = intel_ddi_get_encoder_port(encoder);
+
+     list_for_each_entry(connector, &dev->mode_config.connector_list,
+                         base.head) {
+
+             if (connector->new_encoder != encoder)
+                     continue;
+
+             connectors++;
+             if (WARN_ON(connectors > 1))
+                     return false;
+
+             switch (connector->base.connector_type) {
+             case DRM_MODE_CONNECTOR_HDMIA:
+             case DRM_MODE_CONNECTOR_HDMIB:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_HDMI;
+                     break;
+             case DRM_MODE_CONNECTOR_DisplayPort:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_DISPLAYPORT;
+                     break;
+             case DRM_MODE_CONNECTOR_eDP:
+                     pipe_config->ddi_personality = INTEL_OUTPUT_EDP;
+                     break;
+             default:
+                     WARN(1, "DRM connector type %d\n",
+                          connector->base.connector_type);
+                     return false;
+             }
+
+             if (encoder->type == pipe_config->ddi_personality)
+                     continue;
+
+             /* We expect eDP to always have encoder->type correctly set, so
+              * it shouldn't reach this point. */
+             if (pipe_config->ddi_personality == INTEL_OUTPUT_EDP) {
+                     DRM_ERROR("DDI %c, type %s marked as eDP\n",
+                                port_name(port),
+                                intel_output_name(encoder->type));
+                     return false;
+             }
+
+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command
+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),

Quoted 8 lines

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 3 lines

drivers/gpu/drm/i915/intel_ddi.c +3 -0

+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Ville's patch solves a different problem. I just reviewed it, but we still need the check above. The code above is in case, for example, there's a DP connector actually connected (but without a mode set), and then the user tries to set a mode on the HDMI connector of this encoder.

Quoted 108 lines

Quoted 54 lines

drivers/gpu/drm/i915/intel_ddi.c +32 -3

+
+             DRM_DEBUG_KMS("Setting DDI %c personality to %s\n",
+                           port_name(port),
+                           intel_output_name(pipe_config->ddi_personality));
+     }
+
+     return true;
+
+}
+
+void intel_ddi_commit_personality(struct intel_crtc *crtc)
+{
+     struct intel_encoder *encoder = intel_ddi_get_crtc_encoder(&crtc->base);
+
+     switch (encoder->type) {
+     case INTEL_OUTPUT_HDMI:
+     case INTEL_OUTPUT_DISPLAYPORT:
+     case INTEL_OUTPUT_EDP:
+     case INTEL_OUTPUT_UNKNOWN:
+             encoder->type = crtc->config.ddi_personality;
+             break;
+     case INTEL_OUTPUT_ANALOG:
+             break;
+     default:
+             WARN(1, "Output type %s\n", intel_output_name(encoder->type));
+             break;
+     }
+}
+
 static bool intel_ddi_compute_config(struct intel_encoder *encoder,
                                   struct intel_crtc_config *pipe_config)
 {
-     int type = encoder->type;
      int port = intel_ddi_get_encoder_port(encoder);
 
-     WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+     if (!intel_ddi_set_personality(encoder, pipe_config))
+             return false;
 
      if (port == PORT_A)
              pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-     if (type == INTEL_OUTPUT_HDMI)
+     if (pipe_config->ddi_personality == INTEL_OUTPUT_HDMI)
              return intel_hdmi_compute_config(encoder, pipe_config);
      else
              return intel_dp_compute_config(encoder, pipe_config);

drivers/gpu/drm/i915/intel_display.c +0 -0

index b5dbc88..16750c4 100644

drivers/gpu/drm/i915/intel_display.c static int haswell_crtc_mode_set(struct intel_crtc *crtc, · line 7813

Quoted 4 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

@@ -7810,6 +7810,8 @@ static int haswell_crtc_mode_set(struct intel_crtc *crtc,
                               int x, int y,
                               struct drm_framebuffer *fb)
 {
+     intel_ddi_commit_personality(crtc);

This will conflict with Ander's in-flight patches to completely remove the modeset callback. But I'm not really sure

Also I'm not sure whether we should keep updating encoder->type now that we have ddi_personality - tracking the same state in two places usually leads to bugs. Imo it's better to switch all existing encoder->type checks in the ddi code over to check config->ddi_personality. We might need a prep patch to also set the ddi_personality to FDI for the vga output on hsw. Thinking about this, a separate enum might look pretty for this. Or just match the bitfield enum of the register.

Also I think we should have state readout support for ddi_personality just for paranoia.
-Daniel

Quoted 27 lines

drivers/gpu/drm/i915/intel_display.c +1 -0

+
      if (!intel_ddi_pll_select(crtc))
              return -EINVAL;
 

drivers/gpu/drm/i915/intel_drv.h +4 -1

index 5ab813c..bf3267c 100644
@@ -386,6 +386,9 @@ struct intel_crtc_config {
 
      bool dp_encoder_is_mst;
      int pbn;
+
+     /* This should be INTEL_OUTPUT_*. */
+     int ddi_personality;
 };
 
 struct intel_pipe_wm {

@@ -817,6 +820,7 @@ void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder);
 void intel_ddi_clock_get(struct intel_encoder *encoder,
                       struct intel_crtc_config *pipe_config);
 void intel_ddi_set_vc_payload_alloc(struct drm_crtc *crtc, bool state);
+void intel_ddi_commit_personality(struct intel_crtc *crtc);
 
 /* intel_frontbuffer.c */
 void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj,
--

Quoted 2 lines

2.1.1

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

Signature · 5 lines

Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Quoted 21 lines · Paulo Zanoni

Quoted 20 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_ddi.c +5 -0

+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 10 lines

drivers/gpu/drm/i915/intel_ddi.c +10 -0

+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),
+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Quoted 5 lines

Ville's patch solves a different problem. I just reviewed it, but we still need the check above. The code above is in case, for example, there's a DP connector actually connected (but without a mode set), and then the user tries to set a mode on the HDMI connector of this encoder.

Should we even care about that issue? Forcing a HDMI mode on a port even when there's a DP monitor plugged in does make it easier to test things without having to yank out the cables all the time. Also your patch wouldn't fix that problem for non-ddi platforms.

I would suggest that we should allow this behaviour unless there's some risk of causing problems to the sink. Another reason for rejecting it would be if aux would stop working, but I don't think that should be the case since we can do both gmbus and aux during probing anyway.

Quoted 22 lines · Ville Syrjälä

Quoted 21 lines · Paulo Zanoni

Quoted 20 lines

Quoted 5 lines

drivers/gpu/drm/i915/intel_ddi.c +5 -0

+             /*
+              * We can't change the DDI type if we already have a connected
+              * device on this port. The first time a DDI is used though
+              * (encoder_type is INTEL_OUTPUT_UNKNOWN) and we force a
+              * connector to be connected (either trought the kernel command

drivers/gpu/drm/i915/intel_ddi.c static void intel_ddi_destroy(struct drm_encoder *encoder) · line 1621

Quoted 10 lines

drivers/gpu/drm/i915/intel_ddi.c +10 -0

+              * line or KMS) we need to comply.
+              */
+              if (encoder->type != INTEL_OUTPUT_UNKNOWN &&
+                  connector->base.status == connector_status_connected) {
+                      DRM_DEBUG_KMS("Can't set DDI %c personality to %s, it has a connected %s device\n",
+                                     port_name(port),
+                                     intel_output_name(encoder->type),
+                                     intel_output_name(pipe_config->ddi_personality));
+                      return false;
+             }

I think this part is better done with Ville's more general "do we have both hdmi and dp on the same dig_port?" check. Care to review Ville's patch instead? Thomas Wood is signed up for it on the review board but I guess you can steal that task.

Quoted 5 lines

Quoted 5 lines

Ville's patch solves a different problem. I just reviewed it, but we still need the check above. The code above is in case, for example, there's a DP connector actually connected (but without a mode set), and then the user tries to set a mode on the HDMI connector of this encoder.

My comment was _only_ about the check quoted above, the other part of the loop is imo the entire point of adding ddi_personality.