diff mbox series

[v3] drm/amd/display: fix flickering caused by S/G mode

Message ID 20230420134414.44538-1-hamza.mahfooz@amd.com
State New
Headers
Series [v3] drm/amd/display: fix flickering caused by S/G mode |

Commit Message

Hamza Mahfooz April 20, 2023, 1:44 p.m. UTC 
  Currently, on a handful of ASICs. We allow the framebuffer for a given
plane to exist in either VRAM or GTT. However, if the plane's new
framebuffer is in a different memory domain than it's previous
framebuffer, flipping between them can cause the screen to flicker. So,
to fix this, don't perform an immediate flip in the aforementioned case.

Cc: stable@vger.kernel.org
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
---
v2: make a number of clarifications to the commit message and drop
    locking.
v3: use a stronger check
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Li, Roman April 20, 2023, 2:51 p.m. UTC | #1 
[Public]

Reviewed-by: Roman Li <Roman.Li@amd.com>

> -----Original Message-----
> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of Hamza
> Mahfooz
> Sent: Thursday, April 20, 2023 9:44 AM
> To: amd-gfx@lists.freedesktop.org
> Cc: Wang, Chao-kai (Stylon) <Stylon.Wang@amd.com>; Tuikov, Luben
> <Luben.Tuikov@amd.com>; dri-devel@lists.freedesktop.org; Li, Sun peng (Leo)
> <Sunpeng.Li@amd.com>; David Airlie <airlied@gmail.com>; Zhuo, Qingqing
> (Lillian) <Qingqing.Zhuo@amd.com>; Pan, Xinhui <Xinhui.Pan@amd.com>;
> Siqueira, Rodrigo <Rodrigo.Siqueira@amd.com>; linux-kernel@vger.kernel.org;
> stable@vger.kernel.org; Hans de Goede <hdegoede@redhat.com>; Pillai,
> Aurabindo <Aurabindo.Pillai@amd.com>; Wu, Hersen
> <hersenxs.wu@amd.com>; Mahfooz, Hamza <Hamza.Mahfooz@amd.com>;
> Daniel Vetter <daniel@ffwll.ch>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Wentland, Harry
> <Harry.Wentland@amd.com>; Koenig, Christian <Christian.Koenig@amd.com>
> Subject: [PATCH v3] drm/amd/display: fix flickering caused by S/G mode
>
> Currently, on a handful of ASICs. We allow the framebuffer for a given plane to
> exist in either VRAM or GTT. However, if the plane's new framebuffer is in a
> different memory domain than it's previous framebuffer, flipping between
> them can cause the screen to flicker. So, to fix this, don't perform an
> immediate flip in the aforementioned case.
>
> Cc: stable@vger.kernel.org
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> ---
> v2: make a number of clarifications to the commit message and drop
>     locking.
> v3: use a stronger check
> ---
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 16 ++++++++++++++-
> -
>  1 file changed, 14 insertions(+), 2 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 dfcb9815b5a8..875111340203 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7900,6 +7900,13 @@ static void amdgpu_dm_commit_cursors(struct
> drm_atomic_state *state)
>                       amdgpu_dm_plane_handle_cursor_update(plane,
> old_plane_state);  }
>
> +static inline uint32_t get_mem_type(struct drm_framebuffer *fb) {
> +     struct amdgpu_bo *abo = gem_to_amdgpu_bo(fb->obj[0]);
> +
> +     return abo->tbo.resource ? abo->tbo.resource->mem_type : 0; }
> +
>  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>                                   struct dc_state *dc_state,
>                                   struct drm_device *dev,
> @@ -7919,6 +7926,7 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
>
>       to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
>       int planes_count = 0, vpos, hpos;
>       unsigned long flags;
> +     uint32_t mem_type;
>       u32 target_vblank, last_flip_vblank;
>       bool vrr_active = amdgpu_dm_crtc_vrr_active(acrtc_state);
>       bool cursor_update = false;
> @@ -8040,13 +8048,17 @@ static void amdgpu_dm_commit_planes(struct
> drm_atomic_state *state,
>                       }
>               }
>
> +             mem_type = get_mem_type(old_plane_state->fb);
> +
>               /*
>                * Only allow immediate flips for fast updates that don't
> -              * change FB pitch, DCC state, rotation or mirroing.
> +              * change memory domain, FB pitch, DCC state, rotation or
> +              * mirroring.
>                */
>               bundle->flip_addrs[planes_count].flip_immediate =
>                       crtc->state->async_flip &&
> -                     acrtc_state->update_type == UPDATE_TYPE_FAST;
> +                     acrtc_state->update_type == UPDATE_TYPE_FAST &&
> +                     mem_type && get_mem_type(fb) == mem_type;
>
>               timestamp_ns = ktime_get_ns();
>               bundle->flip_addrs[planes_count].flip_timestamp_in_us =
> div_u64(timestamp_ns, 1000);
> --
> 2.40.0
Christian König April 20, 2023, 3:27 p.m. UTC | #2 
Am 20.04.23 um 15:44 schrieb Hamza Mahfooz:
> Currently, on a handful of ASICs. We allow the framebuffer for a given
> plane to exist in either VRAM or GTT. However, if the plane's new
> framebuffer is in a different memory domain than it's previous
> framebuffer, flipping between them can cause the screen to flicker. So,
> to fix this, don't perform an immediate flip in the aforementioned case.
>
> Cc: stable@vger.kernel.org
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com>
> ---
> v2: make a number of clarifications to the commit message and drop
>      locking.
> v3: use a stronger check
> ---
>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c    | 16 ++++++++++++++--
>   1 file changed, 14 insertions(+), 2 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 dfcb9815b5a8..875111340203 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -7900,6 +7900,13 @@ static void amdgpu_dm_commit_cursors(struct drm_atomic_state *state)
>   			amdgpu_dm_plane_handle_cursor_update(plane, old_plane_state);
>   }
>   
> +static inline uint32_t get_mem_type(struct drm_framebuffer *fb)
> +{
> +	struct amdgpu_bo *abo = gem_to_amdgpu_bo(fb->obj[0]);
> +
> +	return abo->tbo.resource ? abo->tbo.resource->mem_type : 0;
> +}
> +
>   static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   				    struct dc_state *dc_state,
>   				    struct drm_device *dev,
> @@ -7919,6 +7926,7 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   			to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
>   	int planes_count = 0, vpos, hpos;
>   	unsigned long flags;
> +	uint32_t mem_type;
>   	u32 target_vblank, last_flip_vblank;
>   	bool vrr_active = amdgpu_dm_crtc_vrr_active(acrtc_state);
>   	bool cursor_update = false;
> @@ -8040,13 +8048,17 @@ static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
>   			}
>   		}
>   
> +		mem_type = get_mem_type(old_plane_state->fb);
> +
>   		/*
>   		 * Only allow immediate flips for fast updates that don't
> -		 * change FB pitch, DCC state, rotation or mirroing.
> +		 * change memory domain, FB pitch, DCC state, rotation or
> +		 * mirroring.
>   		 */
>   		bundle->flip_addrs[planes_count].flip_immediate =
>   			crtc->state->async_flip &&
> -			acrtc_state->update_type == UPDATE_TYPE_FAST;
> +			acrtc_state->update_type == UPDATE_TYPE_FAST &&
> +			mem_type && get_mem_type(fb) == mem_type;

I think you can actually drop checking mem_type here.

When the FB has no backing store, we actually don't have anything to 
scan out. In this case flickering is the least of your problems :)

Christian


>   
>   		timestamp_ns = ktime_get_ns();
>   		bundle->flip_addrs[planes_count].flip_timestamp_in_us = div_u64(timestamp_ns, 1000);
diff mbox series

Patch

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 dfcb9815b5a8..875111340203 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -7900,6 +7900,13 @@  static void amdgpu_dm_commit_cursors(struct drm_atomic_state *state)
 			amdgpu_dm_plane_handle_cursor_update(plane, old_plane_state);
 }
 
+static inline uint32_t get_mem_type(struct drm_framebuffer *fb)
+{
+	struct amdgpu_bo *abo = gem_to_amdgpu_bo(fb->obj[0]);
+
+	return abo->tbo.resource ? abo->tbo.resource->mem_type : 0;
+}
+
 static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 				    struct dc_state *dc_state,
 				    struct drm_device *dev,
@@ -7919,6 +7926,7 @@  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			to_dm_crtc_state(drm_atomic_get_old_crtc_state(state, pcrtc));
 	int planes_count = 0, vpos, hpos;
 	unsigned long flags;
+	uint32_t mem_type;
 	u32 target_vblank, last_flip_vblank;
 	bool vrr_active = amdgpu_dm_crtc_vrr_active(acrtc_state);
 	bool cursor_update = false;
@@ -8040,13 +8048,17 @@  static void amdgpu_dm_commit_planes(struct drm_atomic_state *state,
 			}
 		}
 
+		mem_type = get_mem_type(old_plane_state->fb);
+
 		/*
 		 * Only allow immediate flips for fast updates that don't
-		 * change FB pitch, DCC state, rotation or mirroing.
+		 * change memory domain, FB pitch, DCC state, rotation or
+		 * mirroring.
 		 */
 		bundle->flip_addrs[planes_count].flip_immediate =
 			crtc->state->async_flip &&
-			acrtc_state->update_type == UPDATE_TYPE_FAST;
+			acrtc_state->update_type == UPDATE_TYPE_FAST &&
+			mem_type && get_mem_type(fb) == mem_type;
 
 		timestamp_ns = ktime_get_ns();
 		bundle->flip_addrs[planes_count].flip_timestamp_in_us = div_u64(timestamp_ns, 1000);