| Message ID | 1433431578-14373-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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 '*'
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 {
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(-)