Messages in this thread | | | Date | Mon, 17 Apr 2023 11:25:53 -0400 | Subject | Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode | From | Hamza Mahfooz <> |
| |
On 4/17/23 11:03, Christian König wrote: > 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.
It seemed like it was necessary to read `tbo.resource` since the documentation for `struct ttm_buffer_object` makes mention of a "bo::resv::reserved" lock.
> > 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); >>> > -- Hamza
| |