[Intel-gfx] [PATCH v1.1 02/13] drm/atomic: Update legacy DPMS state during modesets, v2.

[Daniel Vetter]

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