[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <a13a38a4-6327-7af0-56b7-930f4258eee5@amd.com>
Date: Mon, 17 Apr 2023 17:46:00 +0200
From: Christian König <christian.koenig@....com>
To: Hamza Mahfooz <hamza.mahfooz@....com>,
amd-gfx@...ts.freedesktop.org
Cc: stable@...r.kernel.org, Harry Wentland <harry.wentland@....com>,
Leo Li <sunpeng.li@....com>,
Rodrigo Siqueira <Rodrigo.Siqueira@....com>,
Alex Deucher <alexander.deucher@....com>,
"Pan, Xinhui" <Xinhui.Pan@....com>,
David Airlie <airlied@...il.com>,
Daniel Vetter <daniel@...ll.ch>,
Aurabindo Pillai <aurabindo.pillai@....com>,
Qingqing Zhuo <qingqing.zhuo@....com>,
Hans de Goede <hdegoede@...hat.com>,
Hersen Wu <hersenxs.wu@....com>,
Stylon Wang <stylon.wang@....com>,
Luben Tuikov <luben.tuikov@....com>,
dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode
Am 17.04.23 um 17:25 schrieb 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.
What? No, that sounds like you completely misunderstood that. I think we
need to improve the documentation.
As long as the object is pinned for scanout you don't even need to
reserve it.
Just use something like "abo->tbo.resource ? abo->tbo.resource->mem_type
: 0" here.
Regards,
Christian.
>
>>
>> Regards,
>> Christian.
>>
>>>
>>>>
>>>> Regards,
>>>> Christian.
>>>>
>>>>>
>>>>> Cc: stable@...r.kernel.org
>>>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more
>>>>> flexible (v2)")
>>>>> Signed-off-by: Hamza Mahfooz <hamza.mahfooz@....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);
>>>>
>>
Powered by blists - more mailing lists