[PATCH] drm/amd/display: Don't fail atomic check in MST S3 topology change

[Zuo, Jerry]

OK, I see your point. I originally found this issue that fails in dm_resume() due to the crtc check failure (no stream but mode set required). I'll remove it.

Jerry

-----Original Message-----
From: Lyude Paul <lyude at redhat.com> 
Sent: January 10, 2019 4:26 PM
To: Zuo, Jerry <Jerry.Zuo at amd.com>; amd-gfx at lists.freedesktop.org; Wentland, Harry <Harry.Wentland at amd.com>
Subject: Re: [PATCH] drm/amd/display: Don't fail atomic check in MST S3 topology change

JFYI: In the future patches like this should also be sent to dri-devel at lists.freedesktop.org

Anyway, sorry but NAK

It's usually OK to set crtc_state->mode_changed = true, but it's never okay to set it to false from atomic check code. This is because atomic helpers will sometimes force modesets in situations where they're required but might not otherwise happen (in fact-this is something we'll be doing in the MST helpers once we've implemented fallback link retraining support), so if you unset mode_changed you're breaking part of the atomic state and those helpers.

Additionally, I'm not sure what problem this is supposed to fix? We already have fixes for the suspend/resume issues on amdgpu, and I made amdgpu not fail the suspend/resume process if the atomic check fails since that's the behavior other drivers follow as well. We do eventually want to come up with a solution for atomic checks failing mid suspend/resume, but since I don't know of any cases this has ever broken userspace it's not really a priority right now.
Fixing this is a lot more difficult then it seems: we can't change the state that userspace has set, but we also can't just "turn off" atomic checks per- say for states that aren't physically possible anymore after resume due to connector changes and that sort of thing.

On Thu, 2019-01-10 at 16:17 -0500, Jerry (Fangzhi) Zuo wrote:
> In MST S3 topology change, dc_sink is created in .get_mode hook later 
> than the resume sequence. No stream created in atomic check is normal 
> in MST S3 resume. In such case, mark false to mode_changed flag to 
> skip the check in dm_crtc_helper_atomic_check(), instead of returning 
> error.
> 
> Signed-off-by: Jerry (Fangzhi) Zuo <Jerry.Zuo at amd.com>
> ---
>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 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 a9ed225d2ae0..6e03b2cf0adb 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -5628,10 +5628,9 @@ static int dm_update_crtc_state(struct 
> amdgpu_display_manager *dm,
>  		 */
>  
>  		if (!new_stream) {
> -			DRM_DEBUG_DRIVER("%s: Failed to create new stream for
> crtc %d\n",
> -					__func__, acrtc->base.base.id);
> -			ret = -ENOMEM;
> -			goto fail;
> +			new_crtc_state->mode_changed = false;
> +			DRM_DEBUG_DRIVER("Mode change not expected on %s\n",
> aconnector->base.name);
> +			goto next_crtc;
>  		}
>  
>  		dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
--
Cheers,
	Lyude Paul