Modeset without a detected monitor on DDI platforms v2.5

Modeset without a detected monitor on DDI platforms v2.5

[Damien Lespiau]

Completely untested, so buggy, I'm sure. In any case, might be of some use.
You're in a much better position to do this, I don't have a DDI machine that
works.

-- 
Damien

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

[Damien Lespiau]

That we can use for debugging purposes.

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 5dff7ca..ec5ae69 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -10213,6 +10213,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 2b5bcb6..dd86961 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -623,6 +623,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

[PATCH 2/2] 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 wasne
    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.

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

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index c8382f5..3ed2e3c 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1442,11 +1442,96 @@ 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;
+
+	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;
+
+		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:
+			new_encoder_type = INTEL_OUTPUT_EDP;
+			continue;
+		default:
+			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_ERROR("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));
+
+		connector->new_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");
 
-- 
1.8.3.1

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

[Paulo Zanoni]

2013/11/29 Damien Lespiau <damien.lespiau@intel.com>:
> That we can use for debugging purposes.
>
> 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 5dff7ca..ec5ae69 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -10213,6 +10213,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"

Optional bikeshed: use designated initializers (not sure if they're
acceptable in the Kernel on arrays...).


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

Optional bikeshed: WARN in case we hit this.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> +               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 2b5bcb6..dd86961 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -623,6 +623,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



-- 
Paulo Zanoni

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

[Paulo Zanoni]

2013/11/29 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 wasne
>     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.

Needs bugzilla references.


>
> Signed-off-by: Damien Lespiau <damien.lespiau@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_ddi.c | 85 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 85 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c8382f5..3ed2e3c 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1442,11 +1442,96 @@ 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;
> +
> +       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;

I'd add a check for 2 connectors on the same encoder here.


> +
> +               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:
> +                       new_encoder_type = INTEL_OUTPUT_EDP;

I don't see a reason for this assignment, since we're "continue"ing here.


> +                       continue;
> +               default:
> +                       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",

I think some people would probably complain about the broken string here.


> +                                 port_name(port),
> +                                 intel_output_name(new_encoder_type));
> +                       return false;
> +               }
> +
> +               if (connector->base.status == connector_status_connected) {
> +                       DRM_ERROR("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));

I guess this means: "your user-space is just insane". I'm not sure if
we want to DRM_ERROR here. Maybe just DRM_DEBUG_KMS, otherwise we'll
get bug reports that we can't fix in the Kernel.


> +                       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));
> +
> +               connector->new_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;
>

You'll still hit the warn here, since "type" was assigned before you
called intel_ddi_ensure_encoder_type.


I took your patch, applied it, fixed the things I pointed above,
tested, and also patched IGT to fix the remaining issue. I'll send the
new version implementing the changes I suggested.


>         WARN(type == INTEL_OUTPUT_UNKNOWN, "compute_config() on unknown output!\n");
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

[PATCH 2/2] 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

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 | 96 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index c8382f5..879fbe8 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1442,18 +1442,108 @@ 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 (connectors > 1) {
+			DRM_DEBUG_KMS("Can't set modes for 2 connectors on the same DDI encoder\n");
+			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.3.1

[PATCH IGT] tests/kms_setmode: avoid 2 connectors on the same encoder

[Paulo Zanoni]

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

Don't try to set modes on two connectors that share the same encoder.
That will just fail.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 tests/kms_setmode.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
index 3d18fc7..e1c9c5a 100644
--- a/tests/kms_setmode.c
+++ b/tests/kms_setmode.c
@@ -275,6 +275,7 @@ static void setup_crtcs(drmModeRes *resources, struct connector_config *cconf,
 	int crtc_count;
 	bool config_valid;
 	int i;
+	int encoder_usage_count[resources->count_encoders];
 
 	i = 0;
 	crtc_count = 0;
@@ -337,6 +338,20 @@ static void setup_crtcs(drmModeRes *resources, struct connector_config *cconf,
 		crtc++;
 	}
 
+	memset(encoder_usage_count, 0, sizeof(encoder_usage_count));
+	for (i = 0; i < connector_count; i++) {
+		drmModeConnector *connector = cconf[i].connector;
+		drmModeEncoder *encoder;
+
+		igt_assert(connector->count_encoders == 1);
+		encoder = drmModeGetEncoder(drm_fd, connector->encoders[0]);
+		encoder_usage_count[get_encoder_idx(resources, encoder)]++;
+		drmModeFreeEncoder(encoder);
+	}
+	for (i = 0; i < resources->count_encoders; i++)
+		if (encoder_usage_count[i] > 1)
+			config_valid = false;
+
 	*crtc_count_ret = crtc_count;
 	*config_valid_ret = config_valid;
 }
-- 
1.8.3.1

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

[Daniel Vetter]

On Tue, Dec 03, 2013 at 03:36:20PM -0200, Paulo Zanoni wrote:
> 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
> 
> 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 | 96 ++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index c8382f5..879fbe8 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1442,18 +1442,108 @@ 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 (connectors > 1) {
> +			DRM_DEBUG_KMS("Can't set modes for 2 connectors on the same DDI encoder\n");
> +			return false;
> +		}

This part should be moved into it's own patch and also moved into the
modeset core - sdvo encoders suffer from the same issue. Even better, if
we put that in the common code we can reject modeset attempts even before
we've started to kill the old config, which is always what we want.
Otherwise we'll never get to the great promised land where modesetting
configurations can be checked non-destructively with the atomic ioctl.

With change I'd simply call the function ddi_set_encoder_type or
something.

Cheers, Daniel
> +
> +		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.3.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

[Paulo Zanoni]

2013/12/3 Daniel Vetter <daniel@ffwll.ch>:
> On Tue, Dec 03, 2013 at 03:36:20PM -0200, Paulo Zanoni wrote:
>> 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
>>
>> 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 | 96 ++++++++++++++++++++++++++++++++++++++--
>>  1 file changed, 93 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>> index c8382f5..879fbe8 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1442,18 +1442,108 @@ 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 (connectors > 1) {
>> +                     DRM_DEBUG_KMS("Can't set modes for 2 connectors on the same DDI encoder\n");
>> +                     return false;
>> +             }
>
> This part should be moved into it's own patch and also moved into the
> modeset core - sdvo encoders suffer from the same issue. Even better, if
> we put that in the common code we can reject modeset attempts even before
> we've started to kill the old config, which is always what we want.
> Otherwise we'll never get to the great promised land where modesetting
> configurations can be checked non-destructively with the atomic ioctl.

See "[PATCH] drm/i915: don't set modes for 2 connectors on the same
encoder": http://lists.freedesktop.org/archives/intel-gfx/2013-November/036771.html
. The implementation is based on something you posted to pastebin when
we were discussing this problem a few months ago.

Even with that patch applied, I think we should keep this current path
as-is since it runs before the code that checks for 2 encoders.

>
> With change I'd simply call the function ddi_set_encoder_type or
> something.
>
> Cheers, Daniel
>> +
>> +             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.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



-- 
Paulo Zanoni

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

[Daniel Vetter]

On Thu, Dec 5, 2013 at 1:38 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Even with that patch applied, I think we should keep this current path
> as-is since it runs before the code that checks for 2 encoders.

->compute_config is called much later after stage_output_state. So I'm
a bit confused why we need both ... If you want we could smash a WARN
or so into compute_config for paranoia, but since we have a proper igt
testcase that's imo not required at all.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH IGT] tests/kms_setmode: avoid 2 connectors on the same encoder

[Imre Deak]

On Tue, 2013-12-03 at 15:38 -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Don't try to set modes on two connectors that share the same encoder.
> That will just fail.
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68463
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Looks ok:
Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  tests/kms_setmode.c | 15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/tests/kms_setmode.c b/tests/kms_setmode.c
> index 3d18fc7..e1c9c5a 100644
> --- a/tests/kms_setmode.c
> +++ b/tests/kms_setmode.c
> @@ -275,6 +275,7 @@ static void setup_crtcs(drmModeRes *resources, struct connector_config *cconf,
>  	int crtc_count;
>  	bool config_valid;
>  	int i;
> +	int encoder_usage_count[resources->count_encoders];
>  
>  	i = 0;
>  	crtc_count = 0;
> @@ -337,6 +338,20 @@ static void setup_crtcs(drmModeRes *resources, struct connector_config *cconf,
>  		crtc++;
>  	}
>  
> +	memset(encoder_usage_count, 0, sizeof(encoder_usage_count));
> +	for (i = 0; i < connector_count; i++) {
> +		drmModeConnector *connector = cconf[i].connector;
> +		drmModeEncoder *encoder;
> +
> +		igt_assert(connector->count_encoders == 1);
> +		encoder = drmModeGetEncoder(drm_fd, connector->encoders[0]);
> +		encoder_usage_count[get_encoder_idx(resources, encoder)]++;
> +		drmModeFreeEncoder(encoder);
> +	}
> +	for (i = 0; i < resources->count_encoders; i++)
> +		if (encoder_usage_count[i] > 1)
> +			config_valid = false;
> +
>  	*crtc_count_ret = crtc_count;
>  	*config_valid_ret = config_valid;
>  }

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

[Paulo Zanoni]

2013/12/5 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Dec 5, 2013 at 1:38 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
>> Even with that patch applied, I think we should keep this current path
>> as-is since it runs before the code that checks for 2 encoders.
>
> ->compute_config is called much later after stage_output_state. So I'm
> a bit confused why we need both ... If you want we could smash a WARN
> or so into compute_config for paranoia, but since we have a proper igt
> testcase that's imo not required at all.

Yeah, I was wrong. I changed to WARN and will resend the 3 patches
that should solve bug 68463.

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



-- 
Paulo Zanoni