* [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes @ 2015-06-04 15:26 Chris Wilson 2015-06-04 16:28 ` Ville Syrjälä 2015-06-05 11:10 ` shuang.he 0 siblings, 2 replies; 13+ messages in thread From: Chris Wilson @ 2015-06-04 15:26 UTC (permalink / raw) To: intel-gfx 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; + 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 { -- 1.9.1 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-04 15:26 [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes Chris Wilson @ 2015-06-04 16:28 ` Ville Syrjälä 2015-06-04 20:23 ` Chris Wilson 2015-06-05 7:46 ` Chris Wilson 2015-06-05 11:10 ` shuang.he 1 sibling, 2 replies; 13+ messages in thread From: Ville Syrjälä @ 2015-06-04 16:28 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 -- Ville Syrjälä Intel OTC _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-04 16:28 ` Ville Syrjälä @ 2015-06-04 20:23 ` Chris Wilson 2015-06-05 7:46 ` Chris Wilson 1 sibling, 0 replies; 13+ messages in thread From: Chris Wilson @ 2015-06-04 20:23 UTC (permalink / raw) To: Ville Syrjälä; +Cc: 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 -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-04 16:28 ` Ville Syrjälä 2015-06-04 20:23 ` Chris Wilson @ 2015-06-05 7:46 ` Chris Wilson 2015-06-05 20:37 ` shuang.he 2015-06-15 12:29 ` Daniel Vetter 1 sibling, 2 replies; 13+ messages in thread From: Chris Wilson @ 2015-06-05 7:46 UTC (permalink / raw) To: intel-gfx 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] --- 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 ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-05 7:46 ` Chris Wilson @ 2015-06-05 20:37 ` shuang.he 2015-06-15 12:29 ` Daniel Vetter 1 sibling, 0 replies; 13+ messages in thread From: shuang.he @ 2015-06-05 20:37 UTC (permalink / raw) To: shuang.he, lei.a.liu, intel-gfx, chris Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com) Task id: 6544 -------------------------------------Summary------------------------------------- Platform Delta drm-intel-nightly Series Applied PNV -1 270/270 269/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 *PNV igt@gem_tiled_pread_pwrite PASS(1) FAIL(1) Note: You need to pay more attention to line start with '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-05 7:46 ` Chris Wilson 2015-06-05 20:37 ` shuang.he @ 2015-06-15 12:29 ` Daniel Vetter 2015-06-15 12:35 ` Chris Wilson 1 sibling, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2015-06-15 12:29 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-15 12:29 ` Daniel Vetter @ 2015-06-15 12:35 ` Chris Wilson 2015-06-15 13:03 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Chris Wilson @ 2015-06-15 12:35 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Mon, Jun 15, 2015 at 02:29:28PM +0200, Daniel Vetter wrote: > 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? It's a different path. This also concerns the actual forced reprobe fluctuating between two states. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-15 12:35 ` Chris Wilson @ 2015-06-15 13:03 ` Daniel Vetter 2015-06-15 13:37 ` Chris Wilson 0 siblings, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2015-06-15 13:03 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, intel-gfx On Mon, Jun 15, 2015 at 01:35:21PM +0100, Chris Wilson wrote: > On Mon, Jun 15, 2015 at 02:29:28PM +0200, Daniel Vetter wrote: > > 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? > > It's a different path. This also concerns the actual forced reprobe > fluctuating between two states. In that case I still think it should be in the probe helpers. Which raises the policy decision whether we should ever hand unkown back to userspace since apparently it just can't cope sensible with it. But that call should be done consistently accross drivers. -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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-15 13:03 ` Daniel Vetter @ 2015-06-15 13:37 ` Chris Wilson 2015-06-15 15:18 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Chris Wilson @ 2015-06-15 13:37 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Mon, Jun 15, 2015 at 03:03:39PM +0200, Daniel Vetter wrote: > On Mon, Jun 15, 2015 at 01:35:21PM +0100, Chris Wilson wrote: > > On Mon, Jun 15, 2015 at 02:29:28PM +0200, Daniel Vetter wrote: > > > 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? > > > > It's a different path. This also concerns the actual forced reprobe > > fluctuating between two states. > > In that case I still think it should be in the probe helpers. Which raises > the policy decision whether we should ever hand unkown back to userspace > since apparently it just can't cope sensible with it. But that call should > be done consistently accross drivers. Hmm, the probe helper is not the central arbiter here which is a problem in moving it entirely into its domain. UNKNOWN makes a sense form the hw perspective, and we still need to report that status before any probes are made at all (whether that is init/resume) and the problem in this case is not so much as userspace mishandling it, but the fluctuation between UNKNOWN/DISCONNECTED is causing activity and reconfiguration not entirely of its own fault. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-15 13:37 ` Chris Wilson @ 2015-06-15 15:18 ` Daniel Vetter 2015-06-19 13:21 ` Chris Wilson 0 siblings, 1 reply; 13+ messages in thread From: Daniel Vetter @ 2015-06-15 15:18 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, intel-gfx On Mon, Jun 15, 2015 at 02:37:58PM +0100, Chris Wilson wrote: > On Mon, Jun 15, 2015 at 03:03:39PM +0200, Daniel Vetter wrote: > > On Mon, Jun 15, 2015 at 01:35:21PM +0100, Chris Wilson wrote: > > > On Mon, Jun 15, 2015 at 02:29:28PM +0200, Daniel Vetter wrote: > > > > 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? > > > > > > It's a different path. This also concerns the actual forced reprobe > > > fluctuating between two states. > > > > In that case I still think it should be in the probe helpers. Which raises > > the policy decision whether we should ever hand unkown back to userspace > > since apparently it just can't cope sensible with it. But that call should > > be done consistently accross drivers. > > Hmm, the probe helper is not the central arbiter here which is a problem > in moving it entirely into its domain. UNKNOWN makes a sense form the hw > perspective, and we still need to report that status before any probes > are made at all (whether that is init/resume) and the problem in this > case is not so much as userspace mishandling it, but the fluctuation > between UNKNOWN/DISCONNECTED is causing activity and reconfiguration not > entirely of its own fault. Well for the poll worker part of the probe helpers I went with if (new_status == unknown) connector->status = old_status; which takes care of the init/resume time unknown->(dis)connected transition correctly while filtering the noise. Adding that to the synchronous users probe function (which is the only one that sets force == true) would achieve what you want I think. But it also means that we almost always filter out unknown and never let userspace known that the hw detection is uncertain. Maybe we instead need a drm helper module option to clamp unknown uncondiotionally to disconnected to allow users to hack around silly userspace? -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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-15 15:18 ` Daniel Vetter @ 2015-06-19 13:21 ` Chris Wilson 2015-06-22 12:12 ` Daniel Vetter 0 siblings, 1 reply; 13+ messages in thread From: Chris Wilson @ 2015-06-19 13:21 UTC (permalink / raw) To: Daniel Vetter; +Cc: intel-gfx On Mon, Jun 15, 2015 at 05:18:40PM +0200, Daniel Vetter wrote: > On Mon, Jun 15, 2015 at 02:37:58PM +0100, Chris Wilson wrote: > > On Mon, Jun 15, 2015 at 03:03:39PM +0200, Daniel Vetter wrote: > > > On Mon, Jun 15, 2015 at 01:35:21PM +0100, Chris Wilson wrote: > > > > On Mon, Jun 15, 2015 at 02:29:28PM +0200, Daniel Vetter wrote: > > > > > 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? > > > > > > > > It's a different path. This also concerns the actual forced reprobe > > > > fluctuating between two states. > > > > > > In that case I still think it should be in the probe helpers. Which raises > > > the policy decision whether we should ever hand unkown back to userspace > > > since apparently it just can't cope sensible with it. But that call should > > > be done consistently accross drivers. > > > > Hmm, the probe helper is not the central arbiter here which is a problem > > in moving it entirely into its domain. UNKNOWN makes a sense form the hw > > perspective, and we still need to report that status before any probes > > are made at all (whether that is init/resume) and the problem in this > > case is not so much as userspace mishandling it, but the fluctuation > > between UNKNOWN/DISCONNECTED is causing activity and reconfiguration not > > entirely of its own fault. > > Well for the poll worker part of the probe helpers I went with > > if (new_status == unknown) > connector->status = old_status; > > which takes care of the init/resume time unknown->(dis)connected > transition correctly while filtering the noise. Adding that to the > synchronous users probe function (which is the only one that sets force == > true) would achieve what you want I think. But it also means that we > almost always filter out unknown and never let userspace known that the hw > detection is uncertain. I thought that connected->unknown was less likely and of more significance than disconnected<->unknown. But that was from thinking that the output was in use whilst it was connected, and so should always have the resources available to do the detection. On second thought, we should always just suppress the ->unknown transition. The best way to then report the unreliable detection would to be report an error trying to connect to the CRT/TV (by a vblank load detect cycle) and signal EREMOTEIO in enable -- but the modesetting code still does not report errors, despite their continued prevalence (such as training failures). > Maybe we instead need a drm helper module option to clamp unknown > uncondiotionally to disconnected to allow users to hack around silly > userspace? We have userspace that respects unknown, after it is what the fb_helper was copied from. Unknown is unknown, userspace can either treat it as potentially connector or potentially disconnected depending on best that suites its purposes. So I don't think a global change is sensible. -Chris -- Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-19 13:21 ` Chris Wilson @ 2015-06-22 12:12 ` Daniel Vetter 0 siblings, 0 replies; 13+ messages in thread From: Daniel Vetter @ 2015-06-22 12:12 UTC (permalink / raw) To: Chris Wilson, Daniel Vetter, intel-gfx On Fri, Jun 19, 2015 at 02:21:59PM +0100, Chris Wilson wrote: > > Maybe we instead need a drm helper module option to clamp unknown > > uncondiotionally to disconnected to allow users to hack around silly > > userspace? > > We have userspace that respects unknown, after it is what the fb_helper > was copied from. Unknown is unknown, userspace can either treat it as > potentially connector or potentially disconnected depending on best that > suites its purposes. So I don't think a global change is sensible. Well you implement some kind of clamping in i915 drivers. My stance is that no matter what exact kind of clamping we agree on it should be implemented in shared code in the probe helpers. And we already have quite a bit of unknown duct-taping going on in the probe helpers, but it seems to not be enough. And tbh I'm not sure what more we need short of just clamping unknown in general. -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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes 2015-06-04 15:26 [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes Chris Wilson 2015-06-04 16:28 ` Ville Syrjälä @ 2015-06-05 11:10 ` shuang.he 1 sibling, 0 replies; 13+ messages in thread From: shuang.he @ 2015-06-05 11:10 UTC (permalink / raw) To: shuang.he, lei.a.liu, intel-gfx, 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 '*' _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2015-06-22 12:09 UTC | newest] Thread overview: 13+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-06-04 15:26 [PATCH] drm/i915: Avoid fluctuating to UNKNOWN connector status during forced probes Chris Wilson 2015-06-04 16:28 ` Ville Syrjälä 2015-06-04 20:23 ` Chris Wilson 2015-06-05 7:46 ` Chris Wilson 2015-06-05 20:37 ` shuang.he 2015-06-15 12:29 ` Daniel Vetter 2015-06-15 12:35 ` Chris Wilson 2015-06-15 13:03 ` Daniel Vetter 2015-06-15 13:37 ` Chris Wilson 2015-06-15 15:18 ` Daniel Vetter 2015-06-19 13:21 ` Chris Wilson 2015-06-22 12:12 ` Daniel Vetter 2015-06-05 11:10 ` shuang.he
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox