On Thu, Jul 16, 2015 at 11:24:02AM +0200, Maarten Lankhorst wrote:

Op 16-07-15 om 11:19 schreef Daniel Vetter:

...

On Thu, Jul 16, 2015 at 10:59:15AM +0200, Maarten Lankhorst wrote:

...

This is required for DPMS to work correctly, during a modeset the DPMS property should be turned off, unless the crtc is made active in which case it should be set to DPMS on.

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

---

drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 70e69904291d..cdec643971a2 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -642,6 +642,12 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,

/* clear out existing links */ for_each_connector_in_state(old_state, connector, old_conn_state, i) {

• struct drm_crtc *crtc = connector->state->crtc;
  

• if (crtc &&
  

•     drm_atomic_crtc_needs_modeset(crtc->state))
  

• 	connector->dpms = DRM_MODE_DPMS_OFF;
  

Same here, why only update when something changed? I already applied my patch from yesterday which updates this always (with Daniel Stone's r-b), does that one not work?

Not when in cloned mode.

2 connectors on same crtc.

Update dpms property to off connector 1, commit. update_legacy_modeset_state will reset it to DPMS_ON. Update dpms property to off on connector 2, commit. update_legacy_modeset_state will reset it to DPMS_ON.

Expected result: DPMS on the screen is OFF Actual result: ON with i915.

Ok this definitely needs a comment plus better commit message somewhere since obviously I understood it only now. I'll drop my patch. -Daniel

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;

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.

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. Changes since v2: - Do not preserve dpms property.

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 57847ae8ce8c..0b475fae067d 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -660,15 +660,29 @@ 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) - continue; + if (connector->encoder) { + WARN_ON(!connector->encoder->crtc); + + connector->encoder->crtc = NULL; + connector->encoder = NULL; + }

- WARN_ON(!connector->encoder->crtc); + crtc = connector->state->crtc; + 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;

- connector->encoder->crtc = NULL; - connector->encoder = NULL; + 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 */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 13a6608be689..43b0f17ad1fa 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12349,16 +12349,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; + } } }