On Fri, Jul 19, 2013 at 12:57 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote: > Atm the crtc helper implementation of set_config has really > inconsisten semantics: If just an fb update is good enough, dpms state > will be left as-is, but if we do a full modeset we force everything to > dpms on. > > This change has already been applied to the i915 modeset code in > > commit e3de42b68478a8c95dd27520e9adead2af9477a5 > Author: Imre Deak <imre.deak@xxxxxxxxx> > Date: Fri May 3 19:44:07 2013 +0200 > > drm/i915: force full modeset if the connector is in DPMS OFF mode > > which according to Greg KH seems to aim for a new record in most > Bugzilla: links in a commit message. > > The history of this dpms forcing is pretty interesting. This patch > here is an almost-revert of > > commit 811aaa55ba21ab37407018cfc01770d6b037d3fb > Author: Keith Packard <keithp@xxxxxxxxxx> > Date: Thu Feb 3 16:57:28 2011 -0800 > > drm: Only set DPMS ON when actually configuring a mode > > which fixed the bug of trying to dpms on disabled outputs, but > introduced the new discrepancy between an fb update only and full > modesets. The actual introduction of this goes back to > > commit bf9dc102e284a5aa78c73fc9d72e11d5ccd8669f > Author: Keith Packard <keithp@xxxxxxxxxx> > Date: Fri Nov 26 10:45:58 2010 -0800 > > drm: Set connector DPMS status to ON in drm_crtc_helper_set_config > > And if you'd dig around in the i915 driver code there's even more fun > around forcing dpms on and losing our heads and temper of the > resulting inconsistencies. Especially the DP re-training code had tons > of funny stuff in it. > > v2: So v1 totally blew up on resume on my radeon system here. After > much head-scraching I've figured out that the radeon resume functions > resumes the console system _before_ it actually restores all the > modeset state. And resuming the console systems means that fbdev doeas > an immediate ->set_par call. > > Now up to this patch that ->set_par did absolutely nothing: All the > old sw state from pre-suspend was still around (since the modeset > reset wasn't done yet), which means that the set_config calls done as > a result of the ->set_par where all treated as no-ops (despite that > the real hw state was obviously something completely different). > > Since v1 of this patch just added a bunch of ->dpms calls if the crtc > was enabled, those set_config calls suddenly stopped being no-ops. But > because the hw state wasn't restored the ->dpms callbacks resulted in > decent amounts of hilarity and eventual full hangs. > > Since I can't review all kms drivers for such tricky ordering > constraints v2 opts for a different approach and forces a full modeset > if the connector dpms state isnt' DPMS_ON. Since the ->dpms callbacks > implemented by the modeset helpers update the connector->dpms property > we have the same effect of ensuring that the pipe is ultimately turned > on, even if we just end up updating the fb. This is the same approac > we ended up using in the intel driver. > > Note that besides i915.ko only all other drivers eventually call > drm_helper_connector_dpms with the exception of vmwgfx, which does not > support dmps at all. > > v3: Dave Airlie merged the broken first version of this patch, so > squash in the revert of > > commit 372835a8527f85b3eff20a18c2c339e827dfd4e4 > Author: Daniel Vetter <daniel.vetter@xxxxxxxx> > Date: Sat Jun 15 00:13:13 2013 +0200 > > drm/crtc-helper: explicit DPMS on after modeset > > Also fix up the spelling fail a bit in the commit message while at it. > > Cc: Dave Airlie <airlied@xxxxxxxxxx> > Cc: Alex Deucher <alexdeucher@xxxxxxxxx> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67043 > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxx> Reviewed-by: Alex Deucher <alexander.deucher@xxxxxxx> Confirmed to fix bug 67043. > --- > drivers/gpu/drm/drm_crtc_helper.c | 27 +++++++++++---------------- > 1 file changed, 11 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c > index edb0de5..eb51dc4 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -669,6 +669,11 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > /* don't break so fail path works correct */ > fail = 1; > break; > + > + if (connector->dpms != DRM_MODE_DPMS_ON) { > + DRM_DEBUG_KMS("connector dpms not on, full mode switch\n"); > + mode_changed = true; > + } > } > } > > @@ -746,6 +751,12 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > ret = -EINVAL; > goto fail; > } > + DRM_DEBUG_KMS("Setting connector DPMS state to on\n"); > + for (i = 0; i < set->num_connectors; i++) { > + DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id, > + drm_get_connector_name(set->connectors[i])); > + set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON); > + } > } > drm_helper_disable_unused_functions(dev); > } else if (fb_changed) { > @@ -763,22 +774,6 @@ int drm_crtc_helper_set_config(struct drm_mode_set *set) > } > } > > - /* > - * crtc set_config helpers implicit set the crtc and all connected > - * encoders to DPMS on for a full mode set. But for just an fb update it > - * doesn't do that. To not confuse userspace, do an explicit DPMS_ON > - * unconditionally. This will also ensure driver internal dpms state is > - * consistent again. > - */ > - if (set->crtc->enabled) { > - DRM_DEBUG_KMS("Setting connector DPMS state to on\n"); > - for (i = 0; i < set->num_connectors; i++) { > - DRM_DEBUG_KMS("\t[CONNECTOR:%d:%s] set DPMS on\n", set->connectors[i]->base.id, > - drm_get_connector_name(set->connectors[i])); > - set->connectors[i]->funcs->dpms(set->connectors[i], DRM_MODE_DPMS_ON); > - } > - } > - > kfree(save_connectors); > kfree(save_encoders); > kfree(save_crtcs); > -- > 1.8.3.2 > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
