On 2017-11-17 11:46 PM, Andrey Grodzovsky wrote:
>
>
> On 2017-11-16 10:32 AM, Harry Wentland wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>
>> Within atomic check, dm_update_crtcs_state is called twice. First to
>> remove from the dc_state, and subsequently to add to it.
>>
>> In both calls, a secondary mode-change check is done using dc-level
>> states. We shouldn't be doing this while removing, since a new
>> dc_stream_state has not been created to do the necessary comparison.
>> Because of this, the mode_changed flag within the DRM state can be
>> mistakenly set to false. Doing so only when adding prevents this.
>>
>> We are also guaranteed that a call to add will come after remove, or
>> else the atomic check fails (and a commit will not happen).
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>> Reviewed-by: Tony Cheng <Tony.Cheng at amd.com>
>> Acked-by: Harry Wentland <harry.wentland at amd.com>
>> ---
>> Â drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 2 +-
>> Â 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> 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 d10f8c09469b..05bc98d74a33 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4535,7 +4535,7 @@ static int dm_update_crtcs_state(struct dc *dc,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> Â Â Â Â Â Â Â Â Â }
>> -Â Â Â Â Â Â Â if (dc_is_stream_unchanged(new_stream,
>> dm_old_crtc_state->stream) &&
>> +Â Â Â Â Â Â Â if (enable && dc_is_stream_unchanged(new_stream,
>> dm_old_crtc_state->stream) &&
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dc_is_stream_scaling_unchanged(new_stream,
>> dm_old_crtc_state->stream)) {
>> Â Â Â Â Â Â Â Â Â Â Â Â Â new_crtc_state->mode_changed = false;
>
> Why just not move this logic into the if (aconnector && enable) {} close
> immediately above ? Seems this call has no meaning unless new_stream is
> initialized anyway.
>
> Thanks,
> Andrey
Good point, the intent is clearer that way as well.
I've submitted the change internally, it will pop up in the next promotion.
Thanks,
Leo
>
>
