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 <shiris...@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.
>
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;
>>>>
>
