On 2017-10-26 11:06 PM, Andrey Grodzovsky wrote:
>
>
> On 2017-10-26 02:34 PM, Harry Wentland wrote:
>> From: "Leo (Sunpeng) Li" <sunpeng.li at amd.com>
>>
>> Abandon new_crtcs array and use for_each_new iterator to acquire new
>> crtcs.
>>
>> Signed-off-by: Leo (Sunpeng) Li <sunpeng.li at amd.com>
>> Reviewed-by: Harry Wentland <Harry.Wentland at amd.com>
>> ---
>> Â drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 36
>> +++++++++--------------
>> Â 1 file changed, 14 insertions(+), 22 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 442b399a9400..590f80d29b56 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -4013,10 +4013,8 @@ static void amdgpu_dm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> Â Â Â Â Â struct amdgpu_display_manager *dm = &adev->dm;
>> Â Â Â Â Â struct dm_atomic_state *dm_state;
>> Â Â Â Â Â uint32_t i, j;
>> -Â Â Â uint32_t new_crtcs_count = 0;
>> Â Â Â Â Â struct drm_crtc *crtc;
>> Â Â Â Â Â struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>> -Â Â Â struct amdgpu_crtc *new_crtcs[MAX_STREAMS];
>> Â Â Â Â Â unsigned long flags;
>> Â Â Â Â Â bool wait_for_vblank = true;
>> Â Â Â Â Â struct drm_connector *connector;
>> @@ -4075,25 +4073,9 @@ static void amdgpu_dm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â continue;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â }
>> -
>> Â Â Â Â Â Â Â Â Â Â Â Â Â if (dm_old_crtc_state->stream)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â remove_stream(adev, acrtc, dm_old_crtc_state->stream);
>> -
>> -Â Â Â Â Â Â Â Â Â Â Â /*
>> -Â Â Â Â Â Â Â Â Â Â Â Â * this loop saves set mode crtcs
>> -Â Â Â Â Â Â Â Â Â Â Â Â * we needed to enable vblanks once all
>> -Â Â Â Â Â Â Â Â Â Â Â Â * resources acquired in dc after dc_commit_streams
>> -Â Â Â Â Â Â Â Â Â Â Â Â */
>> -
>> -Â Â Â Â Â Â Â Â Â Â Â /*TODO move all this into dm_crtc_state, get rid of
>> -Â Â Â Â Â Â Â Â Â Â Â Â * new_crtcs array and use old and new atomic states
>> -Â Â Â Â Â Â Â Â Â Â Â Â * instead
>> -Â Â Â Â Â Â Â Â Â Â Â Â */
>> -Â Â Â Â Â Â Â Â Â Â Â new_crtcs[new_crtcs_count] = acrtc;
>> -Â Â Â Â Â Â Â Â Â Â Â new_crtcs_count++;
>> -
>> -Â Â Â Â Â Â Â Â Â Â Â new_crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
>> Â Â Â Â Â Â Â Â Â Â Â Â Â acrtc->enabled = true;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â acrtc->hw_mode = new_crtc_state->mode;
>> Â Â Â Â Â Â Â Â Â Â Â Â Â crtc->hwmode = new_crtc_state->mode;
>> @@ -4221,18 +4203,28 @@ static void
>> amdgpu_dm_atomic_commit_tail(struct drm_atomic_state *state)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â dm_error("%s: Failed to update stream scaling!\n",
>> __func__);
>> Â Â Â Â Â }
>> -Â Â Â for (i = 0; i < new_crtcs_count; i++) {
>> +Â Â Â for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state,
>> +Â Â Â Â Â Â Â Â Â Â Â new_crtc_state, i) {
>> Â Â Â Â Â Â Â Â Â /*
>> Â Â Â Â Â Â Â Â Â Â * loop to enable interrupts on newly arrived crtc
>> Â Â Â Â Â Â Â Â Â Â */
>> -Â Â Â Â Â Â Â struct amdgpu_crtc *acrtc = new_crtcs[i];
>> +Â Â Â Â Â Â Â struct amdgpu_crtc *acrtc = to_amdgpu_crtc(crtc);
>> +Â Â Â Â Â Â Â bool modeset_needed;
>> -Â Â Â Â Â Â Â new_crtc_state = drm_atomic_get_new_crtc_state(state,
>> &acrtc->base);
>> Â Â Â Â Â Â Â Â Â dm_new_crtc_state = to_dm_crtc_state(new_crtc_state);
>> +Â Â Â Â Â Â Â dm_old_crtc_state = to_dm_crtc_state(old_crtc_state);
>> +Â Â Â Â Â Â Â modeset_needed = modeset_required(
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â new_crtc_state,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dm_new_crtc_state->stream,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â dm_old_crtc_state->stream);
>> +
>> +Â Â Â Â Â Â Â if (dm_new_crtc_state->stream == NULL || !modeset_needed)
>> +Â Â Â Â Â Â Â Â Â Â Â continue;
>
> I feel it's a bit future bug prone to repeat the 2 checks above from the
> initial
> for_each_crtc_in_state loop, somebody makes changes there and forget
> about this loop.
> The array is ugly but it avoids logic duplication.
I think a better fix would be to combine this for_each section with the
one above it. The only thing keeping them separate (not shown in the
above context) is an `if (adev->dm.freesync_module)` guard, which can be
pulled into the first for_each loop. It would also eliminate a lot of
duplicated initialization code.
Thoughts?
Leo
>
> Thanks,
> Andrey
>
>> Â Â Â Â Â Â Â Â Â if (adev->dm.freesync_module)
>> Â Â Â Â Â Â Â Â Â Â Â Â Â mod_freesync_notify_mode_change(
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â adev->dm.freesync_module, &dm_new_crtc_state->stream,
>> 1);
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â adev->dm.freesync_module,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â &dm_new_crtc_state->stream, 1);
>> Â Â Â Â Â Â Â Â Â manage_dm_interrupts(adev, acrtc, true);
>> Â Â Â Â Â }
>
