On 10/30/2017 12:36 PM, Leo wrote:
>
>
> 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?
It might be a bit more difficult since inside the clause is flip and
vblank interrupts enabling,
but they rely heavily on acrtc->otg_inst which is being set in between.
Thanks,
Andrey
>
> 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);
>>> }
>>
