Re: [PATCH] drm/crtc-helper: explicit DPMS on after modeset

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux