Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 05, 2015 at 08:46:19AM +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.
> 
> v2: Convert tv_type to use proper unknown enum (Ville)
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=87049
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com> [v1]
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> [v1]

This solution is at odds with

commit b7703726251191cd9f3ef3a80b2d9667901eec95
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Wed Jan 21 08:45:22 2015 +0100

    drm/probe-helper: clamp unknown connector status in the poll work

We're missing this bit of logic from the hotplug handlers, but that was
somewhat intentional since a hotplug should indicate that something has
changed. And in the i915 hpd handler we filter by source.

Given that the report is older than me having resurrect that patch can we
close it as fixed or do I miss something?

Thanks, Daniel

> ---
>  drivers/gpu/drm/i915/intel_crt.c | 39 ++++++++++++++++++---------------------
>  drivers/gpu/drm/i915/intel_tv.c  | 37 ++++++++++++++-----------------------
>  2 files changed, 32 insertions(+), 44 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;
> +			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..135584f 100644
> --- a/drivers/gpu/drm/i915/intel_tv.c
> +++ b/drivers/gpu/drm/i915/intel_tv.c
> @@ -1233,7 +1233,6 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  	intel_wait_for_vblank(intel_tv->base.base.dev,
>  			      to_intel_crtc(intel_tv->base.base.crtc)->pipe);
>  
> -	type = -1;
>  	tv_dac = I915_READ(TV_DAC);
>  	DRM_DEBUG_KMS("TV detected: %x, %x\n", tv_ctl, tv_dac);
>  	/*
> @@ -1253,7 +1252,7 @@ intel_tv_detect_type(struct intel_tv *intel_tv,
>  		type = DRM_MODE_CONNECTOR_Component;
>  	} else {
>  		DRM_DEBUG_KMS("Unrecognised TV connection\n");
> -		type = -1;
> +		type = DRM_MODE_CONNECTOR_Unknown;
>  	}
>  
>  	I915_WRITE(TV_DAC, save_tv_dac & ~TVDAC_STATE_CHG_EN);
> @@ -1313,44 +1312,36 @@ 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 intel_tv *intel_tv = intel_attached_tv(connector);
> +		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);
> +			intel_tv->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
> +			if (intel_tv->type != DRM_MODE_CONNECTOR_Unknown) {
> +				intel_tv_find_better_format(connector);
> +				status = connector_status_connected;
> +			} else
> +				status = connector_status_disconnected;
> +		} 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);
> +	}
>  
> -	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

-- 
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