[PATCH 03/29] drm/amd/display: Complete TODO item: use new DRM iterator

[Andrey Grodzovsky]

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.

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);
>   	}