| Message ID | 1370470646-20006-2-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 06, 2013 at 12:17:26AM +0200, Daniel Vetter wrote: > On some chipset we try to avoid possibly invasive output detection > methods (like load detect which can cause flickering elsewhere) in the > output poll work. Drivers could hence return unknown when a previous > full ->detect call returned a different state. > > This change will generate a hotplug event, forcing userspace to do a > full scan. This in turn updates the connector->status field so that we > will _again_ get a state change when the hotplug work re-runs in 10 > seconds. > > To avoid this ping-pong loop detect this situation and clamp the > connector state to the old value. > > Patch is inspired by a patch from Knut Peterson. Knut's patch > completely ignored connector state changes if either the old or new > status was unknown, which seemed to be a bit too agressive to me. > > References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html > Cc: Knut Petersen <Knut_Petersen@t-online.de> > Cc: Alex Deucher <alexander.deucher@amd.com> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> Also Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> I don't think this has any effect for i915, the circumstances where it might we return connector->status rather than Unknown, so we need some input from nouveau/radeon/architect as to our sanity. -Chris
On Thu, Jun 6, 2013 at 9:49 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Jun 06, 2013 at 12:17:26AM +0200, Daniel Vetter wrote: >> On some chipset we try to avoid possibly invasive output detection >> methods (like load detect which can cause flickering elsewhere) in the >> output poll work. Drivers could hence return unknown when a previous >> full ->detect call returned a different state. >> >> This change will generate a hotplug event, forcing userspace to do a >> full scan. This in turn updates the connector->status field so that we >> will _again_ get a state change when the hotplug work re-runs in 10 >> seconds. >> >> To avoid this ping-pong loop detect this situation and clamp the >> connector state to the old value. >> >> Patch is inspired by a patch from Knut Peterson. Knut's patch >> completely ignored connector state changes if either the old or new >> status was unknown, which seemed to be a bit too agressive to me. >> >> References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html >> Cc: Knut Petersen <Knut_Petersen@t-online.de> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Also Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > I don't think this has any effect for i915, the circumstances where it > might we return connector->status rather than Unknown, so we need some > input from nouveau/radeon/architect as to our sanity. Hm, now I'm confused. The case I've had in mind indeed already works on i915.ko, but Knut's bug report was on a i915gm. The only place where we return unknown is when force==true and we can't get a load detect pipe. That shouldn't be able to ping-pong ... Knut, can you please shed some clue on me here? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Thu, Jun 6, 2013 at 3:49 AM, Chris Wilson <chris@chris-wilson.co.uk> wrote: > On Thu, Jun 06, 2013 at 12:17:26AM +0200, Daniel Vetter wrote: >> On some chipset we try to avoid possibly invasive output detection >> methods (like load detect which can cause flickering elsewhere) in the >> output poll work. Drivers could hence return unknown when a previous >> full ->detect call returned a different state. >> >> This change will generate a hotplug event, forcing userspace to do a >> full scan. This in turn updates the connector->status field so that we >> will _again_ get a state change when the hotplug work re-runs in 10 >> seconds. >> >> To avoid this ping-pong loop detect this situation and clamp the >> connector state to the old value. >> >> Patch is inspired by a patch from Knut Peterson. Knut's patch >> completely ignored connector state changes if either the old or new >> status was unknown, which seemed to be a bit too agressive to me. >> >> References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html >> Cc: Knut Petersen <Knut_Petersen@t-online.de> >> Cc: Alex Deucher <alexander.deucher@amd.com> >> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> > > Also Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > I don't think this has any effect for i915, the circumstances where it > might we return connector->status rather than Unknown, so we need some > input from nouveau/radeon/architect as to our sanity. It shouldn't affect radeon at all. We shouldn't ever see the unknown status in practice. The only time we return unknown is as a sanity check in our load detection functions (e.g., if the vbios is missing the load detect command table or if we somehow end up in dac load detect on a digital encoder). So, FWIW, Acked-by: Alex Deucher <alexander.deucher@amd.com> Alex
diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c index 1c0c0bc..9985a17 100644 --- a/drivers/gpu/drm/drm_crtc_helper.c +++ b/drivers/gpu/drm/drm_crtc_helper.c @@ -1040,6 +1040,24 @@ static void output_poll_execute(struct work_struct *work) if (old_status != connector->status) { const char *old, *new; + /* + * The poll work sets force=false when calling detect so + * that drivers can avoid to do disruptive tests (e.g. + * when load detect cycles could cause flickering on + * other, running displays). This bears the risk that we + * flip-flop between unknown here in the poll work and + * the real state when userspace forces a full detect + * call after receiving a hotplug event due to this + * change. + * + * Hence clamp an unknown detect status to the old + * value. + */ + if (connector->status == connector_status_unknown) { + connector->status = old_status; + continue; + } + old = drm_get_connector_status_name(old_status); new = drm_get_connector_status_name(connector->status);
On some chipset we try to avoid possibly invasive output detection methods (like load detect which can cause flickering elsewhere) in the output poll work. Drivers could hence return unknown when a previous full ->detect call returned a different state. This change will generate a hotplug event, forcing userspace to do a full scan. This in turn updates the connector->status field so that we will _again_ get a state change when the hotplug work re-runs in 10 seconds. To avoid this ping-pong loop detect this situation and clamp the connector state to the old value. Patch is inspired by a patch from Knut Peterson. Knut's patch completely ignored connector state changes if either the old or new status was unknown, which seemed to be a bit too agressive to me. References: http://lists.freedesktop.org/archives/dri-devel/2012-August/025975.html Cc: Knut Petersen <Knut_Petersen@t-online.de> Cc: Alex Deucher <alexander.deucher@amd.com> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- drivers/gpu/drm/drm_crtc_helper.c | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)