On 10/30/2017 01:14 PM, Leo wrote:
>
>
> On 2017-10-30 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?
>>
>> Leo
>>
>
> Ignore what I said, It seems I was looking at the wrong context :) The
> fact this blob is being done after dc_commit_state() is annoying...
>
> The maintainability of this duplicated logic is a good point. Although
> I'm not sure if going back to a shared array using custom iterator
> logic is the best approach. Perhapse we can pull this duplicated logic
> into a seperate function?
That actually a good idea.
Thanks,
Andrey
>
> Thanks,
> 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);
>>>> }
>>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
