Re: [PATCH] drm/amd/display: log amdgpu_dm_atomic_check() failure cause

[Paul Menzel <pmenzel@xxxxxxxxxxxxx>]

Dear Shrish,


Am 08.11.21 um 09:40 schrieb Shirish S:
> update user with next level of info about which condition led to
> atomic check failure.
>
> Signed-off-by: Shirish S <shirish.s@xxxxxxx>
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 70 ++++++++++++++-----
>   1 file changed, 52 insertions(+), 18 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 1e26d9be8993..37ea8a76fa09 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -10746,8 +10746,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	trace_amdgpu_dm_atomic_check_begin(state);
>   
>   	ret = drm_atomic_helper_check_modeset(dev, state);
> -	if (ret)
> +	if (ret) {
> +		DRM_DEV_ERROR(adev->dev, "drm_atomic_helper_check_modeset() failed\n");

Does the Linux kernel provide means (for example ftrace) to trace such 
things, so the (generic) debug lines don’t have to be added? Or is it to 
better debug user bug reports?


Kind regards,

Paul


>   		goto fail;
> +	}
>   
>   	/* Check connector changes */
>   	for_each_oldnew_connector_in_state(state, connector, old_con_state, new_con_state, i) {
> @@ -10763,6 +10765,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   
>   		new_crtc_state = drm_atomic_get_crtc_state(state, new_con_state->crtc);
>   		if (IS_ERR(new_crtc_state)) {
> +			DRM_DEV_ERROR(adev->dev, "drm_atomic_get_crtc_state() failed\n");
>   			ret = PTR_ERR(new_crtc_state);
>   			goto fail;
>   		}
> @@ -10777,8 +10780,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   		for_each_oldnew_crtc_in_state(state, crtc, old_crtc_state, new_crtc_state, i) {
>   			if (drm_atomic_crtc_needs_modeset(new_crtc_state)) {
>   				ret = add_affected_mst_dsc_crtcs(state, crtc);
> -				if (ret)
> +				if (ret) {
> +					DRM_DEV_ERROR(adev->dev, "add_affected_mst_dsc_crtcs() failed\n");
>   					goto fail;
> +				}
>   			}
>   		}
>   	}
> @@ -10793,19 +10798,25 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   			continue;
>   
>   		ret = amdgpu_dm_verify_lut_sizes(new_crtc_state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "amdgpu_dm_verify_lut_sizes() failed\n");
>   			goto fail;
> +		}
>   
>   		if (!new_crtc_state->enable)
>   			continue;
>   
>   		ret = drm_atomic_add_affected_connectors(state, crtc);
> -		if (ret)
> -			return ret;
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "drm_atomic_add_affected_connectors() failed\n");
> +			goto fail;
> +		}
>   
>   		ret = drm_atomic_add_affected_planes(state, crtc);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "drm_atomic_add_affected_planes() failed\n");
>   			goto fail;
> +		}
>   
>   		if (dm_old_crtc_state->dsc_force_changed)
>   			new_crtc_state->mode_changed = true;
> @@ -10842,6 +10853,7 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   
>   			if (IS_ERR(new_plane_state)) {
>   				ret = PTR_ERR(new_plane_state);
> +				DRM_DEV_ERROR(adev->dev, "new_plane_state is BAD\n");
>   				goto fail;
>   			}
>   		}
> @@ -10854,8 +10866,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   					    new_plane_state,
>   					    false,
>   					    &lock_and_validation_needed);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "dm_update_plane_state() failed\n");
>   			goto fail;
> +		}
>   	}
>   
>   	/* Disable all crtcs which require disable */
> @@ -10865,8 +10879,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   					   new_crtc_state,
>   					   false,
>   					   &lock_and_validation_needed);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "DISABLE: dm_update_crtc_state() failed\n");
>   			goto fail;
> +		}
>   	}
>   
>   	/* Enable all crtcs which require enable */
> @@ -10876,8 +10892,10 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   					   new_crtc_state,
>   					   true,
>   					   &lock_and_validation_needed);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "ENABLE: dm_update_crtc_state() failed\n");
>   			goto fail;
> +		}
>   	}
>   
>   	/* Add new/modified planes */
> @@ -10887,20 +10905,26 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   					    new_plane_state,
>   					    true,
>   					    &lock_and_validation_needed);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "dm_update_plane_state() failed\n");
>   			goto fail;
> +		}
>   	}
>   
>   	/* Run this here since we want to validate the streams we created */
>   	ret = drm_atomic_helper_check_planes(dev, state);
> -	if (ret)
> +	if (ret) {
> +		DRM_DEV_ERROR(adev->dev, "drm_atomic_helper_check_planes() failed\n");
>   		goto fail;
> +	}
>   
>   	/* Check cursor planes scaling */
>   	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
>   		ret = dm_check_crtc_cursor(state, crtc, new_crtc_state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "dm_check_crtc_cursor() failed\n");
>   			goto fail;
> +		}
>   	}
>   
>   	if (state->legacy_cursor_update) {
> @@ -10987,20 +11011,28 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   	 */
>   	if (lock_and_validation_needed) {
>   		ret = dm_atomic_get_state(state, &dm_state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "dm_atomic_get_state() failed\n");
>   			goto fail;
> +		}
>   
>   		ret = do_aquire_global_lock(dev, state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "do_aquire_global_lock() failed\n");
>   			goto fail;
> +		}
>   
>   #if defined(CONFIG_DRM_AMD_DC_DCN)
> -		if (!compute_mst_dsc_configs_for_state(state, dm_state->context, vars))
> +		if (!compute_mst_dsc_configs_for_state(state, dm_state->context, vars)) {
> +			DRM_DEV_ERROR(adev->dev, "compute_mst_dsc_configs_for_state() failed\n");
>   			goto fail;
> +		}
>   
>   		ret = dm_update_mst_vcpi_slots_for_dsc(state, dm_state->context, vars);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "dm_update_mst_vcpi_slots_for_dsc() failed\n");
>   			goto fail;
> +		}
>   #endif
>   
>   		/*
> @@ -11010,11 +11042,13 @@ static int amdgpu_dm_atomic_check(struct drm_device *dev,
>   		 * to get stuck in an infinite loop and hang eventually.
>   		 */
>   		ret = drm_dp_mst_atomic_check(state);
> -		if (ret)
> +		if (ret) {
> +			DRM_DEV_ERROR(adev->dev, "drm_dp_mst_atomic_check() failed\n");
>   			goto fail;
> +		}
>   		status = dc_validate_global_state(dc, dm_state->context, false);
>   		if (status != DC_OK) {
> -			drm_dbg_atomic(dev,
> +			DRM_DEV_ERROR(adev->dev,
>   				       "DC global validation failure: %s (%d)",
>   				       dc_status_to_str(status), status);
>   			ret = -EINVAL;