[Intel-gfx] [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Jun 4 09:28:31 PDT 2015
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 at 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 at linux.intel.com>
>
> - return connector_status_connected;
> + return status;
> }
>
> static const struct input_res {
> --
> 1.9.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list