[PATCH 5/5] drm/amd/display: Move the dm update dance to crtc->atomic_check

[Grodzovsky, Andrey]

On 12/18/2018 10:26 AM, sunpeng.li at amd.com wrote:
> From: Leo Li <sunpeng.li at amd.com>
>
> drm_atomic_helper_check_planes() calls the crtc atomic check helpers. In
> an attempt to better align with the DRM framework, we can move the
> entire dm_update dance to the crtc check helper (since it essentially
> checks that we can align DC states to what DRM is requesting)

What if DRM submits a request for plane update without any CRTC in the 
new atomic state - like some type of plane update request e.g. disable 
plane) - Can this approach handle this scenario ?

Andrey

> Signed-off-by: Leo Li <sunpeng.li at amd.com>
> ---
>   drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 145 ++++++++++++----------
>   1 file changed, 80 insertions(+), 65 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 a629544..b4dd65b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3859,15 +3859,88 @@ static bool dm_lock_and_validation_needed(struct drm_atomic_state *state)
>   }
>   
>   static int dm_crtc_helper_atomic_check(struct drm_crtc *crtc,
> -				       struct drm_crtc_state *state)
> +				       struct drm_crtc_state *new_crtc_state)
>   {
>   	struct amdgpu_device *adev = crtc->dev->dev_private;
>   	struct dc *dc = adev->dm.dc;
> -	struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(state);
> +	struct drm_atomic_state *state = new_crtc_state->state;
> +	struct drm_plane *plane;
> +	struct drm_plane_state *old_plane_state, *new_plane_state;
> +	struct drm_crtc_state *old_crtc_state = drm_atomic_get_old_crtc_state(state, crtc);
> +	struct dm_crtc_state *dm_crtc_state = to_dm_crtc_state(new_crtc_state);
>   	int ret = -EINVAL;
> +	int i;
> +
> +	/*
> +	 * Add affected connectors and planes if a connector or plane change
> +	 * affects the DC stream.
> +	 *
> +	 * The additional include of affected connectors shouldn't be required
> +	 * outside of what is already done in drm_atomic_helper_check_modeset().
> +	 * We currently do this because dm_update_crtcs_state() requires the new
> +	 * affected connector state in order to construct a new, updated stream.
> +	 * See create_stream_for_sink().
> +	 */
> +	if (new_crtc_state->enable &&
> +	    (new_crtc_state->color_mgmt_changed ||
> +	     new_crtc_state->vrr_enabled)) {
> +		ret = drm_atomic_add_affected_connectors(state, crtc);
> +		if (ret)
> +			return ret;
> +
> +		ret = drm_atomic_add_affected_planes(state, crtc);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/*
> +	 * Remove exiting planes for this CRTC, if they are modified or removed
> +	 */
> +	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> +		if (!(new_crtc_state->plane_mask & drm_plane_mask(plane)) &&
> +		    !(old_crtc_state->plane_mask & drm_plane_mask(plane)))
> +			continue;
> +
> +		ret = dm_update_planes_state(dc, state, plane,
> +					     old_plane_state,
> +					     new_plane_state,
> +					     false);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	/* Disable this crtc, if required*/
> +	ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> +				    old_crtc_state,
> +				    new_crtc_state,
> +				    false);
> +	if (ret)
> +		return ret;
> +
> +	/* Enable this crtc, if required*/
> +	ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> +				    old_crtc_state,
> +				    new_crtc_state,
> +				    true);
> +	if (ret)
> +		return ret;
> +
> +	/* Add new or add back modified planes */
> +	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> +		if (!(new_crtc_state->plane_mask & drm_plane_mask(plane)) &&
> +		    !(old_crtc_state->plane_mask & drm_plane_mask(plane)))
> +			continue;
> +
> +		ret = dm_update_planes_state(dc, state, plane,
> +					     old_plane_state,
> +					     new_plane_state,
> +					     true);
> +		if (ret)
> +			return ret;
> +	}
>   
>   	if (unlikely(!dm_crtc_state->stream &&
> -		     modeset_required(state, NULL, dm_crtc_state->stream))) {
> +		     modeset_required(new_crtc_state, NULL, dm_crtc_state->stream))) {
>   		WARN_ON(1);
>   		return ret;
>   	}
> @@ -4077,6 +4150,9 @@ static int dm_plane_atomic_check(struct drm_plane *plane,
>   	struct dc *dc = adev->dm.dc;
>   	struct dm_plane_state *dm_plane_state = to_dm_plane_state(state);
>   
> +	if (drm_atomic_plane_disabling(plane->state, state))
> +		return 0;
> +
>   	if (!dm_plane_state->dc_state)
>   		return 0;
>   
> @@ -5911,10 +5987,6 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	struct dc *dc = adev->dm.dc;
>   	struct drm_connector *connector;
>   	struct drm_connector_state *old_con_state, *new_con_state;
> -	struct drm_crtc *crtc;
> -	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
> -	struct drm_plane *plane;
> -	struct drm_plane_state *old_plane_state, *new_plane_state;
>   	enum surface_update_type update_type = UPDATE_TYPE_FAST;
>   	enum surface_update_type overall_update_type = UPDATE_TYPE_FAST;
>   
> @@ -5926,68 +5998,11 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	 */
>   	bool lock_and_validation_needed = false;
>   
> +
>   	ret = drm_atomic_helper_check_modeset(dev, state);
>   	if (ret)
>   		goto fail;
>   
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		if (!drm_atomic_crtc_needs_modeset(new_crtc_state) &&
> -		    !new_crtc_state->color_mgmt_changed &&
> -		    !new_crtc_state->vrr_enabled)
> -			continue;
> -
> -		if (!new_crtc_state->enable)
> -			continue;
> -
> -		ret = drm_atomic_add_affected_connectors(state, crtc);
> -		if (ret)
> -			return ret;
> -
> -		ret = drm_atomic_add_affected_planes(state, crtc);
> -		if (ret)
> -			goto fail;
> -	}
> -
> -	/* Remove exiting planes if they are modified */
> -	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> -		ret = dm_update_planes_state(dc, state, plane,
> -					     old_plane_state,
> -					     new_plane_state,
> -					     false);
> -		if (ret)
> -			goto fail;
> -	}
> -
> -	/* Disable all crtcs which require disable */
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> -					    old_crtc_state,
> -					    new_crtc_state,
> -					    false);
> -		if (ret)
> -			goto fail;
> -	}
> -
> -	/* Enable all crtcs which require enable */
> -	for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
> -		ret = dm_update_crtcs_state(&adev->dm, state, crtc,
> -					    old_crtc_state,
> -					    new_crtc_state,
> -					    true);
> -		if (ret)
> -			goto fail;
> -	}
> -
> -	/* Add new/modified planes */
> -	for_each_oldnew_plane_in_state_reverse(state, plane, old_plane_state, new_plane_state, i) {
> -		ret = dm_update_planes_state(dc, state, plane,
> -					     old_plane_state,
> -					     new_plane_state,
> -					     true);
> -		if (ret)
> -			goto fail;
> -	}
> -
>   	/* Run this here since we want to validate the streams we created */
>   	ret = drm_atomic_helper_check_planes(dev, state);
>   	if (ret)