[02/13] drm/atomic: Update legacy DPMS state during modesets.
diff mbox

Message ID 1437037166-9339-3-git-send-email-maarten.lankhorst@linux.intel.com
State New
Headers show

Commit Message

Maarten Lankhorst July 16, 2015, 8:59 a.m. UTC
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(-)

Comments

Daniel Vetter July 16, 2015, 9:19 a.m. UTC | #1
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?
-Daniel

> +
>  		if (!connector->encoder)
>  			continue;
>  
> @@ -653,14 +659,20 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>  
>  	/* set new links */
>  	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
> -		if (!connector->state->crtc)
> +		struct drm_crtc *crtc = connector->state->crtc;
> +
> +		if (!crtc)
>  			continue;
>  
> +		if (crtc->state->active &&
> +		    drm_atomic_crtc_needs_modeset(crtc->state))
> +			connector->dpms = DRM_MODE_DPMS_ON;
> +
>  		if (WARN_ON(!connector->state->best_encoder))
>  			continue;
>  
>  		connector->encoder = connector->state->best_encoder;
> -		connector->encoder->crtc = connector->state->crtc;
> +		connector->encoder->crtc = crtc;
>  	}
>  
>  	/* set legacy state in the crtc structure */
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
Maarten Lankhorst July 16, 2015, 9:24 a.m. UTC | #2
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.
Daniel Vetter July 16, 2015, 9:31 a.m. UTC | #3
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

Patch
diff mbox

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;
+
 		if (!connector->encoder)
 			continue;
 
@@ -653,14 +659,20 @@  drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 
 	/* set new links */
 	for_each_connector_in_state(old_state, connector, old_conn_state, i) {
-		if (!connector->state->crtc)
+		struct drm_crtc *crtc = connector->state->crtc;
+
+		if (!crtc)
 			continue;
 
+		if (crtc->state->active &&
+		    drm_atomic_crtc_needs_modeset(crtc->state))
+			connector->dpms = DRM_MODE_DPMS_ON;
+
 		if (WARN_ON(!connector->state->best_encoder))
 			continue;
 
 		connector->encoder = connector->state->best_encoder;
-		connector->encoder->crtc = connector->state->crtc;
+		connector->encoder->crtc = crtc;
 	}
 
 	/* set legacy state in the crtc structure */