On Mon, Jul 27, 2015 at 01:04:20PM +0200, Maarten Lankhorst wrote:
> This is required for DPMS to work correctly, during a modeset
> the DPMS property should be turned off, unless the state is
> crtc is made active in which case it should be set to DPMS on.
>
> The legacy dpms handling performs its own dpms updates, so add a
> property to prevent updating the legacy dpms state and use it
> in the atomic dpms helpers and i915 suspend/resume.
>
> Changes since v1:
> - Set DPMS to off when a connector is removed from a crtc too.
> - Update the legacy dpms property too.
> - Add an exception for the legacy dpms paths, it updates its own state.
> - Add an exception for i915 suspend/resume, it should preserve dpms state.
My idea behind updating the dpms prop was to not confuse legacy userspace.
And legacy userspace always does an all-or-nothing dpms over all
connectors anyway, so I don't think we need to go to any length to
preserve that. If we'd need to we won't be able to move dpms from
connector to the crtc anyway. Therefore I think preserve_dpms isn't needed
and we can drop that one.
-Daniel
>
> Cc: dri-devel at lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 5ec13c7cc832..f463f8ce8f4d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -660,15 +660,33 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
> struct drm_crtc_state *old_crtc_state;
> int i;
>
> - /* clear out existing links */
> + /* clear out existing links and update dpms */
> for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> - if (!connector->encoder)
> + if (connector->encoder) {
> + WARN_ON(!connector->encoder->crtc);
> +
> + connector->encoder->crtc = NULL;
> + connector->encoder = NULL;
> + }
> +
> + if (old_state->preserve_dpms)
> continue;
>
> - WARN_ON(!connector->encoder->crtc);
> + crtc = connector->state->crtc;
>
> - connector->encoder->crtc = NULL;
> - connector->encoder = NULL;
> + if ((!crtc && old_conn_state->crtc) ||
> + (crtc && drm_atomic_crtc_needs_modeset(crtc->state))) {
> + struct drm_property *dpms_prop =
> + dev->mode_config.dpms_property;
> + int mode = DRM_MODE_DPMS_OFF;
> +
> + if (crtc && crtc->state->active)
> + mode = DRM_MODE_DPMS_ON;
> +
> + connector->dpms = mode;
> + drm_object_property_set_value(&connector->base,
> + dpms_prop, mode);
> + }
> }
>
> /* set new links */
> @@ -2001,6 +2019,7 @@ void drm_atomic_helper_connector_dpms(struct drm_connector *connector,
> return;
>
> state->acquire_ctx = drm_modeset_legacy_acquire_ctx(crtc);
> + state->preserve_dpms = true;
> retry:
> crtc_state = drm_atomic_get_crtc_state(state, crtc);
> if (IS_ERR(crtc_state))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 28ff75bc9e04..4e49b6667ffa 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6246,7 +6246,7 @@ int intel_display_suspend(struct drm_device *dev)
> return -ENOMEM;
>
> state->acquire_ctx = ctx;
> - state->allow_modeset = true;
> + state->preserve_dpms = true;
>
> for_each_crtc(dev, crtc) {
> struct drm_crtc_state *crtc_state =
> @@ -6309,7 +6309,7 @@ int intel_crtc_control(struct drm_crtc *crtc, bool enable)
> return -ENOMEM;
>
> state->acquire_ctx = ctx;
> - state->allow_modeset = true;
> + state->preserve_dpms = true;
>
> pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
> if (IS_ERR(pipe_config)) {
> @@ -12358,16 +12358,9 @@ intel_modeset_update_state(struct drm_atomic_state *state)
> continue;
>
> if (crtc->state->active) {
> - struct drm_property *dpms_property =
> - dev->mode_config.dpms_property;
> -
> - connector->dpms = DRM_MODE_DPMS_ON;
> - drm_object_property_set_value(&connector->base, dpms_property, DRM_MODE_DPMS_ON);
> -
> intel_encoder = to_intel_encoder(connector->encoder);
> intel_encoder->connectors_active = true;
> - } else
> - connector->dpms = DRM_MODE_DPMS_OFF;
> + }
> }
> }
>
> @@ -15417,6 +15410,7 @@ void intel_display_resume(struct drm_device *dev)
> return;
>
> state->acquire_ctx = dev->mode_config.acquire_ctx;
> + state->preserve_dpms = true;
>
> /* preserve complete old state, including dpll */
> intel_atomic_get_shared_dpll_state(state);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 90a0ff70384a..64d49307c76d 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -937,6 +937,7 @@ struct drm_bridge {
> * @dev: parent DRM device
> * @allow_modeset: allow full modeset
> * @legacy_cursor_update: hint to enforce legacy cursor ioctl semantics
> + * @preserve_dpms: the caller wants to preserve connector->dpms state.
> * @planes: pointer to array of plane pointers
> * @plane_states: pointer to array of plane states pointers
> * @crtcs: pointer to array of CRTC pointers
> @@ -950,6 +951,7 @@ struct drm_atomic_state {
> struct drm_device *dev;
> bool allow_modeset : 1;
> bool legacy_cursor_update : 1;
> + bool preserve_dpms : 1;
> struct drm_plane **planes;
> struct drm_plane_state **plane_states;
> struct drm_crtc **crtcs;
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch