drm/amd/display: Don't set mode_changed=false if the stream was removed

Submitted by Kazlauskas, Nicholas on May 30, 2019, 7:51 p.m.

Details

Message ID 20190530195114.2555-1-nicholas.kazlauskas@amd.com
State New
Headers show
Series "drm/amd/display: Don't set mode_changed=false if the stream was removed" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Kazlauskas, Nicholas May 30, 2019, 7:51 p.m.
[Why]
When switching from vt to desktop with EDID emulation we can receive
an atomic commit such that we have a crtc where mode_changed = true.

During the dm_update_crtc_state disable pass we remove the stream from
the context and free it on the dm_new_crtc_state.

During the enable pass we compare the new provisional stream to the
dm_old_crtc_state->stream and determine that the stream is unchanged
and no scaling has been changed.

Following this, new_crtc_state->mode_changed is then set to false.
The connectors haven't changed and the CRTC active state hasn't changed
so drm_atomic_crtc_needs_modeset returns false, so we jump to
skip_modeset and we hit:

BUG_ON(dm_new_crtc_state->stream == NULL);

...since the old stream is gone from the context and the new stream is
also still NULL.

[How]
Ensure that we still a stream to reuse before checking if we can reuse
the old stream without a full modeset.

Cc: Roman Li <Roman.Li@amd.com>
Cc: Leo Li <sunpeng.li@amd.com>
Cc: Harry Wentland <Harry.Wentland@amd.com>
Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
---
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

Patch hide | download patch | download mbox

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 57359037ed7c..796f83ca7a4c 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -6248,7 +6248,17 @@  static int dm_update_crtc_state(struct amdgpu_display_manager *dm,
 
 		dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;
 
-		if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
+		/*
+		 * If we already removed the old stream from the context
+		 * (and set the new stream to NULL) then we can't reuse
+		 * the old stream even if the stream and scaling are unchanged.
+		 * We'll hit the BUG_ON and black screen.
+		 *
+		 * TODO: Refactor this function to allow this check to work
+		 * in all conditions.
+		 */
+		if (dm_new_crtc_state->stream &&
+		    dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&
 		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {
 			new_crtc_state->mode_changed = false;
 			DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",

Comments

On 2019-05-30 3:51 p.m., Nicholas Kazlauskas wrote:
> [Why]

> When switching from vt to desktop with EDID emulation we can receive

> an atomic commit such that we have a crtc where mode_changed = true.

> 

> During the dm_update_crtc_state disable pass we remove the stream from

> the context and free it on the dm_new_crtc_state.

> 

> During the enable pass we compare the new provisional stream to the

> dm_old_crtc_state->stream and determine that the stream is unchanged

> and no scaling has been changed.

> 

> Following this, new_crtc_state->mode_changed is then set to false.

> The connectors haven't changed and the CRTC active state hasn't changed

> so drm_atomic_crtc_needs_modeset returns false, so we jump to

> skip_modeset and we hit:

> 

> BUG_ON(dm_new_crtc_state->stream == NULL);

> 

> ...since the old stream is gone from the context and the new stream is

> also still NULL.

> 

> [How]

> Ensure that we still a stream to reuse before checking if we can reuse

> the old stream without a full modeset.

> 

> Cc: Roman Li <Roman.Li@amd.com>

> Cc: Leo Li <sunpeng.li@amd.com>

> Cc: Harry Wentland <Harry.Wentland@amd.com>

> Signed-off-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>


Reviewed-by: Harry Wentland <harry.wentland@amd.com>


Harry

> ---

>  drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 12 +++++++++++-

>  1 file changed, 11 insertions(+), 1 deletion(-)

> 

> 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 57359037ed7c..796f83ca7a4c 100644

> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c

> @@ -6248,7 +6248,17 @@ static int dm_update_crtc_state(struct amdgpu_display_manager *dm,

>  

>  		dm_new_crtc_state->abm_level = dm_new_conn_state->abm_level;

>  

> -		if (dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&

> +		/*

> +		 * If we already removed the old stream from the context

> +		 * (and set the new stream to NULL) then we can't reuse

> +		 * the old stream even if the stream and scaling are unchanged.

> +		 * We'll hit the BUG_ON and black screen.

> +		 *

> +		 * TODO: Refactor this function to allow this check to work

> +		 * in all conditions.

> +		 */

> +		if (dm_new_crtc_state->stream &&

> +		    dc_is_stream_unchanged(new_stream, dm_old_crtc_state->stream) &&

>  		    dc_is_stream_scaling_unchanged(new_stream, dm_old_crtc_state->stream)) {

>  			new_crtc_state->mode_changed = false;

>  			DRM_DEBUG_DRIVER("Mode change not required, setting mode_changed to %d",

>