[PATCH 0/6] Fixing sink count related detection over

[PATCH 0/6] Fixing sink count related detection over

[Shubhangi Shrivastava]

This patch set cleans up DP detection logic to bring all DPCD
operations at one place and to create a clear demarcation
between handling of long and short pulses. This simplifies
fixing of sink count related detection for DP panels.

Patches:
1. First two patches clean up intel_dp_detect and form a new
function which will include all DPCD related operations.
2. Third patch splits up intel_dp_check_link_status to form
a new function which will handle short pulse requests.
3. Last three patches fixes the detection logic related to
sink count i.e detect changes in sink count and handle them
appropriately. 

Shubhangi Shrivastava (6):
  drm/i915: Splitting intel_dp_detect
  drm/i915: Cleaning up intel_dp_hpd_pulse
  drm/i915: Splitting intel_dp_check_link_status
  drm/i915: Save sink_count for tracking changes to it
  drm/i915: read sink_count dpcd always
  drm/i915: force full detect on sink count change

 drivers/gpu/drm/i915/intel_dp.c  | 170 +++++++++++++++++++++++++--------------
 drivers/gpu/drm/i915/intel_drv.h |   1 +
 2 files changed, 110 insertions(+), 61 deletions(-)

-- 
2.6.1

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

[PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

intel_dp_detect() is called for not just detection but
during modes enumeration as well. Repeating the whole
sequence during each of these calls is wasteful and
time consuming.
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.
This change will be required by further patches in the series
to avoid performing duplicated DPCD operations on hotplug.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)
v3: Rephrased commit message and intel_dp_unset_dp() is called
    within intel_dp_set_dp() to free the previous EDID. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 56 +++++++++++++++++++++++++----------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 796e3d3..e3b4208 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
 static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
+static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
 static unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
@@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct edid *edid;
 
+	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
@@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-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;
@@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(to_i915(dev), power_domain);
 
@@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	intel_dp_probe_oui(intel_dp);
 
 	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
+	if (ret)
 		goto out;
-	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_display_power_put(to_i915(dev), 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
-- 
2.6.1

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> intel_dp_detect() is called for not just detection but
> during modes enumeration as well. Repeating the whole
> sequence during each of these calls is wasteful and
> time consuming.
> 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.
> This change will be required by further patches in the series
> to avoid performing duplicated DPCD operations on hotplug.
> 
> v2: Moved a hunk to next patch of the series.
>     Moved intel_dp_unset_edid to out. (Ander)
> v3: Rephrased commit message and intel_dp_unset_dp() is called
>     within intel_dp_set_dp() to free the previous EDID. (Ander)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 56 +++++++++++++++++++++++++---------------
> -
>  1 file changed, 35 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 796e3d3..e3b4208 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
> bool sync);
>  static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>  static void vlv_steal_power_sequencer(struct drm_device *dev,
>  				      enum pipe pipe);
> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>  
>  static unsigned int intel_dp_unused_lane_mask(int lane_count)
>  {
> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>  	struct intel_connector *intel_connector = intel_dp
> ->attached_connector;
>  	struct edid *edid;
>  
> +	intel_dp_unset_edid(intel_dp);
>  	edid = intel_dp_get_edid(intel_dp);
>  	intel_connector->detect_edid = edid;
>  
> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>  	intel_dp->has_audio = false;
>  }
>  
> -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;
> @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
>  	intel_display_power_get(to_i915(dev), power_domain);
>  
> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	intel_dp_probe_oui(intel_dp);
>  
>  	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;

This deletion is new in this version of the patch. I think we still need the
hunk above, otherwise we might not properly update the encoder type when we
switch from an HDMI sink connected through a level shifter to an MST sink.

Ander


> -		status = connector_status_disconnected;
> +	if (ret)
>  		goto out;
> -	}
>  
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  	}
>  
>  out:
> +	if (status != connector_status_connected)
> +		intel_dp_unset_edid(intel_dp);
>  	intel_display_power_put(to_i915(dev), 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> > intel_dp_detect() is called for not just detection but
> > during modes enumeration as well. Repeating the whole
> > sequence during each of these calls is wasteful and
> > time consuming.
> > 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.
> > This change will be required by further patches in the series
> > to avoid performing duplicated DPCD operations on hotplug.
> > 
> > v2: Moved a hunk to next patch of the series.
> >     Moved intel_dp_unset_edid to out. (Ander)
> > v3: Rephrased commit message and intel_dp_unset_dp() is called
> >     within intel_dp_set_dp() to free the previous EDID. (Ander)
> > 
> > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > 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 | 56 +++++++++++++++++++++++++-------------
> > --
> > -
> >  1 file changed, 35 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 796e3d3..e3b4208 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
> > bool sync);
> >  static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> >  static void vlv_steal_power_sequencer(struct drm_device *dev,
> >  				      enum pipe pipe);
> > +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
> >  
> >  static unsigned int intel_dp_unused_lane_mask(int lane_count)
> >  {
> > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
> >  	struct intel_connector *intel_connector = intel_dp
> > ->attached_connector;
> >  	struct edid *edid;
> >  
> > +	intel_dp_unset_edid(intel_dp);
> >  	edid = intel_dp_get_edid(intel_dp);
> >  	intel_connector->detect_edid = edid;
> >  
> > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> >  	intel_dp->has_audio = false;
> >  }
> >  
> > -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;
> > @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
> >  	intel_display_power_get(to_i915(dev), power_domain);
> >  
> > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  	intel_dp_probe_oui(intel_dp);
> >  
> >  	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
> > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> 
> This deletion is new in this version of the patch. I think we still need the
> hunk above, otherwise we might not properly update the encoder type when we
> switch from an HDMI sink connected through a level shifter to an MST sink.
> 
> Ander
> 
> 
> > -		status = connector_status_disconnected;
> > +	if (ret)
> >  		goto out;

Also, there is no call to intel_dp_unset_edid() for this case, since the code
will reach the label 'out' with status being connected. So in this case the
return value of intel_dp_detect() will depend on the stale value of
intel_dp->detect_edid.

Ander

> > -	}
> >  
> >  	/*
> >  	 * Clearing NACK and defer counts to get their exact values
> > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
> > force)
> >  	}
> >  
> >  out:
> > +	if (status != connector_status_connected)
> > +		intel_dp_unset_edid(intel_dp);
> >  	intel_display_power_put(to_i915(dev), 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>> intel_dp_detect() is called for not just detection but
>>> during modes enumeration as well. Repeating the whole
>>> sequence during each of these calls is wasteful and
>>> time consuming.
>>> 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.
>>> This change will be required by further patches in the series
>>> to avoid performing duplicated DPCD operations on hotplug.
>>>
>>> v2: Moved a hunk to next patch of the series.
>>>      Moved intel_dp_unset_edid to out. (Ander)
>>> v3: Rephrased commit message and intel_dp_unset_dp() is called
>>>      within intel_dp_set_dp() to free the previous EDID. (Ander)
>>>
>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>> 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 | 56 +++++++++++++++++++++++++-------------
>>> --
>>> -
>>>   1 file changed, 35 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index 796e3d3..e3b4208 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp,
>>> bool sync);
>>>   static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>>>   static void vlv_steal_power_sequencer(struct drm_device *dev,
>>>   				      enum pipe pipe);
>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>>>   
>>>   static unsigned int intel_dp_unused_lane_mask(int lane_count)
>>>   {
>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>>>   	struct intel_connector *intel_connector = intel_dp
>>> ->attached_connector;
>>>   	struct edid *edid;
>>>   
>>> +	intel_dp_unset_edid(intel_dp);
>>>   	edid = intel_dp_get_edid(intel_dp);
>>>   	intel_connector->detect_edid = edid;
>>>   
>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>>   	intel_dp->has_audio = false;
>>>   }
>>>   
>>> -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;
>>> @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
>>>   	intel_display_power_get(to_i915(dev), power_domain);
>>>   
>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	intel_dp_probe_oui(intel_dp);
>>>   
>>>   	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>> This deletion is new in this version of the patch. I think we still need the
>> hunk above, otherwise we might not properly update the encoder type when we
>> switch from an HDMI sink connected through a level shifter to an MST sink.
>>
>> Ander
>>
Encoder type setting for MST is being done in intel_dp_detect(). So, 
don't find a need to add it here.
>>> -		status = connector_status_disconnected;
>>> +	if (ret)
>>>   		goto out;
> Also, there is no call to intel_dp_unset_edid() for this case, since the code
> will reach the label 'out' with status being connected. So in this case the
> return value of intel_dp_detect() will depend on the stale value of
> intel_dp->detect_edid.
>
> Ander
Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() 
check of intel_dp_detect().
>
>>> -	}
>>>   
>>>   	/*
>>>   	 * Clearing NACK and defer counts to get their exact values
>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector, bool
>>> force)
>>>   	}
>>>   
>>>   out:
>>> +	if (status != connector_status_connected)
>>> +		intel_dp_unset_edid(intel_dp);
>>>   	intel_display_power_put(to_i915(dev), 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

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Ander Conselvan De Oliveira]

On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote:
> 
> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
> > On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
> > > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> > > > intel_dp_detect() is called for not just detection but
> > > > during modes enumeration as well. Repeating the whole
> > > > sequence during each of these calls is wasteful and
> > > > time consuming.
> > > > 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.
> > > > This change will be required by further patches in the series
> > > > to avoid performing duplicated DPCD operations on hotplug.
> > > > 
> > > > v2: Moved a hunk to next patch of the series.
> > > >      Moved intel_dp_unset_edid to out. (Ander)
> > > > v3: Rephrased commit message and intel_dp_unset_dp() is called
> > > >      within intel_dp_set_dp() to free the previous EDID. (Ander)
> > > > 
> > > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > > 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 | 56 +++++++++++++++++++++++++--------
> > > > -----
> > > > --
> > > > -
> > > >   1 file changed, 35 insertions(+), 21 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > > b/drivers/gpu/drm/i915/intel_dp.c
> > > > index 796e3d3..e3b4208 100644
> > > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > > @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp
> > > > *intel_dp,
> > > > bool sync);
> > > >   static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
> > > >   static void vlv_steal_power_sequencer(struct drm_device *dev,
> > > >   				      enum pipe pipe);
> > > > +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
> > > >   
> > > >   static unsigned int intel_dp_unused_lane_mask(int lane_count)
> > > >   {
> > > > @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
> > > >   	struct intel_connector *intel_connector = intel_dp
> > > > ->attached_connector;
> > > >   	struct edid *edid;
> > > >   
> > > > +	intel_dp_unset_edid(intel_dp);
> > > >   	edid = intel_dp_get_edid(intel_dp);
> > > >   	intel_connector->detect_edid = edid;
> > > >   
> > > > @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
> > > >   	intel_dp->has_audio = false;
> > > >   }
> > > >   
> > > > -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;
> > > > @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
> > > >   	intel_display_power_get(to_i915(dev), power_domain);
> > > >   
> > > > @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	intel_dp_probe_oui(intel_dp);
> > > >   
> > > >   	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
> > > > -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
> > > This deletion is new in this version of the patch. I think we still need
> > > the
> > > hunk above, otherwise we might not properly update the encoder type when
> > > we
> > > switch from an HDMI sink connected through a level shifter to an MST sink.
> > > 
> > > Ander
> > > 
> Encoder type setting for MST is being done in intel_dp_detect(). So, 
> don't find a need to add it here.

Yes, but that one only covers the case where the device was already previously
identified as MST. For a device identified as MST by the call to
intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will
not be done. Hopefully, Ville's patch that splits the encoder types and makes
this unnecessary will land soon, but for now just leave the override there.

Ander

> > > > -		status = connector_status_disconnected;
> > > > +	if (ret)
> > > >   		goto out;
> > Also, there is no call to intel_dp_unset_edid() for this case, since the
> > code
> > will reach the label 'out' with status being connected. So in this case the
> > return value of intel_dp_detect() will depend on the stale value of
> > intel_dp->detect_edid.
> > 
> > Ander
> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst() 
> check of intel_dp_detect().
> > 
> > > > -	}
> > > >   
> > > >   	/*
> > > >   	 * Clearing NACK and defer counts to get their exact values
> > > > @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector,
> > > > bool
> > > > force)
> > > >   	}
> > > >   
> > > >   out:
> > > > +	if (status != connector_status_connected)
> > > > +		intel_dp_unset_edid(intel_dp);
> > > >   	intel_display_power_put(to_i915(dev), 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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

intel_dp_detect() is called for not just detection but
during modes enumeration as well. Repeating the whole
sequence during each of these calls is wasteful and
time consuming.
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.
This change will be required by further patches in the series
to avoid performing duplicated DPCD operations on hotplug.

v2: Moved a hunk to next patch of the series.
    Moved intel_dp_unset_edid to out. (Ander)
v3: Rephrased commit message and intel_dp_unset_dp() is called
    within intel_dp_set_dp() to free the previous EDID. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 62 ++++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1761254..8969ff9 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp *intel_dp, bool sync);
 static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
 static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
+static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
 static unsigned int intel_dp_unused_lane_mask(int lane_count)
 {
@@ -4577,6 +4578,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	struct edid *edid;
 
+	intel_dp_unset_edid(intel_dp);
 	edid = intel_dp_get_edid(intel_dp);
 	intel_connector->detect_edid = edid;
 
@@ -4597,9 +4599,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
 	intel_dp->has_audio = false;
 }
 
-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;
@@ -4609,17 +4612,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_display_port_aux_power_domain(intel_encoder);
 	intel_display_power_get(to_i915(dev), power_domain);
 
@@ -4640,17 +4632,14 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
+	if (intel_encoder->type != INTEL_OUTPUT_EDP)
+		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
+
 	intel_dp_probe_oui(intel_dp);
 
 	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
-			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
-		status = connector_status_disconnected;
+	if (ret)
 		goto out;
-	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4662,8 +4651,6 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 
 	intel_dp_set_edid(intel_dp);
 
-	if (intel_encoder->type != INTEL_OUTPUT_EDP)
-		intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
 	status = connector_status_connected;
 
 	/* Try to read the source of the interrupt */
@@ -4681,8 +4668,37 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 	}
 
 out:
+	if (status != connector_status_connected)
+		intel_dp_unset_edid(intel_dp);
 	intel_display_power_put(to_i915(dev), 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 */
+		intel_dp_unset_edid(intel_dp);
+		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
-- 
2.6.1

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

Re: [PATCH 1/6] drm/i915: Splitting intel_dp_detect

[Shubhangi Shrivastava]

On Friday 15 January 2016 03:37 PM, Ander Conselvan De Oliveira wrote:
> On Thu, 2016-01-14 at 19:20 +0530, Shubhangi Shrivastava wrote:
>> On Wednesday 13 January 2016 07:03 PM, Ander Conselvan De Oliveira wrote:
>>> On Wed, 2016-01-13 at 13:20 +0200, Ander Conselvan De Oliveira wrote:
>>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>>>> intel_dp_detect() is called for not just detection but
>>>>> during modes enumeration as well. Repeating the whole
>>>>> sequence during each of these calls is wasteful and
>>>>> time consuming.
>>>>> 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.
>>>>> This change will be required by further patches in the series
>>>>> to avoid performing duplicated DPCD operations on hotplug.
>>>>>
>>>>> v2: Moved a hunk to next patch of the series.
>>>>>       Moved intel_dp_unset_edid to out. (Ander)
>>>>> v3: Rephrased commit message and intel_dp_unset_dp() is called
>>>>>       within intel_dp_set_dp() to free the previous EDID. (Ander)
>>>>>
>>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>>> 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 | 56 +++++++++++++++++++++++++--------
>>>>> -----
>>>>> --
>>>>> -
>>>>>    1 file changed, 35 insertions(+), 21 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>>> index 796e3d3..e3b4208 100644
>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>> @@ -129,6 +129,7 @@ static void edp_panel_vdd_off(struct intel_dp
>>>>> *intel_dp,
>>>>> bool sync);
>>>>>    static void vlv_init_panel_power_sequencer(struct intel_dp *intel_dp);
>>>>>    static void vlv_steal_power_sequencer(struct drm_device *dev,
>>>>>    				      enum pipe pipe);
>>>>> +static void intel_dp_unset_edid(struct intel_dp *intel_dp);
>>>>>    
>>>>>    static unsigned int intel_dp_unused_lane_mask(int lane_count)
>>>>>    {
>>>>> @@ -4587,6 +4588,7 @@ intel_dp_set_edid(struct intel_dp *intel_dp)
>>>>>    	struct intel_connector *intel_connector = intel_dp
>>>>> ->attached_connector;
>>>>>    	struct edid *edid;
>>>>>    
>>>>> +	intel_dp_unset_edid(intel_dp);
>>>>>    	edid = intel_dp_get_edid(intel_dp);
>>>>>    	intel_connector->detect_edid = edid;
>>>>>    
>>>>> @@ -4607,9 +4609,10 @@ intel_dp_unset_edid(struct intel_dp *intel_dp)
>>>>>    	intel_dp->has_audio = false;
>>>>>    }
>>>>>    
>>>>> -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;
>>>>> @@ -4619,17 +4622,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_display_port_aux_power_domain(intel_encoder);
>>>>>    	intel_display_power_get(to_i915(dev), power_domain);
>>>>>    
>>>>> @@ -4653,14 +4645,8 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	intel_dp_probe_oui(intel_dp);
>>>>>    
>>>>>    	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 (intel_encoder->type != INTEL_OUTPUT_EDP)
>>>>> -			intel_encoder->type = INTEL_OUTPUT_DISPLAYPORT;
>>>> This deletion is new in this version of the patch. I think we still need
>>>> the
>>>> hunk above, otherwise we might not properly update the encoder type when
>>>> we
>>>> switch from an HDMI sink connected through a level shifter to an MST sink.
>>>>
>>>> Ander
>>>>
>> Encoder type setting for MST is being done in intel_dp_detect(). So,
>> don't find a need to add it here.
> Yes, but that one only covers the case where the device was already previously
> identified as MST. For a device identified as MST by the call to
> intel_dp_probe_mst() in intel_dp_long_pulse(), the encoder type override will
> not be done. Hopefully, Ville's patch that splits the encoder types and makes
> this unnecessary will land soon, but for now just leave the override there.
>
> Ander

Alright.. Moved the overriding of encoder type to be done before probing..

>>>>> -		status = connector_status_disconnected;
>>>>> +	if (ret)
>>>>>    		goto out;
>>> Also, there is no call to intel_dp_unset_edid() for this case, since the
>>> code
>>> will reach the label 'out' with status being connected. So in this case the
>>> return value of intel_dp_detect() will depend on the stale value of
>>> intel_dp->detect_edid.
>>>
>>> Ander
>> Yes.. Thats right.. Will add a call to intel_dp_unset_edid() in is_mst()
>> check of intel_dp_detect().

Added call to intel_dp_unset_edid() in is_mst() check of intel_dp_detect().

>>>>> -	}
>>>>>    
>>>>>    	/*
>>>>>    	 * Clearing NACK and defer counts to get their exact values
>>>>> @@ -4691,8 +4677,36 @@ intel_dp_detect(struct drm_connector *connector,
>>>>> bool
>>>>> force)
>>>>>    	}
>>>>>    
>>>>>    out:
>>>>> +	if (status != connector_status_connected)
>>>>> +		intel_dp_unset_edid(intel_dp);
>>>>>    	intel_display_power_put(to_i915(dev), 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

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

[PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Shubhangi Shrivastava]

Current DP detection has DPCD operations split across
intel_dp_hpd_pulse and intel_dp_detect which contains
duplicates as well. Also intel_dp_detect is called
during modes enumeration as well which will result
in multiple dpcd operations. So this patch tries
to solve both these by bringing all DPCD operations
in one single function and make intel_dp_detect
use existing values instead of repeating same steps.

v2: Pulled in a hunk from last patch of the series to
    this patch. (Ander)
v3: Added MST hotplug handling. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 71 +++++++++++++++++++++++++----------------
 1 file changed, 44 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e3b4208..137757b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4645,8 +4645,19 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	intel_dp_probe_oui(intel_dp);
 
 	ret = intel_dp_probe_mst(intel_dp);
-	if (ret)
+	if (ret) {
+		goto out;
+	} else if (connector->status == connector_status_connected) {
+		/*
+		 * If display was connected already and is still connected
+		 * check links status, there has been known issues of
+		 * link loss triggerring long pulse!!!!
+		 */
+		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+		intel_dp_check_link_status(intel_dp);
+		drm_modeset_unlock(&dev->mode_config.connection_mutex);
 		goto out;
+	}
 
 	/*
 	 * Clearing NACK and defer counts to get their exact values
@@ -4677,8 +4688,21 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 	}
 
 out:
-	if (status != connector_status_connected)
+	if (status != connector_status_connected) {
 		intel_dp_unset_edid(intel_dp);
+		/*
+		 * If we were in MST mode, and device is not there,
+		 * get out of MST mode
+		 */
+		if (intel_dp->is_mst) {
+			DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+				intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+			intel_dp->is_mst = false;
+			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+							intel_dp->is_mst);
+		}
+	}
+
 	intel_display_power_put(to_i915(dev), power_domain);
 	return;
 }
@@ -4701,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool force)
 		return connector_status_disconnected;
 	}
 
-	intel_dp_long_pulse(intel_dp->attached_connector);
+	if (force)
+		intel_dp_long_pulse(intel_dp->attached_connector);
 
 	if (intel_connector->detect_edid)
 		return connector_status_connected;
@@ -5034,25 +5059,25 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		/* indicate that we need to restart link training */
 		intel_dp->train_set_valid = false;
 
-		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
-			goto mst_fail;
-
-		if (!intel_dp_get_dpcd(intel_dp)) {
-			goto mst_fail;
-		}
-
-		intel_dp_probe_oui(intel_dp);
+		intel_dp_long_pulse(intel_dp->attached_connector);
+		if (intel_dp->is_mst)
+			ret = IRQ_HANDLED;
+		goto put_power;
 
-		if (!intel_dp_probe_mst(intel_dp)) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-			goto mst_fail;
-		}
 	} else {
 		if (intel_dp->is_mst) {
-			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
-				goto mst_fail;
+			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
+				/*
+				 * If we were in MST mode, and device is not
+				 * there, get out of MST mode
+				 */
+				DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
+					intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
+				intel_dp->is_mst = false;
+				drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
+								intel_dp->is_mst);
+				goto put_power;
+			}
 		}
 
 		if (!intel_dp->is_mst) {
@@ -5064,14 +5089,6 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 
 	ret = IRQ_HANDLED;
 
-	goto put_power;
-mst_fail:
-	/* if we were in MST mode, and device is not there get out of MST mode */
-	if (intel_dp->is_mst) {
-		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n", intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
-		intel_dp->is_mst = false;
-		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp->is_mst);
-	}
 put_power:
 	intel_display_power_put(dev_priv, power_domain);
 
-- 
2.6.1

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

Re: [PATCH 2/6] drm/i915: Cleaning up intel_dp_hpd_pulse

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> Current DP detection has DPCD operations split across
> intel_dp_hpd_pulse and intel_dp_detect which contains
> duplicates as well. Also intel_dp_detect is called
> during modes enumeration as well which will result
> in multiple dpcd operations. So this patch tries
> to solve both these by bringing all DPCD operations
> in one single function and make intel_dp_detect
> use existing values instead of repeating same steps.
> 
> v2: Pulled in a hunk from last patch of the series to
>     this patch. (Ander)
> v3: Added MST hotplug handling. (Ander)

Reviewed-by: Ander Conselvan de Oliveira <conselvan2@gmail.com>

> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 71 +++++++++++++++++++++++++---------------
> -
>  1 file changed, 44 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index e3b4208..137757b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4645,8 +4645,19 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	intel_dp_probe_oui(intel_dp);
>  
>  	ret = intel_dp_probe_mst(intel_dp);
> -	if (ret)
> +	if (ret) {
> +		goto out;
> +	} else if (connector->status == connector_status_connected) {
> +		/*
> +		 * If display was connected already and is still connected
> +		 * check links status, there has been known issues of
> +		 * link loss triggerring long pulse!!!!
> +		 */
> +		drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +		intel_dp_check_link_status(intel_dp);
> +		drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  		goto out;
> +	}
>  
>  	/*
>  	 * Clearing NACK and defer counts to get their exact values
> @@ -4677,8 +4688,21 @@ intel_dp_long_pulse(struct intel_connector
> *intel_connector)
>  	}
>  
>  out:
> -	if (status != connector_status_connected)
> +	if (status != connector_status_connected) {
>  		intel_dp_unset_edid(intel_dp);
> +		/*
> +		 * If we were in MST mode, and device is not there,
> +		 * get out of MST mode
> +		 */
> +		if (intel_dp->is_mst) {
> +			DRM_DEBUG_KMS("MST device may have disappeared %d vs
> %d\n",
> +				intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +			intel_dp->is_mst = false;
> +			drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr,
> +							intel_dp->is_mst);
> +		}
> +	}
> +
>  	intel_display_power_put(to_i915(dev), power_domain);
>  	return;
>  }
> @@ -4701,7 +4725,8 @@ intel_dp_detect(struct drm_connector *connector, bool
> force)
>  		return connector_status_disconnected;
>  	}
>  
> -	intel_dp_long_pulse(intel_dp->attached_connector);
> +	if (force)
> +		intel_dp_long_pulse(intel_dp->attached_connector);
>  
>  	if (intel_connector->detect_edid)
>  		return connector_status_connected;
> @@ -5034,25 +5059,25 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  		/* indicate that we need to restart link training */
>  		intel_dp->train_set_valid = false;
>  
> -		if (!intel_digital_port_connected(dev_priv, intel_dig_port))
> -			goto mst_fail;
> -
> -		if (!intel_dp_get_dpcd(intel_dp)) {
> -			goto mst_fail;
> -		}
> -
> -		intel_dp_probe_oui(intel_dp);
> +		intel_dp_long_pulse(intel_dp->attached_connector);
> +		if (intel_dp->is_mst)
> +			ret = IRQ_HANDLED;
> +		goto put_power;
>  
> -		if (!intel_dp_probe_mst(intel_dp)) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -			goto mst_fail;
> -		}
>  	} else {
>  		if (intel_dp->is_mst) {
> -			if (intel_dp_check_mst_status(intel_dp) == -EINVAL)
> -				goto mst_fail;
> +			if (intel_dp_check_mst_status(intel_dp) == -EINVAL) {
> +				/*
> +				 * If we were in MST mode, and device is not
> +				 * there, get out of MST mode
> +				 */
> +				DRM_DEBUG_KMS("MST device may have
> disappeared %d vs %d\n",
> +					intel_dp->is_mst, intel_dp
> ->mst_mgr.mst_state);
> +				intel_dp->is_mst = false;
> +				drm_dp_mst_topology_mgr_set_mst(&intel_dp
> ->mst_mgr,
> +								intel_dp
> ->is_mst);
> +				goto put_power;
> +			}
>  		}
>  
>  		if (!intel_dp->is_mst) {
> @@ -5064,14 +5089,6 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  
>  	ret = IRQ_HANDLED;
>  
> -	goto put_power;
> -mst_fail:
> -	/* if we were in MST mode, and device is not there get out of MST
> mode */
> -	if (intel_dp->is_mst) {
> -		DRM_DEBUG_KMS("MST device may have disappeared %d vs %d\n",
> intel_dp->is_mst, intel_dp->mst_mgr.mst_state);
> -		intel_dp->is_mst = false;
> -		drm_dp_mst_topology_mgr_set_mst(&intel_dp->mst_mgr, intel_dp
> ->is_mst);
> -	}
>  put_power:
>  	intel_display_power_put(dev_priv, power_domain);
>  
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status

[Shubhangi Shrivastava]

When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.
The link retraining part when EQ is not correct is
retained to intel_dp_check_link_status whereas other
operations are handled as part of intel_dp_short_pulse.
This change is required to avoid performing all DPCD
related operations on performing link retraining.

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 56 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 137757b..842790e 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4289,6 +4289,33 @@ go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* if link training is requested we should perform it always */
+	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
+		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4298,15 +4325,12 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-
 	/*
 	 * Clearing compliance test variables to allow capturing
 	 * of values for next automated test request.
@@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
 		return;
@@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
-		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
+		if (!intel_dp->is_mst)
+			intel_dp_short_pulse(intel_dp);
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

Re: [PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> When created originally intel_dp_check_link_status()
> was supposed to handle only link training for short
> pulse but has grown into handler for short pulse itself.
> This patch cleans up this function by splitting it into
> two halves. First intel_dp_short_pulse() is called,
> which will be entry point and handle all logic for
> short pulse handling while intel_dp_check_link_status()
> will retain its original purpose of only doing link
> status related work.
> The link retraining part when EQ is not correct is
> retained to intel_dp_check_link_status whereas other
> operations are handled as part of intel_dp_short_pulse.
> This change is required to avoid performing all DPCD
> related operations on performing link retraining.
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 56 ++++++++++++++++++++++++----------------
> -
>  1 file changed, 33 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 137757b..842790e 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4289,6 +4289,33 @@ go_again:
>  	return -EINVAL;
>  }
>  
> +static void
> +intel_dp_check_link_status(struct intel_dp *intel_dp)
> +{
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)
> ->base;
> +	u8 link_status[DP_LINK_STATUS_SIZE];
> +
> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +		DRM_ERROR("Failed to get link status\n");
> +		return;
> +	}
> +
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;
> +
> +	/* if link training is requested we should perform it always */
> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> +				intel_encoder->base.name);
> +		intel_dp_start_link_train(intel_dp);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
> +}
> +
>  /*
>   * According to DP spec
>   * 5.1.2:
> @@ -4298,15 +4325,12 @@ go_again:
>   *  4. Check link status on receipt of hot-plug interrupt
>   */
>  static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> +intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)
> ->base;
>  	u8 sink_irq_vector;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> -

I think it's better to move this WARN to the new intel_dp_check_link_status().

>  	/*
>  	 * Clearing compliance test variables to allow capturing
>  	 * of values for next automated test request.
> @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	intel_dp->compliance_test_type = 0;
>  	intel_dp->compliance_test_data = 0;
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> -
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> -
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>  		return;

There is now two calls to intel_dp_get_link_status()and the value of link_status
is not used in this function, so maybe just remove it from here. Looks good
otherwise.

Ander

> @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq
> unhandled\n");
>  	}
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> -		intel_dp_start_link_train(intel_dp);
> -		intel_dp_stop_link_train(intel_dp);
> -	}
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	intel_dp_check_link_status(intel_dp);
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
>  /* XXX this is probably wrong for multiple downstream ports */
> @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  			}
>  		}
>  
> -		if (!intel_dp->is_mst) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
> NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev
> ->mode_config.connection_mutex);
> -		}
> +		if (!intel_dp->is_mst)
> +			intel_dp_short_pulse(intel_dp);
>  	}
>  
>  	ret = IRQ_HANDLED;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: Splitting intel_dp_check_link_status

[Shubhangi Shrivastava]

When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.
The link retraining part when EQ is not correct is
retained to intel_dp_check_link_status whereas other
operations are handled as part of intel_dp_short_pulse.
This change is required to avoid performing all DPCD
related operations on performing link retraining.

v2: Added WARN_ON to intel_dp_check_link_status()
    Removed a call to intel_dp_get_link_status() (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 65 +++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 82ee18d..f8d9611 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4279,6 +4279,36 @@ go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* if link training is requested we should perform it always */
+	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
+		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4288,14 +4318,10 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
-	u8 link_status[DP_LINK_STATUS_SIZE];
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		return;
-	}
-
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
-		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
+		if (!intel_dp->is_mst)
+			intel_dp_short_pulse(intel_dp);
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Lukas Wunner]

Hi,

On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> When created originally intel_dp_check_link_status()
> was supposed to handle only link training for short
> pulse but has grown into handler for short pulse itself.
> This patch cleans up this function by splitting it into
> two halves. First intel_dp_short_pulse() is called,
> which will be entry point and handle all logic for
> short pulse handling while intel_dp_check_link_status()
> will retain its original purpose of only doing link
> status related work.
> The link retraining part when EQ is not correct is
> retained to intel_dp_check_link_status whereas other
> operations are handled as part of intel_dp_short_pulse.
> This change is required to avoid performing all DPCD
> related operations on performing link retraining.
> 
> v2: Added WARN_ON to intel_dp_check_link_status()
>     Removed a call to intel_dp_get_link_status() (Ander)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 65 +++++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 82ee18d..f8d9611 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4279,6 +4279,36 @@ go_again:
>  	return -EINVAL;
>  }
>  
> +static void
> +intel_dp_check_link_status(struct intel_dp *intel_dp)
> +{
> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> +	u8 link_status[DP_LINK_STATUS_SIZE];
> +
> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> +
> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> +		DRM_ERROR("Failed to get link status\n");
> +		return;
> +	}
> +
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;

Why do you change the order of the three if-clauses above?
The original order seems to make more sense. (Checking for
->base.crtc and ->active is cheap, whereas accessing AUX to
get the link status is time consuming. You don't want to
spend that time only to bail out, should one of the other two
if-clauses fail.)

Best regards,

Lukas

> +
> +	/* if link training is requested we should perform it always */
> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> +				intel_encoder->base.name);
> +		intel_dp_start_link_train(intel_dp);
> +		intel_dp_stop_link_train(intel_dp);
> +	}
> +}
> +
>  /*
>   * According to DP spec
>   * 5.1.2:
> @@ -4288,14 +4318,10 @@ go_again:
>   *  4. Check link status on receipt of hot-plug interrupt
>   */
>  static void
> -intel_dp_check_link_status(struct intel_dp *intel_dp)
> +intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	u8 sink_irq_vector;
> -	u8 link_status[DP_LINK_STATUS_SIZE];
> -
> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
>  	/*
>  	 * Clearing compliance test variables to allow capturing
> @@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  	intel_dp->compliance_test_type = 0;
>  	intel_dp->compliance_test_data = 0;
>  
> -	if (!intel_encoder->base.crtc)
> -		return;
> -
> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> -		return;
> -
> -	/* Try to read receiver status if the link appears to be up */
> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> -		return;
> -	}
> -
>  	/* Now read the DPCD to see if it's actually running */
>  	if (!intel_dp_get_dpcd(intel_dp)) {
>  		return;
> @@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>  	}
>  
> -	/* if link training is requested we should perform it always */
> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
> -			      intel_encoder->base.name);
> -		intel_dp_start_link_train(intel_dp);
> -		intel_dp_stop_link_train(intel_dp);
> -	}
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	intel_dp_check_link_status(intel_dp);
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>  }
>  
>  /* XXX this is probably wrong for multiple downstream ports */
> @@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  			}
>  		}
>  
> -		if (!intel_dp->is_mst) {
> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> -		}
> +		if (!intel_dp->is_mst)
> +			intel_dp_short_pulse(intel_dp);
>  	}
>  
>  	ret = IRQ_HANDLED;
> -- 
> 2.6.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Thulasimani, Sivakumar]

On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> Hi,
>
> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>> When created originally intel_dp_check_link_status()
>> was supposed to handle only link training for short
>> pulse but has grown into handler for short pulse itself.
>> This patch cleans up this function by splitting it into
>> two halves. First intel_dp_short_pulse() is called,
>> which will be entry point and handle all logic for
>> short pulse handling while intel_dp_check_link_status()
>> will retain its original purpose of only doing link
>> status related work.
>> The link retraining part when EQ is not correct is
>> retained to intel_dp_check_link_status whereas other
>> operations are handled as part of intel_dp_short_pulse.
>> This change is required to avoid performing all DPCD
>> related operations on performing link retraining.
>>
>> v2: Added WARN_ON to intel_dp_check_link_status()
>>      Removed a call to intel_dp_get_link_status() (Ander)
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> 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 | 65 +++++++++++++++++++++++------------------
>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 82ee18d..f8d9611 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4279,6 +4279,36 @@ go_again:
>>   	return -EINVAL;
>>   }
>>   
>> +static void
>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>> +
>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> +
>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +		DRM_ERROR("Failed to get link status\n");
>> +		return;
>> +	}
>> +
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
> Why do you change the order of the three if-clauses above?
> The original order seems to make more sense. (Checking for
> ->base.crtc and ->active is cheap, whereas accessing AUX to
> get the link status is time consuming. You don't want to
> spend that time only to bail out, should one of the other two
> if-clauses fail.)
>
> Best regards,
>
> Lukas
Actually it is expected to read link status whenever we receive short 
pulse interrupt
irrespective of the panel being enabled or not. So this change is with 
respect to
that rather than any performance based.
regards,
Sivakumar
>> +
>> +	/* if link training is requested we should perform it always */
>> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> +				intel_encoder->base.name);
>> +		intel_dp_start_link_train(intel_dp);
>> +		intel_dp_stop_link_train(intel_dp);
>> +	}
>> +}
>> +
>>   /*
>>    * According to DP spec
>>    * 5.1.2:
>> @@ -4288,14 +4318,10 @@ go_again:
>>    *  4. Check link status on receipt of hot-plug interrupt
>>    */
>>   static void
>> -intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +intel_dp_short_pulse(struct intel_dp *intel_dp)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	u8 sink_irq_vector;
>> -	u8 link_status[DP_LINK_STATUS_SIZE];
>> -
>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>>   	/*
>>   	 * Clearing compliance test variables to allow capturing
>> @@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   	intel_dp->compliance_test_type = 0;
>>   	intel_dp->compliance_test_data = 0;
>>   
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> -
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> -
>> -	/* Try to read receiver status if the link appears to be up */
>> -	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> -		return;
>> -	}
>> -
>>   	/* Now read the DPCD to see if it's actually running */
>>   	if (!intel_dp_get_dpcd(intel_dp)) {
>>   		return;
>> @@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
>>   	}
>>   
>> -	/* if link training is requested we should perform it always */
>> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> -			      intel_encoder->base.name);
>> -		intel_dp_start_link_train(intel_dp);
>> -		intel_dp_stop_link_train(intel_dp);
>> -	}
>> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +	intel_dp_check_link_status(intel_dp);
>> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>   }
>>   
>>   /* XXX this is probably wrong for multiple downstream ports */
>> @@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   			}
>>   		}
>>   
>> -		if (!intel_dp->is_mst) {
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> -		}
>> +		if (!intel_dp->is_mst)
>> +			intel_dp_short_pulse(intel_dp);
>>   	}
>>   
>>   	ret = IRQ_HANDLED;
>> -- 
>> 2.6.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Daniel Vetter]

On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> >Hi,
> >
> >On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> >>When created originally intel_dp_check_link_status()
> >>was supposed to handle only link training for short
> >>pulse but has grown into handler for short pulse itself.
> >>This patch cleans up this function by splitting it into
> >>two halves. First intel_dp_short_pulse() is called,
> >>which will be entry point and handle all logic for
> >>short pulse handling while intel_dp_check_link_status()
> >>will retain its original purpose of only doing link
> >>status related work.
> >>The link retraining part when EQ is not correct is
> >>retained to intel_dp_check_link_status whereas other
> >>operations are handled as part of intel_dp_short_pulse.
> >>This change is required to avoid performing all DPCD
> >>related operations on performing link retraining.
> >>
> >>v2: Added WARN_ON to intel_dp_check_link_status()
> >>     Removed a call to intel_dp_get_link_status() (Ander)
> >>
> >>Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> >>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 | 65 +++++++++++++++++++++++------------------
> >>  1 file changed, 36 insertions(+), 29 deletions(-)
> >>
> >>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>index 82ee18d..f8d9611 100644
> >>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>@@ -4279,6 +4279,36 @@ go_again:
> >>  	return -EINVAL;
> >>  }
> >>+static void
> >>+intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>+{
> >>+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >>+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>+	u8 link_status[DP_LINK_STATUS_SIZE];
> >>+
> >>+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >>+
> >>+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >>+		DRM_ERROR("Failed to get link status\n");
> >>+		return;
> >>+	}
> >>+
> >>+	if (!intel_encoder->base.crtc)
> >>+		return;
> >>+
> >>+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> >>+		return;
> >Why do you change the order of the three if-clauses above?
> >The original order seems to make more sense. (Checking for
> >->base.crtc and ->active is cheap, whereas accessing AUX to
> >get the link status is time consuming. You don't want to
> >spend that time only to bail out, should one of the other two
> >if-clauses fail.)
> >
> >Best regards,
> >
> >Lukas
> Actually it is expected to read link status whenever we receive short pulse
> interrupt
> irrespective of the panel being enabled or not. So this change is with
> respect to
> that rather than any performance based.

As a general rule please don't make functional changes like these in a
patch that just splits stuff up. Your patch summary sounds like simple
refactoring, which this doesn't seem to be.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Thulasimani, Sivakumar]

On 1/19/2016 2:14 PM, Daniel Vetter wrote:
> On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
>>> Hi,
>>>
>>> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>>>> When created originally intel_dp_check_link_status()
>>>> was supposed to handle only link training for short
>>>> pulse but has grown into handler for short pulse itself.
>>>> This patch cleans up this function by splitting it into
>>>> two halves. First intel_dp_short_pulse() is called,
>>>> which will be entry point and handle all logic for
>>>> short pulse handling while intel_dp_check_link_status()
>>>> will retain its original purpose of only doing link
>>>> status related work.
>>>> The link retraining part when EQ is not correct is
>>>> retained to intel_dp_check_link_status whereas other
>>>> operations are handled as part of intel_dp_short_pulse.
>>>> This change is required to avoid performing all DPCD
>>>> related operations on performing link retraining.
>>>>
>>>> v2: Added WARN_ON to intel_dp_check_link_status()
>>>>      Removed a call to intel_dp_get_link_status() (Ander)
>>>>
>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>> 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 | 65 +++++++++++++++++++++++------------------
>>>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 82ee18d..f8d9611 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -4279,6 +4279,36 @@ go_again:
>>>>   	return -EINVAL;
>>>>   }
>>>> +static void
>>>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>> +{
>>>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>>>> +
>>>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>> +
>>>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>> +		DRM_ERROR("Failed to get link status\n");
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (!intel_encoder->base.crtc)
>>>> +		return;
>>>> +
>>>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>> +		return;
>>> Why do you change the order of the three if-clauses above?
>>> The original order seems to make more sense. (Checking for
>>> ->base.crtc and ->active is cheap, whereas accessing AUX to
>>> get the link status is time consuming. You don't want to
>>> spend that time only to bail out, should one of the other two
>>> if-clauses fail.)
>>>
>>> Best regards,
>>>
>>> Lukas
>> Actually it is expected to read link status whenever we receive short pulse
>> interrupt
>> irrespective of the panel being enabled or not. So this change is with
>> respect to
>> that rather than any performance based.
> As a general rule please don't make functional changes like these in a
> patch that just splits stuff up. Your patch summary sounds like simple
> refactoring, which this doesn't seem to be.
> -Daniel
Understood, will make the appropriate changes and move that to separate 
patch.

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

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Daniel Vetter]

On Tue, Jan 19, 2016 at 02:29:22PM +0530, Thulasimani, Sivakumar wrote:
> 
> 
> On 1/19/2016 2:14 PM, Daniel Vetter wrote:
> >On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
> >>
> >>On 1/19/2016 2:35 AM, Lukas Wunner wrote:
> >>>Hi,
> >>>
> >>>On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
> >>>>When created originally intel_dp_check_link_status()
> >>>>was supposed to handle only link training for short
> >>>>pulse but has grown into handler for short pulse itself.
> >>>>This patch cleans up this function by splitting it into
> >>>>two halves. First intel_dp_short_pulse() is called,
> >>>>which will be entry point and handle all logic for
> >>>>short pulse handling while intel_dp_check_link_status()
> >>>>will retain its original purpose of only doing link
> >>>>status related work.
> >>>>The link retraining part when EQ is not correct is
> >>>>retained to intel_dp_check_link_status whereas other
> >>>>operations are handled as part of intel_dp_short_pulse.
> >>>>This change is required to avoid performing all DPCD
> >>>>related operations on performing link retraining.
> >>>>
> >>>>v2: Added WARN_ON to intel_dp_check_link_status()
> >>>>     Removed a call to intel_dp_get_link_status() (Ander)
> >>>>
> >>>>Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> >>>>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 | 65 +++++++++++++++++++++++------------------
> >>>>  1 file changed, 36 insertions(+), 29 deletions(-)
> >>>>
> >>>>diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >>>>index 82ee18d..f8d9611 100644
> >>>>--- a/drivers/gpu/drm/i915/intel_dp.c
> >>>>+++ b/drivers/gpu/drm/i915/intel_dp.c
> >>>>@@ -4279,6 +4279,36 @@ go_again:
> >>>>  	return -EINVAL;
> >>>>  }
> >>>>+static void
> >>>>+intel_dp_check_link_status(struct intel_dp *intel_dp)
> >>>>+{
> >>>>+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
> >>>>+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
> >>>>+	u8 link_status[DP_LINK_STATUS_SIZE];
> >>>>+
> >>>>+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >>>>+
> >>>>+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >>>>+		DRM_ERROR("Failed to get link status\n");
> >>>>+		return;
> >>>>+	}
> >>>>+
> >>>>+	if (!intel_encoder->base.crtc)
> >>>>+		return;
> >>>>+
> >>>>+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> >>>>+		return;
> >>>Why do you change the order of the three if-clauses above?
> >>>The original order seems to make more sense. (Checking for
> >>>->base.crtc and ->active is cheap, whereas accessing AUX to
> >>>get the link status is time consuming. You don't want to
> >>>spend that time only to bail out, should one of the other two
> >>>if-clauses fail.)
> >>>
> >>>Best regards,
> >>>
> >>>Lukas
> >>Actually it is expected to read link status whenever we receive short pulse
> >>interrupt
> >>irrespective of the panel being enabled or not. So this change is with
> >>respect to
> >>that rather than any performance based.
> >As a general rule please don't make functional changes like these in a
> >patch that just splits stuff up. Your patch summary sounds like simple
> >refactoring, which this doesn't seem to be.
> >-Daniel
> Understood, will make the appropriate changes and move that to separate
> patch.

btw you don't have to split it since really this is a small change.
Changing the subject to something that makes is clearer that it's not just
refactoring is also ok, e.g. "reorganize intel_dp_detect"

Then explain in the commit message why and what changes, like you do
already.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Splitting intel_dp_check_link_status

[Thulasimani, Sivakumar]

On 1/19/2016 2:35 PM, Daniel Vetter wrote:
> On Tue, Jan 19, 2016 at 02:29:22PM +0530, Thulasimani, Sivakumar wrote:
>>
>> On 1/19/2016 2:14 PM, Daniel Vetter wrote:
>>> On Tue, Jan 19, 2016 at 10:14:30AM +0530, Thulasimani, Sivakumar wrote:
>>>> On 1/19/2016 2:35 AM, Lukas Wunner wrote:
>>>>> Hi,
>>>>>
>>>>> On Mon, Jan 18, 2016 at 04:22:19PM +0530, Shubhangi Shrivastava wrote:
>>>>>> When created originally intel_dp_check_link_status()
>>>>>> was supposed to handle only link training for short
>>>>>> pulse but has grown into handler for short pulse itself.
>>>>>> This patch cleans up this function by splitting it into
>>>>>> two halves. First intel_dp_short_pulse() is called,
>>>>>> which will be entry point and handle all logic for
>>>>>> short pulse handling while intel_dp_check_link_status()
>>>>>> will retain its original purpose of only doing link
>>>>>> status related work.
>>>>>> The link retraining part when EQ is not correct is
>>>>>> retained to intel_dp_check_link_status whereas other
>>>>>> operations are handled as part of intel_dp_short_pulse.
>>>>>> This change is required to avoid performing all DPCD
>>>>>> related operations on performing link retraining.
>>>>>>
>>>>>> v2: Added WARN_ON to intel_dp_check_link_status()
>>>>>>      Removed a call to intel_dp_get_link_status() (Ander)
>>>>>>
>>>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>>>> 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 | 65 +++++++++++++++++++++++------------------
>>>>>>   1 file changed, 36 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> index 82ee18d..f8d9611 100644
>>>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>>>> @@ -4279,6 +4279,36 @@ go_again:
>>>>>>   	return -EINVAL;
>>>>>>   }
>>>>>> +static void
>>>>>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>>>>>> +{
>>>>>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>>>> +	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>>>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>>>>>> +
>>>>>> +	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>>>> +
>>>>>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>>>> +		DRM_ERROR("Failed to get link status\n");
>>>>>> +		return;
>>>>>> +	}
>>>>>> +
>>>>>> +	if (!intel_encoder->base.crtc)
>>>>>> +		return;
>>>>>> +
>>>>>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>>>>>> +		return;
>>>>> Why do you change the order of the three if-clauses above?
>>>>> The original order seems to make more sense. (Checking for
>>>>> ->base.crtc and ->active is cheap, whereas accessing AUX to
>>>>> get the link status is time consuming. You don't want to
>>>>> spend that time only to bail out, should one of the other two
>>>>> if-clauses fail.)
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Lukas
>>>> Actually it is expected to read link status whenever we receive short pulse
>>>> interrupt
>>>> irrespective of the panel being enabled or not. So this change is with
>>>> respect to
>>>> that rather than any performance based.
>>> As a general rule please don't make functional changes like these in a
>>> patch that just splits stuff up. Your patch summary sounds like simple
>>> refactoring, which this doesn't seem to be.
>>> -Daniel
>> Understood, will make the appropriate changes and move that to separate
>> patch.
> btw you don't have to split it since really this is a small change.
> Changing the subject to something that makes is clearer that it's not just
> refactoring is also ok, e.g. "reorganize intel_dp_detect"
>
> Then explain in the commit message why and what changes, like you do
> already.
> -Daniel
Sure, that will save some time in redoing ULT+upstreaming :).
to give some background, the movement was supposed to be a separate
patch but got merged during this cleanup. Will make sure that gets
documented and split clearly as required hence forth.

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

[PATCH] drm/i915: Reorganizing intel_dp_check_link_status

[Shubhangi Shrivastava]

When created originally intel_dp_check_link_status()
was supposed to handle only link training for short
pulse but has grown into handler for short pulse itself.
This patch cleans up this function by splitting it into
two halves. First intel_dp_short_pulse() is called,
which will be entry point and handle all logic for
short pulse handling while intel_dp_check_link_status()
will retain its original purpose of only doing link
status related work.

intel_dp_short_pulse: All existing code other than
link status read and link training upon error status.

intel_dp_check_link_status:
The link status should be read on short pulse
irrespective of panel being enabled or not so
intel_dp_get_link_status() performs dpcd read first
then based on crtc active / enabled it will
perform the link training.

v2: Added WARN_ON to intel_dp_check_link_status()
    Removed a call to intel_dp_get_link_status() (Ander)

v3: Changed commit message to explain need of link status
    being read before performing encoder checks (Daniel)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 65 +++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 82ee18d..f8d9611 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4279,6 +4279,36 @@ go_again:
 	return -EINVAL;
 }
 
+static void
+intel_dp_check_link_status(struct intel_dp *intel_dp)
+{
+	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_device *dev = intel_dp_to_dev(intel_dp);
+	u8 link_status[DP_LINK_STATUS_SIZE];
+
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+
+	if (!intel_dp_get_link_status(intel_dp, link_status)) {
+		DRM_ERROR("Failed to get link status\n");
+		return;
+	}
+
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
+	/* if link training is requested we should perform it always */
+	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
+		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
+		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
+				intel_encoder->base.name);
+		intel_dp_start_link_train(intel_dp);
+		intel_dp_stop_link_train(intel_dp);
+	}
+}
+
 /*
  * According to DP spec
  * 5.1.2:
@@ -4288,14 +4318,10 @@ go_again:
  *  4. Check link status on receipt of hot-plug interrupt
  */
 static void
-intel_dp_check_link_status(struct intel_dp *intel_dp)
+intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
-	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	u8 sink_irq_vector;
-	u8 link_status[DP_LINK_STATUS_SIZE];
-
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4305,17 +4331,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	intel_dp->compliance_test_type = 0;
 	intel_dp->compliance_test_data = 0;
 
-	if (!intel_encoder->base.crtc)
-		return;
-
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
-
-	/* Try to read receiver status if the link appears to be up */
-	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		return;
-	}
-
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
 		return;
@@ -4335,14 +4350,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	/* if link training is requested we should perform it always */
-	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
-		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
-		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
-			      intel_encoder->base.name);
-		intel_dp_start_link_train(intel_dp);
-		intel_dp_stop_link_train(intel_dp);
-	}
+	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
+	intel_dp_check_link_status(intel_dp);
+	drm_modeset_unlock(&dev->mode_config.connection_mutex);
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5072,11 +5082,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst) {
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
-		}
+		if (!intel_dp->is_mst)
+			intel_dp_short_pulse(intel_dp);
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

Re: [PATCH 3/6] drm/i915: Splitting intel_dp_check_link_status

[Shubhangi Shrivastava]

On Wednesday 13 January 2016 08:34 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>> When created originally intel_dp_check_link_status()
>> was supposed to handle only link training for short
>> pulse but has grown into handler for short pulse itself.
>> This patch cleans up this function by splitting it into
>> two halves. First intel_dp_short_pulse() is called,
>> which will be entry point and handle all logic for
>> short pulse handling while intel_dp_check_link_status()
>> will retain its original purpose of only doing link
>> status related work.
>> The link retraining part when EQ is not correct is
>> retained to intel_dp_check_link_status whereas other
>> operations are handled as part of intel_dp_short_pulse.
>> This change is required to avoid performing all DPCD
>> related operations on performing link retraining.
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> 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 | 56 ++++++++++++++++++++++++----------------
>> -
>>   1 file changed, 33 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 137757b..842790e 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4289,6 +4289,33 @@ go_again:
>>   	return -EINVAL;
>>   }
>>   
>> +static void
>> +intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +{
>> +	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)
>> ->base;
>> +	u8 link_status[DP_LINK_STATUS_SIZE];
>> +
>> +	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> +		DRM_ERROR("Failed to get link status\n");
>> +		return;
>> +	}
>> +
>> +	if (!intel_encoder->base.crtc)
>> +		return;
>> +
>> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> +		return;
>> +
>> +	/* if link training is requested we should perform it always */
>> +	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> +		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> +		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> +				intel_encoder->base.name);
>> +		intel_dp_start_link_train(intel_dp);
>> +		intel_dp_stop_link_train(intel_dp);
>> +	}
>> +}
>> +
>>   /*
>>    * According to DP spec
>>    * 5.1.2:
>> @@ -4298,15 +4325,12 @@ go_again:
>>    *  4. Check link status on receipt of hot-plug interrupt
>>    */
>>   static void
>> -intel_dp_check_link_status(struct intel_dp *intel_dp)
>> +intel_dp_short_pulse(struct intel_dp *intel_dp)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>> -	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)
>> ->base;
>>   	u8 sink_irq_vector;
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>   
>> -	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> -
> I think it's better to move this WARN to the new intel_dp_check_link_status().

Sure.. Done..

>>   	/*
>>   	 * Clearing compliance test variables to allow capturing
>>   	 * of values for next automated test request.
>> @@ -4315,12 +4339,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   	intel_dp->compliance_test_type = 0;
>>   	intel_dp->compliance_test_data = 0;
>>   
>> -	if (!intel_encoder->base.crtc)
>> -		return;
>> -
>> -	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> -		return;
>> -
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>   		return;
> There is now two calls to intel_dp_get_link_status()and the value of link_status
> is not used in this function, so maybe just remove it from here. Looks good
> otherwise.
>
> Ander

Sure.. Done..

>> @@ -4345,14 +4363,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>   			DRM_DEBUG_DRIVER("CP or sink specific irq
>> unhandled\n");
>>   	}
>>   
>> -	/* if link training is requested we should perform it always */
>> -	if ((intel_dp->compliance_test_type == DP_TEST_LINK_TRAINING) ||
>> -		(!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count))) {
>> -		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>> -			      intel_encoder->base.name);
>> -		intel_dp_start_link_train(intel_dp);
>> -		intel_dp_stop_link_train(intel_dp);
>> -	}
>> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> +	intel_dp_check_link_status(intel_dp);
>> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>   }
>>   
>>   /* XXX this is probably wrong for multiple downstream ports */
>> @@ -5080,11 +5093,8 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port, bool long_hpd)
>>   			}
>>   		}
>>   
>> -		if (!intel_dp->is_mst) {
>> -			drm_modeset_lock(&dev->mode_config.connection_mutex,
>> NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> -			drm_modeset_unlock(&dev
>> ->mode_config.connection_mutex);
>> -		}
>> +		if (!intel_dp->is_mst)
>> +			intel_dp_short_pulse(intel_dp);
>>   	}
>>   
>>   	ret = IRQ_HANDLED;

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

[PATCH 4/6] drm/i915: Save sink_count for tracking changes to it

[Shubhangi Shrivastava]

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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  | 7 +++----
 drivers/gpu/drm/i915/intel_drv.h | 1 +
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 842790e..c2e8516 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4385,14 +4385,13 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	/* If we're HPD-aware, SINK_COUNT changes dynamically */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
-		uint8_t reg;
 
 		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
+					    &intel_dp->sink_count, 1) < 0)
 			return connector_status_unknown;
 
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
+		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
+		connector_status_connected : connector_status_disconnected;
 	}
 
 	/* If no HPD, poke DDC gently */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0438b57..88b05ba 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -757,6 +757,7 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	uint8_t sink_count;
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
-- 
2.6.1

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

Re: [PATCH 4/6] drm/i915: Save sink_count for tracking changes to it

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> Sink count can change between short pulse hpd hence this patch
> adds a member variable to intel_dp so we can track any changes
> between short pulse interrupts.
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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  | 7 +++----
>  drivers/gpu/drm/i915/intel_drv.h | 1 +
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 842790e..c2e8516 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4385,14 +4385,13 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> -		uint8_t reg;
>  
>  		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &reg, 1) < 0)
> +					    &intel_dp->sink_count, 1) < 0)
>  			return connector_status_unknown;
>  
> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
> -					      :
> connector_status_disconnected;
> +		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
> +		connector_status_connected : connector_status_disconnected;

I think it would be better to have the value of intel_dp->sink_count ready for
consumption, i.e., store the result of DP_GET_SINK_COUNT().

Ander

>  	}
>  
>  	/* If no HPD, poke DDC gently */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 0438b57..88b05ba 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -757,6 +757,7 @@ struct intel_dp {
>  	uint32_t DP;
>  	int link_rate;
>  	uint8_t lane_count;
> +	uint8_t sink_count;
>  	bool has_audio;
>  	enum hdmi_force_audio force_audio;
>  	bool limited_color_range;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/6] drm/i915: Save sink_count for tracking changes to it

[Shubhangi Shrivastava]

On Thursday 14 January 2016 06:30 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>> Sink count can change between short pulse hpd hence this patch
>> adds a member variable to intel_dp so we can track any changes
>> between short pulse interrupts.
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> 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  | 7 +++----
>>   drivers/gpu/drm/i915/intel_drv.h | 1 +
>>   2 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 842790e..c2e8516 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4385,14 +4385,13 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	/* If we're HPD-aware, SINK_COUNT changes dynamically */
>>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>   	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>> -		uint8_t reg;
>>   
>>   		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> -					    &reg, 1) < 0)
>> +					    &intel_dp->sink_count, 1) < 0)
>>   			return connector_status_unknown;
>>   
>> -		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
>> -					      :
>> connector_status_disconnected;
>> +		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>> +		connector_status_connected : connector_status_disconnected;
> I think it would be better to have the value of intel_dp->sink_count ready for
> consumption, i.e., store the result of DP_GET_SINK_COUNT().
>
> Ander

Sure.. Done..

>>   	}
>>   
>>   	/* If no HPD, poke DDC gently */
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index 0438b57..88b05ba 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -757,6 +757,7 @@ struct intel_dp {
>>   	uint32_t DP;
>>   	int link_rate;
>>   	uint8_t lane_count;
>> +	uint8_t sink_count;
>>   	bool has_audio;
>>   	enum hdmi_force_audio force_audio;
>>   	bool limited_color_range;

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

[PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

This patch reads sink_count dpcd always and removes its
read operation based on values in downstream port dpcd.

SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
SINK_COUNT denotes if a display is attached, while
DOWNSTREAM_PORT_PRESET indicates how many ports are available
in the dongle where display can be attached. so it is possible
for sink count to change irrespective of value in downstream
port dpcd.

Here is a table of possible values and scenarios

sink_count      downstream_port
                present
0               0               no display is attached
0               1               dongle is connected without display
1               0               display connected directly
1               1               display connected through dongle

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c2e8516..0d58bfd 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+				    &intel_dp->sink_count, 1) < 0)
+		return false;
+
+	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
 
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &intel_dp->sink_count, 1) < 0)
-			return connector_status_unknown;
-
 		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
 		connector_status_connected : connector_status_disconnected;
 	}
-- 
2.6.1

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

Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> This patch reads sink_count dpcd always and removes its
> read operation based on values in downstream port dpcd.
> 
> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
> SINK_COUNT denotes if a display is attached, while
> DOWNSTREAM_PORT_PRESET indicates how many ports are available
> in the dongle where display can be attached. so it is possible
> for sink count to change irrespective of value in downstream
> port dpcd.
> 
> Here is a table of possible values and scenarios
> 
> sink_count      downstream_port
>                 present
> 0               0               no display is attached
> 0               1               dongle is connected without display
> 1               0               display connected directly
> 1               1               display connected through dongle
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c2e8516..0d58bfd 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>  		return false; /* DPCD not present */
>  
> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> +				    &intel_dp->sink_count, 1) < 0)
> +		return false;
> +
> +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
> +		return false;
> +

My understanding is that this function should only read the DPCD data while
detection based on that data is done in intel_dp_detect_dpcd(). With the return
on sink_count == 0 here, we skip the end of the function, which updates the
cached downstream port information. Is there a reason why we need this early
return here?

Also, I think this could be squashed with the previous patch.

Ander

>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>  
> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &intel_dp->sink_count, 1) < 0)
> -			return connector_status_unknown;
> -
>  		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>  		connector_status_connected : connector_status_disconnected;
>  	}
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>> This patch reads sink_count dpcd always and removes its
>> read operation based on values in downstream port dpcd.
>>
>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>> SINK_COUNT denotes if a display is attached, while
>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>> in the dongle where display can be attached. so it is possible
>> for sink count to change irrespective of value in downstream
>> port dpcd.
>>
>> Here is a table of possible values and scenarios
>>
>> sink_count      downstream_port
>>                  present
>> 0               0               no display is attached
>> 0               1               dongle is connected without display
>> 1               0               display connected directly
>> 1               1               display connected through dongle
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> 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 | 11 +++++++----
>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index c2e8516..0d58bfd 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>   		return false; /* DPCD not present */
>>   
>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> +				    &intel_dp->sink_count, 1) < 0)
>> +		return false;
>> +
>> +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
>> +		return false;
>> +
> My understanding is that this function should only read the DPCD data while
> detection based on that data is done in intel_dp_detect_dpcd(). With the return
> on sink_count == 0 here, we skip the end of the function, which updates the
> cached downstream port information. Is there a reason why we need this early
> return here?
>
> Also, I think this could be squashed with the previous patch.
>
> Ander
As described in the commit message, if sink_count is 0, then there is no 
display present. So, irrespective of value of downstream port, we should 
terminate the function and thus, an early return is present here.

>>   	/* Check if the panel supports PSR */
>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>   	if (is_edp(intel_dp)) {
>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>   	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>   
>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>> -					    &intel_dp->sink_count, 1) < 0)
>> -			return connector_status_unknown;
>> -
>>   		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>>   		connector_status_connected : connector_status_disconnected;
>>   	}

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

Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

On Monday 18 January 2016 06:14 PM, Shubhangi Shrivastava wrote:
>
>
> On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote:
>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>> This patch reads sink_count dpcd always and removes its
>>> read operation based on values in downstream port dpcd.
>>>
>>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>>> SINK_COUNT denotes if a display is attached, while
>>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>>> in the dongle where display can be attached. so it is possible
>>> for sink count to change irrespective of value in downstream
>>> port dpcd.
>>>
>>> Here is a table of possible values and scenarios
>>>
>>> sink_count      downstream_port
>>>                  present
>>> 0               0               no display is attached
>>> 0               1               dongle is connected without display
>>> 1               0               display connected directly
>>> 1               1               display connected through dongle
>>>
>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>> 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 | 11 +++++++----
>>>   1 file changed, 7 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index c2e8516..0d58bfd 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>       if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>>           return false; /* DPCD not present */
>>>   +    if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>> +                    &intel_dp->sink_count, 1) < 0)
>>> +        return false;
>>> +
>>> +    if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
>>> +        return false;
>>> +
>> My understanding is that this function should only read the DPCD data 
>> while
>> detection based on that data is done in intel_dp_detect_dpcd(). With 
>> the return
>> on sink_count == 0 here, we skip the end of the function, which 
>> updates the
>> cached downstream port information. Is there a reason why we need 
>> this early
>> return here?
>>
>> Also, I think this could be squashed with the previous patch.
>>
>> Ander
> As described in the commit message, if sink_count is 0, then there is 
> no display present. So, irrespective of value of downstream port, we 
> should terminate the function and thus, an early return is present here.
>
Squashing this patch with the previous one.
>>>       /* Check if the panel supports PSR */
>>>       memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>       if (is_edp(intel_dp)) {
>>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>       if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>           intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>>   -        if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>> -                        &intel_dp->sink_count, 1) < 0)
>>> -            return connector_status_unknown;
>>> -
>>>           return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>>>           connector_status_connected : connector_status_disconnected;
>>>       }
>

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

[PATCH] drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always

[Shubhangi Shrivastava]

Sink count can change between short pulse hpd hence this patch
adds a member variable to intel_dp so we can track any changes
between short pulse interrupts.

This patch reads sink_count dpcd always and removes its
read operation based on values in downstream port dpcd.

SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
SINK_COUNT denotes if a display is attached, while
DOWNSTREAM_PORT_PRESET indicates how many ports are available
in the dongle where display can be attached. so it is possible
for sink count to change irrespective of value in downstream
port dpcd.

Here is a table of possible values and scenarios

sink_count      downstream_port
                present
0               0               no display is attached
0               1               dongle is connected without display
1               0               display connected directly
1               1               display connected through dongle

v2: Storing value of intel_dp->sink_count that is ready
    for consumption. (Ander)
    Squashing two commits into one. (Ander)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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  | 18 +++++++++++-------
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f8d9611..cdf4919 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3855,6 +3855,15 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
 		return false; /* DPCD not present */
 
+	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
+				    &intel_dp->sink_count, 1) < 0)
+		return false;
+
+	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+
+	if (!intel_dp->sink_count)
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4372,14 +4381,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	/* If we're HPD-aware, SINK_COUNT changes dynamically */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
-		uint8_t reg;
-
-		if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
-			return connector_status_unknown;
 
-		return DP_GET_SINK_COUNT(reg) ? connector_status_connected
-					      : connector_status_disconnected;
+		return intel_dp->sink_count ?
+		connector_status_connected : connector_status_disconnected;
 	}
 
 	/* If no HPD, poke DDC gently */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 059b46e..0879466 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -774,6 +774,7 @@ struct intel_dp {
 	uint32_t DP;
 	int link_rate;
 	uint8_t lane_count;
+	uint8_t sink_count;
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
-- 
2.6.1

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

Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Ander Conselvan De Oliveira]

On Mon, 2016-01-18 at 18:14 +0530, Shubhangi Shrivastava wrote:
> 
> On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote:
> > On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> > > This patch reads sink_count dpcd always and removes its
> > > read operation based on values in downstream port dpcd.
> > > 
> > > SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
> > > SINK_COUNT denotes if a display is attached, while
> > > DOWNSTREAM_PORT_PRESET indicates how many ports are available
> > > in the dongle where display can be attached. so it is possible
> > > for sink count to change irrespective of value in downstream
> > > port dpcd.
> > > 
> > > Here is a table of possible values and scenarios
> > > 
> > > sink_count      downstream_port
> > >                  present
> > > 0               0               no display is attached
> > > 0               1               dongle is connected without display
> > > 1               0               display connected directly
> > > 1               1               display connected through dongle
> > > 
> > > Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> > > 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 | 11 +++++++----
> > >   1 file changed, 7 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > > b/drivers/gpu/drm/i915/intel_dp.c
> > > index c2e8516..0d58bfd 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
> > >   	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
> > >   		return false; /* DPCD not present */
> > >   
> > > +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> > > +				    &intel_dp->sink_count, 1) < 0)
> > > +		return false;
> > > +
> > > +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
> > > +		return false;
> > > +
> > My understanding is that this function should only read the DPCD data while
> > detection based on that data is done in intel_dp_detect_dpcd(). With the
> > return
> > on sink_count == 0 here, we skip the end of the function, which updates the
> > cached downstream port information. Is there a reason why we need this early
> > return here?
> > 
> > Also, I think this could be squashed with the previous patch.
> > 
> > Ander
> As described in the commit message, if sink_count is 0, then there is no 
> display present. So, irrespective of value of downstream port, we should 
> terminate the function and thus, an early return is present here.

You wrote that "SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT
dpcd". Now, the get_dpcd() function is called from different places with the
purpose of retrieving information stored in dpcd. By adding the early return,
the downstream port information, which you claimed is independent from sink
count, is not updated.

The way I see it, you should terminate detection when sink count is 0, not the
reading of DPCD. That way the logical split between intel_dp_get_dpcd() and
intel_dp_detect_dpcd() is maintained. The former reads DPCD and the latter
reasons about it.

Ander

> 
> > >   	/* Check if the panel supports PSR */
> > >   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
> > >   	if (is_edp(intel_dp)) {
> > > @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
> > >   	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> > >   	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
> > >   
> > > -		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
> > > DP_SINK_COUNT,
> > > -					    &intel_dp->sink_count, 1) <
> > > 0)
> > > -			return connector_status_unknown;
> > > -
> > >   		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
> > >   		connector_status_connected :
> > > connector_status_disconnected;
> > >   	}
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/6] drm/i915: read sink_count dpcd always

[Shubhangi Shrivastava]

On Monday 18 January 2016 06:30 PM, Ander Conselvan De Oliveira wrote:
> On Mon, 2016-01-18 at 18:14 +0530, Shubhangi Shrivastava wrote:
>> On Thursday 14 January 2016 06:34 PM, Ander Conselvan De Oliveira wrote:
>>> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>>>> This patch reads sink_count dpcd always and removes its
>>>> read operation based on values in downstream port dpcd.
>>>>
>>>> SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd.
>>>> SINK_COUNT denotes if a display is attached, while
>>>> DOWNSTREAM_PORT_PRESET indicates how many ports are available
>>>> in the dongle where display can be attached. so it is possible
>>>> for sink count to change irrespective of value in downstream
>>>> port dpcd.
>>>>
>>>> Here is a table of possible values and scenarios
>>>>
>>>> sink_count      downstream_port
>>>>                   present
>>>> 0               0               no display is attached
>>>> 0               1               dongle is connected without display
>>>> 1               0               display connected directly
>>>> 1               1               display connected through dongle
>>>>
>>>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>>>> 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 | 11 +++++++----
>>>>    1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>>> b/drivers/gpu/drm/i915/intel_dp.c
>>>> index c2e8516..0d58bfd 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3865,6 +3865,13 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>>    	if (intel_dp->dpcd[DP_DPCD_REV] == 0)
>>>>    		return false; /* DPCD not present */
>>>>    
>>>> +	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
>>>> +				    &intel_dp->sink_count, 1) < 0)
>>>> +		return false;
>>>> +
>>>> +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
>>>> +		return false;
>>>> +
>>> My understanding is that this function should only read the DPCD data while
>>> detection based on that data is done in intel_dp_detect_dpcd(). With the
>>> return
>>> on sink_count == 0 here, we skip the end of the function, which updates the
>>> cached downstream port information. Is there a reason why we need this early
>>> return here?
>>>
>>> Also, I think this could be squashed with the previous patch.
>>>
>>> Ander
>> As described in the commit message, if sink_count is 0, then there is no
>> display present. So, irrespective of value of downstream port, we should
>> terminate the function and thus, an early return is present here.
> You wrote that "SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT
> dpcd". Now, the get_dpcd() function is called from different places with the
> purpose of retrieving information stored in dpcd. By adding the early return,
> the downstream port information, which you claimed is independent from sink
> count, is not updated.
>
> The way I see it, you should terminate detection when sink count is 0, not the
> reading of DPCD. That way the logical split between intel_dp_get_dpcd() and
> intel_dp_detect_dpcd() is maintained. The former reads DPCD and the latter
> reasons about it.
>
> Ander
Yes, that's how it is.. But, SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT 
== 1 implies that a dongle is present but no display. Unless we require 
to know if a dongle is present or not, we don't need to update 
downstream port information. So, an early return here saves time from 
performing other operations which are not required.
>>>>    	/* Check if the panel supports PSR */
>>>>    	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>>    	if (is_edp(intel_dp)) {
>>>> @@ -4386,10 +4393,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>>    	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>>>>    	    intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) {
>>>>    
>>>> -		if (intel_dp_dpcd_read_wake(&intel_dp->aux,
>>>> DP_SINK_COUNT,
>>>> -					    &intel_dp->sink_count, 1) <
>>>> 0)
>>>> -			return connector_status_unknown;
>>>> -
>>>>    		return DP_GET_SINK_COUNT(intel_dp->sink_count) ?
>>>>    		connector_status_connected :
>>>> connector_status_disconnected;
>>>>    	}

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

[PATCH 6/6] drm/i915: force full detect on sink count change

[Shubhangi Shrivastava]

This patch checks for changes in sink count between short pulse
hpds and forces full detect when there is a change.

This will allow both detection of hotplug and unplug of panels
through dongles that give only short pulse for such events.

v2: changed variable type from u8 to bool (Jani)
    return immediately if perform_full_detect is set(Siva)

v3: changed method of determining full detection from using
    pointer to return code (Siva)

Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
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 | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 0d58bfd..8a659ee 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4331,12 +4331,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
  *  3. Use Link Training from 2.5.3.3 and 3.5.1.3
  *  4. Check link status on receipt of hot-plug interrupt
  */
-static void
+static bool
 intel_dp_short_pulse(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
+	u8 old_sink_count = intel_dp->sink_count;
+	bool ret;
 
 	/*
 	 * Clearing compliance test variables to allow capturing
@@ -4348,12 +4350,20 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		return;
+		return false;
 	}
 
-	/* Now read the DPCD to see if it's actually running */
-	if (!intel_dp_get_dpcd(intel_dp)) {
-		return;
+	/*
+	 * Now read the DPCD to see if it's actually running
+	 * Don't return immediately if dpcd read failed,
+	 * if sink count was 1 and dpcd read failed we need
+	 * to do full detection
+	 */
+	ret = intel_dp_get_dpcd(intel_dp);
+
+	if ((old_sink_count != intel_dp->sink_count) || !ret) {
+		/* No need to proceed if we are going to do full detect */
+		return false;
 	}
 
 	/* Try to read the source of the interrupt */
@@ -4373,6 +4383,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
 	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 	intel_dp_check_link_status(intel_dp);
 	drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+	return true;
 }
 
 /* XXX this is probably wrong for multiple downstream ports */
@@ -5095,8 +5107,12 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 			}
 		}
 
-		if (!intel_dp->is_mst)
-			intel_dp_short_pulse(intel_dp);
+		if (!intel_dp->is_mst) {
+			if (!intel_dp_short_pulse(intel_dp)) {
+				intel_dp_long_pulse(intel_dp->attached_connector);
+				goto put_power;
+			}
+		}
 	}
 
 	ret = IRQ_HANDLED;
-- 
2.6.1

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

Re: [PATCH 6/6] drm/i915: force full detect on sink count change

[Ander Conselvan De Oliveira]

On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
> This patch checks for changes in sink count between short pulse
> hpds and forces full detect when there is a change.
> 
> This will allow both detection of hotplug and unplug of panels
> through dongles that give only short pulse for such events.
> 
> v2: changed variable type from u8 to bool (Jani)
>     return immediately if perform_full_detect is set(Siva)
> 
> v3: changed method of determining full detection from using
>     pointer to return code (Siva)
> 
> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
> 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 | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 0d58bfd..8a659ee 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4331,12 +4331,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>   *  3. Use Link Training from 2.5.3.3 and 3.5.1.3
>   *  4. Check link status on receipt of hot-plug interrupt
>   */
> -static void
> +static bool

Please expand the comment above to indicate what the return value of this
function is supposed to mean.


>  intel_dp_short_pulse(struct intel_dp *intel_dp)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	u8 sink_irq_vector;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
> +	u8 old_sink_count = intel_dp->sink_count;
> +	bool ret;
>  
>  	/*
>  	 * Clearing compliance test variables to allow capturing
> @@ -4348,12 +4350,20 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  
>  	/* Try to read receiver status if the link appears to be up */
>  	if (!intel_dp_get_link_status(intel_dp, link_status)) {
> -		return;
> +		return false;
>  	}
>  
> -	/* Now read the DPCD to see if it's actually running */
> -	if (!intel_dp_get_dpcd(intel_dp)) {
> -		return;
> +	/*
> +	 * Now read the DPCD to see if it's actually running
> +	 * Don't return immediately if dpcd read failed,
> +	 * if sink count was 1 and dpcd read failed we need
> +	 * to do full detection
> +	 */
> +	ret = intel_dp_get_dpcd(intel_dp);
> +
> +	if ((old_sink_count != intel_dp->sink_count) || !ret) {

I don't see the connection of the comment above with this. If the dpcd read
fails, the 'return false' will be reached regardless of the previous value of
intel_dp->sink_count. Did you intend to do something different or did I miss
something?


> +		/* No need to proceed if we are going to do full detect */
> +		return false;
>  	}
>  
>  	/* Try to read the source of the interrupt */
> @@ -4373,6 +4383,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>  	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>  	intel_dp_check_link_status(intel_dp);
>  	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +	return true;
>  }
>  
>  /* XXX this is probably wrong for multiple downstream ports */
> @@ -5095,8 +5107,12 @@ intel_dp_hpd_pulse(struct intel_digital_port
> *intel_dig_port, bool long_hpd)
>  			}
>  		}
>  
> -		if (!intel_dp->is_mst)
> -			intel_dp_short_pulse(intel_dp);
> +		if (!intel_dp->is_mst) {
> +			if (!intel_dp_short_pulse(intel_dp)) {
> +				intel_dp_long_pulse(intel_dp
> ->attached_connector);
> +				goto put_power;

It could be in a follow up patch, but I think its a good moment to get rid of
the goto put_power. The only thing they do is skip the 'ret = IRQ_HANDLED'
assignment now.

Ander

> +			}
> +		}
>  	}
>  
>  	ret = IRQ_HANDLED;
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/6] drm/i915: force full detect on sink count change

[Shubhangi Shrivastava]

On Thursday 14 January 2016 07:20 PM, Ander Conselvan De Oliveira wrote:
> On Tue, 2016-01-05 at 18:20 +0530, Shubhangi Shrivastava wrote:
>> This patch checks for changes in sink count between short pulse
>> hpds and forces full detect when there is a change.
>>
>> This will allow both detection of hotplug and unplug of panels
>> through dongles that give only short pulse for such events.
>>
>> v2: changed variable type from u8 to bool (Jani)
>>      return immediately if perform_full_detect is set(Siva)
>>
>> v3: changed method of determining full detection from using
>>      pointer to return code (Siva)
>>
>> Tested-by: Nathan D Ciobanu <nathan.d.ciobanu@intel.com>
>> 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 | 30 +++++++++++++++++++++++-------
>>   1 file changed, 23 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 0d58bfd..8a659ee 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4331,12 +4331,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>>    *  3. Use Link Training from 2.5.3.3 and 3.5.1.3
>>    *  4. Check link status on receipt of hot-plug interrupt
>>    */
>> -static void
>> +static bool
> Please expand the comment above to indicate what the return value of this
> function is supposed to mean.
>
Sure.. Will add..

>>   intel_dp_short_pulse(struct intel_dp *intel_dp)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>   	u8 sink_irq_vector;
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>> +	u8 old_sink_count = intel_dp->sink_count;
>> +	bool ret;
>>   
>>   	/*
>>   	 * Clearing compliance test variables to allow capturing
>> @@ -4348,12 +4350,20 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>>   
>>   	/* Try to read receiver status if the link appears to be up */
>>   	if (!intel_dp_get_link_status(intel_dp, link_status)) {
>> -		return;
>> +		return false;
>>   	}
>>   
>> -	/* Now read the DPCD to see if it's actually running */
>> -	if (!intel_dp_get_dpcd(intel_dp)) {
>> -		return;
>> +	/*
>> +	 * Now read the DPCD to see if it's actually running
>> +	 * Don't return immediately if dpcd read failed,
>> +	 * if sink count was 1 and dpcd read failed we need
>> +	 * to do full detection
>> +	 */
>> +	ret = intel_dp_get_dpcd(intel_dp);
>> +
>> +	if ((old_sink_count != intel_dp->sink_count) || !ret) {
> I don't see the connection of the comment above with this. If the dpcd read
> fails, the 'return false' will be reached regardless of the previous value of
> intel_dp->sink_count. Did you intend to do something different or did I miss
> something?
>
The code was changed but comment was not updated.. Will change the 
comment to explain correctly.

>> +		/* No need to proceed if we are going to do full detect */
>> +		return false;
>>   	}
>>   
>>   	/* Try to read the source of the interrupt */
>> @@ -4373,6 +4383,8 @@ intel_dp_short_pulse(struct intel_dp *intel_dp)
>>   	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>   	intel_dp_check_link_status(intel_dp);
>>   	drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +	return true;
>>   }
>>   
>>   /* XXX this is probably wrong for multiple downstream ports */
>> @@ -5095,8 +5107,12 @@ intel_dp_hpd_pulse(struct intel_digital_port
>> *intel_dig_port, bool long_hpd)
>>   			}
>>   		}
>>   
>> -		if (!intel_dp->is_mst)
>> -			intel_dp_short_pulse(intel_dp);
>> +		if (!intel_dp->is_mst) {
>> +			if (!intel_dp_short_pulse(intel_dp)) {
>> +				intel_dp_long_pulse(intel_dp
>> ->attached_connector);
>> +				goto put_power;
> It could be in a follow up patch, but I think its a good moment to get rid of
> the goto put_power. The only thing they do is skip the 'ret = IRQ_HANDLED'
> assignment now.
>
> Ander
Sure.. Will remove the goto put_power in follow up patch.

>> +			}
>> +		}
>>   	}
>>   
>>   	ret = IRQ_HANDLED;

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

✗ warning: Fi.CI.BAT

[Patchwork]

== Summary ==

Built on 05ade905f2fda5416476677509e016ef830d181a drm-intel-nightly: 2016y-01m-05d-13h-00m-24s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                dmesg-warn -> PASS       (bdw-nuci7)
                dmesg-warn -> PASS       (skl-i7k-2)
Test kms_flip:
        Subgroup basic-flip-vs-modeset:
                dmesg-warn -> PASS       (bsw-nuc-2) UNSTABLE
                pass       -> DMESG-WARN (hsw-gt2) UNSTABLE
        Subgroup basic-plain-flip:
                dmesg-warn -> PASS       (bdw-ultra)
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-a:
                pass       -> DMESG-WARN (snb-x220t) UNSTABLE
                dmesg-warn -> PASS       (skl-i7k-2) UNSTABLE
        Subgroup read-crc-pipe-b:
                dmesg-warn -> PASS       (snb-dellxps)
        Subgroup suspend-read-crc-pipe-a:
                dmesg-warn -> PASS       (snb-x220t)
Test kms_setmode:
        Subgroup basic-clone-single-crtc:
                pass       -> DMESG-WARN (snb-dellxps)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bdw-ultra)

bdw-nuci7        total:132  pass:122  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:132  pass:124  dwarn:2   dfail:0   fail:0   skip:6  
bsw-nuc-2        total:135  pass:114  dwarn:1   dfail:0   fail:0   skip:20 
hsw-brixbox      total:135  pass:126  dwarn:2   dfail:0   fail:0   skip:7  
hsw-gt2          total:135  pass:129  dwarn:2   dfail:0   fail:0   skip:4  
hsw-xps12        total:132  pass:125  dwarn:3   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:135  pass:100  dwarn:0   dfail:0   fail:0   skip:35 
ivb-t430s        total:135  pass:127  dwarn:2   dfail:0   fail:0   skip:6  
skl-i5k-2        total:135  pass:124  dwarn:3   dfail:0   fail:0   skip:8  
skl-i7k-2        total:135  pass:125  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:135  pass:121  dwarn:2   dfail:0   fail:0   skip:12 
snb-x220t        total:135  pass:121  dwarn:2   dfail:0   fail:1   skip:11 

Results at /archive/results/CI_IGT_test/Patchwork_1083/

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

✗ Fi.CI.BAT: warning for Fixing sink count related detection over (rev6)

[Patchwork]

== Summary ==

Built on 2dd73bef9cf525196545f96aa8cb42053620f2e6 drm-intel-nightly: 2016y-01m-18d-09h-59m-27s UTC integration manifest

Test gem_storedw_loop:
        Subgroup basic-render:
                pass       -> DMESG-WARN (bdw-nuci7) UNSTABLE
Test kms_pipe_crc_basic:
        Subgroup read-crc-pipe-b:
                pass       -> DMESG-WARN (ilk-hp8440p)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (skl-i7k-2)

bdw-nuci7        total:140  pass:130  dwarn:1   dfail:0   fail:0   skip:9  
bdw-ultra        total:140  pass:133  dwarn:0   dfail:1   fail:0   skip:6  
bsw-nuc-2        total:143  pass:117  dwarn:2   dfail:0   fail:0   skip:24 
byt-nuc          total:143  pass:125  dwarn:3   dfail:0   fail:0   skip:15 
hsw-brixbox      total:143  pass:136  dwarn:0   dfail:0   fail:0   skip:7  
hsw-gt2          total:143  pass:139  dwarn:0   dfail:0   fail:0   skip:4  
ilk-hp8440p      total:143  pass:101  dwarn:4   dfail:0   fail:0   skip:38 
ivb-t430s        total:137  pass:124  dwarn:3   dfail:4   fail:0   skip:6  
skl-i5k-2        total:143  pass:134  dwarn:1   dfail:0   fail:0   skip:8  
skl-i7k-2        total:143  pass:133  dwarn:2   dfail:0   fail:0   skip:8  
snb-dellxps      total:143  pass:124  dwarn:5   dfail:0   fail:0   skip:14 
snb-x220t        total:143  pass:124  dwarn:5   dfail:0   fail:1   skip:13 

Results at /archive/results/CI_IGT_test/Patchwork_1209/

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

✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev7)

[Patchwork]

== Summary ==

HEAD is now at 2dd73be drm-intel-nightly: 2016y-01m-18d-09h-59m-27s UTC integration manifest
Applying: drm/i915: Splitting intel_dp_detect
Applying: drm/i915: Cleaning up intel_dp_hpd_pulse
Applying: drm/i915: Splitting intel_dp_check_link_status
Applying: drm/i915: Save sink_count for tracking changes to it
Applying: drm/i915: read sink_count dpcd always
Applying: drm/i915: force full detect on sink count change
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_dp.c
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_dp.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_dp.c
Patch failed at 0006 drm/i915: force full detect on sink count change

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

✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev8)

[Patchwork]

== Summary ==

HEAD is now at 2dd73be drm-intel-nightly: 2016y-01m-18d-09h-59m-27s UTC integration manifest
Applying: drm/i915: Splitting intel_dp_detect
Applying: drm/i915: Cleaning up intel_dp_hpd_pulse
Applying: drm/i915: Splitting intel_dp_check_link_status
Applying: drm/i915: Save sink_count for tracking changes to it
Applying: drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_dp.c
M	drivers/gpu/drm/i915/intel_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_dp.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_dp.c
Patch failed at 0005 drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always

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

Re: ✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev8)

[Ander Conselvan De Oliveira]

Hi Shubhangi,

On Mon, 2016-01-18 at 13:01 +0000, Patchwork wrote:
> == Summary ==
> 
> HEAD is now at 2dd73be drm-intel-nightly: 2016y-01m-18d-09h-59m-27s UTC
> integration manifest
> Applying: drm/i915: Splitting intel_dp_detect
> Applying: drm/i915: Cleaning up intel_dp_hpd_pulse
> Applying: drm/i915: Splitting intel_dp_check_link_status
> Applying: drm/i915: Save sink_count for tracking changes to it
> Applying: drm/i915: Save sink_count for tracking changes to it and read
> sink_count dpcd always

It seems patchwork got confused about the order of the new patches in this
series. When you change multiple patches at once, it is common to resend the
entire series without --in-reply-to. It makes it easier for humans and patchwork
to figure out which are the new patches.

Can you please resend the series as a new thread so we can have the patches go
through CI run?

Thanks,
Ander

> Using index info to reconstruct a base tree...
> M	drivers/gpu/drm/i915/intel_dp.c
> M	drivers/gpu/drm/i915/intel_drv.h
> Falling back to patching base and 3-way merge...
> Auto-merging drivers/gpu/drm/i915/intel_dp.c
> CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_dp.c
> Patch failed at 0005 drm/i915: Save sink_count for tracking changes to it and
> read sink_count dpcd always
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: ✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev8)

[Shrivastava, Shubhangi]

Sure Ander.. Have resent this series of patches.. Will ensure to resend the series for multiple patch changes.. :)

Thanks and Regards,
Shubhangi Shrivastava.

-----Original Message-----
From: Ander Conselvan De Oliveira [mailto:conselvan2@gmail.com] 
Sent: Tuesday, January 19, 2016 3:09 PM
To: Patchwork <patchwork@annarchy.freedesktop.org>; Shrivastava, Shubhangi <shubhangi.shrivastava@intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] ✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev8)

Hi Shubhangi,

On Mon, 2016-01-18 at 13:01 +0000, Patchwork wrote:
> == Summary ==
> 
> HEAD is now at 2dd73be drm-intel-nightly: 2016y-01m-18d-09h-59m-27s 
> UTC integration manifest
> Applying: drm/i915: Splitting intel_dp_detect
> Applying: drm/i915: Cleaning up intel_dp_hpd_pulse
> Applying: drm/i915: Splitting intel_dp_check_link_status
> Applying: drm/i915: Save sink_count for tracking changes to it
> Applying: drm/i915: Save sink_count for tracking changes to it and 
> read sink_count dpcd always

It seems patchwork got confused about the order of the new patches in this series. When you change multiple patches at once, it is common to resend the entire series without --in-reply-to. It makes it easier for humans and patchwork to figure out which are the new patches.

Can you please resend the series as a new thread so we can have the patches go through CI run?

Thanks,
Ander

> Using index info to reconstruct a base tree...
> M	drivers/gpu/drm/i915/intel_dp.c
> M	drivers/gpu/drm/i915/intel_drv.h
> Falling back to patching base and 3-way merge...
> Auto-merging drivers/gpu/drm/i915/intel_dp.c CONFLICT (content): Merge 
> conflict in drivers/gpu/drm/i915/intel_dp.c Patch failed at 0005 
> drm/i915: Save sink_count for tracking changes to it and read 
> sink_count dpcd always
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Fixing sink count related detection over (rev9)

[Patchwork]

== Summary ==

HEAD is now at 00a0c7d drm-intel-nightly: 2016y-01m-18d-16h-50m-37s UTC integration manifest
Applying: drm/i915: Splitting intel_dp_detect
Applying: drm/i915: Cleaning up intel_dp_hpd_pulse
Applying: drm/i915: Reorganizing intel_dp_check_link_status
Applying: drm/i915: Save sink_count for tracking changes to it
Applying: drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/intel_dp.c
M	drivers/gpu/drm/i915/intel_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/intel_dp.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/intel_dp.c
Patch failed at 0005 drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always

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