lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <PH8PR12MB6962186DEF22F5CE45458675FD9C9@PH8PR12MB6962.namprd12.prod.outlook.com>
Date:   Mon, 17 Apr 2023 15:41:56 +0000
From:   "Wu, Hersen" <hersenxs.wu@....com>
To:     "Mahfooz, Hamza" <Hamza.Mahfooz@....com>,
        "Koenig, Christian" <Christian.Koenig@....com>,
        "amd-gfx@...ts.freedesktop.org" <amd-gfx@...ts.freedesktop.org>
CC:     "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "Wentland, Harry" <Harry.Wentland@....com>,
        "Li, Sun peng (Leo)" <Sunpeng.Li@....com>,
        "Siqueira, Rodrigo" <Rodrigo.Siqueira@....com>,
        "Deucher, Alexander" <Alexander.Deucher@....com>,
        "Pan, Xinhui" <Xinhui.Pan@....com>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        "Pillai, Aurabindo" <Aurabindo.Pillai@....com>,
        "Zhuo, Qingqing (Lillian)" <Qingqing.Zhuo@....com>,
        Hans de Goede <hdegoede@...hat.com>,
        "Wang, Chao-kai (Stylon)" <Stylon.Wang@....com>,
        "Tuikov, Luben" <Luben.Tuikov@....com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH] drm/amd/display: fix flickering caused by S/G mode

[AMD Official Use Only - General]

Hi,

The change applies to all AMD GPU ASIC.
Please communicate with issue reporter to see if the issue could be reproduced older ASIC, like Mendocino, CZN.

Thanks!
Hersen


-----Original Message-----
From: Mahfooz, Hamza <Hamza.Mahfooz@....com> 
Sent: Monday, April 17, 2023 11:26 AM
To: Koenig, Christian <Christian.Koenig@....com>; amd-gfx@...ts.freedesktop.org
Cc: stable@...r.kernel.org; Wentland, Harry <Harry.Wentland@....com>; Li, Sun peng (Leo) <Sunpeng.Li@....com>; Siqueira, Rodrigo <Rodrigo.Siqueira@....com>; Deucher, Alexander <Alexander.Deucher@....com>; Pan, Xinhui <Xinhui.Pan@....com>; David Airlie <airlied@...il.com>; Daniel Vetter <daniel@...ll.ch>; Pillai, Aurabindo <Aurabindo.Pillai@....com>; Zhuo, Qingqing (Lillian) <Qingqing.Zhuo@....com>; Hans de Goede <hdegoede@...hat.com>; Wu, Hersen <hersenxs.wu@....com>; Wang, Chao-kai (Stylon) <Stylon.Wang@....com>; Tuikov, Luben <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


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@...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);
>>>
> 
--
Hamza

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ