Messages in this thread | | | Date | Mon, 17 Apr 2023 17:03:34 +0200 | Subject | Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode | From | Christian König <> |
| |
Am 17.04.23 um 16:51 schrieb Hamza Mahfooz: > > On 4/17/23 01:59, Christian König wrote: >> Am 14.04.23 um 21:33 schrieb Hamza Mahfooz: >>> Currently, we allow the framebuffer for a given plane to move between >>> memory domains, however when that happens it causes the screen to >>> flicker, it is even possible for the framebuffer to change memory >>> domains on every plane update (causing a continuous flicker effect). >>> So, >>> to fix this, don't perform an immediate flip in the aforementioned >>> case. >> >> That sounds strongly like you just forget to wait for the move to >> finish! >> >> What is the order of things done here? E.g. who calls amdgpu_bo_pin() >> and who waits for fences for finish signaling? Is that maybe just in >> the wrong order? > > The pinning logic is in dm_plane_helper_prepare_fb(). Also, it seems > like we wait for the fences in amdgpu_dm_atomic_commit_tail(), using > drm_atomic_helper_wait_for_fences(). The ordering should be fine as > well, since prepare_fb() is always called before atomic_commit_tail().
Ok, then why is there any flickering?
BTW reserving a fence slot is completely unnecessary. That looks like you copy&pasted the code from somewhere else without checking what it actually does.
Regards, Christian.
> >> >> Regards, >> Christian. >> >>> >>> Cc: stable@vger.kernel.org >>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible >>> (v2)") >>> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@amd.com> >>> --- >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 41 >>> ++++++++++++++++++- >>> 1 file changed, 39 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 da3045fdcb6d..9a4e7408384a 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> @@ -7897,6 +7897,34 @@ 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 amdgpu_device *adev, >>> + struct drm_gem_object *obj, >>> + bool check_domain) >>> +{ >>> + struct amdgpu_bo *abo = gem_to_amdgpu_bo(obj); >>> + uint32_t mem_type; >>> + >>> + if (unlikely(amdgpu_bo_reserve(abo, true))) >>> + return 0; >>> + >>> + if (unlikely(dma_resv_reserve_fences(abo->tbo.base.resv, 1))) >>> + goto err; >>> + >>> + if (check_domain && >>> + amdgpu_display_supported_domains(adev, abo->flags) != >>> + (AMDGPU_GEM_DOMAIN_VRAM | AMDGPU_GEM_DOMAIN_GTT)) >>> + goto err; >>> + >>> + mem_type = abo->tbo.resource->mem_type; >>> + amdgpu_bo_unreserve(abo); >>> + >>> + return mem_type; >>> + >>> +err: >>> + amdgpu_bo_unreserve(abo); >>> + return 0; >>> +} >>> + >>> static void amdgpu_dm_commit_planes(struct drm_atomic_state *state, >>> struct dc_state *dc_state, >>> struct drm_device *dev, >>> @@ -7916,6 +7944,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; >>> @@ -8035,13 +8064,21 @@ static void amdgpu_dm_commit_planes(struct >>> drm_atomic_state *state, >>> } >>> } >>> + mem_type = get_mem_type(dm->adev, old_plane_state->fb->obj[0], >>> + true); >>> + >>> /* >>> * 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 || (mem_type && get_mem_type(dm->adev, >>> + fb->obj[0], >>> + false) == >>> + mem_type)); >>> timestamp_ns = ktime_get_ns(); >>> bundle->flip_addrs[planes_count].flip_timestamp_in_us = >>> div_u64(timestamp_ns, 1000); >>
| |