drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes
diff mbox

Message ID 1433431578-14373-1-git-send-email-chris@chris-wilson.co.uk
State New
Headers show

Commit Message

Chris Wilson June 4, 2015, 3:26 p.m. UTC
For old-school component TV and CRT connectors, we require a heavyweight
load-detection mechanism. This involves setting up a CRTC and sending a
signal to the output, before reading back any response. As that is quite
slow and CPU heavy, the process is only performed when the output
detection is forced by user request. As it requires a driving CRTC, we
often don't have the resources to complete the probe. This leaves us in
a quandary where the unforced path just returns the old connector
status, but the forced detection path elects to return UNKNOWN. If we
have an active connection, we likely have the resources available to
complete the probe - but if it is currently disconnected, then it
becomes unknown and triggers a hotplug event, with often quite unfortunate
userspace behaviour (e.g. one output is blanked and the spurious TV
turned on).

To reduce spurious hotplug events on older devices, we can prevent
transitions between disconnected <-> unknown.

References: https://bugs.freedesktop.org/show_bug.cgi?id=87049
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c | 39 ++++++++++++++++++---------------------
 drivers/gpu/drm/i915/intel_tv.c  | 25 +++++++++----------------
 2 files changed, 27 insertions(+), 37 deletions(-)

Comments

Ville Syrjälä June 4, 2015, 4:28 p.m. UTC | #1
On Thu, Jun 04, 2015 at 04:26:18PM +0100, Chris Wilson wrote:
> For old-school component TV and CRT connectors, we require a heavyweight
> load-detection mechanism. This involves setting up a CRTC and sending a
> signal to the output, before reading back any response. As that is quite
> slow and CPU heavy, the process is only performed when the output
> detection is forced by user request. As it requires a driving CRTC, we
> often don't have the resources to complete the probe. This leaves us in
> a quandary where the unforced path just returns the old connector
> status, but the forced detection path elects to return UNKNOWN. If we
> have an active connection, we likely have the resources available to
> complete the probe - but if it is currently disconnected, then it
> becomes unknown and triggers a hotplug event, with often quite unfortunate
> userspace behaviour (e.g. one output is blanked and the spurious TV
> turned on).
> 
> To reduce spurious hotplug events on older devices, we can prevent
> transitions between disconnected <-> unknown.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=87049
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_crt.c | 39 ++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_tv.c  | 25 +++++++++----------------
>  2 files changed, 27 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 521af2c..70c5288 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -666,8 +666,6 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  	struct intel_encoder *intel_encoder = &crt->base;
>  	enum intel_display_power_domain power_domain;
>  	enum drm_connector_status status;
> -	struct intel_load_detect_pipe tmp;
> -	struct drm_modeset_acquire_ctx ctx;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, connector->name,
> @@ -703,27 +701,26 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  		goto out;
>  	}
>  
> -	if (!force) {
> -		status = connector->status;
> -		goto out;
> -	}
> -
> -	drm_modeset_acquire_init(&ctx, 0);
> -
> -	/* for pre-945g platforms use load detect */
> -	if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
> -		if (intel_crt_detect_ddc(connector))
> -			status = connector_status_connected;
> -		else if (INTEL_INFO(dev)->gen < 4)
> -			status = intel_crt_load_detect(crt);
> -		else
> +	status = connector->status;
> +	if (force) {
> +		struct intel_load_detect_pipe tmp;
> +		struct drm_modeset_acquire_ctx ctx;
> +
> +		drm_modeset_acquire_init(&ctx, 0);
> +
> +		/* for pre-945g platforms use load detect */
> +		if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
> +			if (intel_crt_detect_ddc(connector))
> +				status = connector_status_connected;

Hmm. I wonder what this DDC detection is doing here in the first place.
It doesn't need a load detect pipe, and if we end up here we've already
tried and failed at DDC based detection earlier.

> +			else if (INTEL_INFO(dev)->gen < 4)
> +				status = intel_crt_load_detect(crt);
> +			intel_release_load_detect_pipe(connector, &tmp, &ctx);
> +		} else if (status == connector_status_connected)
>  			status = connector_status_unknown;
> -		intel_release_load_detect_pipe(connector, &tmp, &ctx);
> -	} else
> -		status = connector_status_unknown;
>  
> -	drm_modeset_drop_locks(&ctx);
> -	drm_modeset_acquire_fini(&ctx);
> +		drm_modeset_drop_locks(&ctx);
> +		drm_modeset_acquire_fini(&ctx);
> +	}

The main bit there seems to be the
'else if (status == connector_status_connected)' check. The rest is
mostly indentation changes, but this does pull the load detect
variables into the same block, so perhaps it's better this way.

The logic makes sense to me. If it was disconnected and we don't have
free pipes to figure out if it might be connected now, just leave it
as disconnected instead of changing to unknown.

>  
>  out:
>  	intel_display_power_put(dev_priv, power_domain);
> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
> index 8b9d325..e3c0fed 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1313,44 +1313,37 @@ static void intel_tv_find_better_format(struct drm_connector *connector)
>  static enum drm_connector_status
>  intel_tv_detect(struct drm_connector *connector, bool force)
>  {
> -	struct drm_display_mode mode;
>  	struct intel_tv *intel_tv = intel_attached_tv(connector);
> -	enum drm_connector_status status;
> -	int type;
> +	enum drm_connector_status status = connector->status;
>  
>  	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
>  		      connector->base.id, connector->name,
>  		      force);
> -
> -	mode = reported_modes[0];
> -
>  	if (force) {
> +		struct drm_display_mode mode = reported_modes[0];
>  		struct intel_load_detect_pipe tmp;
>  		struct drm_modeset_acquire_ctx ctx;
>  
>  		drm_modeset_acquire_init(&ctx, 0);
>  
>  		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
> -			type = intel_tv_detect_type(intel_tv, connector);
> +			int type = intel_tv_detect_type(intel_tv, connector);
>  			intel_release_load_detect_pipe(connector, &tmp, &ctx);
>  			status = type < 0 ?
>  				connector_status_disconnected :
>  				connector_status_connected;
> -		} else
> +			intel_tv->type = type;
> +		} else if (status == connector_status_connected)
>  			status = connector_status_unknown;
>  
>  		drm_modeset_drop_locks(&ctx);
>  		drm_modeset_acquire_fini(&ctx);
> -	} else
> -		return connector->status;
>  
> -	if (status != connector_status_connected)
> -		return status;
> -
> -	intel_tv->type = type;
> -	intel_tv_find_better_format(connector);
> +		if (status == connector_status_connected)
> +			intel_tv_find_better_format(connector);
> +	}

So besides avoiding the disconnected->unknown transition, the other thing
we now seem to do is set intel_tv->type to -1 when we find that things
are disconnected. However that doesn't seem to be user visible, and I
can't see any place in the code where it should cause problems.

It is a bit inconsistent however due to the fact that we initialize type
to DRM_MODE_CONNECTOR_Unknown, and later we use the -1 to indicate more
or less the same thing. Seems like we should pick or the other for
consistency.

Apart from the little things like the weird pre-exising extra
ddc detect and the intel_tv->type inconsistency this patch looks OK to
me, so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  
> -	return connector_status_connected;
> +	return status;
>  }
>  
>  static const struct input_res {
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chris Wilson June 4, 2015, 8:23 p.m. UTC | #2
On Thu, Jun 04, 2015 at 07:28:31PM +0300, Ville Syrjälä wrote:
> So besides avoiding the disconnected->unknown transition, the other thing
> we now seem to do is set intel_tv->type to -1 when we find that things
> are disconnected. However that doesn't seem to be user visible, and I
> can't see any place in the code where it should cause problems.

Mostly because after the first refactor, it was not immediately obvious
that doing intel_tv->type = type would be valid, so I only wanted to set
intel_tv->type when doing load_detect. Fortunately, the value is only
used when connected - but it could conceivable be used when the
connector status is UNKNOWN but then it is either still set as
DRM_MODE_CONNECTOR_Unknown from init, or the last known active value.

> It is a bit inconsistent however due to the fact that we initialize type
> to DRM_MODE_CONNECTOR_Unknown, and later we use the -1 to indicate more
> or less the same thing. Seems like we should pick or the other for
> consistency.

Yup. Might as well do that now, or else it will another few years before
the next pass through tumbleweed town.
 
> Apart from the little things like the weird pre-exising extra
> ddc detect and the intel_tv->type inconsistency this patch looks OK to
> me, so
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

That DDC is definitely weird, but one I'm not inclined to touch without
good reason.  Preparing a v2 with the minor tv_type change, I'll
presumptively stick your r-b on it ;)
-chris
Shuang He June 5, 2015, 11:10 a.m. UTC | #3
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6536
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  270/270              270/270
ILK                                  303/303              303/303
SNB                                  312/312              312/312
IVB                                  343/343              343/343
BYT                                  287/287              287/287
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 521af2c..70c5288 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -666,8 +666,6 @@  intel_crt_detect(struct drm_connector *connector, bool force)
 	struct intel_encoder *intel_encoder = &crt->base;
 	enum intel_display_power_domain power_domain;
 	enum drm_connector_status status;
-	struct intel_load_detect_pipe tmp;
-	struct drm_modeset_acquire_ctx ctx;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, connector->name,
@@ -703,27 +701,26 @@  intel_crt_detect(struct drm_connector *connector, bool force)
 		goto out;
 	}
 
-	if (!force) {
-		status = connector->status;
-		goto out;
-	}
-
-	drm_modeset_acquire_init(&ctx, 0);
-
-	/* for pre-945g platforms use load detect */
-	if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
-		if (intel_crt_detect_ddc(connector))
-			status = connector_status_connected;
-		else if (INTEL_INFO(dev)->gen < 4)
-			status = intel_crt_load_detect(crt);
-		else
+	status = connector->status;
+	if (force) {
+		struct intel_load_detect_pipe tmp;
+		struct drm_modeset_acquire_ctx ctx;
+
+		drm_modeset_acquire_init(&ctx, 0);
+
+		/* for pre-945g platforms use load detect */
+		if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) {
+			if (intel_crt_detect_ddc(connector))
+				status = connector_status_connected;
+			else if (INTEL_INFO(dev)->gen < 4)
+				status = intel_crt_load_detect(crt);
+			intel_release_load_detect_pipe(connector, &tmp, &ctx);
+		} else if (status == connector_status_connected)
 			status = connector_status_unknown;
-		intel_release_load_detect_pipe(connector, &tmp, &ctx);
-	} else
-		status = connector_status_unknown;
 
-	drm_modeset_drop_locks(&ctx);
-	drm_modeset_acquire_fini(&ctx);
+		drm_modeset_drop_locks(&ctx);
+		drm_modeset_acquire_fini(&ctx);
+	}
 
 out:
 	intel_display_power_put(dev_priv, power_domain);
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 8b9d325..e3c0fed 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1313,44 +1313,37 @@  static void intel_tv_find_better_format(struct drm_connector *connector)
 static enum drm_connector_status
 intel_tv_detect(struct drm_connector *connector, bool force)
 {
-	struct drm_display_mode mode;
 	struct intel_tv *intel_tv = intel_attached_tv(connector);
-	enum drm_connector_status status;
-	int type;
+	enum drm_connector_status status = connector->status;
 
 	DRM_DEBUG_KMS("[CONNECTOR:%d:%s] force=%d\n",
 		      connector->base.id, connector->name,
 		      force);
-
-	mode = reported_modes[0];
-
 	if (force) {
+		struct drm_display_mode mode = reported_modes[0];
 		struct intel_load_detect_pipe tmp;
 		struct drm_modeset_acquire_ctx ctx;
 
 		drm_modeset_acquire_init(&ctx, 0);
 
 		if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) {
-			type = intel_tv_detect_type(intel_tv, connector);
+			int type = intel_tv_detect_type(intel_tv, connector);
 			intel_release_load_detect_pipe(connector, &tmp, &ctx);
 			status = type < 0 ?
 				connector_status_disconnected :
 				connector_status_connected;
-		} else
+			intel_tv->type = type;
+		} else if (status == connector_status_connected)
 			status = connector_status_unknown;
 
 		drm_modeset_drop_locks(&ctx);
 		drm_modeset_acquire_fini(&ctx);
-	} else
-		return connector->status;
 
-	if (status != connector_status_connected)
-		return status;
-
-	intel_tv->type = type;
-	intel_tv_find_better_format(connector);
+		if (status == connector_status_connected)
+			intel_tv_find_better_format(connector);
+	}
 
-	return connector_status_connected;
+	return status;
 }
 
 static const struct input_res {