[PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <damien.lespiau@intel.com> (for the
previous versions of the patch).
Cc: Damien Lespiau <damien.lespiau@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index cb5367c..5cdc2f4 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -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),
+					intel_output_name(pipe_config->ddi_personality));
+			 return false;
+		}
+
+		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);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b5dbc88..16750c4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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);
+
 	if (!intel_ddi_pll_select(crtc))
 		return -EINVAL;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5ab813c..bf3267c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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,
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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 <paulo.r.zanoni@intel.com>
---
 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(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 61ea8da..a79f83c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -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);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16750c4..83b8771 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -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++;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bf3267c..deb1bc6 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -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 {
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter]

On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <damien.lespiau@intel.com> (for the
> previous versions of the patch).
> Cc: Damien Lespiau <damien.lespiau@intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index cb5367c..5cdc2f4 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -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),
> +					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.

> +
> +		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);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b5dbc88..16750c4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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

> +
>  	if (!intel_ddi_pll_select(crtc))
>  		return -EINVAL;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 5ab813c..bf3267c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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,
> -- 
> 2.1.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: transform INTEL_OUTPUT_* into an enum

[Daniel Vetter]

On Mon, Oct 27, 2014 at 05:47:52PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> 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 <paulo.r.zanoni@intel.com>

I like this, so went right ahead and applied it and resolved the little
rebase conflict.
-Daniel

> ---
>  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(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 61ea8da..a79f83c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -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);
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 16750c4..83b8771 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -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++;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bf3267c..deb1bc6 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -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 {
> -- 
> 2.1.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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Paulo Zanoni]

2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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 <damien.lespiau@intel.com> (for the
>> previous versions of the patch).
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index cb5367c..5cdc2f4 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -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),
>> +                                     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.


>
>> +
>> +             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);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b5dbc88..16750c4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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.
>

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.


> 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...

> -Daniel
>
>> +
>>       if (!intel_ddi_pll_select(crtc))
>>               return -EINVAL;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 5ab813c..bf3267c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -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,
>> --
>> 2.1.1
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter]

On Tue, Oct 28, 2014 at 08:46:21AM -0200, Paulo Zanoni wrote:
> 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> 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 <damien.lespiau@intel.com> (for the
> >> previous versions of the patch).
> >> Cc: Damien Lespiau <damien.lespiau@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  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(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index cb5367c..5cdc2f4 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -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),
> >> +                                     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.

drm/i915: Reject modeset when the same digital port is used more than once

> >
> >> +
> >> +             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);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index b5dbc88..16750c4 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -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.
> >
> 
> 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.

On a quick check I've spotted just one that would apply to hsw/bdw, and
that can be replaced with a check to config->has_dp_encoder. We've come a
long way with properly tracking all the insane details in the pipe config,
at least on modern platforms ;-) vlv/gmch platforms are a different beast.
> 
> 
> > 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...


diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2688bc940879..3ee83464bb83 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1511,11 +1511,15 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 	case TRANS_DDI_MODE_SELECT_HDMI:
 		pipe_config->has_hdmi_sink = true;
 	case TRANS_DDI_MODE_SELECT_DVI:
+		pipe_config->ddi_personality = DDI_HDMI;
+		break;
 	case TRANS_DDI_MODE_SELECT_FDI:
+		pipe_config->ddi_personality = DDI_FDI;
 		break;
 	case TRANS_DDI_MODE_SELECT_DP_SST:
 	case TRANS_DDI_MODE_SELECT_DP_MST:
 		pipe_config->has_dp_encoder = true;
+		pipe_config->ddi_personality = DDI_DP;
 		intel_dp_get_m_n(intel_crtc, pipe_config);
 		break;
 	default:
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ee982f5412d6..481a05622f24 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10322,6 +10322,7 @@ intel_pipe_config_compare(struct drm_device *dev,
 	PIPE_CONF_CHECK_I(double_wide);
 
 	PIPE_CONF_CHECK_X(ddi_pll_sel);
+	PIPE_CONF_CHECK_I(ddi_personality);
 
 	PIPE_CONF_CHECK_I(shared_dpll);
 	PIPE_CONF_CHECK_X(dpll_hw_state.dpll);
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Paulo Zanoni]

2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> 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 <damien.lespiau@intel.com> (for the
>> previous versions of the patch).
>> Cc: Damien Lespiau <damien.lespiau@intel.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  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(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index cb5367c..5cdc2f4 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -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),
>> +                                     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.

>
>> +
>> +             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);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index b5dbc88..16750c4 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -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
>
>> +
>>       if (!intel_ddi_pll_select(crtc))
>>               return -EINVAL;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 5ab813c..bf3267c 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -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,
>> --
>> 2.1.1
>>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Ville Syrjälä]

On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote:
> 2014-10-28 5:49 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>:
> > On Mon, Oct 27, 2014 at 05:47:51PM -0200, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> 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 <damien.lespiau@intel.com> (for the
> >> previous versions of the patch).
> >> Cc: Damien Lespiau <damien.lespiau@intel.com>
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  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(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> >> index cb5367c..5cdc2f4 100644
> >> --- a/drivers/gpu/drm/i915/intel_ddi.c
> >> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> >> @@ -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),
> >> +                                     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.

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.

> 
> >
> >> +
> >> +             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);
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> >> index b5dbc88..16750c4 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -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
> >
> >> +
> >>       if (!intel_ddi_pll_select(crtc))
> >>               return -EINVAL;
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> >> index 5ab813c..bf3267c 100644
> >> --- a/drivers/gpu/drm/i915/intel_drv.h
> >> +++ b/drivers/gpu/drm/i915/intel_drv.h
> >> @@ -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,
> >> --
> >> 2.1.1
> >>
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter]

On Tue, Oct 28, 2014 at 04:30:41PM +0200, Ville Syrjälä wrote:
> On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote:
> > >> +             /*
> > >> +              * 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),
> > >> +                                     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.

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

> 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.

This is useful for injection arbitrary modeset configs, something Thomas
Wood is slowly chipping away on. And I think we definitely want that for
increased automated test coverage.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: introduce pipe_config->ddi_personality

[Daniel Vetter]

On Mon, Nov 03, 2014 at 01:15:24PM +0100, Daniel Vetter wrote:
> On Tue, Oct 28, 2014 at 04:30:41PM +0200, Ville Syrjälä wrote:
> > On Tue, Oct 28, 2014 at 11:26:51AM -0200, Paulo Zanoni wrote:
> > > >> +             /*
> > > >> +              * 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),
> > > >> +                                     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.
> 
> My comment was _only_ about the check quoted above, the other part of the
> loop is imo the entire point of adding ddi_personality.

Ok, misunderstood what this does - I've thought it's just checking that we
don't use both hdmi and DP on the same ddi encoder. Imo that's all we need
to check really.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx