[PATCH 1/5] drm/i915/dp: Refactor common eDP lid detection

[PATCH 1/5] drm/i915/dp: Refactor common eDP lid detection

[Chris Wilson]

Both gmch and pch detection routines used the exact same routine for
eDP, so de-duplicate.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: : Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 38 +++++++++++++++++---------------------
 1 file changed, 17 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d2ee403f72da..0aa134ce114a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3725,20 +3725,24 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 }
 
 static enum drm_connector_status
+edp_detect(struct intel_dp *intel_dp)
+{
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	enum drm_connector_status status;
+
+	status = intel_panel_detect(dev);
+	if (status == connector_status_unknown)
+		status = connector_status_connected;
+
+	return status;
+}
+
+static enum drm_connector_status
 ironlake_dp_detect(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	enum drm_connector_status status;
-
-	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp)) {
-		status = intel_panel_detect(dev);
-		if (status == connector_status_unknown)
-			status = connector_status_connected;
-		return status;
-	}
 
 	if (!ibx_digital_port_connected(dev_priv, intel_dig_port))
 		return connector_status_disconnected;
@@ -3794,16 +3798,6 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	int ret;
 
-	/* Can't disconnect eDP, but you can close the lid... */
-	if (is_edp(intel_dp)) {
-		enum drm_connector_status status;
-
-		status = intel_panel_detect(dev);
-		if (status == connector_status_unknown)
-			status = connector_status_connected;
-		return status;
-	}
-
 	ret = g4x_digital_port_connected(dev, intel_dig_port);
 	if (ret == -EINVAL)
 		return connector_status_unknown;
@@ -3877,11 +3871,13 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	intel_dp->has_audio = false;
 
-	if (HAS_PCH_SPLIT(dev))
+	/* Can't disconnect eDP, but you can close the lid... */
+	if (is_edp(intel_dp))
+		status = edp_detect(intel_dp);
+	else if (HAS_PCH_SPLIT(dev))
 		status = ironlake_dp_detect(intel_dp);
 	else
 		status = g4x_dp_detect(intel_dp);
-
 	if (status != connector_status_connected)
 		goto out;
 
-- 
2.1.0

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

[PATCH 2/5] drm/i915/dp: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

v2: Implement connector->force() callback so that edid is associated
with the connector for user overrides as well (Ville)

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  | 156 ++++++++++++++++++++++-----------------
 drivers/gpu/drm/i915/intel_drv.h |   1 +
 2 files changed, 90 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0aa134ce114a..d5f769788f95 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3808,9 +3808,9 @@ g4x_dp_detect(struct intel_dp *intel_dp)
 }
 
 static struct edid *
-intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
+intel_dp_get_edid(struct intel_dp *intel_dp)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 
 	/* use cached edid if we have one */
 	if (intel_connector->edid) {
@@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
 			return NULL;
 
 		return drm_edid_duplicate(intel_connector->edid);
-	}
+	} else
+		return drm_get_edid(&intel_connector->base,
+				    &intel_dp->aux.ddc);
+}
 
-	return drm_get_edid(connector, adapter);
+static void
+intel_dp_set_edid(struct intel_dp *intel_dp)
+{
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct edid *edid;
+
+	edid = intel_dp_get_edid(intel_dp);
+	intel_connector->detect_edid = edid;
+
+	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
+		intel_dp->has_audio = intel_dp->force_audio == HDMI_AUDIO_ON;
+	else
+		intel_dp->has_audio = drm_detect_monitor_audio(edid);
 }
 
-static int
-intel_dp_get_edid_modes(struct drm_connector *connector, struct i2c_adapter *adapter)
+static void
+intel_dp_unset_edid(struct intel_dp *intel_dp)
 {
-	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 
-	/* use cached edid if we have one */
-	if (intel_connector->edid) {
-		/* invalid edid */
-		if (IS_ERR(intel_connector->edid))
-			return 0;
+	kfree(intel_connector->detect_edid);
+	intel_connector->detect_edid = NULL;
 
-		return intel_connector_update_modes(connector,
-						    intel_connector->edid);
-	}
+	intel_dp->has_audio = false;
+}
 
-	return intel_ddc_get_modes(connector, adapter);
+static enum intel_display_power_domain
+intel_dp_power_get(struct intel_dp *dp)
+{
+	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
+	enum intel_display_power_domain power_domain;
+
+	power_domain = intel_display_port_power_domain(encoder);
+	intel_display_power_get(to_i915(encoder->base.dev), power_domain);
+
+	return power_domain;
+}
+
+static void
+intel_dp_power_put(struct intel_dp *dp,
+		   enum intel_display_power_domain power_domain)
+{
+	struct intel_encoder *encoder = &dp_to_dig_port(dp)->base;
+	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
 static enum drm_connector_status
@@ -3849,27 +3877,22 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct intel_encoder *intel_encoder = &intel_dig_port->base;
 	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	enum drm_connector_status status;
 	enum intel_display_power_domain power_domain;
-	struct edid *edid = NULL;
 	bool ret;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
 		      connector->base.id, connector->name);
+	intel_dp_unset_edid(intel_dp);
 
 	if (intel_dp->is_mst) {
 		/* MST devices are disconnected from a monitor POV */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
-		goto out;
+		return connector_status_disconnected;
 	}
 
-	intel_dp->has_audio = false;
+	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
 	if (is_edp(intel_dp))
@@ -3893,82 +3916,78 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	if (intel_dp->force_audio != HDMI_AUDIO_AUTO) {
-		intel_dp->has_audio = (intel_dp->force_audio == HDMI_AUDIO_ON);
-	} else {
-		edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-		if (edid) {
-			intel_dp->has_audio = drm_detect_monitor_audio(edid);
-			kfree(edid);
-		}
-	}
+	intel_dp_set_edid(intel_dp);
 
 	if (intel_encoder->type != INTEL_OUTPUT_EDP)
 		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 	status = connector_status_connected;
 
 out:
-	intel_display_power_put(dev_priv, power_domain);
+	intel_dp_power_put(intel_dp, power_domain);
 	return status;
 }
 
-static int intel_dp_get_modes(struct drm_connector *connector)
+static void
+intel_dp_force(struct drm_connector *connector)
 {
 	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct intel_connector *intel_connector = to_intel_connector(connector);
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	enum intel_display_power_domain power_domain;
-	int ret;
 
-	/* We should parse the EDID data and find out if it has an audio sink
-	 */
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+	intel_dp_unset_edid(intel_dp);
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	if (connector->status != connector_status_connected)
+		return;
 
-	ret = intel_dp_get_edid_modes(connector, &intel_dp->aux.ddc);
-	intel_display_power_put(dev_priv, power_domain);
-	if (ret)
-		return ret;
+	power_domain = intel_dp_power_get(intel_dp);
+
+	intel_dp_set_edid(intel_dp);
+
+	intel_dp_power_put(intel_dp, power_domain);
+
+	if (intel_encoder->type != INTEL_OUTPUT_EDP)
+		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+}
+
+static int intel_dp_get_modes(struct drm_connector *connector)
+{
+	struct intel_connector *intel_connector = to_intel_connector(connector);
+	struct edid *edid;
+
+	edid = intel_connector->detect_edid;
+	if (edid) {
+		int ret = intel_connector_update_modes(connector, edid);
+		if (ret)
+			return ret;
+	}
 
 	/* if eDP has no EDID, fall back to fixed mode */
-	if (is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+	if (is_edp(intel_attached_dp(connector)) &&
+	    intel_connector->panel.fixed_mode) {
 		struct drm_display_mode *mode;
-		mode = drm_mode_duplicate(dev,
+
+		mode = drm_mode_duplicate(connector->dev,
 					  intel_connector->panel.fixed_mode);
 		if (mode) {
 			drm_mode_probed_add(connector, mode);
 			return 1;
 		}
 	}
+
 	return 0;
 }
 
 static bool
 intel_dp_detect_audio(struct drm_connector *connector)
 {
-	struct intel_dp *intel_dp = intel_attached_dp(connector);
-	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_device *dev = connector->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = intel_dp_get_edid(connector, &intel_dp->aux.ddc);
-	if (edid) {
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid)
 		has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
 
 	return has_audio;
 }
@@ -4066,6 +4085,8 @@ intel_dp_connector_destroy(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 
+	intel_dp_unset_edid(intel_attached_dp(connector));
+
 	if (!IS_ERR_OR_NULL(intel_connector->edid))
 		kfree(intel_connector->edid);
 
@@ -4118,6 +4139,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_dp_detect,
+	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_set_property,
 	.destroy = intel_dp_connector_destroy,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 99aa0c6cf915..353321222940 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -214,6 +214,7 @@ struct intel_connector {
 
 	/* Cached EDID for eDP and LVDS. May hold ERR_PTR for invalid EDID. */
 	struct edid *edid;
+	struct edid *detect_edid;
 
 	/* since POLL and HPD connectors may use the same HPD line keep the native
 	   state of connector->polled in case hotplug storm detection changes it */
-- 
2.1.0

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

[PATCH 3/5] drm/i915/hdmi: Cache EDID for a detection cycle

[Chris Wilson]

As we may query the edid multiple times following a detect, record the
EDID found during output discovery and reuse it. This is a separate
issue from caching the output EDID across detection cycles.

v2: Also hookup the force() callback for audio detection when the user
forces the connection status.
v3: Ville spots a typo, s/==/!=/

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 145 +++++++++++++++++++++-----------------
 1 file changed, 80 insertions(+), 65 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 9169786dbbc3..3b21a769ef54 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -962,104 +962,117 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
 	return true;
 }
 
-static enum drm_connector_status
-intel_hdmi_detect(struct drm_connector *connector, bool force)
+static void
+intel_hdmi_unset_edid(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
 	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-	struct intel_digital_port *intel_dig_port =
-		hdmi_to_dig_port(intel_hdmi);
-	struct intel_encoder *intel_encoder = &intel_dig_port->base;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct edid *edid;
-	enum intel_display_power_domain power_domain;
-	enum drm_connector_status status = connector_status_disconnected;
 
-	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
-		      connector->base.id, connector->name);
+	intel_hdmi->has_hdmi_sink = false;
+	intel_hdmi->has_audio = false;
+	intel_hdmi->rgb_quant_range_selectable = false;
+
+	kfree(to_intel_connector(connector)->detect_edid);
+	to_intel_connector(connector)->detect_edid = NULL;
+}
+
+static bool
+intel_hdmi_set_edid(struct drm_connector *connector)
+{
+	struct drm_i915_private *dev_priv = to_i915(connector->dev);
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct intel_encoder *intel_encoder =
+		&hdmi_to_dig_port(intel_hdmi)->base;
+	enum intel_display_power_domain power_domain;
+	struct edid *edid;
+	bool connected = false;
 
 	power_domain = intel_display_port_power_domain(intel_encoder);
 	intel_display_power_get(dev_priv, power_domain);
 
-	intel_hdmi->has_hdmi_sink = false;
-	intel_hdmi->has_audio = false;
-	intel_hdmi->rgb_quant_range_selectable = false;
 	edid = drm_get_edid(connector,
 			    intel_gmbus_get_adapter(dev_priv,
 						    intel_hdmi->ddc_bus));
 
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL) {
-			status = connector_status_connected;
-			if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
-				intel_hdmi->has_hdmi_sink =
-						drm_detect_hdmi_monitor(edid);
-			intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
-			intel_hdmi->rgb_quant_range_selectable =
-				drm_rgb_quant_range_selectable(edid);
-		}
-		kfree(edid);
-	}
+	intel_display_power_put(dev_priv, power_domain);
+
+	to_intel_connector(connector)->detect_edid = edid;
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
+		intel_hdmi->rgb_quant_range_selectable =
+			drm_rgb_quant_range_selectable(edid);
 
-	if (status == connector_status_connected) {
+		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
 		if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
 			intel_hdmi->has_audio =
-				(intel_hdmi->force_audio == HDMI_AUDIO_ON);
-		intel_encoder->type = INTEL_OUTPUT_HDMI;
+				intel_hdmi->force_audio == HDMI_AUDIO_ON;
+
+		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
+			intel_hdmi->has_hdmi_sink =
+				drm_detect_hdmi_monitor(edid);
+
+		connected = true;
 	}
 
-	intel_display_power_put(dev_priv, power_domain);
+	return connected;
+}
+
+static enum drm_connector_status
+intel_hdmi_detect(struct drm_connector *connector, bool force)
+{
+	enum drm_connector_status status;
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	intel_hdmi_unset_edid(connector);
+
+	if (intel_hdmi_set_edid(connector)) {
+		struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+
+		hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+		status = connector_status_connected;
+	} else
+		status = connector_status_disconnected;
 
 	return status;
 }
 
-static int intel_hdmi_get_modes(struct drm_connector *connector)
+static void
+intel_hdmi_force(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	int ret;
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
 
-	/* We should parse the EDID data and find out if it's an HDMI sink so
-	 * we can send audio to it.
-	 */
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
+	intel_hdmi_unset_edid(connector);
 
-	ret = intel_ddc_get_modes(connector,
-				   intel_gmbus_get_adapter(dev_priv,
-							   intel_hdmi->ddc_bus));
+	if (connector->status != connector_status_connected)
+		return;
 
-	intel_display_power_put(dev_priv, power_domain);
+	intel_hdmi_set_edid(connector);
+	hdmi_to_dig_port(intel_hdmi)->base.type = INTEL_OUTPUT_HDMI;
+}
 
-	return ret;
+static int intel_hdmi_get_modes(struct drm_connector *connector)
+{
+	struct edid *edid;
+
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid == NULL)
+		return 0;
+
+	return intel_connector_update_modes(connector, edid);
 }
 
 static bool
 intel_hdmi_detect_audio(struct drm_connector *connector)
 {
-	struct intel_encoder *intel_encoder = intel_attached_encoder(connector);
-	struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&intel_encoder->base);
-	struct drm_i915_private *dev_priv = connector->dev->dev_private;
-	enum intel_display_power_domain power_domain;
-	struct edid *edid;
 	bool has_audio = false;
+	struct edid *edid;
 
-	power_domain = intel_display_port_power_domain(intel_encoder);
-	intel_display_power_get(dev_priv, power_domain);
-
-	edid = drm_get_edid(connector,
-			    intel_gmbus_get_adapter(dev_priv,
-						    intel_hdmi->ddc_bus));
-	if (edid) {
-		if (edid->input & DRM_EDID_INPUT_DIGITAL)
-			has_audio = drm_detect_monitor_audio(edid);
-		kfree(edid);
-	}
-
-	intel_display_power_put(dev_priv, power_domain);
+	edid = to_intel_connector(connector)->detect_edid;
+	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
+		has_audio = drm_detect_monitor_audio(edid);
 
 	return has_audio;
 }
@@ -1479,6 +1492,7 @@ static void chv_hdmi_pre_enable(struct intel_encoder *encoder)
 
 static void intel_hdmi_destroy(struct drm_connector *connector)
 {
+	intel_hdmi_unset_edid(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
 }
@@ -1486,6 +1500,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
 	.dpms = intel_connector_dpms,
 	.detect = intel_hdmi_detect,
+	.force = intel_hdmi_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_hdmi_set_property,
 	.destroy = intel_hdmi_destroy,
-- 
2.1.0

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

[PATCH 4/5] drm/i915/hdmi: Refactor force_audio -> has_audio coupling

[Chris Wilson]

The routines for deciding whether we have audio (depending upon
force_audio and the associated EDID) are common between detection and
set-property. Refactor the code to remove the duplication.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_hdmi.c | 60 ++++++++++++++++++---------------------
 1 file changed, 27 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 3b21a769ef54..ad7b523d39a8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -976,6 +976,30 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
 }
 
 static bool
+intel_hdmi_update_audio(struct drm_connector *connector)
+{
+	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
+	struct edid *edid = to_intel_connector(connector)->detect_edid;
+	bool has_audio, has_sink;
+	bool changed = false;
+
+	if (intel_hdmi->force_audio == HDMI_AUDIO_AUTO)
+		has_audio = drm_detect_monitor_audio(edid);
+	else
+		has_audio = intel_hdmi->force_audio == HDMI_AUDIO_ON;
+	changed |= intel_hdmi->has_audio |= has_audio;
+	intel_hdmi->has_audio = has_audio;
+
+	has_sink = false;
+	if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
+		has_sink = drm_detect_hdmi_monitor(edid);
+	changed |= intel_hdmi->has_hdmi_sink |= has_sink;
+	intel_hdmi->has_hdmi_sink = has_sink;
+
+	return changed;
+}
+
+static bool
 intel_hdmi_set_edid(struct drm_connector *connector)
 {
 	struct drm_i915_private *dev_priv = to_i915(connector->dev);
@@ -999,16 +1023,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
 	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
 		intel_hdmi->rgb_quant_range_selectable =
 			drm_rgb_quant_range_selectable(edid);
-
-		intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
-		if (intel_hdmi->force_audio != HDMI_AUDIO_AUTO)
-			intel_hdmi->has_audio =
-				intel_hdmi->force_audio == HDMI_AUDIO_ON;
-
-		if (intel_hdmi->force_audio != HDMI_AUDIO_OFF_DVI)
-			intel_hdmi->has_hdmi_sink =
-				drm_detect_hdmi_monitor(edid);
-
+		intel_hdmi_update_audio(connector);
 		connected = true;
 	}
 
@@ -1064,19 +1079,6 @@ static int intel_hdmi_get_modes(struct drm_connector *connector)
 	return intel_connector_update_modes(connector, edid);
 }
 
-static bool
-intel_hdmi_detect_audio(struct drm_connector *connector)
-{
-	bool has_audio = false;
-	struct edid *edid;
-
-	edid = to_intel_connector(connector)->detect_edid;
-	if (edid && edid->input & DRM_EDID_INPUT_DIGITAL)
-		has_audio = drm_detect_monitor_audio(edid);
-
-	return has_audio;
-}
-
 static int
 intel_hdmi_set_property(struct drm_connector *connector,
 			struct drm_property *property,
@@ -1094,22 +1096,14 @@ intel_hdmi_set_property(struct drm_connector *connector,
 
 	if (property == dev_priv->force_audio_property) {
 		enum hdmi_force_audio i = val;
-		bool has_audio;
 
 		if (i == intel_hdmi->force_audio)
 			return 0;
 
 		intel_hdmi->force_audio = i;
+		if (!intel_hdmi_update_audio(connector))
+			return 0;
 
-		if (i == HDMI_AUDIO_AUTO)
-			has_audio = intel_hdmi_detect_audio(connector);
-		else
-			has_audio = (i == HDMI_AUDIO_ON);
-
-		if (i == HDMI_AUDIO_OFF_DVI)
-			intel_hdmi->has_hdmi_sink = 0;
-
-		intel_hdmi->has_audio = has_audio;
 		goto done;
 	}
 
-- 
2.1.0

[PATCH 5/5] drm/i915/dp: Refactor force_audio -> has_audio coupling

[Chris Wilson]

Similar to the previous commit for HDMI, refector the common routine
for setting the audio flags between detection and set-property.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_dp.c | 47 ++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index d5f769788f95..34af2beb34b7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3824,6 +3824,25 @@ intel_dp_get_edid(struct intel_dp *intel_dp)
 				    &intel_dp->aux.ddc);
 }
 
+static bool
+intel_dp_update_audio(struct intel_dp *intel_dp)
+{
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct edid *edid = intel_connector->detect_edid ;
+	bool has_audio;
+
+	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
+		has_audio = intel_dp->force_audio == HDMI_AUDIO_ON;
+	else
+		has_audio = drm_detect_monitor_audio(edid);
+
+	if (has_audio == intel_dp->has_audio)
+		return false;
+
+	intel_dp->has_audio = has_audio;
+	return true;
+}
+
 static void
 intel_dp_set_edid(struct intel_dp *intel_dp)
 {
@@ -3833,10 +3852,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	edid = intel_dp_get_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
-	if (intel_dp->force_audio != HDMI_AUDIO_AUTO)
-		intel_dp->has_audio = intel_dp->force_audio == HDMI_AUDIO_ON;
-	else
-		intel_dp->has_audio = drm_detect_monitor_audio(edid);
+	intel_dp_update_audio(intel_dp);
 }
 
 static void
@@ -3979,19 +3995,6 @@ static int intel_dp_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
-static bool
-intel_dp_detect_audio(struct drm_connector *connector)
-{
-	bool has_audio = false;
-	struct edid *edid;
-
-	edid = to_intel_connector(connector)->detect_edid;
-	if (edid)
-		has_audio = drm_detect_monitor_audio(edid);
-
-	return has_audio;
-}
-
 static int
 intel_dp_set_property(struct drm_connector *connector,
 		      struct drm_property *property,
@@ -4009,22 +4012,14 @@ intel_dp_set_property(struct drm_connector *connector,
 
 	if (property == dev_priv->force_audio_property) {
 		int i = val;
-		bool has_audio;
 
 		if (i == intel_dp->force_audio)
 			return 0;
 
 		intel_dp->force_audio = i;
-
-		if (i == HDMI_AUDIO_AUTO)
-			has_audio = intel_dp_detect_audio(connector);
-		else
-			has_audio = (i == HDMI_AUDIO_ON);
-
-		if (has_audio == intel_dp->has_audio)
+		if (!intel_dp_update_audio(intel_dp))
 			return 0;
 
-		intel_dp->has_audio = has_audio;
 		goto done;
 	}
 
-- 
2.1.0

Re: [PATCH 4/5] drm/i915/hdmi: Refactor force_audio -> has_audio coupling

[Chris Wilson]

On Tue, Sep 02, 2014 at 08:04:02PM +0100, Chris Wilson wrote:
> The routines for deciding whether we have audio (depending upon
> force_audio and the associated EDID) are common between detection and
> set-property. Refactor the code to remove the duplication.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_hdmi.c | 60 ++++++++++++++++++---------------------
>  1 file changed, 27 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 3b21a769ef54..ad7b523d39a8 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -976,6 +976,30 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
>  }
>  
>  static bool
> +intel_hdmi_update_audio(struct drm_connector *connector)
> +{
> +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> +	struct edid *edid = to_intel_connector(connector)->detect_edid;
> +	bool has_audio, has_sink;
> +	bool changed = false;
> +
> +	if (intel_hdmi->force_audio == HDMI_AUDIO_AUTO)
> +		has_audio = drm_detect_monitor_audio(edid);
> +	else
> +		has_audio = intel_hdmi->force_audio == HDMI_AUDIO_ON;
> +	changed |= intel_hdmi->has_audio |= has_audio;

Oh dear, it is obviously time to give up.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915/dp: Cache EDID for a detection cycle

[Jani Nikula]

On Tue, 02 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> @@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>  			return NULL;
>  
>  		return drm_edid_duplicate(intel_connector->edid);
> -	}
> +	} else
> +		return drm_get_edid(&intel_connector->base,
> +				    &intel_dp->aux.ddc);

Nitpick, I'd like to see braces on all branches if one branch requires
them. Also CodingStyle.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 2/5] drm/i915/dp: Cache EDID for a detection cycle

[Daniel Vetter]

On Wed, Sep 03, 2014 at 11:23:09AM +0300, Jani Nikula wrote:
> On Tue, 02 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > @@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> >  			return NULL;
> >  
> >  		return drm_edid_duplicate(intel_connector->edid);
> > -	}
> > +	} else
> > +		return drm_get_edid(&intel_connector->base,
> > +				    &intel_dp->aux.ddc);
> 
> Nitpick, I'd like to see braces on all branches if one branch requires
> them. Also CodingStyle.

This is actually one of the case where I tend to routinely ignore
checkpatch and CodingStyle. We're already super-inconsistent about it at
least, so I'm not terribly fond of enforcing it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/5] drm/i915/hdmi: Refactor force_audio -> has_audio coupling

[Daniel Vetter]

On Tue, Sep 02, 2014 at 08:08:20PM +0100, Chris Wilson wrote:
> On Tue, Sep 02, 2014 at 08:04:02PM +0100, Chris Wilson wrote:
> > The routines for deciding whether we have audio (depending upon
> > force_audio and the associated EDID) are common between detection and
> > set-property. Refactor the code to remove the duplication.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 60 ++++++++++++++++++---------------------
> >  1 file changed, 27 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 3b21a769ef54..ad7b523d39a8 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -976,6 +976,30 @@ intel_hdmi_unset_edid(struct drm_connector *connector)
> >  }
> >  
> >  static bool
> > +intel_hdmi_update_audio(struct drm_connector *connector)
> > +{
> > +	struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
> > +	struct edid *edid = to_intel_connector(connector)->detect_edid;
> > +	bool has_audio, has_sink;
> > +	bool changed = false;
> > +
> > +	if (intel_hdmi->force_audio == HDMI_AUDIO_AUTO)
> > +		has_audio = drm_detect_monitor_audio(edid);
> > +	else
> > +		has_audio = intel_hdmi->force_audio == HDMI_AUDIO_ON;
> > +	changed |= intel_hdmi->has_audio |= has_audio;
> 
> Oh dear, it is obviously time to give up.

Aside of that I like the general direction, since we need to refactor this
stuff a bit anyway to make Thomas' EDID injection work for audio. And
hopefully we'll have atomic modeset this century so that we can finally
ditch all that fragile "has this set_prop resulted in an actual change"
logic ...

Pulled in the first 3 patches from this series, thanks.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/5] drm/i915/dp: Cache EDID for a detection cycle

[Chris Wilson]

On Wed, Sep 03, 2014 at 11:23:09AM +0300, Jani Nikula wrote:
> On Tue, 02 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > @@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
> >  			return NULL;
> >  
> >  		return drm_edid_duplicate(intel_connector->edid);
> > -	}
> > +	} else
> > +		return drm_get_edid(&intel_connector->base,
> > +				    &intel_dp->aux.ddc);
> 
> Nitpick, I'd like to see braces on all branches if one branch requires
> them. Also CodingStyle.

It's one instance where CodingStyle doesn't match a dominant pattern in the kernel.
After all it's a fairly recent addition in 2007 and they haven't won the
flame war yet.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/5] drm/i915/dp: Cache EDID for a detection cycle

[Jani Nikula]

On Wed, 03 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Sep 03, 2014 at 11:23:09AM +0300, Jani Nikula wrote:
>> On Tue, 02 Sep 2014, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> > @@ -3819,27 +3819,55 @@ intel_dp_get_edid(struct drm_connector *connector, struct i2c_adapter *adapter)
>> >  			return NULL;
>> >  
>> >  		return drm_edid_duplicate(intel_connector->edid);
>> > -	}
>> > +	} else
>> > +		return drm_get_edid(&intel_connector->base,
>> > +				    &intel_dp->aux.ddc);
>> 
>> Nitpick, I'd like to see braces on all branches if one branch requires
>> them. Also CodingStyle.
>
> It's one instance where CodingStyle doesn't match a dominant pattern in the kernel.
> After all it's a fairly recent addition in 2007 and they haven't won the
> flame war yet.

As I tried to express, it is secondary to me what CodingStyle and
checkpatch.pl say; my personal preference would be to have braces on all
branches if one branch requires them. No use arguing further on
it. Carry on.

BR,
Jani.


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center