[v2,1/6] drm/amd/display: Use DRM new-style object iterators.
diff mbox

Message ID 1507842911-16975-2-git-send-email-sunpeng.li@amd.com
State New
Headers show

Commit Message

Leo Oct. 12, 2017, 9:15 p.m. UTC
From: "Leo (Sunpeng) Li" <sunpeng.li@amd.com>

Use the correct for_each_new/old_* iterators instead of for_each_*

List of affected functions:

amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
    - Old from_state_var flag was always choosing the new state

amdgpu_dm_display_resume: use for_each_new
    - drm_atomic_helper_duplicate_state is called during suspend to
      cache the state
    - It sets 'state' within the state triplet to 'new_state'

amdgpu_dm_commit_planes: use for_each_old
    - Called after the state was swapped (via atomic commit tail)

amdgpu_dm_atomic_commit: use for_each_new
    - Called before the state is swapped

amdgpu_dm_atomic_commit_tail: use for_each_old
    - Called after the state was swapped

dm_update_crtcs_state: use for_each_new
    - Called before the state is swapped (via atomic check)

amdgpu_dm_atomic_check: use for_each_new
    - Called before the state is swapped

v2: Split out typo fixes to a new patch.

Signed-off-by: Leo (Sunpeng) Li <sunpeng.li@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
 2 files changed, 12 insertions(+), 23 deletions(-)

Comments

Andrey Grodzovsky Oct. 13, 2017, 3:03 p.m. UTC | #1
On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>
> Use the correct for_each_new/old_* iterators instead of for_each_*
>
> List of affected functions:
>
> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>      - Old from_state_var flag was always choosing the new state
>
> amdgpu_dm_display_resume: use for_each_new
>      - drm_atomic_helper_duplicate_state is called during suspend to
>        cache the state
>      - It sets 'state' within the state triplet to 'new_state'

It seems to me you missed that one.

Thanks,
Andrey

>
> amdgpu_dm_commit_planes: use for_each_old
>      - Called after the state was swapped (via atomic commit tail)
>
> amdgpu_dm_atomic_commit: use for_each_new
>      - Called before the state is swapped
>
> amdgpu_dm_atomic_commit_tail: use for_each_old
>      - Called after the state was swapped
>
> dm_update_crtcs_state: use for_each_new
>      - Called before the state is swapped (via atomic check)
>
> amdgpu_dm_atomic_check: use for_each_new
>      - Called before the state is swapped
>
> v2: Split out typo fixes to a new patch.
>
> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 ++++++++---------------
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>   2 files changed, 12 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index 9bfe1f9..cc024ab 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var)
> +	struct drm_crtc *crtc)
>   {
>   	uint32_t i;
>   	struct drm_connector_state *conn_state;
>   	struct drm_connector *connector;
>   	struct drm_crtc *crtc_from_state;
>   
> -	for_each_new_connector_in_state(
> -		state,
> -		connector,
> -		conn_state,
> -		i) {
> -		crtc_from_state =
> -			from_state_var ?
> -				conn_state->crtc :
> -				connector->state->crtc;
> +	for_each_new_connector_in_state(state, connector, conn_state, i) {
> +		crtc_from_state = conn_state->crtc;
>   
>   		if (crtc_from_state == crtc)
>   			return to_amdgpu_dm_connector(connector);
> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   	unsigned long flags;
>   
>   	/* update planes when needed */
> -	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
> +	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>   		struct drm_plane_state *plane_state = plane->state;
>   		struct drm_crtc *crtc = plane_state->crtc;
>   		struct drm_framebuffer *fb = plane_state->fb;
> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	dm_state = to_dm_atomic_state(state);
>   
>   	/* update changed items */
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   		struct drm_crtc_state *new_state = crtc->state;
>   
> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>   			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
>   
>   			new_stream = new_acrtc_state->stream;
> -			aconnector =
> -				amdgpu_dm_find_first_crct_matching_connector(
> +			aconnector = amdgpu_dm_find_first_crct_matching_connector(
>   					state,
> -					&new_crtcs[i]->base,
> -					false);
> +					&new_crtcs[i]->base);
>   			if (!aconnector) {
>   				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
>   					 "skipping freesync init\n",
> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* Handle scaling and undersacn changes*/
> -	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
> +	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
>   		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
>   		struct dm_connector_state *con_new_state =
>   				to_dm_connector_state(aconnector->base.state);
> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	}
>   
>   	/* update planes when needed per crtc*/
> -	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
> +	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>   		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>   
>   		if (new_acrtc_state->stream)
> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>   	 * mark consumed event for drm_atomic_helper_commit_hw_done
>   	 */
>   	spin_lock_irqsave(&adev->ddev->event_lock, flags);
> -	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
> +	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>   		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>   
>   		if (acrtc->base.state->event)
> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>   		new_acrtc_state = to_dm_crtc_state(crtc_state);
>   		acrtc = to_amdgpu_crtc(crtc);
>   
> -		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
> +		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>   
>   		/* TODO This hack should go away */
>   		if (aconnector) {
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 630e6cd..1c55a0b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>   
>   struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
>   	struct drm_atomic_state *state,
> -	struct drm_crtc *crtc,
> -	bool from_state_var);
> +	struct drm_crtc *crtc);
>   
>   
>   struct amdgpu_framebuffer;
> -- 2.7.4 _______________________________________________ amd-gfx 
> mailing list amd-gfx@lists.freedesktop.org 
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Leo Oct. 13, 2017, 3:41 p.m. UTC | #2
On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
> 
> 
> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>
>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>
>> List of affected functions:
>>
>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>      - Old from_state_var flag was always choosing the new state
>>
>> amdgpu_dm_display_resume: use for_each_new
>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>        cache the state
>>      - It sets 'state' within the state triplet to 'new_state'
> 
> It seems to me you missed that one.
> 
> Thanks,
> Andrey
> 

Good catch, seems like that change was stripped out while I was cp-ing
from the internal tree. Some changes in this function have not been 
promoted to Dave's branch yet.

I'll remove this comment for now, I think it makes sense to have a 
follow-up patch to this after more changes have been promoted.

Thanks,
Leo

>>
>> amdgpu_dm_commit_planes: use for_each_old
>>      - Called after the state was swapped (via atomic commit tail)
>>
>> amdgpu_dm_atomic_commit: use for_each_new
>>      - Called before the state is swapped
>>
>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>      - Called after the state was swapped
>>
>> dm_update_crtcs_state: use for_each_new
>>      - Called before the state is swapped (via atomic check)
>>
>> amdgpu_dm_atomic_check: use for_each_new
>>      - Called before the state is swapped
>>
>> v2: Split out typo fixes to a new patch.
>>
>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>> ---
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>> ++++++++---------------
>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> index 9bfe1f9..cc024ab 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var)
>> +    struct drm_crtc *crtc)
>>   {
>>       uint32_t i;
>>       struct drm_connector_state *conn_state;
>>       struct drm_connector *connector;
>>       struct drm_crtc *crtc_from_state;
>> -    for_each_new_connector_in_state(
>> -        state,
>> -        connector,
>> -        conn_state,
>> -        i) {
>> -        crtc_from_state =
>> -            from_state_var ?
>> -                conn_state->crtc :
>> -                connector->state->crtc;
>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>> +        crtc_from_state = conn_state->crtc;
>>           if (crtc_from_state == crtc)
>>               return to_amdgpu_dm_connector(connector);
>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>> drm_atomic_state *state,
>>       unsigned long flags;
>>       /* update planes when needed */
>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>           struct drm_plane_state *plane_state = plane->state;
>>           struct drm_crtc *crtc = plane_state->crtc;
>>           struct drm_framebuffer *fb = plane_state->fb;
>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       dm_state = to_dm_atomic_state(state);
>>       /* update changed items */
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>           struct drm_crtc_state *new_state = crtc->state;
>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>               new_acrtc_state = 
>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>               new_stream = new_acrtc_state->stream;
>> -            aconnector =
>> -                amdgpu_dm_find_first_crct_matching_connector(
>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>                       state,
>> -                    &new_crtcs[i]->base,
>> -                    false);
>> +                    &new_crtcs[i]->base);
>>               if (!aconnector) {
>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>> connector for acrtc id:%d "
>>                        "skipping freesync init\n",
>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* Handle scaling and undersacn changes*/
>> -    for_each_new_connector_in_state(state, connector, old_conn_state, 
>> i) {
>> +    for_each_old_connector_in_state(state, connector, old_conn_state, 
>> i) {
>>           struct amdgpu_dm_connector *aconnector = 
>> to_amdgpu_dm_connector(connector);
>>           struct dm_connector_state *con_new_state =
>>                   to_dm_connector_state(aconnector->base.state);
>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>       }
>>       /* update planes when needed per crtc*/
>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>           if (new_acrtc_state->stream)
>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>        */
>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>           if (acrtc->base.state->event)
>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>           acrtc = to_amdgpu_crtc(crtc);
>> -        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>> +        aconnector = 
>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>           /* TODO This hack should go away */
>>           if (aconnector) {
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> index 630e6cd..1c55a0b 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>   struct amdgpu_dm_connector 
>> *amdgpu_dm_find_first_crct_matching_connector(
>>       struct drm_atomic_state *state,
>> -    struct drm_crtc *crtc,
>> -    bool from_state_var);
>> +    struct drm_crtc *crtc);
>>   struct amdgpu_framebuffer;
>> -- 2.7.4 _______________________________________________ amd-gfx 
>> mailing list amd-gfx@lists.freedesktop.org 
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> 
>
Andrey Grodzovsky Oct. 13, 2017, 3:56 p.m. UTC | #3
On 10/13/2017 11:41 AM, Leo wrote:
>
>
> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>
>>
>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>
>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>
>>> List of affected functions:
>>>
>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>      - Old from_state_var flag was always choosing the new state
>>>
>>> amdgpu_dm_display_resume: use for_each_new
>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>        cache the state
>>>      - It sets 'state' within the state triplet to 'new_state'
>>
>> It seems to me you missed that one.
>>
>> Thanks,
>> Andrey
>>
>
> Good catch, seems like that change was stripped out while I was cp-ing
> from the internal tree. Some changes in this function have not been 
> promoted to Dave's branch yet.
>
> I'll remove this comment for now, I think it makes sense to have a 
> follow-up patch to this after more changes have been promoted.
>
> Thanks,
> Leo

With that fixed the change is Reviewed-by: Andrey Grodzovsky 
<andrey.grodzovsky@amd.com>
>
>>>
>>> amdgpu_dm_commit_planes: use for_each_old
>>>      - Called after the state was swapped (via atomic commit tail)
>>>
>>> amdgpu_dm_atomic_commit: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>      - Called after the state was swapped
>>>
>>> dm_update_crtcs_state: use for_each_new
>>>      - Called before the state is swapped (via atomic check)
>>>
>>> amdgpu_dm_atomic_check: use for_each_new
>>>      - Called before the state is swapped
>>>
>>> v2: Split out typo fixes to a new patch.
>>>
>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>> ++++++++---------------
>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> index 9bfe1f9..cc024ab 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var)
>>> +    struct drm_crtc *crtc)
>>>   {
>>>       uint32_t i;
>>>       struct drm_connector_state *conn_state;
>>>       struct drm_connector *connector;
>>>       struct drm_crtc *crtc_from_state;
>>> -    for_each_new_connector_in_state(
>>> -        state,
>>> -        connector,
>>> -        conn_state,
>>> -        i) {
>>> -        crtc_from_state =
>>> -            from_state_var ?
>>> -                conn_state->crtc :
>>> -                connector->state->crtc;
>>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>> +        crtc_from_state = conn_state->crtc;
>>>           if (crtc_from_state == crtc)
>>>               return to_amdgpu_dm_connector(connector);
>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>> drm_atomic_state *state,
>>>       unsigned long flags;
>>>       /* update planes when needed */
>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>           struct drm_plane_state *plane_state = plane->state;
>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>           struct drm_framebuffer *fb = plane_state->fb;
>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       dm_state = to_dm_atomic_state(state);
>>>       /* update changed items */
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>           struct drm_crtc_state *new_state = crtc->state;
>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>               new_acrtc_state = 
>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>               new_stream = new_acrtc_state->stream;
>>> -            aconnector =
>>> -                amdgpu_dm_find_first_crct_matching_connector(
>>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>>                       state,
>>> -                    &new_crtcs[i]->base,
>>> -                    false);
>>> +                    &new_crtcs[i]->base);
>>>               if (!aconnector) {
>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>> connector for acrtc id:%d "
>>>                        "skipping freesync init\n",
>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* Handle scaling and undersacn changes*/
>>> -    for_each_new_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>> +    for_each_old_connector_in_state(state, connector, 
>>> old_conn_state, i) {
>>>           struct amdgpu_dm_connector *aconnector = 
>>> to_amdgpu_dm_connector(connector);
>>>           struct dm_connector_state *con_new_state =
>>> to_dm_connector_state(aconnector->base.state);
>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>       }
>>>       /* update planes when needed per crtc*/
>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>           if (new_acrtc_state->stream)
>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>        */
>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>           if (acrtc->base.state->event)
>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>           acrtc = to_amdgpu_crtc(crtc);
>>> -        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>> +        aconnector = 
>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>           /* TODO This hack should go away */
>>>           if (aconnector) {
>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> index 630e6cd..1c55a0b 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>   struct amdgpu_dm_connector 
>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>       struct drm_atomic_state *state,
>>> -    struct drm_crtc *crtc,
>>> -    bool from_state_var);
>>> +    struct drm_crtc *crtc);
>>>   struct amdgpu_framebuffer;
>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>> mailing list amd-gfx@lists.freedesktop.org 
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>>
Leo Oct. 13, 2017, 4:35 p.m. UTC | #4
On 2017-10-13 11:56 AM, Andrey Grodzovsky wrote:
> 
> 
> On 10/13/2017 11:41 AM, Leo wrote:
>>
>>
>> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>>
>>>
>>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>>
>>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>>
>>>> List of affected functions:
>>>>
>>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>>      - Old from_state_var flag was always choosing the new state
>>>>
>>>> amdgpu_dm_display_resume: use for_each_new
>>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>>        cache the state
>>>>      - It sets 'state' within the state triplet to 'new_state'
>>>
>>> It seems to me you missed that one.
>>>
>>> Thanks,
>>> Andrey
>>>
>>
>> Good catch, seems like that change was stripped out while I was cp-ing
>> from the internal tree. Some changes in this function have not been 
>> promoted to Dave's branch yet.
>>
>> I'll remove this comment for now, I think it makes sense to have a 
>> follow-up patch to this after more changes have been promoted.
>>
>> Thanks,
>> Leo
> 
> With that fixed the change is Reviewed-by: Andrey Grodzovsky 
> <andrey.grodzovsky@amd.com>

On second look, this comment is addressing the change within Dave's
patch, on which this series apply. I was trying to justify all the 
changes made, including the ones already done by Dave. See here:

https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next-amd-dc-staging&id=e7b8e99bed73e9c42f1c074ad6009cb59a79bd52

I think changing "List of affected functions" to "The following 
functions were considered" would make it less confusing, and will
still make sense if it gets squashed with Dave's patch.

Leo

>>
>>>>
>>>> amdgpu_dm_commit_planes: use for_each_old
>>>>      - Called after the state was swapped (via atomic commit tail)
>>>>
>>>> amdgpu_dm_atomic_commit: use for_each_new
>>>>      - Called before the state is swapped
>>>>
>>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>>      - Called after the state was swapped
>>>>
>>>> dm_update_crtcs_state: use for_each_new
>>>>      - Called before the state is swapped (via atomic check)
>>>>
>>>> amdgpu_dm_atomic_check: use for_each_new
>>>>      - Called before the state is swapped
>>>>
>>>> v2: Split out typo fixes to a new patch.
>>>>
>>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>>> ++++++++---------------
>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> index 9bfe1f9..cc024ab 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>>   struct amdgpu_dm_connector 
>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>       struct drm_atomic_state *state,
>>>> -    struct drm_crtc *crtc,
>>>> -    bool from_state_var)
>>>> +    struct drm_crtc *crtc)
>>>>   {
>>>>       uint32_t i;
>>>>       struct drm_connector_state *conn_state;
>>>>       struct drm_connector *connector;
>>>>       struct drm_crtc *crtc_from_state;
>>>> -    for_each_new_connector_in_state(
>>>> -        state,
>>>> -        connector,
>>>> -        conn_state,
>>>> -        i) {
>>>> -        crtc_from_state =
>>>> -            from_state_var ?
>>>> -                conn_state->crtc :
>>>> -                connector->state->crtc;
>>>> +    for_each_new_connector_in_state(state, connector, conn_state, i) {
>>>> +        crtc_from_state = conn_state->crtc;
>>>>           if (crtc_from_state == crtc)
>>>>               return to_amdgpu_dm_connector(connector);
>>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>>> drm_atomic_state *state,
>>>>       unsigned long flags;
>>>>       /* update planes when needed */
>>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>>           struct drm_plane_state *plane_state = plane->state;
>>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>>           struct drm_framebuffer *fb = plane_state->fb;
>>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       dm_state = to_dm_atomic_state(state);
>>>>       /* update changed items */
>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>           struct drm_crtc_state *new_state = crtc->state;
>>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>>               new_acrtc_state = 
>>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>>               new_stream = new_acrtc_state->stream;
>>>> -            aconnector =
>>>> -                amdgpu_dm_find_first_crct_matching_connector(
>>>> +            aconnector = amdgpu_dm_find_first_crct_matching_connector(
>>>>                       state,
>>>> -                    &new_crtcs[i]->base,
>>>> -                    false);
>>>> +                    &new_crtcs[i]->base);
>>>>               if (!aconnector) {
>>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>>> connector for acrtc id:%d "
>>>>                        "skipping freesync init\n",
>>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       }
>>>>       /* Handle scaling and undersacn changes*/
>>>> -    for_each_new_connector_in_state(state, connector, 
>>>> old_conn_state, i) {
>>>> +    for_each_old_connector_in_state(state, connector, 
>>>> old_conn_state, i) {
>>>>           struct amdgpu_dm_connector *aconnector = 
>>>> to_amdgpu_dm_connector(connector);
>>>>           struct dm_connector_state *con_new_state =
>>>> to_dm_connector_state(aconnector->base.state);
>>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>       }
>>>>       /* update planes when needed per crtc*/
>>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>>           if (new_acrtc_state->stream)
>>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>>        */
>>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>           if (acrtc->base.state->event)
>>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>>           acrtc = to_amdgpu_crtc(crtc);
>>>> -        aconnector = 
>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>>> +        aconnector = 
>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>>           /* TODO This hack should go away */
>>>>           if (aconnector) {
>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> index 630e6cd..1c55a0b 100644
>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>>   struct amdgpu_dm_connector 
>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>       struct drm_atomic_state *state,
>>>> -    struct drm_crtc *crtc,
>>>> -    bool from_state_var);
>>>> +    struct drm_crtc *crtc);
>>>>   struct amdgpu_framebuffer;
>>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>>> mailing list amd-gfx@lists.freedesktop.org 
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>>
>
Andrey Grodzovsky Oct. 13, 2017, 5:28 p.m. UTC | #5
On 10/13/2017 12:35 PM, Leo wrote:
>
>
> On 2017-10-13 11:56 AM, Andrey Grodzovsky wrote:
>>
>>
>> On 10/13/2017 11:41 AM, Leo wrote:
>>>
>>>
>>> On 2017-10-13 11:03 AM, Andrey Grodzovsky wrote:
>>>>
>>>>
>>>> On 10/12/2017 05:15 PM, sunpeng.li@amd.com wrote:
>>>>> From: "Leo (Sunpeng) Li"<sunpeng.li@amd.com>
>>>>>
>>>>> Use the correct for_each_new/old_* iterators instead of for_each_*
>>>>>
>>>>> List of affected functions:
>>>>>
>>>>> amdgpu_dm_find_first_crtc_matching_connector: use for_each_new
>>>>>      - Old from_state_var flag was always choosing the new state
>>>>>
>>>>> amdgpu_dm_display_resume: use for_each_new
>>>>>      - drm_atomic_helper_duplicate_state is called during suspend to
>>>>>        cache the state
>>>>>      - It sets 'state' within the state triplet to 'new_state'
>>>>
>>>> It seems to me you missed that one.
>>>>
>>>> Thanks,
>>>> Andrey
>>>>
>>>
>>> Good catch, seems like that change was stripped out while I was cp-ing
>>> from the internal tree. Some changes in this function have not been 
>>> promoted to Dave's branch yet.
>>>
>>> I'll remove this comment for now, I think it makes sense to have a 
>>> follow-up patch to this after more changes have been promoted.
>>>
>>> Thanks,
>>> Leo
>>
>> With that fixed the change is Reviewed-by: Andrey Grodzovsky 
>> <andrey.grodzovsky@amd.com>
>
> On second look, this comment is addressing the change within Dave's
> patch, on which this series apply. I was trying to justify all the 
> changes made, including the ones already done by Dave. See here:
>
> https://cgit.freedesktop.org/~airlied/linux/commit/?h=drm-next-amd-dc-staging&id=e7b8e99bed73e9c42f1c074ad6009cb59a79bd52 
>
>
> I think changing "List of affected functions" to "The following 
> functions were considered" would make it less confusing, and will
> still make sense if it gets squashed with Dave's patch.
>
> Leo

Makes sense to me to.

Thanks,
Andrey

>
>>>
>>>>>
>>>>> amdgpu_dm_commit_planes: use for_each_old
>>>>>      - Called after the state was swapped (via atomic commit tail)
>>>>>
>>>>> amdgpu_dm_atomic_commit: use for_each_new
>>>>>      - Called before the state is swapped
>>>>>
>>>>> amdgpu_dm_atomic_commit_tail: use for_each_old
>>>>>      - Called after the state was swapped
>>>>>
>>>>> dm_update_crtcs_state: use for_each_new
>>>>>      - Called before the state is swapped (via atomic check)
>>>>>
>>>>> amdgpu_dm_atomic_check: use for_each_new
>>>>>      - Called before the state is swapped
>>>>>
>>>>> v2: Split out typo fixes to a new patch.
>>>>>
>>>>> Signed-off-by: Leo (Sunpeng) Li<sunpeng.li@amd.com>
>>>>> ---
>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 32 
>>>>> ++++++++---------------
>>>>>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +--
>>>>>   2 files changed, 12 insertions(+), 23 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c 
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> index 9bfe1f9..cc024ab 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>>>> @@ -570,23 +570,15 @@ static int dm_suspend(void *handle)
>>>>>   struct amdgpu_dm_connector 
>>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>>       struct drm_atomic_state *state,
>>>>> -    struct drm_crtc *crtc,
>>>>> -    bool from_state_var)
>>>>> +    struct drm_crtc *crtc)
>>>>>   {
>>>>>       uint32_t i;
>>>>>       struct drm_connector_state *conn_state;
>>>>>       struct drm_connector *connector;
>>>>>       struct drm_crtc *crtc_from_state;
>>>>> -    for_each_new_connector_in_state(
>>>>> -        state,
>>>>> -        connector,
>>>>> -        conn_state,
>>>>> -        i) {
>>>>> -        crtc_from_state =
>>>>> -            from_state_var ?
>>>>> -                conn_state->crtc :
>>>>> -                connector->state->crtc;
>>>>> +    for_each_new_connector_in_state(state, connector, conn_state, 
>>>>> i) {
>>>>> +        crtc_from_state = conn_state->crtc;
>>>>>           if (crtc_from_state == crtc)
>>>>>               return to_amdgpu_dm_connector(connector);
>>>>> @@ -3890,7 +3882,7 @@ static void amdgpu_dm_commit_planes(struct 
>>>>> drm_atomic_state *state,
>>>>>       unsigned long flags;
>>>>>       /* update planes when needed */
>>>>> -    for_each_new_plane_in_state(state, plane, old_plane_state, i) {
>>>>> +    for_each_old_plane_in_state(state, plane, old_plane_state, i) {
>>>>>           struct drm_plane_state *plane_state = plane->state;
>>>>>           struct drm_crtc *crtc = plane_state->crtc;
>>>>>           struct drm_framebuffer *fb = plane_state->fb;
>>>>> @@ -4024,7 +4016,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       dm_state = to_dm_atomic_state(state);
>>>>>       /* update changed items */
>>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>           struct drm_crtc_state *new_state = crtc->state;
>>>>> @@ -4113,11 +4105,9 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>               new_acrtc_state = 
>>>>> to_dm_crtc_state(new_crtcs[i]->base.state);
>>>>>               new_stream = new_acrtc_state->stream;
>>>>> -            aconnector =
>>>>> - amdgpu_dm_find_first_crct_matching_connector(
>>>>> +            aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(
>>>>>                       state,
>>>>> -                    &new_crtcs[i]->base,
>>>>> -                    false);
>>>>> +                    &new_crtcs[i]->base);
>>>>>               if (!aconnector) {
>>>>>                   DRM_DEBUG_DRIVER("Atomic commit: Failed to find 
>>>>> connector for acrtc id:%d "
>>>>>                        "skipping freesync init\n",
>>>>> @@ -4151,7 +4141,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       }
>>>>>       /* Handle scaling and undersacn changes*/
>>>>> -    for_each_new_connector_in_state(state, connector, 
>>>>> old_conn_state, i) {
>>>>> +    for_each_old_connector_in_state(state, connector, 
>>>>> old_conn_state, i) {
>>>>>           struct amdgpu_dm_connector *aconnector = 
>>>>> to_amdgpu_dm_connector(connector);
>>>>>           struct dm_connector_state *con_new_state =
>>>>> to_dm_connector_state(aconnector->base.state);
>>>>> @@ -4205,7 +4195,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>       }
>>>>>       /* update planes when needed per crtc*/
>>>>> -    for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>> +    for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
>>>>>           new_acrtc_state = to_dm_crtc_state(pcrtc->state);
>>>>>           if (new_acrtc_state->stream)
>>>>> @@ -4218,7 +4208,7 @@ void amdgpu_dm_atomic_commit_tail(
>>>>>        * mark consumed event for drm_atomic_helper_commit_hw_done
>>>>>        */
>>>>>       spin_lock_irqsave(&adev->ddev->event_lock, flags);
>>>>> -    for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>> +    for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
>>>>>           struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>>>>>           if (acrtc->base.state->event)
>>>>> @@ -4402,7 +4392,7 @@ static int dm_update_crtcs_state(
>>>>>           new_acrtc_state = to_dm_crtc_state(crtc_state);
>>>>>           acrtc = to_amdgpu_crtc(crtc);
>>>>> -        aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
>>>>> +        aconnector = 
>>>>> amdgpu_dm_find_first_crct_matching_connector(state, crtc);
>>>>>           /* TODO This hack should go away */
>>>>>           if (aconnector) {
>>>>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h 
>>>>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> index 630e6cd..1c55a0b 100644
>>>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>>>>> @@ -228,8 +228,7 @@ void amdgpu_dm_update_connector_after_detect(
>>>>>   struct amdgpu_dm_connector 
>>>>> *amdgpu_dm_find_first_crct_matching_connector(
>>>>>       struct drm_atomic_state *state,
>>>>> -    struct drm_crtc *crtc,
>>>>> -    bool from_state_var);
>>>>> +    struct drm_crtc *crtc);
>>>>>   struct amdgpu_framebuffer;
>>>>> -- 2.7.4 _______________________________________________ amd-gfx 
>>>>> mailing list amd-gfx@lists.freedesktop.org 
>>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>>
>>>>
>>

Patch
diff mbox

diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
index 9bfe1f9..cc024ab 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -570,23 +570,15 @@  static int dm_suspend(void *handle)
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var)
+	struct drm_crtc *crtc)
 {
 	uint32_t i;
 	struct drm_connector_state *conn_state;
 	struct drm_connector *connector;
 	struct drm_crtc *crtc_from_state;
 
-	for_each_new_connector_in_state(
-		state,
-		connector,
-		conn_state,
-		i) {
-		crtc_from_state =
-			from_state_var ?
-				conn_state->crtc :
-				connector->state->crtc;
+	for_each_new_connector_in_state(state, connector, conn_state, i) {
+		crtc_from_state = conn_state->crtc;
 
 		if (crtc_from_state == crtc)
 			return to_amdgpu_dm_connector(connector);
@@ -3890,7 +3882,7 @@  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 	unsigned long flags;
 
 	/* update planes when needed */
-	for_each_new_plane_in_state(state, plane, old_plane_state, i) {
+	for_each_old_plane_in_state(state, plane, old_plane_state, i) {
 		struct drm_plane_state *plane_state = plane->state;
 		struct drm_crtc *crtc = plane_state->crtc;
 		struct drm_framebuffer *fb = plane_state->fb;
@@ -4024,7 +4016,7 @@  void amdgpu_dm_atomic_commit_tail(
 	dm_state = to_dm_atomic_state(state);
 
 	/* update changed items */
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 		struct drm_crtc_state *new_state = crtc->state;
 
@@ -4113,11 +4105,9 @@  void amdgpu_dm_atomic_commit_tail(
 			new_acrtc_state = to_dm_crtc_state(new_crtcs[i]->base.state);
 
 			new_stream = new_acrtc_state->stream;
-			aconnector =
-				amdgpu_dm_find_first_crct_matching_connector(
+			aconnector = amdgpu_dm_find_first_crct_matching_connector(
 					state,
-					&new_crtcs[i]->base,
-					false);
+					&new_crtcs[i]->base);
 			if (!aconnector) {
 				DRM_DEBUG_DRIVER("Atomic commit: Failed to find connector for acrtc id:%d "
 					 "skipping freesync init\n",
@@ -4151,7 +4141,7 @@  void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* Handle scaling and undersacn changes*/
-	for_each_new_connector_in_state(state, connector, old_conn_state, i) {
+	for_each_old_connector_in_state(state, connector, old_conn_state, i) {
 		struct amdgpu_dm_connector *aconnector = to_amdgpu_dm_connector(connector);
 		struct dm_connector_state *con_new_state =
 				to_dm_connector_state(aconnector->base.state);
@@ -4205,7 +4195,7 @@  void amdgpu_dm_atomic_commit_tail(
 	}
 
 	/* update planes when needed per crtc*/
-	for_each_new_crtc_in_state(state, pcrtc, old_crtc_state, j) {
+	for_each_old_crtc_in_state(state, pcrtc, old_crtc_state, j) {
 		new_acrtc_state = to_dm_crtc_state(pcrtc->state);
 
 		if (new_acrtc_state->stream)
@@ -4218,7 +4208,7 @@  void amdgpu_dm_atomic_commit_tail(
 	 * mark consumed event for drm_atomic_helper_commit_hw_done
 	 */
 	spin_lock_irqsave(&adev->ddev->event_lock, flags);
-	for_each_new_crtc_in_state(state, crtc, old_crtc_state, i) {
+	for_each_old_crtc_in_state(state, crtc, old_crtc_state, i) {
 		struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
 
 		if (acrtc->base.state->event)
@@ -4402,7 +4392,7 @@  static int dm_update_crtcs_state(
 		new_acrtc_state = to_dm_crtc_state(crtc_state);
 		acrtc = to_amdgpu_crtc(crtc);
 
-		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc, true);
+		aconnector = amdgpu_dm_find_first_crct_matching_connector(state, crtc);
 
 		/* TODO This hack should go away */
 		if (aconnector) {
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 630e6cd..1c55a0b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -228,8 +228,7 @@  void amdgpu_dm_update_connector_after_detect(
 
 struct amdgpu_dm_connector *amdgpu_dm_find_first_crct_matching_connector(
 	struct drm_atomic_state *state,
-	struct drm_crtc *crtc,
-	bool from_state_var);
+	struct drm_crtc *crtc);
 
 
 struct amdgpu_framebuffer;