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: [email protected]
Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
Signed-off-by: Hamza Mahfooz <[email protected]>
---
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
[Public]
Reviewed-by: Roman Li <[email protected]>
> -----Original Message-----
> From: amd-gfx <[email protected]> On Behalf Of Hamza
> Mahfooz
> Sent: Thursday, April 20, 2023 9:44 AM
> To: [email protected]
> Cc: Wang, Chao-kai (Stylon) <[email protected]>; Tuikov, Luben
> <[email protected]>; [email protected]; Li, Sun peng (Leo)
> <[email protected]>; David Airlie <[email protected]>; Zhuo, Qingqing
> (Lillian) <[email protected]>; Pan, Xinhui <[email protected]>;
> Siqueira, Rodrigo <[email protected]>; [email protected];
> [email protected]; Hans de Goede <[email protected]>; Pillai,
> Aurabindo <[email protected]>; Wu, Hersen
> <[email protected]>; Mahfooz, Hamza <[email protected]>;
> Daniel Vetter <[email protected]>; Deucher, Alexander
> <[email protected]>; Wentland, Harry
> <[email protected]>; Koenig, Christian <[email protected]>
> 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: [email protected]
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> Signed-off-by: Hamza Mahfooz <[email protected]>
> ---
> 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
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: [email protected]
> Link: https://gitlab.freedesktop.org/drm/amd/-/issues/2354
> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> Signed-off-by: Hamza Mahfooz <[email protected]>
> ---
> 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);