On Fri, Jul 19, 2013 at 12:57 PM, Daniel Vetter <daniel.vetter at ffwll.ch>
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 at intel.com>
> 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 at keithp.com>
> 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 at keithp.com>
> 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 at ffwll.ch>
> 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 at redhat.com>
> Cc: Alex Deucher <alexdeucher at gmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67043
> Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch>
Reviewed-by: Alex Deucher <alexander.deucher at amd.com>
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
>
