[PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Maarten Lankhorst]

Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 0898afbc9e23..70e69904291d 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
 		crtc->mode = crtc->state->mode;
 		crtc->enabled = crtc->state->enable;
-		crtc->x = crtc->primary->state->src_x >> 16;
-		crtc->y = crtc->primary->state->src_y >> 16;
+
+		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
+			crtc->x = crtc->primary->state->src_x >> 16;
+			crtc->y = crtc->primary->state->src_y >> 16;
+		}
 
 		if (crtc->state->enable)
 			drm_calc_timestamping_constants(crtc,
-- 
2.1.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Maarten Lankhorst]

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;
+
 		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

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Maarten Lankhorst]

Op 16-07-15 om 11:19 schreef Daniel Vetter:
> On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>> index 0898afbc9e23..70e69904291d 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>>  		crtc->mode = crtc->state->mode;
>>  		crtc->enabled = crtc->state->enable;
>> -		crtc->x = crtc->primary->state->src_x >> 16;
>> -		crtc->y = crtc->primary->state->src_y >> 16;
>> +
>> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
>> +			crtc->x = crtc->primary->state->src_x >> 16;
>> +			crtc->y = crtc->primary->state->src_y >> 16;
>> +		}
> What's the benefit here of only updating when something changed? The
> atomic state should be the master source so copying a few too many times
> shouldn't matter really.
Because you might not be holding plane lock, so primary->state may be garbage.

~Maarten
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> ---
>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index 0898afbc9e23..70e69904291d 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>  		crtc->mode = crtc->state->mode;
>  		crtc->enabled = crtc->state->enable;
> -		crtc->x = crtc->primary->state->src_x >> 16;
> -		crtc->y = crtc->primary->state->src_y >> 16;
> +
> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
> +			crtc->x = crtc->primary->state->src_x >> 16;
> +			crtc->y = crtc->primary->state->src_y >> 16;
> +		}

What's the benefit here of only updating when something changed? The
atomic state should be the master source so copying a few too many times
shouldn't matter really.
-Daniel

>  
>  		if (crtc->state->enable)
>  			drm_calc_timestamping_constants(crtc,
> -- 
> 2.1.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 02/13] drm/atomic: Update legacy DPMS state during modesets.

[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?
-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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/13] drm/atomic: Update legacy DPMS state during modesets.

[Maarten Lankhorst]

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.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
> Op 16-07-15 om 11:19 schreef Daniel Vetter:
> > On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
> >>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >> index 0898afbc9e23..70e69904291d 100644
> >> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
> >>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> >>  		crtc->mode = crtc->state->mode;
> >>  		crtc->enabled = crtc->state->enable;
> >> -		crtc->x = crtc->primary->state->src_x >> 16;
> >> -		crtc->y = crtc->primary->state->src_y >> 16;
> >> +
> >> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
> >> +			crtc->x = crtc->primary->state->src_x >> 16;
> >> +			crtc->y = crtc->primary->state->src_y >> 16;
> >> +		}
> > What's the benefit here of only updating when something changed? The
> > atomic state should be the master source so copying a few too many times
> > shouldn't matter really.
> Because you might not be holding plane lock, so primary->state may be garbage.

Anyone who wants to touch primary plane must grab the crtc lock, so crtc
lock would give you an implicit read lock. At least that's been my
thinking, but it could be that the primary plane is used on some other
crtc, and then this is indeed garbage.

So maybe we need even more checks than what you propose:

      if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) &&
          crtc->primary->state->crtc == crtc) {
      	crtc->x = crtc->primary->state->src_x >> 16;
      	crtc->y = crtc->primary->state->src_y >> 16;
      }

I think a comment explaining this would help (or at least in the commit
message!).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/13] drm/atomic: Update legacy DPMS state during modesets.

[Daniel Vetter]

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Maarten Lankhorst]

Op 16-07-15 om 11:29 schreef Daniel Vetter:
> On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
>> Op 16-07-15 om 11:19 schreef Daniel Vetter:
>>> On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
>>>> Cc: dri-devel@lists.freedesktop.org
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> ---
>>>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>> index 0898afbc9e23..70e69904291d 100644
>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>>>>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>>>>  		crtc->mode = crtc->state->mode;
>>>>  		crtc->enabled = crtc->state->enable;
>>>> -		crtc->x = crtc->primary->state->src_x >> 16;
>>>> -		crtc->y = crtc->primary->state->src_y >> 16;
>>>> +
>>>> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
>>>> +			crtc->x = crtc->primary->state->src_x >> 16;
>>>> +			crtc->y = crtc->primary->state->src_y >> 16;
>>>> +		}
>>> What's the benefit here of only updating when something changed? The
>>> atomic state should be the master source so copying a few too many times
>>> shouldn't matter really.
>> Because you might not be holding plane lock, so primary->state may be garbage.
> Anyone who wants to touch primary plane must grab the crtc lock, so crtc
> lock would give you an implicit read lock. At least that's been my
> thinking, but it could be that the primary plane is used on some other
> crtc, and then this is indeed garbage.
This is only true if the plane is active. If there is none you can still update properties and
swap the plane state without locking the crtc.

> So maybe we need even more checks than what you propose:
>
>       if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) &&
>           crtc->primary->state->crtc == crtc) {
>       	crtc->x = crtc->primary->state->src_x >> 16;
>       	crtc->y = crtc->primary->state->src_y >> 16;
>       }
>
> I think a comment explaining this would help (or at least in the commit
> message!).
But the primary and cursor planes are not allowed to move between crtc's?

~Maarten
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 11:38:18AM +0200, Maarten Lankhorst wrote:
> Op 16-07-15 om 11:29 schreef Daniel Vetter:
> > On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
> >> Op 16-07-15 om 11:19 schreef Daniel Vetter:
> >>> On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
> >>>> Cc: dri-devel@lists.freedesktop.org
> >>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >>>> ---
> >>>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
> >>>>  1 file changed, 5 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> index 0898afbc9e23..70e69904291d 100644
> >>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
> >>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> >>>> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
> >>>>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> >>>>  		crtc->mode = crtc->state->mode;
> >>>>  		crtc->enabled = crtc->state->enable;
> >>>> -		crtc->x = crtc->primary->state->src_x >> 16;
> >>>> -		crtc->y = crtc->primary->state->src_y >> 16;
> >>>> +
> >>>> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
> >>>> +			crtc->x = crtc->primary->state->src_x >> 16;
> >>>> +			crtc->y = crtc->primary->state->src_y >> 16;
> >>>> +		}
> >>> What's the benefit here of only updating when something changed? The
> >>> atomic state should be the master source so copying a few too many times
> >>> shouldn't matter really.
> >> Because you might not be holding plane lock, so primary->state may be garbage.
> > Anyone who wants to touch primary plane must grab the crtc lock, so crtc
> > lock would give you an implicit read lock. At least that's been my
> > thinking, but it could be that the primary plane is used on some other
> > crtc, and then this is indeed garbage.
> This is only true if the plane is active. If there is none you can still update properties and
> swap the plane state without locking the crtc.

Ah right, so still possible to chase a being-freed primary->state pointer.

> > So maybe we need even more checks than what you propose:
> >
> >       if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) &&
> >           crtc->primary->state->crtc == crtc) {
> >       	crtc->x = crtc->primary->state->src_x >> 16;
> >       	crtc->y = crtc->primary->state->src_y >> 16;
> >       }
> >
> > I think a comment explaining this would help (or at least in the commit
> > message!).
> But the primary and cursor planes are not allowed to move between crtc's?

They are allowed to do that actually. crtc->primary and crtc->cursor is
only really a hint to implement backwards compatibility. If you have
generic plane hw with 2 crtc and planes can be freely assigned it would be
silly to artificially restrict the backwards compat planes to 1 crtc.
Otherwise we'd force 2 planes to be unusable when there's no external
screen plugged in, defeating a lot of the value of making planes freely
assignable.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/13] drm/atomic: Only update crtc->x/y if it's part of the state.

[Maarten Lankhorst]

Op 16-07-15 om 14:34 schreef Daniel Vetter:
> On Thu, Jul 16, 2015 at 11:38:18AM +0200, Maarten Lankhorst wrote:
>> Op 16-07-15 om 11:29 schreef Daniel Vetter:
>>> On Thu, Jul 16, 2015 at 11:17:29AM +0200, Maarten Lankhorst wrote:
>>>> Op 16-07-15 om 11:19 schreef Daniel Vetter:
>>>>> On Thu, Jul 16, 2015 at 10:59:14AM +0200, Maarten Lankhorst wrote:
>>>>>> Cc: dri-devel@lists.freedesktop.org
>>>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>>>> ---
>>>>>>  drivers/gpu/drm/drm_atomic_helper.c | 7 +++++--
>>>>>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> index 0898afbc9e23..70e69904291d 100644
>>>>>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
>>>>>> @@ -667,8 +667,11 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>>>>>>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
>>>>>>  		crtc->mode = crtc->state->mode;
>>>>>>  		crtc->enabled = crtc->state->enable;
>>>>>> -		crtc->x = crtc->primary->state->src_x >> 16;
>>>>>> -		crtc->y = crtc->primary->state->src_y >> 16;
>>>>>> +
>>>>>> +		if (drm_atomic_get_existing_plane_state(old_state, crtc->primary)) {
>>>>>> +			crtc->x = crtc->primary->state->src_x >> 16;
>>>>>> +			crtc->y = crtc->primary->state->src_y >> 16;
>>>>>> +		}
>>>>> What's the benefit here of only updating when something changed? The
>>>>> atomic state should be the master source so copying a few too many times
>>>>> shouldn't matter really.
>>>> Because you might not be holding plane lock, so primary->state may be garbage.
>>> Anyone who wants to touch primary plane must grab the crtc lock, so crtc
>>> lock would give you an implicit read lock. At least that's been my
>>> thinking, but it could be that the primary plane is used on some other
>>> crtc, and then this is indeed garbage.
>> This is only true if the plane is active. If there is none you can still update properties and
>> swap the plane state without locking the crtc.
> Ah right, so still possible to chase a being-freed primary->state pointer.
>
>>> So maybe we need even more checks than what you propose:
>>>
>>>       if (drm_atomic_get_existing_plane_state(old_state, crtc->primary) &&
>>>           crtc->primary->state->crtc == crtc) {
>>>       	crtc->x = crtc->primary->state->src_x >> 16;
>>>       	crtc->y = crtc->primary->state->src_y >> 16;
>>>       }
>>>
>>> I think a comment explaining this would help (or at least in the commit
>>> message!).
>> But the primary and cursor planes are not allowed to move between crtc's?
> They are allowed to do that actually. crtc->primary and crtc->cursor is
> only really a hint to implement backwards compatibility. If you have
> generic plane hw with 2 crtc and planes can be freely assigned it would be
> silly to artificially restrict the backwards compat planes to 1 crtc.
> Otherwise we'd force 2 planes to be unusable when there's no external
> screen plugged in, defeating a lot of the value of making planes freely
> assignable.
Ok, in that case your change looks reasonable. I'll respin.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v1.1 01/13] drm/atomic: Only update crtc->x/y if it's part of the state, v2.

[Maarten Lankhorst]

Universal planes may not be assigned to the current crtc, so only
update crtc->x/y when the primary is part of the state and bound
to the current crtc.

Changes since v1:
- Add the crtc check.

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 e52dfc828e60..9ede58365ae1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -665,10 +665,16 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
 
 	/* set legacy state in the crtc structure */
 	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+		struct drm_plane *primary = crtc->primary;
+
 		crtc->mode = crtc->state->mode;
 		crtc->enabled = crtc->state->enable;
-		crtc->x = crtc->primary->state->src_x >> 16;
-		crtc->y = crtc->primary->state->src_y >> 16;
+
+		if (drm_atomic_get_existing_plane_state(old_state, primary) &&
+		    primary->state->crtc == crtc) {
+			crtc->x = primary->state->src_x >> 16;
+			crtc->y = primary->state->src_y >> 16;
+		}
 
 		if (crtc->state->enable)
 			drm_calc_timestamping_constants(crtc,

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v1.1 01/13] drm/atomic: Only update crtc->x/y if it's part of the state, v2.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 03:51:01PM +0200, Maarten Lankhorst wrote:
> Universal planes may not be assigned to the current crtc, so only
> update crtc->x/y when the primary is part of the state and bound
> to the current crtc.
> 
> Changes since v1:
> - Add the crtc check.
> 
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Applied to topic/drm-misc, thanks.
-Daniel

> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index e52dfc828e60..9ede58365ae1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -665,10 +665,16 @@ drm_atomic_helper_update_legacy_modeset_state(struct drm_device *dev,
>  
>  	/* set legacy state in the crtc structure */
>  	for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> +		struct drm_plane *primary = crtc->primary;
> +
>  		crtc->mode = crtc->state->mode;
>  		crtc->enabled = crtc->state->enable;
> -		crtc->x = crtc->primary->state->src_x >> 16;
> -		crtc->y = crtc->primary->state->src_y >> 16;
> +
> +		if (drm_atomic_get_existing_plane_state(old_state, primary) &&
> +		    primary->state->crtc == crtc) {
> +			crtc->x = primary->state->src_x >> 16;
> +			crtc->y = primary->state->src_y >> 16;
> +		}
>  
>  		if (crtc->state->enable)
>  			drm_calc_timestamping_constants(crtc,
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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>
---
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;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Maarten Lankhorst]

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..
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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@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;
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v1.2 02/13] drm/atomic: Update legacy DPMS state during modesets, v3.

[Maarten Lankhorst]

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;
+		}
 	}
 }
 

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v1.2 02/13] drm/atomic: Update legacy DPMS state during modesets, v3.

[Daniel Vetter]

On Mon, Jul 27, 2015 at 01:24:29PM +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.
> 
> 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>

Yeah I think that's the one, applied to drm-misc.

Thanks, Daniel

> ---
> 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;
> +		}
>  	}
>  }
>  
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx