[PATCH 1/3] drm/i915: don't set modes for 2 connectors on the same encoder

[PATCH 1/3] drm/i915: don't set modes for 2 connectors on the same encoder

[Paulo Zanoni]

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

In some cases we have more than 1 connector associated to an encoder
(e.g., SDVO, Haswell DP/HDMI) and we can only set a mode for one of
these connectors. If we only allowed modesets for connected connectors
we would never need this patch, but since we do allow modeset for
disconnected connectors we may see user space trying to set modes on
the two connectors attached to the same encoder, so we need to forbid
that.

This problem can be reproduced by running the following
intel-gpu-tools test case:
  ./kms_setmode --run-subtest clone-exclusive-crtc

Thanks to Daniel Vetter for providing a version of this patch on
pastebin.

Credits-to: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a562eef..7cc67b3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -9922,17 +9922,21 @@ intel_modeset_stage_output_state(struct drm_device *dev,
 	/* Check for any encoders that needs to be disabled. */
 	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
 			    base.head) {
+		int num_connectors = 0;
 		list_for_each_entry(connector,
 				    &dev->mode_config.connector_list,
 				    base.head) {
 			if (connector->new_encoder == encoder) {
 				WARN_ON(!connector->new_encoder->new_crtc);
-
-				goto next_encoder;
+				num_connectors++;
 			}
 		}
-		encoder->new_crtc = NULL;
-next_encoder:
+
+		if (num_connectors == 0)
+			encoder->new_crtc = NULL;
+		else if (num_connectors > 1)
+			return -EINVAL;
+
 		/* Only now check for crtc changes so we don't miss encoders
 		 * that will be disabled. */
 		if (&encoder->new_crtc->base != encoder->base.crtc) {
-- 
1.8.4.2

[PATCH 2/3] drm/i915: Introduce new intel_output_name()

[Paulo Zanoni]

From: Damien Lespiau <damien.lespiau@intel.com>

That we can use for debugging purposes.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7cc67b3..034952c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10253,6 +10253,19 @@ static bool has_edp_a(struct drm_device *dev)
 	return true;
 }
 
+const char *intel_output_name(int output)
+{
+	static const char *names[] = {
+		"Unused", "Analog", "DVO", "SDVO", "LVDS", "TV", "HDMI",
+		"DisplayPort", "eDP", "DSI", "Unknown"
+	};
+
+	if (output < 0 || output >= ARRAY_SIZE(names))
+		return "Invalid";
+
+	return names[output];
+}
+
 static void intel_setup_outputs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..25bbbb5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 
 
 /* intel_display.c */
+const char *intel_output_name(int output);
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-- 
1.8.4.2

[PATCH 3/3] drm/i915: Set the digital port encoder personality during modeset

[Paulo Zanoni]

From: Damien Lespiau <damien.lespiau@intel.com>

The ->detect() vfunc of connectors is usually responsible for setting the
encoder type on intel_digital_ports when a hotplug event happens.

However we sometimes want to force a modeset on a specific connector:
  - the user can ask the SETCRTC ioctl to use a connector that isn't
    marked as connected (because we never received a hotplug event for it).
    This can be also used in tests to do a modeset without the need of a
    plugged-in monitor.
  - the command line video= option can be used to force modesets, eg.:
    video=HDMI-A-1:1024x768e

So, before we try to do anything with the DDI encoder as part of a modeset,
we need to ensure that the personality of the encoder matches the selected
connector.

v2: Made by Paulo:
  - Return false if we have more than one connector
  - WARN in case it's not eDP/DP/HDMI
  - Don't break error strings in pieces
  - Change the "status == connected" message from DRM_ERROR to
    DRM_DEBUG_KMS
  - Don't store+use the old encoder->type at compute_config
  - Add bugzilla reference
  - Add testcase reference
v3: Made by Paulo:
  - WARN in case "connectors > 1", since we have a patch that should
    catch this case earlier

Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 94 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 91 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 1488b28..4ec1665 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1512,18 +1512,106 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
 	intel_dp_encoder_destroy(encoder);
 }
 
+/*
+ * The ->detect() vfunc of connectors is usually responsible for setting the
+ * encoder type on intel_digital_ports when a hotplug event happens.
+ *
+ * However we sometimes want to force a modeset on a specific connector:
+ *   - the user can ask the SETCRTC ioctl to use a connector that isn't marked
+ *     as connected (because we never received a hotplug event for it).
+ *     This can be also used in tests to do a modeset without the need of a
+ *     plugged-in monitor.
+ *   - the command line video= option can be used to force modesets, eg.:
+ *     video=HDMI-A-1:1024x768e
+ *
+ * So, before we try to do anything with the DDI encoder as part of a modeset,
+ * we need to ensure that the personality of the encoder matches the selected
+ * connector.
+ */
+static bool
+intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct intel_connector *connector;
+	int connectors = 0;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+		int connector_type, old_encoder_type, new_encoder_type;
+		int port;
+
+		if (connector->new_encoder != encoder)
+			continue;
+
+		connectors++;
+		if (WARN_ON(connectors > 1))
+			return false;
+
+		connector_type = connector->base.connector_type;
+		old_encoder_type = encoder->type;
+		switch (connector_type) {
+		case DRM_MODE_CONNECTOR_HDMIA:
+		case DRM_MODE_CONNECTOR_HDMIB:
+			new_encoder_type = INTEL_OUTPUT_HDMI;
+			break;
+		case DRM_MODE_CONNECTOR_DisplayPort:
+			new_encoder_type = INTEL_OUTPUT_DISPLAYPORT;
+			break;
+		case DRM_MODE_CONNECTOR_eDP:
+			continue;
+		default:
+			WARN(1, "DRM connector type %d\n", connector_type);
+			continue;
+		}
+
+		if (old_encoder_type == new_encoder_type)
+			continue;
+
+		port = intel_ddi_get_encoder_port(encoder);
+
+		if (old_encoder_type == INTEL_OUTPUT_EDP) {
+			DRM_ERROR("Can't change DDI %c personality to %s, it's an eDP DDI\n",
+				  port_name(port),
+				  intel_output_name(new_encoder_type));
+			return false;
+		}
+
+		if (connector->base.status == connector_status_connected) {
+			DRM_DEBUG_KMS("Can't change DDI %c personality to %s, it has a connected %s device\n",
+				      port_name(port),
+				      intel_output_name(new_encoder_type),
+				      intel_output_name(old_encoder_type));
+			return false;
+		}
+
+		DRM_DEBUG_KMS("Changing DDI %c from %s to %s\n",
+			      port_name(port),
+			      intel_output_name(old_encoder_type),
+			      intel_output_name(new_encoder_type));
+
+		encoder->type = new_encoder_type;
+	}
+
+	return true;
+}
+
 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);
+	int ret;
+
+	ret = intel_ddi_ensure_encoder_type(encoder);
+	if (!ret)
+		return false;
 
-	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+	WARN(encoder->type == INTEL_OUTPUT_UNKNOWN,
+	     "compute_config() on unknown output!\n");
 
 	if (port == PORT_A)
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-	if (type == INTEL_OUTPUT_HDMI)
+	if (encoder->type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
 		return intel_dp_compute_config(encoder, pipe_config);
-- 
1.8.4.2

Re: [PATCH 2/3] drm/i915: Introduce new intel_output_name()

[Jani Nikula]

On Tue, 07 Jan 2014, Paulo Zanoni <przanoni@gmail.com> wrote:
> From: Damien Lespiau <damien.lespiau@intel.com>
>
> That we can use for debugging purposes.
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 13 +++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 7cc67b3..034952c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10253,6 +10253,19 @@ static bool has_edp_a(struct drm_device *dev)
>  	return true;
>  }
>  
> +const char *intel_output_name(int output)
> +{
> +	static const char *names[] = {
> +		"Unused", "Analog", "DVO", "SDVO", "LVDS", "TV", "HDMI",
> +		"DisplayPort", "eDP", "DSI", "Unknown"

Please initialize this with designated initializers:

		[INTEL_OUTPUT_UNUSED] = "Unused",
                [INTEL_OUTPUT_ANALOG] = "Analog",
		...

> +	};
> +
> +	if (output < 0 || output >= ARRAY_SIZE(names))

and also check for names[output] != NULL here (just to be defensive with
the designated initializers).

BR,
Jani.

> +		return "Invalid";
> +
> +	return names[output];
> +}
> +
>  static void intel_setup_outputs(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..25bbbb5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  
>  
>  /* intel_display.c */
> +const char *intel_output_name(int output);
>  int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> -- 
> 1.8.4.2
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

[PATCH 2/3 v2] drm/i915: Introduce new intel_output_name()

[Damien Lespiau]

That we can use for debugging purposes.

v2: Use designated initializers for the 'names' array (Paulo Zanoni,
    Jani Nikula).
    Add a check in case the array has a hole (which can now remain
    unnoticed with designated initializers) (Jani Nikula)

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (for v1)
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ba9d62e..40a6247 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10261,6 +10261,28 @@ static bool has_edp_a(struct drm_device *dev)
 	return true;
 }
 
+const char *intel_output_name(int output)
+{
+	static const char *names[] = {
+		[INTEL_OUTPUT_UNUSED] = "Unused",
+		[INTEL_OUTPUT_ANALOG] = "Analog",
+		[INTEL_OUTPUT_DVO] = "DVO",
+		[INTEL_OUTPUT_SDVO] = "SDVO",
+		[INTEL_OUTPUT_LVDS] = "LVDS",
+		[INTEL_OUTPUT_TVOUT] = "TV",
+		[INTEL_OUTPUT_HDMI] = "HDMI",
+		[INTEL_OUTPUT_DISPLAYPORT] = "DisplayPort",
+		[INTEL_OUTPUT_EDP] = "eDP",
+		[INTEL_OUTPUT_DSI] = "DSI",
+		[INTEL_OUTPUT_UNKNOWN] = "Unknown",
+	};
+
+	if (output < 0 || output >= ARRAY_SIZE(names) || !names[output])
+		return "Invalid";
+
+	return names[output];
+}
+
 static void intel_setup_outputs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..25bbbb5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
 
 
 /* intel_display.c */
+const char *intel_output_name(int output);
 int intel_pch_rawclk(struct drm_device *dev);
 void intel_mark_busy(struct drm_device *dev);
 void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
-- 
1.8.3.1

Re: [PATCH 2/3 v2] drm/i915: Introduce new intel_output_name()

[Jani Nikula]

On Wed, 08 Jan 2014, Damien Lespiau <damien.lespiau@intel.com> wrote:
> That we can use for debugging purposes.
>
> v2: Use designated initializers for the 'names' array (Paulo Zanoni,
>     Jani Nikula).
>     Add a check in case the array has a hole (which can now remain
>     unnoticed with designated initializers) (Jani Nikula)
>

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> (for v1)
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 22 ++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 23 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ba9d62e..40a6247 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10261,6 +10261,28 @@ static bool has_edp_a(struct drm_device *dev)
>  	return true;
>  }
>  
> +const char *intel_output_name(int output)
> +{
> +	static const char *names[] = {
> +		[INTEL_OUTPUT_UNUSED] = "Unused",
> +		[INTEL_OUTPUT_ANALOG] = "Analog",
> +		[INTEL_OUTPUT_DVO] = "DVO",
> +		[INTEL_OUTPUT_SDVO] = "SDVO",
> +		[INTEL_OUTPUT_LVDS] = "LVDS",
> +		[INTEL_OUTPUT_TVOUT] = "TV",
> +		[INTEL_OUTPUT_HDMI] = "HDMI",
> +		[INTEL_OUTPUT_DISPLAYPORT] = "DisplayPort",
> +		[INTEL_OUTPUT_EDP] = "eDP",
> +		[INTEL_OUTPUT_DSI] = "DSI",
> +		[INTEL_OUTPUT_UNKNOWN] = "Unknown",
> +	};
> +
> +	if (output < 0 || output >= ARRAY_SIZE(names) || !names[output])
> +		return "Invalid";
> +
> +	return names[output];
> +}
> +
>  static void intel_setup_outputs(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..25bbbb5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -625,6 +625,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>  
>  
>  /* intel_display.c */
> +const char *intel_output_name(int output);
>  int intel_pch_rawclk(struct drm_device *dev);
>  void intel_mark_busy(struct drm_device *dev);
>  void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> -- 
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/3] drm/i915: don't set modes for 2 connectors on the same encoder

[Damien Lespiau]

On Tue, Jan 07, 2014 at 02:55:53PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> In some cases we have more than 1 connector associated to an encoder
> (e.g., SDVO, Haswell DP/HDMI) and we can only set a mode for one of
> these connectors. If we only allowed modesets for connected connectors
> we would never need this patch, but since we do allow modeset for
> disconnected connectors we may see user space trying to set modes on
> the two connectors attached to the same encoder, so we need to forbid
> that.
> 
> This problem can be reproduced by running the following
> intel-gpu-tools test case:
>   ./kms_setmode --run-subtest clone-exclusive-crtc
> 
> Thanks to Daniel Vetter for providing a version of this patch on
> pastebin.
> 
> Credits-to: Daniel Vetter <daniel.vetter@ffwll.ch>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

The comment telling what the block does could use a little update, in
any case:

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

I can't review patch 3/3 as I wrote a part of it, we need to find a
victim.

-- 
Damien

> ---
>  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a562eef..7cc67b3 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -9922,17 +9922,21 @@ intel_modeset_stage_output_state(struct drm_device *dev,
>  	/* Check for any encoders that needs to be disabled. */
>  	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
>  			    base.head) {
> +		int num_connectors = 0;
>  		list_for_each_entry(connector,
>  				    &dev->mode_config.connector_list,
>  				    base.head) {
>  			if (connector->new_encoder == encoder) {
>  				WARN_ON(!connector->new_encoder->new_crtc);
> -
> -				goto next_encoder;
> +				num_connectors++;
>  			}
>  		}
> -		encoder->new_crtc = NULL;
> -next_encoder:
> +
> +		if (num_connectors == 0)
> +			encoder->new_crtc = NULL;
> +		else if (num_connectors > 1)
> +			return -EINVAL;
> +
>  		/* Only now check for crtc changes so we don't miss encoders
>  		 * that will be disabled. */
>  		if (&encoder->new_crtc->base != encoder->base.crtc) {
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Set the digital port encoder personality during modeset

[Damien Lespiau]

On Tue, Jan 07, 2014 at 02:55:55PM -0200, Paulo Zanoni wrote:
> +static bool
> +intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)

[...]

> +		if (connector->base.status == connector_status_connected) {
> +			DRM_DEBUG_KMS("Can't change DDI %c personality to %s, it has a connected %s device\n",
> +				      port_name(port),
> +				      intel_output_name(new_encoder_type),
> +				      intel_output_name(old_encoder_type));
> +			return false;
> +		}

Actually, this test should be:

                if (old_encoder_type != INTEL_OUTPUT_UNKNOWN &&
                    connector->base.status == connector_status_connected) {

Otherwise we miss the case I wanted the patch for in the first place: Forcing a
not-yet-sexed DDI connector (type == UNKNOWN) on the command line, say.
video=HDMI-A-1:1024x768e. In that case, we end up with type == UNKNOWN and
connector->base.status == connector_status_connected, but still want to force
the DDI personality to HDMI.

--
Damien

[PATCH 3/3 v4] drm/i915: Set the digital port encoder personality during modeset

[Damien Lespiau]

The ->detect() vfunc of connectors is usually responsible for setting the
encoder type on intel_digital_ports when a hotplug event happens.

However we sometimes want to force a modeset on a specific connector:
  - the user can ask the SETCRTC ioctl to use a connector that isn't
    marked as connected (because we never received a hotplug event for it).
    This can be also used in tests to do a modeset without the need of a
    plugged-in monitor.
  - the command line video= option can be used to force modesets, eg.:
    video=HDMI-A-1:1024x768e

So, before we try to do anything with the DDI encoder as part of a modeset,
we need to ensure that the personality of the encoder matches the selected
connector.

v2: Made by Paulo:
  - Return false if we have more than one connector
  - WARN in case it's not eDP/DP/HDMI
  - Don't break error strings in pieces
  - Change the "status == connected" message from DRM_ERROR to
    DRM_DEBUG_KMS
  - Don't store+use the old encoder->type at compute_config
  - Add bugzilla reference
  - Add testcase reference
v3: Made by Paulo:
  - WARN in case "connectors > 1", since we have a patch that should
    catch this case earlier
v4: Authorize unknown encoders with a "connected" connector to change
    personality (Damien)

Testcase: igt/kms_setmode/clone-exclusive-crtc
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c | 102 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 99 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 74749c6..79776f8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1518,18 +1518,114 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
 	intel_dp_encoder_destroy(encoder);
 }
 
+/*
+ * The ->detect() vfunc of connectors is usually responsible for setting the
+ * encoder type on intel_digital_ports when a hotplug event happens.
+ *
+ * However we sometimes want to force a modeset on a specific connector:
+ *   - the user can ask the SETCRTC ioctl to use a connector that isn't marked
+ *     as connected (because we never received a hotplug event for it).
+ *     This can be also used in tests to do a modeset without the need of a
+ *     plugged-in monitor.
+ *   - the command line video= option can be used to force modesets, eg.:
+ *     video=HDMI-A-1:1024x768e
+ *
+ * So, before we try to do anything with the DDI encoder as part of a modeset,
+ * we need to ensure that the personality of the encoder matches the selected
+ * connector.
+ */
+static bool
+intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)
+{
+	struct drm_device *dev = encoder->base.dev;
+	struct intel_connector *connector;
+	int connectors = 0;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list,
+			    base.head) {
+		int connector_type, old_encoder_type, new_encoder_type;
+		int port;
+
+		if (connector->new_encoder != encoder)
+			continue;
+
+		connectors++;
+		if (WARN_ON(connectors > 1))
+			return false;
+
+		connector_type = connector->base.connector_type;
+		old_encoder_type = encoder->type;
+		switch (connector_type) {
+		case DRM_MODE_CONNECTOR_HDMIA:
+		case DRM_MODE_CONNECTOR_HDMIB:
+			new_encoder_type = INTEL_OUTPUT_HDMI;
+			break;
+		case DRM_MODE_CONNECTOR_DisplayPort:
+			new_encoder_type = INTEL_OUTPUT_DISPLAYPORT;
+			break;
+		case DRM_MODE_CONNECTOR_eDP:
+			continue;
+		default:
+			WARN(1, "DRM connector type %d\n", connector_type);
+			continue;
+		}
+
+		if (old_encoder_type == new_encoder_type)
+			continue;
+
+		port = intel_ddi_get_encoder_port(encoder);
+
+		if (old_encoder_type == INTEL_OUTPUT_EDP) {
+			DRM_ERROR("Can't change DDI %c personality to %s, it's an eDP DDI\n",
+				  port_name(port),
+				  intel_output_name(new_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 (old_encoder_type != INTEL_OUTPUT_UNKNOWN &&
+		     connector->base.status == connector_status_connected) {
+			DRM_DEBUG_KMS("Can't change DDI %c personality to %s, it has a connected %s device\n",
+				      port_name(port),
+				      intel_output_name(new_encoder_type),
+				      intel_output_name(old_encoder_type));
+			return false;
+		}
+
+		DRM_DEBUG_KMS("Changing DDI %c from %s to %s\n",
+			      port_name(port),
+			      intel_output_name(old_encoder_type),
+			      intel_output_name(new_encoder_type));
+
+		encoder->type = new_encoder_type;
+	}
+
+	return true;
+}
+
 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);
+	int ret;
+
+	ret = intel_ddi_ensure_encoder_type(encoder);
+	if (!ret)
+		return false;
 
-	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
+	WARN(encoder->type == INTEL_OUTPUT_UNKNOWN,
+	     "compute_config() on unknown output!\n");
 
 	if (port == PORT_A)
 		pipe_config->cpu_transcoder = TRANSCODER_EDP;
 
-	if (type == INTEL_OUTPUT_HDMI)
+	if (encoder->type == INTEL_OUTPUT_HDMI)
 		return intel_hdmi_compute_config(encoder, pipe_config);
 	else
 		return intel_dp_compute_config(encoder, pipe_config);
-- 
1.8.3.1

Re: [PATCH 3/3 v4] drm/i915: Set the digital port encoder personality during modeset

[Daniel Vetter]

On Mon, Jan 13, 2014 at 06:03:01PM +0000, Damien Lespiau wrote:
> The ->detect() vfunc of connectors is usually responsible for setting the
> encoder type on intel_digital_ports when a hotplug event happens.
> 
> However we sometimes want to force a modeset on a specific connector:
>   - the user can ask the SETCRTC ioctl to use a connector that isn't
>     marked as connected (because we never received a hotplug event for it).
>     This can be also used in tests to do a modeset without the need of a
>     plugged-in monitor.
>   - the command line video= option can be used to force modesets, eg.:
>     video=HDMI-A-1:1024x768e
> 
> So, before we try to do anything with the DDI encoder as part of a modeset,
> we need to ensure that the personality of the encoder matches the selected
> connector.
> 
> v2: Made by Paulo:
>   - Return false if we have more than one connector
>   - WARN in case it's not eDP/DP/HDMI
>   - Don't break error strings in pieces
>   - Change the "status == connected" message from DRM_ERROR to
>     DRM_DEBUG_KMS
>   - Don't store+use the old encoder->type at compute_config
>   - Add bugzilla reference
>   - Add testcase reference
> v3: Made by Paulo:
>   - WARN in case "connectors > 1", since we have a patch that should
>     catch this case earlier
> v4: Authorize unknown encoders with a "connected" connector to change
>     personality (Damien)
> 
> Testcase: igt/kms_setmode/clone-exclusive-crtc
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
> Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 102 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 99 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 74749c6..79776f8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1518,18 +1518,114 @@ static void intel_ddi_destroy(struct drm_encoder *encoder)
>  	intel_dp_encoder_destroy(encoder);
>  }
>  
> +/*
> + * The ->detect() vfunc of connectors is usually responsible for setting the
> + * encoder type on intel_digital_ports when a hotplug event happens.
> + *
> + * However we sometimes want to force a modeset on a specific connector:
> + *   - the user can ask the SETCRTC ioctl to use a connector that isn't marked
> + *     as connected (because we never received a hotplug event for it).
> + *     This can be also used in tests to do a modeset without the need of a
> + *     plugged-in monitor.
> + *   - the command line video= option can be used to force modesets, eg.:
> + *     video=HDMI-A-1:1024x768e
> + *
> + * So, before we try to do anything with the DDI encoder as part of a modeset,
> + * we need to ensure that the personality of the encoder matches the selected
> + * connector.
> + */
> +static bool
> +intel_ddi_ensure_encoder_type(struct intel_encoder *encoder)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +	struct intel_connector *connector;
> +	int connectors = 0;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list,
> +			    base.head) {
> +		int connector_type, old_encoder_type, new_encoder_type;
> +		int port;
> +
> +		if (connector->new_encoder != encoder)
> +			continue;
> +
> +		connectors++;
> +		if (WARN_ON(connectors > 1))
> +			return false;
> +
> +		connector_type = connector->base.connector_type;
> +		old_encoder_type = encoder->type;
> +		switch (connector_type) {
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			new_encoder_type = INTEL_OUTPUT_HDMI;
> +			break;
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +			new_encoder_type = INTEL_OUTPUT_DISPLAYPORT;
> +			break;
> +		case DRM_MODE_CONNECTOR_eDP:
> +			continue;
> +		default:
> +			WARN(1, "DRM connector type %d\n", connector_type);
> +			continue;
> +		}
> +
> +		if (old_encoder_type == new_encoder_type)
> +			continue;
> +
> +		port = intel_ddi_get_encoder_port(encoder);
> +
> +		if (old_encoder_type == INTEL_OUTPUT_EDP) {
> +			DRM_ERROR("Can't change DDI %c personality to %s, it's an eDP DDI\n",
> +				  port_name(port),
> +				  intel_output_name(new_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 (old_encoder_type != INTEL_OUTPUT_UNKNOWN &&
> +		     connector->base.status == connector_status_connected) {
> +			DRM_DEBUG_KMS("Can't change DDI %c personality to %s, it has a connected %s device\n",
> +				      port_name(port),
> +				      intel_output_name(new_encoder_type),
> +				      intel_output_name(old_encoder_type));
> +			return false;
> +		}
> +
> +		DRM_DEBUG_KMS("Changing DDI %c from %s to %s\n",
> +			      port_name(port),
> +			      intel_output_name(old_encoder_type),
> +			      intel_output_name(new_encoder_type));
> +
> +		encoder->type = new_encoder_type;
> +	}
> +
> +	return true;
> +}
> +
>  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);
> +	int ret;
> +
> +	ret = intel_ddi_ensure_encoder_type(encoder);

Nope, you can't change encoder->type in ->compute_config. Otherwise if we
run the atomic modeset stuff in the "check-only" mode this could wreak
utter havoc, and the goal is that userspace will fully enumerate all
configs using that. So fallout will happen. So we can only commit the
new encoder personality to anything outside of the staging state in
->mode_set.

There's two possible approaches that could work:
- Do a generic encoder personality thing which uses the linked up
  connector in the new_ staging pointers to do all the heavy lifting in
  the compute_config stage. Bit of work, especially since the sdvo output
  doesn't need all that much state really. So feels a bit like overkill
  just for ddi.

- Add a dynamic intel_ddi_staged_personality functions which walks the
  new_ pointers to figure out which personality to use in
  ->compute_config. We'd do the same for sdvo then (which uses bitmasks
  for that purpose). A bit fragile since then we need to carefully review
  every place and decide whether to use this or the old encoder->type.

- Add a ddi_personality field to the pipe config which you set once in
  ddi_compute_config and then use everywhere else. The generic pipe config
  update code will take care of committing the value. encoder->type would
  die.

Personally I'm leaning towards the last approach, but the others should
also work out. Another reason for this approach is that state
cross-checking (which we imo should have) naturally falls out by placing
the ddi personality into the pipe config - we only need to wire up a
little bit of code.

For the first approach we'd need to add a linked_connector out parameter
to encoder->get_config, which will add a bit of ugly code to the core
checker. And for the 2nd approach I don't really have a good idea how to
cross-check it.

Sorry that I didn't jump into this discussion earlier.

Cheers, Daniel

> +	if (!ret)
> +		return false;
>  
> -	WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
> +	WARN(encoder->type == INTEL_OUTPUT_UNKNOWN,
> +	     "compute_config() on unknown output!\n");
>  
>  	if (port == PORT_A)
>  		pipe_config->cpu_transcoder = TRANSCODER_EDP;
>  
> -	if (type == INTEL_OUTPUT_HDMI)
> +	if (encoder->type == INTEL_OUTPUT_HDMI)
>  		return intel_hdmi_compute_config(encoder, pipe_config);
>  	else
>  		return intel_dp_compute_config(encoder, pipe_config);
> -- 
> 1.8.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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