[PATCH 0/4] Detect DP displays based on sink count change

[PATCH 0/4] Detect DP displays based on sink count change

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

These patches together help detect DP displays on short pulse HPD 
and pass the respective compliance test case (4.2.2.8)

v2: modifed first patch so we will read sink_count independent of
downstream ports availablility.
v3: split first patch so crtc enabled check is done in one patch
and rest of sink_count changes in second patch.

Thulasimani,Sivakumar (4):
  drm/i915: read dpcd 0 - 12 & link_status always
  drm/i915: read sink_count dpcd always
  drm/i915: Save sink_count for tracking changes to it
  drm/i915: force full detect on sink count change

 drivers/gpu/drm/i915/intel_dp.c  |   57 +++++++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 35 insertions(+), 23 deletions(-)

-- 
1.7.9.5

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

[PATCH 1/4] drm/i915: read dpcd 0 - 12 & link_status always

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

Compliance requires the driver to read dpcd register 0 to 12 and
registers 0x200 to 0x205 to be read always.
Current code performs dpcd read for short pulse interrupts only
if the sink is enabled. This patch forces read for link status
and registers 0 to 12.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8a66a44..76561e0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
-	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;
@@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		return;
 	}
 
+	if (!intel_encoder->base.crtc)
+		return;
+
+	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
+		return;
+
 	/* Try to read the source of the interrupt */
 	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
 	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
-- 
1.7.9.5

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

[PATCH 2/4] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

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: moved out crtc enabled checks to prior patch(Jani)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 76561e0..9e4e27d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
+	uint8_t reg;
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
 				    sizeof(intel_dp->dpcd)) < 0)
@@ -3908,6 +3909,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,
+					    &reg, 1) < 0)
+			return false;
+
+	if (!DP_GET_SINK_COUNT(reg))
+		return false;
+
 	/* Check if the panel supports PSR */
 	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
 	if (is_edp(intel_dp)) {
@@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
 	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
 		return connector_status_connected;
 
-	/* 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;
-	}
-
 	/* If no HPD, poke DDC gently */
 	if (drm_probe_ddc(&intel_dp->aux.ddc))
 		return connector_status_connected;
-- 
1.7.9.5

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

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

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

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.

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c  |    5 ++---
 drivers/gpu/drm/i915/intel_drv.h |    1 +
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9e4e27d..834f513 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3898,7 +3898,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	uint8_t rev;
-	uint8_t reg;
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
 				    sizeof(intel_dp->dpcd)) < 0)
@@ -3910,10 +3909,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		return false; /* DPCD not present */
 
 	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
-					    &reg, 1) < 0)
+					    &intel_dp->sink_count, 1) < 0)
 			return false;
 
-	if (!DP_GET_SINK_COUNT(reg))
+	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
 		return false;
 
 	/* Check if the panel supports PSR */
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..8aca5bb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -712,6 +712,7 @@ struct intel_dp {
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
 	bool color_range_auto;
+	uint8_t sink_count;
 	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
 	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
 	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
-- 
1.7.9.5

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

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

[Sivakumar Thulasimani]

From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>

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)

Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
 1 file changed, 22 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 834f513..279e52c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	u8 sink_irq_vector;
+	u8 old_sink_count = intel_dp->sink_count;
+	bool ret;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
 	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
 
+	*perform_full_detect = false;
+
 	/* 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)) {
+	/* 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)
+		*perform_full_detect = true;
+
+	/* No need to proceed if we are going to do full detect */
+	if (!ret || *perform_full_detect)
 		return;
-	}
 
 	if (!intel_encoder->base.crtc)
 		return;
@@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
+			bool full_detect;
 			/*
 			 * we'll check the link status via the normal hot plug path later -
 			 * but for short hpds we should check it now
 			 */
 			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
-			intel_dp_check_link_status(intel_dp);
+			intel_dp_check_link_status(intel_dp, &full_detect);
 			drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+			if (full_detect)
+				goto put_power;
 		}
 	}
 
-- 
1.7.9.5

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

Re: [PATCH 1/4] drm/i915: read dpcd 0 - 12 & link_status always

[Jani Nikula]

On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> Compliance requires the driver to read dpcd register 0 to 12 and
> registers 0x200 to 0x205 to be read always.
> Current code performs dpcd read for short pulse interrupts only
> if the sink is enabled. This patch forces read for link status
> and registers 0 to 12.

While this seems a bit silly from the driver perspective, I don't see it
doing much harm either.

Note that We do read dpcd 0 to 14 no matter what the spec says.

Reviewed-by: Jani Nikula <jani.nikula@intel.com>

>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8a66a44..76561e0 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> -	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;
> @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		return;
>  	}
>  
> +	if (!intel_encoder->base.crtc)
> +		return;
> +
> +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> +		return;
> +
>  	/* Try to read the source of the interrupt */
>  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: read sink_count dpcd always

[Jani Nikula]

On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> 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.

This makes sense.

I'm scared this may break something, if there are displays/adapters out
there that don't set sink count properly. I guess only one way to find
out... And this might actually fix something else.

> 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

Do you think you handle this last case properly now? Previously we'd
return with "connected" at the sink count check, but now we'll go for
the "pke ddc gently" phase.

See below.

>
> v2: moved out crtc enabled checks to prior patch(Jani)
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 76561e0..9e4e27d 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
> +	uint8_t reg;
>  
>  	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>  				    sizeof(intel_dp->dpcd)) < 0)
> @@ -3908,6 +3909,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,
> +					    &reg, 1) < 0)
> +			return false;
> +
> +	if (!DP_GET_SINK_COUNT(reg))
> +		return false;
> +
>  	/* Check if the panel supports PSR */
>  	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>  	if (is_edp(intel_dp)) {
> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>  	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>  		return connector_status_connected;
>  
> -	/* 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;
> -	}
> -

Why do we proceed here if we know that we have sink count > 0 and we
have a downstream port present? Admittedly I'm not sure I understand (or
remember) why we had this logic in the first place...

BR,
Jani.

>  	/* If no HPD, poke DDC gently */
>  	if (drm_probe_ddc(&intel_dp->aux.ddc))
>  		return connector_status_connected;
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> 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)

The general idea LGTM, however see below.

>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 834f513..279e52c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>  	u8 sink_irq_vector;
> +	u8 old_sink_count = intel_dp->sink_count;
> +	bool ret;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> +	*perform_full_detect = false;
> +
>  	/* 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)) {
> +	/* 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)
> +		*perform_full_detect = true;

The point I was trying to make earlier is that how can you trust
intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where
it failed, and ->sink_count may or may not have changed. Maybe it has
changed for real but you didn't have a chance to read it just yet.

> +
> +	/* No need to proceed if we are going to do full detect */
> +	if (!ret || *perform_full_detect)
>  		return;
> -	}

So if you're returning because of !ret, *perform_full_detect may be true
or false, you don't know. I'd like the code to be explicit about what
you want the caller to do when intel_dp_get_dpcd returns false, full
detect or not.

BR,
Jani.

>  
>  	if (!intel_encoder->base.crtc)
>  		return;
> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		}
>  
>  		if (!intel_dp->is_mst) {
> +			bool full_detect;
>  			/*
>  			 * we'll check the link status via the normal hot plug path later -
>  			 * but for short hpds we should check it now
>  			 */
>  			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> +			intel_dp_check_link_status(intel_dp, &full_detect);
>  			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +			if (full_detect)
> +				goto put_power;
>  		}
>  	}
>  
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jani Nikula]

On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>
> 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.
>
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>

Provided the earlier patch gets a proper explanation,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> ---
>  drivers/gpu/drm/i915/intel_dp.c  |    5 ++---
>  drivers/gpu/drm/i915/intel_drv.h |    1 +
>  2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9e4e27d..834f513 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3898,7 +3898,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  	struct drm_device *dev = dig_port->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	uint8_t rev;
> -	uint8_t reg;
>  
>  	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>  				    sizeof(intel_dp->dpcd)) < 0)
> @@ -3910,10 +3909,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>  		return false; /* DPCD not present */
>  
>  	if (intel_dp_dpcd_read_wake(&intel_dp->aux, DP_SINK_COUNT,
> -					    &reg, 1) < 0)
> +					    &intel_dp->sink_count, 1) < 0)
>  			return false;
>  
> -	if (!DP_GET_SINK_COUNT(reg))
> +	if (!DP_GET_SINK_COUNT(intel_dp->sink_count))
>  		return false;
>  
>  	/* Check if the panel supports PSR */
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 81b7d77..8aca5bb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -712,6 +712,7 @@ struct intel_dp {
>  	enum hdmi_force_audio force_audio;
>  	bool limited_color_range;
>  	bool color_range_auto;
> +	uint8_t sink_count;
>  	uint8_t dpcd[DP_RECEIVER_CAP_SIZE];
>  	uint8_t psr_dpcd[EDP_PSR_RECEIVER_CAP_SIZE];
>  	uint8_t downstream_ports[DP_MAX_DOWNSTREAM_PORTS];
> -- 
> 1.7.9.5
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Sivakumar Thulasimani]

On 9/1/2015 4:12 PM, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> 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)
> The general idea LGTM, however see below.
>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 834f513..279e52c 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>   	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	struct intel_crtc *crtc =
>>   		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>>   	u8 sink_irq_vector;
>> +	u8 old_sink_count = intel_dp->sink_count;
>> +	bool ret;
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> +	*perform_full_detect = false;
>> +
>>   	/* 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)) {
>> +	/* 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)
>> +		*perform_full_detect = true;
> The point I was trying to make earlier is that how can you trust
> intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where
> it failed, and ->sink_count may or may not have changed. Maybe it has
> changed for real but you didn't have a chance to read it just yet.
>
>> +
>> +	/* No need to proceed if we are going to do full detect */
>> +	if (!ret || *perform_full_detect)
>>   		return;
>> -	}
> So if you're returning because of !ret, *perform_full_detect may be true
> or false, you don't know. I'd like the code to be explicit about what
> you want the caller to do when intel_dp_get_dpcd returns false, full
> detect or not.
>
> BR,
> Jani.
the idea i had in mind was if intel_dp_get_dpcd failed we should do a 
full detect.

my assumption here was that sink_count would be zero so it would take care
of this, but now that it seems to be a wrong assumption how about the 
following code ?
this makes it explicit that if dpcd read failed we will do full detect.

+	if ((old_sink_count != intel_dp->sink_count) || !ret)
+		*perform_full_detect = true;

+
+	/* No need to proceed if we are going to do full detect */
+	if (*perform_full_detect)
  		return;

>
>>   
>>   	if (!intel_encoder->base.crtc)
>>   		return;
>> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   		}
>>   
>>   		if (!intel_dp->is_mst) {
>> +			bool full_detect;
>>   			/*
>>   			 * we'll check the link status via the normal hot plug path later -
>>   			 * but for short hpds we should check it now
>>   			 */
>>   			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> +			intel_dp_check_link_status(intel_dp, &full_detect);
>>   			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +			if (full_detect)
>> +				goto put_power;
>>   		}
>>   	}
>>   
>> -- 
>> 1.7.9.5
>>

-- 
regards,
Sivakumar

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

Re: [PATCH 2/4] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

On 9/1/2015 3:59 PM, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> 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.
> This makes sense.
>
> I'm scared this may break something, if there are displays/adapters out
> there that don't set sink count properly. I guess only one way to find
> out... And this might actually fix something else.
>
>> 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
> Do you think you handle this last case properly now? Previously we'd
> return with "connected" at the sink count check, but now we'll go for
> the "pke ddc gently" phase.
>
> See below.
>
>> v2: moved out crtc enabled checks to prior patch(Jani)
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>   1 file changed, 8 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 76561e0..9e4e27d 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>   	struct drm_device *dev = dig_port->base.base.dev;
>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>   	uint8_t rev;
>> +	uint8_t reg;
>>   
>>   	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>   				    sizeof(intel_dp->dpcd)) < 0)
>> @@ -3908,6 +3909,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,
>> +					    &reg, 1) < 0)
>> +			return false;
>> +
>> +	if (!DP_GET_SINK_COUNT(reg))
>> +		return false;
>> +
>>   	/* Check if the panel supports PSR */
>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>   	if (is_edp(intel_dp)) {
>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>   	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>   		return connector_status_connected;
>>   
>> -	/* 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;
>> -	}
>> -
> Why do we proceed here if we know that we have sink count > 0 and we
> have a downstream port present? Admittedly I'm not sure I understand (or
> remember) why we had this logic in the first place...
>
> BR,
> Jani.
based on what little i can dig up, we proceed in case of dongles to 
check if the
display is CRT or not.
if edid read succeeded, it is DP or any display with proper edid to work 
with.
If edid read failed, then it could be CRT, without edid

we can make it work with a fake edid, but it seems current code just
sets it as connector_status_unknown and ignores the display

>>   	/* If no HPD, poke DDC gently */
>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>   		return connector_status_connected;
>> -- 
>> 1.7.9.5
>>

-- 
regards,
Sivakumar

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

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

[Jani Nikula]

On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> On 9/1/2015 4:12 PM, Jani Nikula wrote:
>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>>
>>> 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)
>> The general idea LGTM, however see below.
>>
>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
>>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 834f513..279e52c 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
>>>   {
>>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>>   	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>>   	struct intel_crtc *crtc =
>>>   		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>>>   	u8 sink_irq_vector;
>>> +	u8 old_sink_count = intel_dp->sink_count;
>>> +	bool ret;
>>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>>   
>>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>>   
>>> +	*perform_full_detect = false;
>>> +
>>>   	/* 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)) {
>>> +	/* 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)
>>> +		*perform_full_detect = true;
>> The point I was trying to make earlier is that how can you trust
>> intel_dp->sink_count if intel_dp_get_dpcd failed? You don't know where
>> it failed, and ->sink_count may or may not have changed. Maybe it has
>> changed for real but you didn't have a chance to read it just yet.
>>
>>> +
>>> +	/* No need to proceed if we are going to do full detect */
>>> +	if (!ret || *perform_full_detect)
>>>   		return;
>>> -	}
>> So if you're returning because of !ret, *perform_full_detect may be true
>> or false, you don't know. I'd like the code to be explicit about what
>> you want the caller to do when intel_dp_get_dpcd returns false, full
>> detect or not.
>>
>> BR,
>> Jani.
> the idea i had in mind was if intel_dp_get_dpcd failed we should do a 
> full detect.
>
> my assumption here was that sink_count would be zero so it would take care
> of this, but now that it seems to be a wrong assumption how about the 
> following code ?
> this makes it explicit that if dpcd read failed we will do full detect.
>
> +	if ((old_sink_count != intel_dp->sink_count) || !ret)
> +		*perform_full_detect = true;
>
> +
> +	/* No need to proceed if we are going to do full detect */
> +	if (*perform_full_detect)
>   		return;

Ok by me.

Jani.



>
>>
>>>   
>>>   	if (!intel_encoder->base.crtc)
>>>   		return;
>>> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>>   		}
>>>   
>>>   		if (!intel_dp->is_mst) {
>>> +			bool full_detect;
>>>   			/*
>>>   			 * we'll check the link status via the normal hot plug path later -
>>>   			 * but for short hpds we should check it now
>>>   			 */
>>>   			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>>> -			intel_dp_check_link_status(intel_dp);
>>> +			intel_dp_check_link_status(intel_dp, &full_detect);
>>>   			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>>> +
>>> +			if (full_detect)
>>> +				goto put_power;
>>>   		}
>>>   	}
>>>   
>>> -- 
>>> 1.7.9.5
>>>
>
> -- 
> regards,
> Sivakumar
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: read sink_count dpcd always

[Jani Nikula]

On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> On 9/1/2015 3:59 PM, Jani Nikula wrote:
>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>>
>>> 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.
>> This makes sense.
>>
>> I'm scared this may break something, if there are displays/adapters out
>> there that don't set sink count properly. I guess only one way to find
>> out... And this might actually fix something else.
>>
>>> 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
>> Do you think you handle this last case properly now? Previously we'd
>> return with "connected" at the sink count check, but now we'll go for
>> the "pke ddc gently" phase.
>>
>> See below.
>>
>>> v2: moved out crtc enabled checks to prior patch(Jani)
>>>
>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>>   1 file changed, 8 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 76561e0..9e4e27d 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>   	struct drm_device *dev = dig_port->base.base.dev;
>>>   	struct drm_i915_private *dev_priv = dev->dev_private;
>>>   	uint8_t rev;
>>> +	uint8_t reg;
>>>   
>>>   	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>>   				    sizeof(intel_dp->dpcd)) < 0)
>>> @@ -3908,6 +3909,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,
>>> +					    &reg, 1) < 0)
>>> +			return false;
>>> +
>>> +	if (!DP_GET_SINK_COUNT(reg))
>>> +		return false;
>>> +
>>>   	/* Check if the panel supports PSR */
>>>   	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>   	if (is_edp(intel_dp)) {
>>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>   	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>>   		return connector_status_connected;
>>>   
>>> -	/* 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;
>>> -	}
>>> -
>> Why do we proceed here if we know that we have sink count > 0 and we
>> have a downstream port present? Admittedly I'm not sure I understand (or
>> remember) why we had this logic in the first place...
>>
>> BR,
>> Jani.
> based on what little i can dig up, we proceed in case of dongles to 
> check if the
> display is CRT or not.
> if edid read succeeded, it is DP or any display with proper edid to work 
> with.
> If edid read failed, then it could be CRT, without edid

My point is, with the current code, if we have downstream port present
and sink count > 0 and the first downsteam port supports hpd, we'll
directly say status is connected. The hpd check may be bogus, but we
still won't go probing ddc on them.

With your patch, we'd always do the ddc probe if there is a downstream
port present and the sink count > 0.

BR,
Jani.



>
> we can make it work with a fake edid, but it seems current code just
> sets it as connector_status_unknown and ignores the display
>
>>>   	/* If no HPD, poke DDC gently */
>>>   	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>>   		return connector_status_connected;
>>> -- 
>>> 1.7.9.5
>>>
>
> -- 
> regards,
> Sivakumar
>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/4] drm/i915: read sink_count dpcd always

[Sivakumar Thulasimani]

On 9/1/2015 6:45 PM, Jani Nikula wrote:
> On Tue, 01 Sep 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> On 9/1/2015 3:59 PM, Jani Nikula wrote:
>>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>>>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>>>
>>>> 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.
>>> This makes sense.
>>>
>>> I'm scared this may break something, if there are displays/adapters out
>>> there that don't set sink count properly. I guess only one way to find
>>> out... And this might actually fix something else.
>>>
>>>> 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
>>> Do you think you handle this last case properly now? Previously we'd
>>> return with "connected" at the sink count check, but now we'll go for
>>> the "pke ddc gently" phase.
>>>
>>> See below.
>>>
>>>> v2: moved out crtc enabled checks to prior patch(Jani)
>>>>
>>>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/intel_dp.c |   21 ++++++++-------------
>>>>    1 file changed, 8 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index 76561e0..9e4e27d 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -3898,6 +3898,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
>>>>    	struct drm_device *dev = dig_port->base.base.dev;
>>>>    	struct drm_i915_private *dev_priv = dev->dev_private;
>>>>    	uint8_t rev;
>>>> +	uint8_t reg;
>>>>    
>>>>    	if (intel_dp_dpcd_read_wake(&intel_dp->aux, 0x000, intel_dp->dpcd,
>>>>    				    sizeof(intel_dp->dpcd)) < 0)
>>>> @@ -3908,6 +3909,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,
>>>> +					    &reg, 1) < 0)
>>>> +			return false;
>>>> +
>>>> +	if (!DP_GET_SINK_COUNT(reg))
>>>> +		return false;
>>>> +
>>>>    	/* Check if the panel supports PSR */
>>>>    	memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd));
>>>>    	if (is_edp(intel_dp)) {
>>>> @@ -4427,19 +4435,6 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp)
>>>>    	if (!(dpcd[DP_DOWNSTREAMPORT_PRESENT] & DP_DWN_STRM_PORT_PRESENT))
>>>>    		return connector_status_connected;
>>>>    
>>>> -	/* 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;
>>>> -	}
>>>> -
>>> Why do we proceed here if we know that we have sink count > 0 and we
>>> have a downstream port present? Admittedly I'm not sure I understand (or
>>> remember) why we had this logic in the first place...
>>>
>>> BR,
>>> Jani.
>> based on what little i can dig up, we proceed in case of dongles to
>> check if the
>> display is CRT or not.
>> if edid read succeeded, it is DP or any display with proper edid to work
>> with.
>> If edid read failed, then it could be CRT, without edid
> My point is, with the current code, if we have downstream port present
> and sink count > 0 and the first downsteam port supports hpd, we'll
> directly say status is connected. The hpd check may be bogus, but we
> still won't go probing ddc on them.
>
> With your patch, we'd always do the ddc probe if there is a downstream
> port present and the sink count > 0.
>
> BR,
> Jani.
understood, will check once by adding the old check if hpd supported 
downstream
is present and share new patch.
>
>
>> we can make it work with a fake edid, but it seems current code just
>> sets it as connector_status_unknown and ignores the display
>>
>>>>    	/* If no HPD, poke DDC gently */
>>>>    	if (drm_probe_ddc(&intel_dp->aux.ddc))
>>>>    		return connector_status_connected;
>>>> -- 
>>>> 1.7.9.5
>>>>
>> -- 
>> regards,
>> Sivakumar
>>

-- 
regards,
Sivakumar

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

Re: [PATCH 1/4] drm/i915: read dpcd 0 - 12 & link_status always

[Daniel Vetter]

On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> >
> > Compliance requires the driver to read dpcd register 0 to 12 and
> > registers 0x200 to 0x205 to be read always.
> > Current code performs dpcd read for short pulse interrupts only
> > if the sink is enabled. This patch forces read for link status
> > and registers 0 to 12.
> 
> While this seems a bit silly from the driver perspective, I don't see it
> doing much harm either.

I don't think this is silly, but fixing it like this might be - currently
we don't do _any_ detection of sink ports, so if you have a hub with two
outputs and then plug in another one and plug out the first userspace
won't reprobe. So probably what we should be doing is not just read the
dpcd, but actually look at it, figure out whether something has changed
and make sure we send out the uevent even if the hpd line stays unchanged
on short pulses to make userspace aware of the changes.

Punting on this for now since I suspect there's a bigger story to be had
here ...
-Daniel

> 
> Note that We do read dpcd 0 to 14 no matter what the spec says.
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> >
> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
> >  1 file changed, 6 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 8a66a44..76561e0 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >  
> >  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
> >  
> > -	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;
> > @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
> >  		return;
> >  	}
> >  
> > +	if (!intel_encoder->base.crtc)
> > +		return;
> > +
> > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
> > +		return;
> > +
> >  	/* Try to read the source of the interrupt */
> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
> >  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
> > -- 
> > 1.7.9.5
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 4/4] drm/i915: force full detect on sink count change

[Daniel Vetter]

On Thu, Aug 27, 2015 at 02:18:32PM +0530, Sivakumar Thulasimani wrote:
> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> 
> 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)
> 
> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>

That's still incomplete since in the hotplug work function we check
whether ->detect status has changed. If that doesn't happen (e.g. sink
count changes from 1->2 or whatever) then we'll fail to generate the
required uevent.

I suspect the only clean way to fix this is to merge all the dp hpd
handling together into one place and stop using ->detect for some parts of
it. Then we would have one place only that decides whether anything
changed, and whether we need to send anything to userspace or not.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
>  1 file changed, 22 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 834f513..279e52c 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
>  {
>  	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>  	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>  	struct intel_crtc *crtc =
>  		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>  	u8 sink_irq_vector;
> +	u8 old_sink_count = intel_dp->sink_count;
> +	bool ret;
>  	u8 link_status[DP_LINK_STATUS_SIZE];
>  
>  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>  
> +	*perform_full_detect = false;
> +
>  	/* 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)) {
> +	/* 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)
> +		*perform_full_detect = true;
> +
> +	/* No need to proceed if we are going to do full detect */
> +	if (!ret || *perform_full_detect)
>  		return;
> -	}
>  
>  	if (!intel_encoder->base.crtc)
>  		return;
> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>  		}
>  
>  		if (!intel_dp->is_mst) {
> +			bool full_detect;
>  			/*
>  			 * we'll check the link status via the normal hot plug path later -
>  			 * but for short hpds we should check it now
>  			 */
>  			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> -			intel_dp_check_link_status(intel_dp);
> +			intel_dp_check_link_status(intel_dp, &full_detect);
>  			drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +			if (full_detect)
> +				goto put_power;
>  		}
>  	}
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 4/4] drm/i915: force full detect on sink count change

[Sivakumar Thulasimani]

On 9/2/2015 2:43 PM, Daniel Vetter wrote:
> On Thu, Aug 27, 2015 at 02:18:32PM +0530, Sivakumar Thulasimani wrote:
>> From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>>
>> 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)
>>
>> Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
> That's still incomplete since in the hotplug work function we check
> whether ->detect status has changed. If that doesn't happen (e.g. sink
> count changes from 1->2 or whatever) then we'll fail to generate the
> required uevent.
>
> I suspect the only clean way to fix this is to merge all the dp hpd
> handling together into one place and stop using ->detect for some parts of
> it. Then we would have one place only that decides whether anything
> changed, and whether we need to send anything to userspace or not.
> -Daniel
i doubt sink count change from 1->2 is valid.
1)  we support only edp and SST displays in intel_dp_detect.
we should handle all mst related code in intel_dp_hpd_pulse.
2) check_link_status is called from withing !mst check so i dont see it 
change from 1 to 2.
3) even if it changes from 1 to 2, for the given port without mst there 
is no way to
switch between first sink to second one outside mst logic.

correct me if i am wrong, the only reason to call intel_dp_detect is to 
see if
"one display is connected or disconnected" which i feel is fine with 
current code.

But i do agree that we need to clean up intel_dp_hpd_pulse since it does 
a lot of things
then repeats those same operations in intel_dp_detect. for example we 
read dpcd 0-14,
link status and probe oui only to repeat the same steps in the 
intel_dp_detect. considering
that our dpcd read has 1ms delay this can add up pretty quick.

if the intention of intel_dp_hpd_pulse is to handle mst displays, that 
should be checked first
and continue only if display is mst otherwise we should just jump to 
intel_dp_detect.
>> ---
>>   drivers/gpu/drm/i915/intel_dp.c |   27 ++++++++++++++++++++++-----
>>   1 file changed, 22 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 834f513..279e52c 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -4370,26 +4370,39 @@ 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_check_link_status(struct intel_dp *intel_dp, bool *perform_full_detect)
>>   {
>>   	struct drm_device *dev = intel_dp_to_dev(intel_dp);
>>   	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
>>   	struct intel_crtc *crtc =
>>   		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
>>   	u8 sink_irq_vector;
>> +	u8 old_sink_count = intel_dp->sink_count;
>> +	bool ret;
>>   	u8 link_status[DP_LINK_STATUS_SIZE];
>>   
>>   	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>>   
>> +	*perform_full_detect = false;
>> +
>>   	/* 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)) {
>> +	/* 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)
>> +		*perform_full_detect = true;
>> +
>> +	/* No need to proceed if we are going to do full detect */
>> +	if (!ret || *perform_full_detect)
>>   		return;
>> -	}
>>   
>>   	if (!intel_encoder->base.crtc)
>>   		return;
>> @@ -5031,13 +5044,17 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
>>   		}
>>   
>>   		if (!intel_dp->is_mst) {
>> +			bool full_detect;
>>   			/*
>>   			 * we'll check the link status via the normal hot plug path later -
>>   			 * but for short hpds we should check it now
>>   			 */
>>   			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
>> -			intel_dp_check_link_status(intel_dp);
>> +			intel_dp_check_link_status(intel_dp, &full_detect);
>>   			drm_modeset_unlock(&dev->mode_config.connection_mutex);
>> +
>> +			if (full_detect)
>> +				goto put_power;
>>   		}
>>   	}
>>   
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
regards,
Sivakumar

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

Re: [PATCH 1/4] drm/i915: read dpcd 0 - 12 & link_status always

[Jani Nikula]

On Wed, 02 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
>> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
>> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
>> >
>> > Compliance requires the driver to read dpcd register 0 to 12 and
>> > registers 0x200 to 0x205 to be read always.
>> > Current code performs dpcd read for short pulse interrupts only
>> > if the sink is enabled. This patch forces read for link status
>> > and registers 0 to 12.
>> 
>> While this seems a bit silly from the driver perspective, I don't see it
>> doing much harm either.
>
> I don't think this is silly, but fixing it like this might be - currently
> we don't do _any_ detection of sink ports, so if you have a hub with two
> outputs and then plug in another one and plug out the first userspace
> won't reprobe. So probably what we should be doing is not just read the
> dpcd, but actually look at it, figure out whether something has changed
> and make sure we send out the uevent even if the hpd line stays unchanged
> on short pulses to make userspace aware of the changes.
>
> Punting on this for now since I suspect there's a bigger story to be had
> here ...

Well, I'll bet this would be a preliminary step for that anyway.

BR,
Jani.


> -Daniel
>
>> 
>> Note that We do read dpcd 0 to 14 no matter what the spec says.
>> 
>> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
>> 
>> >
>> > Signed-off-by: Sivakumar Thulasimani <sivakumar.thulasimani@intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/intel_dp.c |   12 ++++++------
>> >  1 file changed, 6 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> > index 8a66a44..76561e0 100644
>> > --- a/drivers/gpu/drm/i915/intel_dp.c
>> > +++ b/drivers/gpu/drm/i915/intel_dp.c
>> > @@ -4374,12 +4374,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> >  
>> >  	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
>> >  
>> > -	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;
>> > @@ -4390,6 +4384,12 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>> >  		return;
>> >  	}
>> >  
>> > +	if (!intel_encoder->base.crtc)
>> > +		return;
>> > +
>> > +	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
>> > +		return;
>> > +
>> >  	/* Try to read the source of the interrupt */
>> >  	if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 &&
>> >  	    intel_dp_get_sink_irq(intel_dp, &sink_irq_vector)) {
>> > -- 
>> > 1.7.9.5
>> >
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/4] drm/i915: read dpcd 0 - 12 & link_status always

[Daniel Vetter]

On Thu, Sep 03, 2015 at 03:25:04PM +0300, Jani Nikula wrote:
> On Wed, 02 Sep 2015, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Tue, Sep 01, 2015 at 01:16:49PM +0300, Jani Nikula wrote:
> >> On Thu, 27 Aug 2015, Sivakumar Thulasimani <sivakumar.thulasimani@intel.com> wrote:
> >> > From: "Thulasimani,Sivakumar" <sivakumar.thulasimani@intel.com>
> >> >
> >> > Compliance requires the driver to read dpcd register 0 to 12 and
> >> > registers 0x200 to 0x205 to be read always.
> >> > Current code performs dpcd read for short pulse interrupts only
> >> > if the sink is enabled. This patch forces read for link status
> >> > and registers 0 to 12.
> >> 
> >> While this seems a bit silly from the driver perspective, I don't see it
> >> doing much harm either.
> >
> > I don't think this is silly, but fixing it like this might be - currently
> > we don't do _any_ detection of sink ports, so if you have a hub with two
> > outputs and then plug in another one and plug out the first userspace
> > won't reprobe. So probably what we should be doing is not just read the
> > dpcd, but actually look at it, figure out whether something has changed
> > and make sure we send out the uevent even if the hpd line stays unchanged
> > on short pulses to make userspace aware of the changes.
> >
> > Punting on this for now since I suspect there's a bigger story to be had
> > here ...
> 
> Well, I'll bet this would be a preliminary step for that anyway.

Then the commit message should explain that. Adding code that's not needed
just for compliance to be happy just feels very fishy. And if we have more
work to do won't really gain us much since for the full monty we probably
need to juggle a few pieces in the overall picture.
-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