[PATCH] drm/amd/display: log amdgpu_dm_atomic_check() failure cause
Christian König
ckoenig.leichtzumerken at gmail.com
Mon Nov 8 11:23:36 UTC 2021
Am 08.11.21 um 12:13 schrieb S, Shirish:
> Hi Paul,
>
> On 11/8/2021 2:27 PM, Paul Menzel wrote:
>> 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 at amd.com>
>>> ---
>>> .../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?
>>
> ftrace requires additional tooling, am trying to avoid it and make the
> error reporting more trivial to the developers, in case there is a
> failure in atomic_check.
Yeah, but Paul is right that here looks like totally overkill to me as well.
And especially calls to functions like drm_atomic_helper_check_modeset()
sound like parameter validation to me which the kernel should absolute
NOT report about on default severity level.
Otherwise you allow userspace to flood the logs with trivial error messages.
Regards,
Christian.
>
> Regards,
>
> Shirish S
>
>>
>> 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;
>>>
More information about the amd-gfx
mailing list