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

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

[Jerry (Fangzhi) Zuo]

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@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;
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Lyude Paul]

JFYI: In the future patches like this should also be sent to 
dri-devel@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@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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [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@redhat.com> 
Sent: January 10, 2019 4:26 PM
To: Zuo, Jerry <Jerry.Zuo@amd.com>; amd-gfx@lists.freedesktop.org; Wentland, Harry <Harry.Wentland@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@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@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

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx