RE: [PATCH] drm/amd/display: Don't fail atomic check in MST S3 topology change
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
Sent: January 10, 2019 4:26 PM
To: Zuo, Jerry ; amd-gfx@lists.freedesktop.org; Wentland,
Harry
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-de...@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
> ---
> 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
JFYI: In the future patches like this should also be sent to
dri-de...@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
> ---
> 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
[PATCH] drm/amd/display: Don't fail atomic check in MST S3 topology change
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
---
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
