diff mbox

drm/i915: Splitting intel_dp_detect

Message ID 1447741601-32700-1-git-send-email-shubhangi.shrivastava@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shubhangi Shrivastava Nov. 17, 2015, 6:26 a.m. UTC
This patch moves probing for panel, DPCD read etc to another
function intel_dp_long_pulse, while intel_dp_detect returns
the status as connected or disconnected depending on
whether the edid is available or not.
This change will be required by further patches in the series
to avoid performing multiple DPCD operations and removing
their duplication.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++-------------
 1 file changed, 36 insertions(+), 16 deletions(-)

Comments

Ander Conselvan de Oliveira Nov. 20, 2015, 2:18 p.m. UTC | #1
On Tue, 2015-11-17 at 11:56 +0530, Shubhangi Shrivastava wrote:
> This patch moves probing for panel, DPCD read etc to another
> function intel_dp_long_pulse, while intel_dp_detect returns
> the status as connected or disconnected depending on
> whether the edid is available or not.

May i suggest a slight rephrase?

"This patch moves probing for panel, DPCD read, etc done in intel_dp_detect() to
a new function: intel_dp_long_pulse(). Note that the behavior of
intel_dp_detect() is changed to report connected or disconnected depending on
whether the edid is available or not."

I think we also need a note why the EDID change is necessary and at the same
doesn't break anything.


> This change will be required by further patches in the series
> to avoid performing multiple DPCD operations and removing
> their duplication.

I found this last part confusing. Maybe replace "avoid multiple DPCD operations
and removing their duplication" with "avoid duplicated DPCD operations on
hotplug", since we are not going into details here.

> 
> v2: Moved a hunk to next patch of the series.
>     Moved intel_dp_unset_edid to out. (Ander)
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> Signed-off-by: Shubhangi Shrivastava <shubhangi.shrivastava@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 52 ++++++++++++++++++++++++++++------------
> -
>  1 file changed, 36 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8071247..dd2b9da 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4631,9 +4631,10 @@ intel_dp_power_put(struct intel_dp *dp,
>  	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
>  }
>  
> -static enum drm_connector_status
> -intel_dp_detect(struct drm_connector *connector, bool force)
> +static void
> +intel_dp_long_pulse(struct intel_connector *intel_connector)
>  {
> +	struct drm_connector *connector = &intel_connector->base;
>  	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;
> @@ -4643,17 +4644,6 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	bool ret;
>  	u8 sink_irq_vector;
>  
> -	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;
> -		return connector_status_disconnected;
> -	}
> -
>  	power_domain = intel_dp_power_get(intel_dp);
>  
>  	/* Can't disconnect eDP, but you can close the lid... */
> @@ -4670,8 +4660,10 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  
>  	ret = intel_dp_probe_mst(intel_dp);
>  	if (ret) {
> -		/* if we are in MST mode then this connector
> -		   won't appear connected or have anything with EDID on it */
> +		/*
> +		 * if we are in MST mode then this connector
> +		 * won't appear connected or have anything with EDID on it
> +		 */
>  		if (intel_encoder->type != INTEL_OUTPUT_EDP)
>  			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>  		status = connector_status_disconnected;
> @@ -4699,8 +4691,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	}
>  
>  out:
> +	if (status != connector_status_connected)
> +		intel_dp_unset_edid(intel_dp);

I didn't notice this at first, but intel_dp_set_edid() doesn't free the previous
edid, so I think we also need to call it just before intel_dp_set_edid(). Patch
looks good otherwise.

Ander

>  	intel_dp_power_put(intel_dp, power_domain);
> -	return status;
> +	return;
> +}
> +
> +static enum drm_connector_status
> +intel_dp_detect(struct drm_connector *connector, bool force)
> +{
> +	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);
> +
> +	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
> +		      connector->base.id, connector->name);
> +
> +	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;
> +		return connector_status_disconnected;
> +	}
> +
> +	intel_dp_long_pulse(intel_dp->attached_connector);
> +
> +	if (intel_connector->detect_edid)
> +		return connector_status_connected;
> +	else
> +		return connector_status_disconnected;
>  }
>  
>  static void
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8071247..dd2b9da 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4631,9 +4631,10 @@  intel_dp_power_put(struct intel_dp *dp,
 	intel_display_power_put(to_i915(encoder->base.dev), power_domain);
 }
 
-static enum drm_connector_status
-intel_dp_detect(struct drm_connector *connector, bool force)
+static void
+intel_dp_long_pulse(struct intel_connector *intel_connector)
 {
+	struct drm_connector *connector = &intel_connector->base;
 	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;
@@ -4643,17 +4644,6 @@  intel_dp_detect(struct drm_connector *connector, bool force)
 	bool ret;
 	u8 sink_irq_vector;
 
-	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;
-		return connector_status_disconnected;
-	}
-
 	power_domain = intel_dp_power_get(intel_dp);
 
 	/* Can't disconnect eDP, but you can close the lid... */
@@ -4670,8 +4660,10 @@  intel_dp_detect(struct drm_connector *connector, bool force)
 
 	ret = intel_dp_probe_mst(intel_dp);
 	if (ret) {
-		/* if we are in MST mode then this connector
-		   won't appear connected or have anything with EDID on it */
+		/*
+		 * if we are in MST mode then this connector
+		 * won't appear connected or have anything with EDID on it
+		 */
 		if (intel_encoder->type != INTEL_OUTPUT_EDP)
 			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 		status = connector_status_disconnected;
@@ -4699,8 +4691,36 @@  intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_dp_power_put(intel_dp, power_domain);
-	return status;
+	return;
+}
+
+static enum drm_connector_status
+intel_dp_detect(struct drm_connector *connector, bool force)
+{
+	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);
+
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n",
+		      connector->base.id, connector->name);
+
+	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;
+		return connector_status_disconnected;
+	}
+
+	intel_dp_long_pulse(intel_dp->attached_connector);
+
+	if (intel_connector->detect_edid)
+		return connector_status_connected;
+	else
+		return connector_status_disconnected;
 }
 
 static void