[PATCH 00/13] Make i915 dpms atomic.

[PATCH 00/13] Make i915 dpms atomic.

[Maarten Lankhorst]

Now that i915 is fully atomic lets get rid of the special
i915 dpms handling, and get rid of intel_crtc_control.

Maarten Lankhorst (13):
  drm/atomic: Only update crtc->x/y if it's part of the state.
  drm/atomic: Update legacy DPMS state during modesets.
  drm/i915: Make the force_thru workaround atomic.
  drm/i915: Remove special dpms handling from intel_crt.c
  drm/i915: Remove special dpms handling from intel_crtc.c
  drm/i915: Remove special dpms handling from intel_sdvo.c
  drm/i915: Get rid of dpms handling.
  drm/i915: Use proper locking for intel_dp_check_link_status.
  drm/i915: Remove some unneeded checks from check_crtc_state.
  drm/i915: Remove connectors_active from state checking.
  drm/i915: Remove connectors_active from sanitization.
  drm/i915: Remove connectors_active from intel_dp.c.
  drm/i915: Remove connectors_active.

 drivers/gpu/drm/drm_atomic_helper.c  |  23 +++-
 drivers/gpu/drm/i915/i915_debugfs.c  |  82 +++++---------
 drivers/gpu/drm/i915/intel_crt.c     |  49 +--------
 drivers/gpu/drm/i915/intel_display.c | 204 +++++------------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  38 +++++--
 drivers/gpu/drm/i915/intel_dp_mst.c  |  10 +-
 drivers/gpu/drm/i915/intel_drv.h     |   4 -
 drivers/gpu/drm/i915/intel_dsi.c     |   2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |  46 +-------
 drivers/gpu/drm/i915/intel_hdmi.c    |   2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |   2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |  47 +-------
 drivers/gpu/drm/i915/intel_tv.c      |   2 +-
 13 files changed, 109 insertions(+), 402 deletions(-)

-- 
2.1.0

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

[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

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

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

[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

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

[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

[PATCH 03/13] drm/i915: Make the force_thru workaround atomic.

[Maarten Lankhorst]

Set active_changed to force a modeset if the panel fitter's force
enabled.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  | 82 +++++++++++-------------------------
 drivers/gpu/drm/i915/intel_display.c |  3 ++
 2 files changed, 27 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index bc817da9fef7..5671a6c55e7f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -3643,74 +3643,40 @@ static int ilk_pipe_crc_ctl_reg(enum intel_pipe_crc_source *source,
 	return 0;
 }
 
-static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
+static void hsw_trans_edp_pipe_A_crc_wa(struct drm_device *dev, bool enable)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *crtc =
 		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
 	struct intel_crtc_state *pipe_config;
+	struct drm_atomic_state *state;
+	int ret = 0;
 
 	drm_modeset_lock_all(dev);
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
-	    !pipe_config->pch_pfit.enabled) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = true;
-
-		intel_display_power_get(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
-
-		if (active)
-			intel_crtc_control(&crtc->base, true);
+	state = drm_atomic_state_alloc(dev);
+	if (!state) {
+		ret = -ENOMEM;
+		goto out;
 	}
-	drm_modeset_unlock_all(dev);
-}
 
-static void hsw_undo_trans_edp_pipe_A_crc_wa(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dev_priv->pipe_to_crtc_mapping[PIPE_A]);
-	struct intel_crtc_state *pipe_config;
-
-	drm_modeset_lock_all(dev);
-	/*
-	 * If we use the eDP transcoder we need to make sure that we don't
-	 * bypass the pfit, since otherwise the pipe CRC source won't work. Only
-	 * relevant on hsw with pipe A when using the always-on power well
-	 * routing.
-	 */
-	pipe_config = to_intel_crtc_state(crtc->base.state);
-	if (pipe_config->pch_pfit.force_thru) {
-		bool active = pipe_config->base.active;
-
-		if (active) {
-			intel_crtc_control(&crtc->base, false);
-			pipe_config = to_intel_crtc_state(crtc->base.state);
-		}
-
-		pipe_config->pch_pfit.force_thru = false;
+	state->acquire_ctx = drm_modeset_legacy_acquire_ctx(&crtc->base);
+	pipe_config = intel_atomic_get_crtc_state(state, crtc);
+	if (IS_ERR(pipe_config)) {
+		ret = PTR_ERR(pipe_config);
+		goto out;
+	}
 
-		intel_display_power_put(dev_priv,
-					POWER_DOMAIN_PIPE_PANEL_FITTER(PIPE_A));
+	pipe_config->pch_pfit.force_thru = enable;
+	if (pipe_config->cpu_transcoder == TRANSCODER_EDP &&
+	    pipe_config->pch_pfit.enabled != enable)
+		pipe_config->base.active_changed = true;
 
-		if (active)
-			intel_crtc_control(&crtc->base, true);
-	}
+	ret = drm_atomic_commit(state);
+out:
 	drm_modeset_unlock_all(dev);
+	WARN(ret, "Toggling workaround to %i returns %i\n", enable, ret);
+	if (ret)
+		drm_atomic_state_free(state);
 }
 
 static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
@@ -3730,7 +3696,7 @@ static int ivb_pipe_crc_ctl_reg(struct drm_device *dev,
 		break;
 	case INTEL_PIPE_CRC_SOURCE_PF:
 		if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, true);
 
 		*val = PIPE_CRC_ENABLE | PIPE_CRC_SOURCE_PF_IVB;
 		break;
@@ -3842,7 +3808,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe,
 		else if (IS_VALLEYVIEW(dev))
 			vlv_undo_pipe_scramble_reset(dev, pipe);
 		else if (IS_HASWELL(dev) && pipe == PIPE_A)
-			hsw_undo_trans_edp_pipe_A_crc_wa(dev);
+			hsw_trans_edp_pipe_A_crc_wa(dev, false);
 
 		hsw_enable_ips(crtc);
 	}
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ede652867596..e56806f23199 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12158,6 +12158,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	struct intel_dpll_hw_state dpll_hw_state;
 	enum intel_dpll_id shared_dpll;
 	uint32_t ddi_pll_sel;
+	bool force_thru;
 
 	/* FIXME: before the switch to atomic started, a new pipe_config was
 	 * kzalloc'd. Code that depends on any field being zero should be
@@ -12169,6 +12170,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	shared_dpll = crtc_state->shared_dpll;
 	dpll_hw_state = crtc_state->dpll_hw_state;
 	ddi_pll_sel = crtc_state->ddi_pll_sel;
+	force_thru = crtc_state->pch_pfit.force_thru;
 
 	memset(crtc_state, 0, sizeof *crtc_state);
 
@@ -12177,6 +12179,7 @@ clear_intel_crtc_state(struct intel_crtc_state *crtc_state)
 	crtc_state->shared_dpll = shared_dpll;
 	crtc_state->dpll_hw_state = dpll_hw_state;
 	crtc_state->ddi_pll_sel = ddi_pll_sel;
+	crtc_state->pch_pfit.force_thru = force_thru;
 }
 
 static int
-- 
2.1.0

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

[PATCH 04/13] drm/i915: Remove special dpms handling from intel_crt.c

[Maarten Lankhorst]

There's not much point in handling anything but DPMS ON/OFF,
no need to get smart about it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c | 49 +---------------------------------------
 1 file changed, 1 insertion(+), 48 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 521af2c069cb..354e96ea2a5c 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -236,53 +236,6 @@ static void intel_enable_crt(struct intel_encoder *encoder)
 	intel_crt_set_dpms(encoder, crt->connector->base.dpms);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static void intel_crt_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_device *dev = connector->dev;
-	struct intel_encoder *encoder = intel_attached_encoder(connector);
-	struct drm_crtc *crtc;
-	int old_dpms;
-
-	/* PCH platforms and VLV only support on/off. */
-	if (INTEL_INFO(dev)->gen >= 5 && mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return;
-
-	old_dpms = connector->dpms;
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = encoder->base.crtc;
-	if (!crtc) {
-		encoder->connectors_active = false;
-		return;
-	}
-
-	/* We need the pipe to run for anything but OFF. */
-	if (mode == DRM_MODE_DPMS_OFF)
-		encoder->connectors_active = false;
-	else
-		encoder->connectors_active = true;
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode < old_dpms) {
-		/* From off to on, enable the pipe first. */
-		intel_crtc_update_dpms(crtc);
-
-		intel_crt_set_dpms(encoder, mode);
-	} else {
-		intel_crt_set_dpms(encoder, mode);
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	intel_modeset_check_state(connector->dev);
-}
-
 static enum drm_mode_status
 intel_crt_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -798,7 +751,7 @@ static void intel_crt_reset(struct drm_connector *connector)
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.reset = intel_crt_reset,
-	.dpms = intel_crt_dpms,
+	.dpms = intel_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = intel_crt_destroy,
-- 
2.1.0

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

[PATCH 05/13] drm/i915: Remove special dpms handling from intel_crtc.c

[Maarten Lankhorst]

There's not much point in handling anything but DPMS on/off,
no need to get smart about it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dvo.c | 46 +---------------------------------------
 1 file changed, 1 insertion(+), 45 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index ece5bd754f85..e854ddd0881d 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -196,50 +196,6 @@ static void intel_enable_dvo(struct intel_encoder *encoder)
 	intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static void intel_dvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct intel_dvo *intel_dvo = intel_attached_dvo(connector);
-	struct drm_crtc *crtc;
-	struct intel_crtc_state *config;
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_dvo->base.base.crtc;
-	if (!crtc) {
-		intel_dvo->base.connectors_active = false;
-		return;
-	}
-
-	/* We call connector dpms manually below in case pipe dpms doesn't
-	 * change due to cloning. */
-	if (mode == DRM_MODE_DPMS_ON) {
-		config = to_intel_crtc(crtc)->config;
-
-		intel_dvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, true);
-	} else {
-		intel_dvo->dev.dev_ops->dpms(&intel_dvo->dev, false);
-
-		intel_dvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	}
-
-	intel_modeset_check_state(connector->dev);
-}
-
 static enum drm_mode_status
 intel_dvo_mode_valid(struct drm_connector *connector,
 		     struct drm_display_mode *mode)
@@ -387,7 +343,7 @@ static void intel_dvo_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dvo_connector_funcs = {
-	.dpms = intel_dvo_dpms,
+	.dpms = intel_connector_dpms,
 	.detect = intel_dvo_detect,
 	.destroy = intel_dvo_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-- 
2.1.0

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

[PATCH 06/13] drm/i915: Remove special dpms handling from intel_sdvo.c

[Maarten Lankhorst]

There's not much point in handling anything but DPMS on/off,
no need to get smart about it.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_sdvo.c | 47 +--------------------------------------
 1 file changed, 1 insertion(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index aa2fd751609c..5a527681e861 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -1508,51 +1508,6 @@ static void intel_enable_sdvo(struct intel_encoder *encoder)
 	intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
 }
 
-/* Special dpms function to support cloning between dvo/sdvo/crt. */
-static void intel_sdvo_dpms(struct drm_connector *connector, int mode)
-{
-	struct drm_crtc *crtc;
-	struct intel_sdvo *intel_sdvo = intel_attached_sdvo(connector);
-
-	/* dvo supports only 2 dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	crtc = intel_sdvo->base.base.crtc;
-	if (!crtc) {
-		intel_sdvo->base.connectors_active = false;
-		return;
-	}
-
-	/* We set active outputs manually below in case pipe dpms doesn't change
-	 * due to cloning. */
-	if (mode != DRM_MODE_DPMS_ON) {
-		intel_sdvo_set_active_outputs(intel_sdvo, 0);
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-
-		intel_sdvo->base.connectors_active = false;
-
-		intel_crtc_update_dpms(crtc);
-	} else {
-		intel_sdvo->base.connectors_active = true;
-
-		intel_crtc_update_dpms(crtc);
-
-		if (0)
-			intel_sdvo_set_encoder_power_state(intel_sdvo, mode);
-		intel_sdvo_set_active_outputs(intel_sdvo, intel_sdvo->attached_output);
-	}
-
-	intel_modeset_check_state(connector->dev);
-}
-
 static enum drm_mode_status
 intel_sdvo_mode_valid(struct drm_connector *connector,
 		      struct drm_display_mode *mode)
@@ -2190,7 +2145,7 @@ done:
 }
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
-	.dpms = intel_sdvo_dpms,
+	.dpms = intel_connector_dpms,
 	.detect = intel_sdvo_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_sdvo_set_property,
-- 
2.1.0

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

[PATCH 07/13] drm/i915: Get rid of dpms handling.

[Maarten Lankhorst]

This is now done completely atomically.
Keep connectors_active for now, but make it mirror crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_crt.c     |  2 +-
 drivers/gpu/drm/i915/intel_display.c | 97 ------------------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c  | 10 ++--
 drivers/gpu/drm/i915/intel_drv.h     |  3 --
 drivers/gpu/drm/i915/intel_dsi.c     |  2 +-
 drivers/gpu/drm/i915/intel_dvo.c     |  2 +-
 drivers/gpu/drm/i915/intel_hdmi.c    |  2 +-
 drivers/gpu/drm/i915/intel_lvds.c    |  2 +-
 drivers/gpu/drm/i915/intel_sdvo.c    |  2 +-
 drivers/gpu/drm/i915/intel_tv.c      |  2 +-
 11 files changed, 13 insertions(+), 113 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 354e96ea2a5c..af5e43bef4a4 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -751,7 +751,7 @@ static void intel_crt_reset(struct drm_connector *connector)
 
 static const struct drm_connector_funcs intel_crt_connector_funcs = {
 	.reset = intel_crt_reset,
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_crt_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = intel_crt_destroy,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e56806f23199..2ca50e589ea4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6271,67 +6271,6 @@ free:
 	return ret;
 }
 
-/* Master function to enable/disable CRTC and corresponding power wells */
-int intel_crtc_control(struct drm_crtc *crtc, bool enable)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_mode_config *config = &dev->mode_config;
-	struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	struct intel_crtc_state *pipe_config;
-	struct drm_atomic_state *state;
-	int ret;
-
-	if (enable == intel_crtc->active)
-		return 0;
-
-	if (enable && !crtc->state->enable)
-		return 0;
-
-	/* this function should be called with drm_modeset_lock_all for now */
-	if (WARN_ON(!ctx))
-		return -EIO;
-	lockdep_assert_held(&ctx->ww_ctx);
-
-	state = drm_atomic_state_alloc(dev);
-	if (WARN_ON(!state))
-		return -ENOMEM;
-
-	state->acquire_ctx = ctx;
-	state->allow_modeset = true;
-
-	pipe_config = intel_atomic_get_crtc_state(state, intel_crtc);
-	if (IS_ERR(pipe_config)) {
-		ret = PTR_ERR(pipe_config);
-		goto err;
-	}
-	pipe_config->base.active = enable;
-
-	ret = drm_atomic_commit(state);
-	if (!ret)
-		return ret;
-
-err:
-	DRM_ERROR("Updating crtc active failed with %i\n", ret);
-	drm_atomic_state_free(state);
-	return ret;
-}
-
-/**
- * Sets the power management mode of the pipe and plane.
- */
-void intel_crtc_update_dpms(struct drm_crtc *crtc)
-{
-	struct drm_device *dev = crtc->dev;
-	struct intel_encoder *intel_encoder;
-	bool enable = false;
-
-	for_each_encoder_on_crtc(dev, crtc, intel_encoder)
-		enable |= intel_encoder->connectors_active;
-
-	intel_crtc_control(crtc, enable);
-}
-
 void intel_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct intel_encoder *intel_encoder = to_intel_encoder(encoder);
@@ -6340,22 +6279,6 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 	kfree(intel_encoder);
 }
 
-/* Simple dpms helper for encoders with just one connector, no cloning and only
- * one kind of off state. It clamps all !ON modes to fully OFF and changes the
- * state of the entire output pipe. */
-static void intel_encoder_dpms(struct intel_encoder *encoder, int mode)
-{
-	if (mode == DRM_MODE_DPMS_ON) {
-		encoder->connectors_active = true;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	} else {
-		encoder->connectors_active = false;
-
-		intel_crtc_update_dpms(encoder->base.crtc);
-	}
-}
-
 /* Cross check the actual hw state with our own modeset state tracking (and it's
  * internal consistency). */
 static void intel_connector_check_state(struct intel_connector *connector)
@@ -6427,26 +6350,6 @@ struct intel_connector *intel_connector_alloc(void)
 	return connector;
 }
 
-/* Even simpler default implementation, if there's really no special case to
- * consider. */
-void intel_connector_dpms(struct drm_connector *connector, int mode)
-{
-	/* All the simple cases only support two dpms states. */
-	if (mode != DRM_MODE_DPMS_ON)
-		mode = DRM_MODE_DPMS_OFF;
-
-	if (mode == connector->dpms)
-		return;
-
-	connector->dpms = mode;
-
-	/* Only need to change hw state when actually enabled */
-	if (connector->encoder)
-		intel_encoder_dpms(to_intel_encoder(connector->encoder), mode);
-
-	intel_modeset_check_state(connector->dev);
-}
-
 /* Simple connector->get_hw_state implementation for encoders that support only
  * one connector and no cloning and hence the encoder state determines the state
  * of the connector. */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f1b9f939b435..cea7d1785d13 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4824,7 +4824,7 @@ static void intel_dp_encoder_reset(struct drm_encoder *encoder)
 }
 
 static const struct drm_connector_funcs intel_dp_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_detect,
 	.force = intel_dp_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 6e4cc5334f47..edd8fa41e85e 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -328,7 +328,7 @@ intel_dp_mst_connector_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dp_mst_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dp_mst_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_dp_mst_set_property,
@@ -454,10 +454,10 @@ static void intel_dp_destroy_mst_connector(struct drm_dp_mst_topology_mgr *mgr,
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
 	struct drm_device *dev = connector->dev;
-	/* need to nuke the connector */
-	mutex_lock(&dev->mode_config.mutex);
-	intel_connector_dpms(connector, DRM_MODE_DPMS_OFF);
-	mutex_unlock(&dev->mode_config.mutex);
+
+	drm_modeset_lock_all(dev);
+	drm_atomic_helper_connector_dpms(connector, DRM_MODE_DPMS_OFF);
+	drm_modeset_unlock_all(dev);
 
 	intel_connector->unregister(intel_connector);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0fcfa7f179c4..318ddfb963a2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -986,12 +986,9 @@ void intel_mark_busy(struct drm_device *dev);
 void intel_mark_idle(struct drm_device *dev);
 void intel_crtc_restore_mode(struct drm_crtc *crtc);
 int intel_display_suspend(struct drm_device *dev);
-int intel_crtc_control(struct drm_crtc *crtc, bool enable);
-void intel_crtc_update_dpms(struct drm_crtc *crtc);
 void intel_encoder_destroy(struct drm_encoder *encoder);
 int intel_connector_init(struct intel_connector *);
 struct intel_connector *intel_connector_alloc(void);
-void intel_connector_dpms(struct drm_connector *, int mode);
 bool intel_connector_get_hw_state(struct intel_connector *connector);
 void intel_modeset_check_state(struct drm_device *dev);
 bool ibx_digital_port_connected(struct drm_i915_private *dev_priv,
diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c
index f4438eb5b458..c7198623dfc8 100644
--- a/drivers/gpu/drm/i915/intel_dsi.c
+++ b/drivers/gpu/drm/i915/intel_dsi.c
@@ -964,7 +964,7 @@ static const struct drm_connector_helper_funcs intel_dsi_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_dsi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dsi_detect,
 	.destroy = intel_dsi_connector_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_dvo.c b/drivers/gpu/drm/i915/intel_dvo.c
index e854ddd0881d..dc532bb61d22 100644
--- a/drivers/gpu/drm/i915/intel_dvo.c
+++ b/drivers/gpu/drm/i915/intel_dvo.c
@@ -343,7 +343,7 @@ static void intel_dvo_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_dvo_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_dvo_detect,
 	.destroy = intel_dvo_destroy,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 70bad5bf1d48..51cbea8247fe 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1909,7 +1909,7 @@ static void intel_hdmi_destroy(struct drm_connector *connector)
 }
 
 static const struct drm_connector_funcs intel_hdmi_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_hdmi_detect,
 	.force = intel_hdmi_force,
 	.fill_modes = drm_helper_probe_single_connector_modes,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index cb634f48e7d9..881b5d13592e 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -549,7 +549,7 @@ static const struct drm_connector_helper_funcs intel_lvds_connector_helper_funcs
 };
 
 static const struct drm_connector_funcs intel_lvds_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_lvds_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_lvds_set_property,
diff --git a/drivers/gpu/drm/i915/intel_sdvo.c b/drivers/gpu/drm/i915/intel_sdvo.c
index 5a527681e861..c98098e884cc 100644
--- a/drivers/gpu/drm/i915/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/intel_sdvo.c
@@ -2145,7 +2145,7 @@ done:
 }
 
 static const struct drm_connector_funcs intel_sdvo_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_sdvo_detect,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.set_property = intel_sdvo_set_property,
diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c
index 8b9d325bda3c..0568ae6ec9dd 100644
--- a/drivers/gpu/drm/i915/intel_tv.c
+++ b/drivers/gpu/drm/i915/intel_tv.c
@@ -1509,7 +1509,7 @@ out:
 }
 
 static const struct drm_connector_funcs intel_tv_connector_funcs = {
-	.dpms = intel_connector_dpms,
+	.dpms = drm_atomic_helper_connector_dpms,
 	.detect = intel_tv_detect,
 	.destroy = intel_tv_destroy,
 	.set_property = intel_tv_set_property,
-- 
2.1.0

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

[PATCH 08/13] drm/i915: Use proper locking for intel_dp_check_link_status.

[Maarten Lankhorst]

To look at encoder->crtc->state the crtc mutex is required.
Get rid of connectors_active, and only check crtc state.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index cea7d1785d13..4b16023fffd0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -4235,19 +4235,17 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
+	struct drm_crtc *crtc;
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
-	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
-
-	if (!intel_encoder->connectors_active)
-		return;
 
-	if (WARN_ON(!intel_encoder->base.crtc))
+	crtc = intel_encoder->base.crtc;
+	if (!crtc || !crtc->state->active)
 		return;
 
-	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
-		return;
+	WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex));
+	WARN_ON(!drm_modeset_is_locked(&crtc->mutex));
 
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
@@ -4906,13 +4904,31 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd)
 		}
 
 		if (!intel_dp->is_mst) {
+			struct drm_modeset_acquire_ctx ctx;
+
+			drm_modeset_acquire_init(&ctx, 0);
+
+			while (1) {
+				int ret;
+
+				ret = drm_modeset_lock(&dev->mode_config.connection_mutex, &ctx);
+				if (!ret && intel_encoder->base.crtc)
+					ret = drm_modeset_lock(&intel_encoder->base.crtc->mutex, &ctx);
+
+				if (!ret || WARN_ON(ret != -EDEADLK))
+					break;
+
+				drm_modeset_backoff(&ctx);
+			}
+
 			/*
 			 * we'll check the link status via the normal hot plug path later -
 			 * but for short hpds we should check it now
 			 */
-			drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
 			intel_dp_check_link_status(intel_dp);
-			drm_modeset_unlock(&dev->mode_config.connection_mutex);
+
+			drm_modeset_drop_locks(&ctx);
+			drm_modeset_acquire_fini(&ctx);
 		}
 	}
 
-- 
2.1.0

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

[PATCH 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Maarten Lankhorst]

This is handled by the atomic core now, no need to check this for ourself.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2ca50e589ea4..30ece88703f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12715,8 +12715,7 @@ check_crtc_state(struct drm_device *dev)
 	struct intel_crtc_state pipe_config;
 
 	for_each_intel_crtc(dev, crtc) {
-		bool enabled = false;
-		bool active = false;
+		bool active;
 
 		memset(&pipe_config, 0, sizeof(pipe_config));
 
@@ -12726,22 +12725,6 @@ check_crtc_state(struct drm_device *dev)
 		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
 		     "active crtc, but not enabled in sw tracking\n");
 
-		for_each_intel_encoder(dev, encoder) {
-			if (encoder->base.crtc != &crtc->base)
-				continue;
-			enabled = true;
-			if (encoder->connectors_active)
-				active = true;
-		}
-
-		I915_STATE_WARN(active != crtc->active,
-		     "crtc's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, crtc->active);
-		I915_STATE_WARN(enabled != crtc->base.state->enable,
-		     "crtc's computed enabled state doesn't match tracked enabled state "
-		     "(expected %i, found %i)\n", enabled,
-				crtc->base.state->enable);
-
 		active = dev_priv->display.get_pipe_config(crtc,
 							   &pipe_config);
 
-- 
2.1.0

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

Re: [PATCH 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote:
> This is handled by the atomic core now, no need to check this for ourself.
> 
> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

For all these "Remove ..." patches I think it'd be better to rewrite the
changed code to use atomic state for whatever it does directly and stop
using any of the legacy state (whether drm core or i915 legacy state). If
we do that conversion it's possible to review whether there's any cases
we're no longer checking. Trying to do that while we just rip out code
makes that harder.

hw state checker would then only compare hw state against atomic state,
and it would be the job of update_legacy_state and friends to make sure
atomic state matches up with legacy state.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 +------------------
>  1 file changed, 1 insertion(+), 18 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2ca50e589ea4..30ece88703f0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12715,8 +12715,7 @@ check_crtc_state(struct drm_device *dev)
>  	struct intel_crtc_state pipe_config;
>  
>  	for_each_intel_crtc(dev, crtc) {
> -		bool enabled = false;
> -		bool active = false;
> +		bool active;
>  
>  		memset(&pipe_config, 0, sizeof(pipe_config));
>  
> @@ -12726,22 +12725,6 @@ check_crtc_state(struct drm_device *dev)
>  		I915_STATE_WARN(crtc->active && !crtc->base.state->enable,
>  		     "active crtc, but not enabled in sw tracking\n");
>  
> -		for_each_intel_encoder(dev, encoder) {
> -			if (encoder->base.crtc != &crtc->base)
> -				continue;
> -			enabled = true;
> -			if (encoder->connectors_active)
> -				active = true;
> -		}
> -
> -		I915_STATE_WARN(active != crtc->active,
> -		     "crtc's computed active state doesn't match tracked active state "
> -		     "(expected %i, found %i)\n", active, crtc->active);
> -		I915_STATE_WARN(enabled != crtc->base.state->enable,
> -		     "crtc's computed enabled state doesn't match tracked enabled state "
> -		     "(expected %i, found %i)\n", enabled,
> -				crtc->base.state->enable);
> -
>  		active = dev_priv->display.get_pipe_config(crtc,
>  							   &pipe_config);
>  
> -- 
> 2.1.0
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Maarten Lankhorst]

Hey,

Op 16-07-15 om 11:24 schreef Daniel Vetter:
> On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote:
>> This is handled by the atomic core now, no need to check this for ourself.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> For all these "Remove ..." patches I think it'd be better to rewrite the
> changed code to use atomic state for whatever it does directly and stop
> using any of the legacy state (whether drm core or i915 legacy state). If
> we do that conversion it's possible to review whether there's any cases
> we're no longer checking. Trying to do that while we just rip out code
> makes that harder.
>
> hw state checker would then only compare hw state against atomic state,
> and it would be the job of update_legacy_state and friends to make sure
> atomic state matches up with legacy state.
>
I think converting the hw state checker to take atomic state would be a lot of work,
which should really be its own followup patch series.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Daniel Vetter]

On Thu, Jul 16, 2015 at 11:38:33AM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 16-07-15 om 11:24 schreef Daniel Vetter:
> > On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote:
> >> This is handled by the atomic core now, no need to check this for ourself.
> >>
> >> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > For all these "Remove ..." patches I think it'd be better to rewrite the
> > changed code to use atomic state for whatever it does directly and stop
> > using any of the legacy state (whether drm core or i915 legacy state). If
> > we do that conversion it's possible to review whether there's any cases
> > we're no longer checking. Trying to do that while we just rip out code
> > makes that harder.
> >
> > hw state checker would then only compare hw state against atomic state,
> > and it would be the job of update_legacy_state and friends to make sure
> > atomic state matches up with legacy state.
> >
> I think converting the hw state checker to take atomic state would be a lot of work,
> which should really be its own followup patch series.

Yeah that's kinda my point, I'd like to split this off from at least the
dpms series. Or is connectors_active causing troubles?
-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 09/13] drm/i915: Remove some unneeded checks from check_crtc_state.

[Maarten Lankhorst]

Op 16-07-15 om 17:01 schreef Daniel Vetter:
> On Thu, Jul 16, 2015 at 11:38:33AM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>> Op 16-07-15 om 11:24 schreef Daniel Vetter:
>>> On Thu, Jul 16, 2015 at 10:59:22AM +0200, Maarten Lankhorst wrote:
>>>> This is handled by the atomic core now, no need to check this for ourself.
>>>>
>>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> For all these "Remove ..." patches I think it'd be better to rewrite the
>>> changed code to use atomic state for whatever it does directly and stop
>>> using any of the legacy state (whether drm core or i915 legacy state). If
>>> we do that conversion it's possible to review whether there's any cases
>>> we're no longer checking. Trying to do that while we just rip out code
>>> makes that harder.
>>>
>>> hw state checker would then only compare hw state against atomic state,
>>> and it would be the job of update_legacy_state and friends to make sure
>>> atomic state matches up with legacy state.
>>>
>> I think converting the hw state checker to take atomic state would be a lot of work,
>> which should really be its own followup patch series.
> Yeah that's kinda my point, I'd like to split this off from at least the
> dpms series. Or is connectors_active causing troubles?
> -Daniel
No, but I thought connectors_active would be a good end goal for this series. :)
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/13] drm/i915: Remove connectors_active from state checking.

[Maarten Lankhorst]

Connectors are updated atomically now, so the only interaction
with the encoder is through base.crtc.

If it's NULL the encoder's not part of any crtc, and if it's
not NULL then active should be equal to crtc_state->active.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 24 ++++++++----------------
 1 file changed, 8 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 30ece88703f0..bd6319bba2e3 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6303,9 +6303,6 @@ static void intel_connector_check_state(struct intel_connector *connector)
 		     "active connector not linked to encoder\n");
 
 		if (encoder) {
-			I915_STATE_WARN(!encoder->connectors_active,
-			     "encoder->connectors_active not set\n");
-
 			encoder_enabled = encoder->get_hw_state(encoder, &pipe);
 			I915_STATE_WARN(!encoder_enabled, "encoder not enabled\n");
 			if (I915_STATE_WARN_ON(!encoder->base.crtc))
@@ -12655,9 +12652,6 @@ check_encoder_state(struct drm_device *dev)
 			      encoder->base.base.id,
 			      encoder->base.name);
 
-		I915_STATE_WARN(encoder->connectors_active && !encoder->base.crtc,
-		     "encoder's active_connectors set, but no crtc\n");
-
 		for_each_intel_connector(dev, connector) {
 			if (connector->base.encoder != &encoder->base)
 				continue;
@@ -12684,18 +12678,16 @@ check_encoder_state(struct drm_device *dev)
 		I915_STATE_WARN(active && !encoder->base.crtc,
 		     "active encoder with no crtc\n");
 
-		I915_STATE_WARN(encoder->connectors_active != active,
-		     "encoder's computed active state doesn't match tracked active state "
-		     "(expected %i, found %i)\n", active, encoder->connectors_active);
-
 		active = encoder->get_hw_state(encoder, &pipe);
-		I915_STATE_WARN(active != encoder->connectors_active,
-		     "encoder's hw state doesn't match sw tracking "
-		     "(expected %i, found %i)\n",
-		     encoder->connectors_active, active);
-
-		if (!encoder->base.crtc)
+		if (!encoder->base.crtc) {
+			I915_STATE_WARN(active,
+			     "encoder's active but not enabled in sw tracking");
 			continue;
+		}
+
+		I915_STATE_WARN(active != encoder->base.crtc->state->active,
+			     "encoder's active state %i doesn't match crtc state %i\n",
+			     active, encoder->base.crtc->state->active);
 
 		tracked_pipe = to_intel_crtc(encoder->base.crtc)->pipe;
 		I915_STATE_WARN(active && pipe != tracked_pipe,
-- 
2.1.0

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

[PATCH 11/13] drm/i915: Remove connectors_active from sanitization.

[Maarten Lankhorst]

connectors_active will be removed, so just calculate this right here.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bd6319bba2e3..790b46f8bf54 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14958,8 +14958,10 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 	/* Adjust the state of the output pipe according to whether we
 	 * have active connectors/encoders. */
 	enable = false;
-	for_each_encoder_on_crtc(dev, &crtc->base, encoder)
-		enable |= encoder->connectors_active;
+	for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+		enable = true;
+		break;
+	}
 
 	if (!enable)
 		intel_crtc_disable_noatomic(&crtc->base);
@@ -15015,6 +15017,7 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 {
 	struct intel_connector *connector;
 	struct drm_device *dev = encoder->base.dev;
+	bool active = false;
 
 	/* We need to check both for a crtc link (meaning that the
 	 * encoder is active and trying to read from a pipe) and the
@@ -15022,7 +15025,15 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 	bool has_active_crtc = encoder->base.crtc &&
 		to_intel_crtc(encoder->base.crtc)->active;
 
-	if (encoder->connectors_active && !has_active_crtc) {
+	for_each_intel_connector(dev, connector) {
+		if (connector->encoder != encoder)
+			continue;
+
+		active = true;
+		break;
+	}
+
+	if (active && !has_active_crtc) {
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] has active connectors but no active pipe!\n",
 			      encoder->base.base.id,
 			      encoder->base.name);
-- 
2.1.0

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

[PATCH 12/13] drm/i915: Remove connectors_active from intel_dp.c.

[Maarten Lankhorst]

Now that everything's atomic, checking encoder->base.crtc is enough.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4b16023fffd0..51aaab0c7a51 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2624,7 +2624,7 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 		DRM_DEBUG_KMS("stealing pipe %c power sequencer from port %c\n",
 			      pipe_name(pipe), port_name(port));
 
-		WARN(encoder->connectors_active,
+		WARN(encoder->base.crtc,
 		     "stealing pipe %c power sequencer from active eDP port %c\n",
 		     pipe_name(pipe), port_name(port));
 
-- 
2.1.0

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

[PATCH 13/13] drm/i915: Remove connectors_active.

[Maarten Lankhorst]

There are no more users, byebye!

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 44 +-----------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 -
 2 files changed, 1 insertion(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 790b46f8bf54..45fbbbf478ab 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12204,27 +12204,12 @@ static bool intel_crtc_in_use(struct drm_crtc *crtc)
 static void
 intel_modeset_update_state(struct drm_atomic_state *state)
 {
-	struct drm_device *dev = state->dev;
-	struct intel_encoder *intel_encoder;
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
-	struct drm_connector *connector;
 	int i;
 
 	intel_shared_dpll_commit(state);
 
-	for_each_intel_encoder(dev, intel_encoder) {
-		if (!intel_encoder->base.crtc)
-			continue;
-
-		crtc = intel_encoder->base.crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->state))
-			continue;
-
-		intel_encoder->connectors_active = false;
-	}
-
 	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 
 	/* Double check state. */
@@ -12239,28 +12224,6 @@ intel_modeset_update_state(struct drm_atomic_state *state)
 		else
 			crtc->hwmode.crtc_clock = 0;
 	}
-
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		if (!connector->encoder || !connector->encoder->crtc)
-			continue;
-
-		crtc = connector->encoder->crtc;
-		crtc_state = drm_atomic_get_existing_crtc_state(state, crtc);
-		if (!crtc_state || !needs_modeset(crtc->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;
-	}
 }
 
 static bool intel_fuzzy_clock_check(int clock1, int clock2)
@@ -14988,10 +14951,8 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 		 *  actually up, hence no need to break them. */
 		WARN_ON(crtc->active);
 
-		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
-			WARN_ON(encoder->connectors_active);
+		for_each_encoder_on_crtc(dev, &crtc->base, encoder)
 			encoder->base.crtc = NULL;
-		}
 	}
 
 	if (crtc->active || HAS_GMCH_DISPLAY(dev)) {
@@ -15050,7 +15011,6 @@ static void intel_sanitize_encoder(struct intel_encoder *encoder)
 				encoder->post_disable(encoder);
 		}
 		encoder->base.crtc = NULL;
-		encoder->connectors_active = false;
 
 		/* Inconsistent output/port/pipe state happens presumably due to
 		 * a bug in one of the get_hw_state functions. Or someplace else
@@ -15220,7 +15180,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 			encoder->base.crtc = NULL;
 		}
 
-		encoder->connectors_active = false;
 		DRM_DEBUG_KMS("[ENCODER:%d:%s] hw state readout: %s, pipe %c\n",
 			      encoder->base.base.id,
 			      encoder->base.name,
@@ -15231,7 +15190,6 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
 	for_each_intel_connector(dev, connector) {
 		if (connector->get_hw_state(connector)) {
 			connector->base.dpms = DRM_MODE_DPMS_ON;
-			connector->encoder->connectors_active = true;
 			connector->base.encoder = &connector->encoder->base;
 		} else {
 			connector->base.dpms = DRM_MODE_DPMS_OFF;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 318ddfb963a2..87f636519cc7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -133,7 +133,6 @@ struct intel_encoder {
 
 	enum intel_output_type type;
 	unsigned int cloneable;
-	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
 	bool (*compute_config)(struct intel_encoder *,
 			       struct intel_crtc_state *);
-- 
2.1.0

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

[PATCH 14/13] drm/i915: Only update mode related state if a modeset happened.

[Maarten Lankhorst]

The rest will be a noop anyway, since without modeset there will be
no updated dplls and no modeset state to update.

Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
---
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 45fbbbf478ab..fcbde8a1a4c5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12189,33 +12189,15 @@ fail:
 	return ret;
 }
 
-static bool intel_crtc_in_use(struct drm_crtc *crtc)
-{
-	struct drm_encoder *encoder;
-	struct drm_device *dev = crtc->dev;
-
-	list_for_each_entry(encoder, &dev->mode_config.encoder_list, head)
-		if (encoder->crtc == crtc)
-			return true;
-
-	return false;
-}
-
 static void
-intel_modeset_update_state(struct drm_atomic_state *state)
+intel_modeset_update_crtc_state(struct drm_atomic_state *state)
 {
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *crtc_state;
 	int i;
 
-	intel_shared_dpll_commit(state);
-
-	drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
-
 	/* Double check state. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {
-		WARN_ON(crtc->state->enable != intel_crtc_in_use(crtc));
-
 		to_intel_crtc(crtc)->config = to_intel_crtc_state(crtc->state);
 
 		/* Update hwmode for vblank functions */
@@ -13128,12 +13110,14 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	/* Only after disabling all output pipelines that will be changed can we
 	 * update the the output configuration. */
-	intel_modeset_update_state(state);
+	intel_modeset_update_crtc_state(state);
 
-	/* The state has been swaped above, so state actually contains the
-	 * old state now. */
-	if (any_ms)
+	if (any_ms) {
+		intel_shared_dpll_commit(state);
+
+		drm_atomic_helper_update_legacy_modeset_state(state->dev, state);
 		modeset_update_crtc_power_domains(state);
+	}
 
 	/* Now enable the clocks, plane, pipe, and connectors that we set up. */
 	for_each_crtc_in_state(state, crtc, crtc_state, i) {

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