diff mbox

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

Message ID 55B61034.8000909@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maarten Lankhorst July 27, 2015, 11:04 a.m. UTC 
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.

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---

Comments

Maarten Lankhorst July 27, 2015, 11:15 a.m. UTC | #1 
Op 27-07-15 om 13:16 schreef 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.
>
In that case I'll respin..
Daniel Vetter July 27, 2015, 11:16 a.m. UTC | #2 
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@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@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;
>
diff mbox

Patch

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;