[PATCH] drm/i915/crt: Keep the EDID for the whole detect cycle

[PATCH] drm/i915/crt: Keep the EDID for the whole detect cycle

[Chris Wilson]

Other than reducing the number of EDID reads required for probing the
modes on a connector, this refactor has the importance of centralising
the logic for deciding when we need to probe for an analogue output via
DVI-I and then consistently using that information between detection and
mode computation.

References: https://bugs.freedesktop.org/show_bug.cgi?id=97971
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 98 +++++++++++++++-------------------------
 1 file changed, 36 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 88ebbdde185a..b9b88b0b73e7 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -459,8 +459,8 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 	return ret;
 }
 
-static struct edid *intel_crt_get_edid(struct drm_connector *connector,
-				struct i2c_adapter *i2c)
+static struct edid *__intel_crt_get_edid(struct drm_connector *connector,
+					 struct i2c_adapter *i2c)
 {
 	struct edid *edid;
 
@@ -473,59 +473,50 @@ static struct edid *intel_crt_get_edid(struct drm_connector *connector,
 		intel_gmbus_force_bit(i2c, false);
 	}
 
-	return edid;
-}
-
-/* local version of intel_ddc_get_modes() to use intel_crt_get_edid() */
-static int intel_crt_ddc_get_modes(struct drm_connector *connector,
-				struct i2c_adapter *adapter)
-{
-	struct edid *edid;
-	int ret;
-
-	edid = intel_crt_get_edid(connector, adapter);
-	if (!edid)
-		return 0;
-
-	ret = intel_connector_update_modes(connector, edid);
-	kfree(edid);
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+		DRM_DEBUG_KMS("EDID reports a digital output, ignoring for this VGA connector\n");
+		kfree(edid);
+		edid = NULL;
+	}
 
-	return ret;
+	return edid;
 }
 
-static bool intel_crt_detect_ddc(struct drm_connector *connector)
+static struct edid *intel_crt_get_edid(struct drm_connector *connector)
 {
-	struct intel_crt *crt = intel_attached_crt(connector);
-	struct drm_i915_private *dev_priv = to_i915(crt->base.base.dev);
-	struct edid *edid;
+#define HAS_DVI_I(p) 1 /* XXX anything with HDMI, i.e. g33+ */
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct i2c_adapter *i2c;
-
-	BUG_ON(crt->base.type != INTEL_OUTPUT_ANALOG);
+	struct edid *edid;
 
 	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
-	edid = intel_crt_get_edid(connector, i2c);
-
-	if (edid) {
-		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
-
+	edid = __intel_crt_get_edid(connector, i2c);
+	if (!edid && HAS_DVI_I(dev_priv)) {
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
 		 */
-		if (!is_digital) {
-			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
-			return true;
-		}
-
-		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
-	} else {
-		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [no valid EDID found]\n");
+		i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
+		edid = __intel_crt_get_edid(connector, i2c);
 	}
 
-	kfree(edid);
+	return edid;
+}
+
+static bool intel_crt_detect_ddc(struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	kfree(to_intel_connector(connector)->detect_edid);
+	to_intel_connector(connector)->detect_edid = NULL;
 
-	return false;
+	edid = intel_crt_get_edid(connector);
+	if (!edid)
+		return false;
+
+	to_intel_connector(connector)->detect_edid = edid;
+	return true;
 }
 
 static enum drm_connector_status
@@ -727,30 +718,13 @@ static void intel_crt_destroy(struct drm_connector *connector)
 
 static int intel_crt_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crt *crt = intel_attached_crt(connector);
-	struct intel_encoder *intel_encoder = &crt->base;
-	enum intel_display_power_domain power_domain;
-	int ret;
-	struct i2c_adapter *i2c;
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
-	ret = intel_crt_ddc_get_modes(connector, i2c);
-	if (ret || !IS_G4X(dev))
-		goto out;
-
-	/* Try to probe digital port for output in DVI-I -> VGA mode. */
-	i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
-	ret = intel_crt_ddc_get_modes(connector, i2c);
+	struct edid *edid;
 
-out:
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (!edid)
+		return 0;
 
-	return ret;
+	return intel_connector_update_modes(connector, edid);
 }
 
 static int intel_crt_set_property(struct drm_connector *connector,
-- 
2.9.3

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

✗ Fi.CI.BAT: warning for drm/i915/crt: Keep the EDID for the whole detect cycle

[Patchwork]

== Series Details ==

Series: drm/i915/crt: Keep the EDID for the whole detect cycle
URL   : https://patchwork.freedesktop.org/series/13066/
State : warning

== Summary ==

Series 13066v1 drm/i915/crt: Keep the EDID for the whole detect cycle
https://patchwork.freedesktop.org/api/1.0/series/13066/revisions/1/mbox/

Test drv_module_reload_basic:
                pass       -> DMESG-WARN (fi-snb-2520m)
Test kms_psr_sink_crc:
        Subgroup psr_basic:
                dmesg-warn -> PASS       (fi-skl-6700hq)

fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
fi-bxt-t5700     total:244  pass:214  dwarn:0   dfail:0   fail:0   skip:30 
fi-byt-j1900     total:244  pass:210  dwarn:2   dfail:0   fail:1   skip:31 
fi-hsw-4770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-hsw-4770r     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ilk-650       total:244  pass:182  dwarn:0   dfail:0   fail:2   skip:60 
fi-ivb-3520m     total:244  pass:219  dwarn:0   dfail:0   fail:0   skip:25 
fi-ivb-3770      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6700k     total:244  pass:219  dwarn:1   dfail:0   fail:0   skip:24 
fi-skl-6770hq    total:244  pass:227  dwarn:2   dfail:0   fail:1   skip:14 
fi-snb-2520m     total:244  pass:207  dwarn:1   dfail:0   fail:0   skip:36 
fi-snb-2600      total:244  pass:207  dwarn:0   dfail:0   fail:0   skip:37 
fi-byt-n2820 failed to connect after reboot

Results at /archive/results/CI_IGT_test/Patchwork_2593/

994488193b917b39e41d30889c7828073988ed13 drm-intel-nightly: 2016y-09m-29d-09h-58m-47s UTC integration manifest
64e342c drm/i915/crt: Keep the EDID for the whole detect cycle

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

Re: [PATCH] drm/i915/crt: Keep the EDID for the whole detect cycle

[Imre Deak]

On to, 2016-09-29 at 11:52 +0100, Chris Wilson wrote:
> Other than reducing the number of EDID reads required for probing the
> modes on a connector, this refactor has the importance of centralising
> the logic for deciding when we need to probe for an analogue output via
> DVI-I and then consistently using that information between detection and
> mode computation.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=97971
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>

kfree(detect_edid) during connector destroy.
Also shouldn't connector forcing also force rereading the EDID?

Otherwise looks ok to me.

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 98 +++++++++++++++-------------------------
>  1 file changed, 36 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 88ebbdde185a..b9b88b0b73e7 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -459,8 +459,8 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
>  	return ret;
>  }
>  
> -static struct edid *intel_crt_get_edid(struct drm_connector *connector,
> -				struct i2c_adapter *i2c)
> +static struct edid *__intel_crt_get_edid(struct drm_connector *connector,
> +					 struct i2c_adapter *i2c)
>  {
>  	struct edid *edid;
>  
> @@ -473,59 +473,50 @@ static struct edid *intel_crt_get_edid(struct drm_connector *connector,
>  		intel_gmbus_force_bit(i2c, false);
>  	}
>  
> -	return edid;
> -}
> -
> -/* local version of intel_ddc_get_modes() to use intel_crt_get_edid() */
> -static int intel_crt_ddc_get_modes(struct drm_connector *connector,
> -				struct i2c_adapter *adapter)
> -{
> -	struct edid *edid;
> -	int ret;
> -
> -	edid = intel_crt_get_edid(connector, adapter);
> -	if (!edid)
> -		return 0;
> -
> -	ret = intel_connector_update_modes(connector, edid);
> -	kfree(edid);
> +	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
> +		DRM_DEBUG_KMS("EDID reports a digital output, ignoring for this VGA connector\n");
> +		kfree(edid);
> +		edid = NULL;
> +	}
>  
> -	return ret;
> +	return edid;
>  }
>  
> -static bool intel_crt_detect_ddc(struct drm_connector *connector)
> +static struct edid *intel_crt_get_edid(struct drm_connector *connector)
>  {
> -	struct intel_crt *crt = intel_attached_crt(connector);
> -	struct drm_i915_private *dev_priv = to_i915(crt->base.base.dev);
> -	struct edid *edid;
> +#define HAS_DVI_I(p) 1 /* XXX anything with HDMI, i.e. g33+ */
> +	struct drm_i915_private *dev_priv = to_i915(connector->dev);
>  	struct i2c_adapter *i2c;
> -
> -	BUG_ON(crt->base.type != INTEL_OUTPUT_ANALOG);
> +	struct edid *edid;
>  
>  	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
> -	edid = intel_crt_get_edid(connector, i2c);
> -
> -	if (edid) {
> -		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
> -
> +	edid = __intel_crt_get_edid(connector, i2c);
> +	if (!edid && HAS_DVI_I(dev_priv)) {
>  		/*
>  		 * This may be a DVI-I connector with a shared DDC
>  		 * link between analog and digital outputs, so we
>  		 * have to check the EDID input spec of the attached device.
>  		 */
> -		if (!is_digital) {
> -			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
> -			return true;
> -		}
> -
> -		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
> -	} else {
> -		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [no valid EDID found]\n");
> +		i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
> +		edid = __intel_crt_get_edid(connector, i2c);
>  	}
>  
> -	kfree(edid);
> +	return edid;
> +}
> +
> +static bool intel_crt_detect_ddc(struct drm_connector *connector)
> +{
> +	struct edid *edid;
> +
> +	kfree(to_intel_connector(connector)->detect_edid);
> +	to_intel_connector(connector)->detect_edid = NULL;
>  
> -	return false;
> +	edid = intel_crt_get_edid(connector);
> +	if (!edid)
> +		return false;
> +
> +	to_intel_connector(connector)->detect_edid = edid;
> +	return true;
>  }
>  
>  static enum drm_connector_status
> @@ -727,30 +718,13 @@ static void intel_crt_destroy(struct drm_connector *connector)
>  
>  static int intel_crt_get_modes(struct drm_connector *connector)
>  {
> -	struct drm_device *dev = connector->dev;
> -	struct drm_i915_private *dev_priv = to_i915(dev);
> -	struct intel_crt *crt = intel_attached_crt(connector);
> -	struct intel_encoder *intel_encoder = &crt->base;
> -	enum intel_display_power_domain power_domain;
> -	int ret;
> -	struct i2c_adapter *i2c;
> -
> -	power_domain = intel_display_port_power_domain(intel_encoder);
> -	intel_display_power_get(dev_priv, power_domain);
> -
> -	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
> -	ret = intel_crt_ddc_get_modes(connector, i2c);
> -	if (ret || !IS_G4X(dev))
> -		goto out;
> -
> -	/* Try to probe digital port for output in DVI-I -> VGA mode. */
> -	i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
> -	ret = intel_crt_ddc_get_modes(connector, i2c);
> +	struct edid *edid;
>  
> -out:
> -	intel_display_power_put(dev_priv, power_domain);
> +	edid = to_intel_connector(connector)->detect_edid;
> +	if (!edid)
> +		return 0;
>  
> -	return ret;
> +	return intel_connector_update_modes(connector, edid);
>  }
>  
>  static int intel_crt_set_property(struct drm_connector *connector,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915/crt: Keep the EDID for the whole detect cycle

[Chris Wilson]

Other than reducing the number of EDID reads required for probing the
modes on a connector, this refactor has the importance of centralising
the logic for deciding when we need to probe for an analogue output via
DVI-I and then consistently using that information between detection and
mode computation.

References: https://bugs.freedesktop.org/show_bug.cgi?id=97971
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 108 +++++++++++++++------------------------
 1 file changed, 41 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 385e29af8baa..a94b6c981fea 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -459,8 +459,8 @@ static bool intel_crt_detect_hotplug(struct drm_connector *connector)
 	return ret;
 }
 
-static struct edid *intel_crt_get_edid(struct drm_connector *connector,
-				struct i2c_adapter *i2c)
+static struct edid *__intel_crt_get_edid(struct drm_connector *connector,
+					 struct i2c_adapter *i2c)
 {
 	struct edid *edid;
 
@@ -473,59 +473,50 @@ static struct edid *intel_crt_get_edid(struct drm_connector *connector,
 		intel_gmbus_force_bit(i2c, false);
 	}
 
-	return edid;
-}
-
-/* local version of intel_ddc_get_modes() to use intel_crt_get_edid() */
-static int intel_crt_ddc_get_modes(struct drm_connector *connector,
-				struct i2c_adapter *adapter)
-{
-	struct edid *edid;
-	int ret;
-
-	edid = intel_crt_get_edid(connector, adapter);
-	if (!edid)
-		return 0;
-
-	ret = intel_connector_update_modes(connector, edid);
-	kfree(edid);
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+		DRM_DEBUG_KMS("EDID reports a digital output, ignoring for this VGA connector\n");
+		kfree(edid);
+		edid = NULL;
+	}
 
-	return ret;
+	return edid;
 }
 
-static bool intel_crt_detect_ddc(struct drm_connector *connector)
+static struct edid *intel_crt_get_edid(struct drm_connector *connector)
 {
-	struct intel_crt *crt = intel_attached_crt(connector);
-	struct drm_i915_private *dev_priv = to_i915(crt->base.base.dev);
-	struct edid *edid;
+#define HAS_DVI_I(p) 1 /* XXX anything with HDMI, i.e. g33+ */
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
 	struct i2c_adapter *i2c;
-
-	BUG_ON(crt->base.type != INTEL_OUTPUT_ANALOG);
+	struct edid *edid;
 
 	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
-	edid = intel_crt_get_edid(connector, i2c);
-
-	if (edid) {
-		bool is_digital = edid->input & DRM_EDID_INPUT_DIGITAL;
-
+	edid = __intel_crt_get_edid(connector, i2c);
+	if (!edid && HAS_DVI_I(dev_priv)) {
 		/*
 		 * This may be a DVI-I connector with a shared DDC
 		 * link between analog and digital outputs, so we
 		 * have to check the EDID input spec of the attached device.
 		 */
-		if (!is_digital) {
-			DRM_DEBUG_KMS("CRT detected via DDC:0x50 [EDID]\n");
-			return true;
-		}
-
-		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [EDID reports a digital panel]\n");
-	} else {
-		DRM_DEBUG_KMS("CRT not detected via DDC:0x50 [no valid EDID found]\n");
+		i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
+		edid = __intel_crt_get_edid(connector, i2c);
 	}
 
-	kfree(edid);
+	return edid;
+}
+
+static bool intel_crt_detect_ddc(struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	kfree(to_intel_connector(connector)->detect_edid);
+	to_intel_connector(connector)->detect_edid = NULL;
 
-	return false;
+	edid = intel_crt_get_edid(connector);
+	if (!edid)
+		return false;
+
+	to_intel_connector(connector)->detect_edid = edid;
+	return true;
 }
 
 static enum drm_connector_status
@@ -691,6 +682,11 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
+	if (intel_crt_detect_ddc(connector)) {
+		status = connector_status_connected;
+		goto out;
+	}
+
 	if (I915_HAS_HOTPLUG(dev_priv)) {
 		/* We can not rely on the HPD pin always being correctly wired
 		 * up, for example many KVM do not pass it through, and so
@@ -704,11 +700,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
 	}
 
-	if (intel_crt_detect_ddc(connector)) {
-		status = connector_status_connected;
-		goto out;
-	}
-
 	/* Load detection is broken on HPD capable machines. Whoever wants a
 	 * broken monitor (without edid) to work behind a broken kvm (that fails
 	 * to have the right resistors for HP detection) needs to fix this up.
@@ -756,30 +747,13 @@ static void intel_crt_destroy(struct drm_connector *connector)
 
 static int intel_crt_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = to_i915(dev);
-	struct intel_crt *crt = intel_attached_crt(connector);
-	struct intel_encoder *intel_encoder = &crt->base;
-	enum intel_display_power_domain power_domain;
-	int ret;
-	struct i2c_adapter *i2c;
-
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	i2c = intel_gmbus_get_adapter(dev_priv, dev_priv->vbt.crt_ddc_pin);
-	ret = intel_crt_ddc_get_modes(connector, i2c);
-	if (ret || !IS_G4X(dev_priv))
-		goto out;
-
-	/* Try to probe digital port for output in DVI-I -> VGA mode. */
-	i2c = intel_gmbus_get_adapter(dev_priv, GMBUS_PIN_DPB);
-	ret = intel_crt_ddc_get_modes(connector, i2c);
+	struct edid *edid;
 
-out:
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (!edid)
+		return 0;
 
-	return ret;
+	return intel_connector_update_modes(connector, edid);
 }
 
 static int intel_crt_set_property(struct drm_connector *connector,
-- 
2.11.0

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