[Intel-gfx] [PATCH v3 6/7] drm/atomic: Convert get_existing_state callers to get_old/new_state, v2.

[Maarten Lankhorst]

Op 17-01-17 om 02:27 schreef Laurent Pinchart:
> Hi Maarten,
>
> One more thing.
>
> On Monday 16 Jan 2017 10:37:43 Maarten Lankhorst wrote:
>> This is a straightforward conversion that converts all the users of
>> get_existing_state in atomic core to use get_old_state or get_new_state
>>
>> Changes since v1:
>> - Fix using the wrong state in
>> drm_atomic_helper_update_legacy_modeset_state.
>>
>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c        |  6 +++---
>>  drivers/gpu/drm/drm_atomic_helper.c | 39 ++++++++++++++--------------------
>>  drivers/gpu/drm/drm_blend.c         |  3 +--
>>  3 files changed, 22 insertions(+), 26 deletions(-)
> [snip]
>
>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
>> b/drivers/gpu/drm/drm_atomic_helper.c index b26cf786ce12..1de8d5fbc8d3
>> 100644
>> --- a/drivers/gpu/drm/drm_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> [snip]
>
>> @@ -698,8 +695,7 @@ disable_outputs(struct drm_device *dev, struct
>> drm_atomic_state *old_state) if (!old_conn_state->crtc)
>>  			continue;
>>
>> -		old_crtc_state = drm_atomic_get_existing_crtc_state(old_state,
>> -								    
> old_conn_state->crtc);
>> +		old_crtc_state = drm_atomic_get_new_crtc_state(old_state,
>> old_conn_state->crtc);
> This looks wrong. I believe you should call drm_atomic_get_old_crtc_state() 
> here. If I'm not mistaken drm_atomic_get_existing_crtc_state() did the right 
> thing as old_state->crtcs[*].state was set to the old state by the state swap 
> operation.
This is wrong, even. Fixed in next version. At least looking at the code made it obvious. :)
> On the other hand, drm_atomic_helper_update_legacy_modeset_state() uses 
> drm_atomic_get_new_plane_state() the same way you do here, even though it 
> operates after state swap, and your changelog specifically mentions that 
> you've fixed that in v2. It's getting too late to properly wrap my head around 
> this, I'll let you check which option is correct (but I reserve the right to 
> challenge it if your explanation isn't convincing enough ;-)).
update_legacy_modeset_state was looking at primary->state (new state) before,
but was using get_existing_state to check whether it's part of the commit, so it's
valid to look at plane->state.

This was fixed to get the new state, so it makes sense there.
>>  		if (!old_crtc_state->active ||
>>  		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc-
>> state))