On 2021-11-08 06:23, Christian König wrote:
>
>
> 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@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?
>>>
>> 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.
>
Atomic_check is also expected to fail as userspace might want to just query whether an atomic_state can be applied.
Debug messages might make sense here and would help with debug. These shouldn't be error prints, though.
Harry
> 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;
>>>>
>
