2023-04-14 19:39:11

by Hamza Mahfooz

[permalink] [raw]
Subject: [PATCH] drm/amd/display: fix flickering caused by S/G mode

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.

Cc: [email protected]
Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
Signed-off-by: Hamza Mahfooz <[email protected]>
---
.../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);
--
2.40.0


2023-04-17 06:01:34

by Christian König

[permalink] [raw]
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode

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?

Regards,
Christian.

>
> Cc: [email protected]
> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> Signed-off-by: Hamza Mahfooz <[email protected]>
> ---
> .../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);

2023-04-17 14:56:31

by Hamza Mahfooz

[permalink] [raw]
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode


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().

>
> Regards,
> Christian.
>
>>
>> Cc: [email protected]
>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible
>> (v2)")
>> Signed-off-by: Hamza Mahfooz <[email protected]>
>> ---
>>   .../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

2023-04-17 15:12:46

by Christian König

[permalink] [raw]
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode

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: [email protected]
>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible
>>> (v2)")
>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>> ---
>>>   .../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);
>>

2023-04-17 15:31:50

by Hamza Mahfooz

[permalink] [raw]
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: [email protected]
>>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible
>>>> (v2)")
>>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>>> ---
>>>>   .../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

2023-04-17 15:49:10

by Wu, Hersen

[permalink] [raw]
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 <[email protected]>
Sent: Monday, April 17, 2023 11:26 AM
To: Koenig, Christian <[email protected]>; [email protected]
Cc: [email protected]; Wentland, Harry <[email protected]>; Li, Sun peng (Leo) <[email protected]>; Siqueira, Rodrigo <[email protected]>; Deucher, Alexander <[email protected]>; Pan, Xinhui <[email protected]>; David Airlie <[email protected]>; Daniel Vetter <[email protected]>; Pillai, Aurabindo <[email protected]>; Zhuo, Qingqing (Lillian) <[email protected]>; Hans de Goede <[email protected]>; Wu, Hersen <[email protected]>; Wang, Chao-kai (Stylon) <[email protected]>; Tuikov, Luben <[email protected]>; [email protected]; [email protected]
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: [email protected]
>>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more
>>>> flexible
>>>> (v2)")
>>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>>> ---
>>>>   .../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

2023-04-17 15:49:37

by Christian König

[permalink] [raw]
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: [email protected]
>>>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more
>>>>> flexible (v2)")
>>>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>>>> ---
>>>>>   .../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);
>>>>
>>

2023-04-17 16:39:16

by Hamza Mahfooz

[permalink] [raw]
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode

On 4/17/23 11:41, Wu, Hersen wrote:
> [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.

From the community reports, it can be reproduced on as far back as the
Ryzen 4800H (which I guess is Renoir).

>
> Thanks!
> Hersen
>
>
> -----Original Message-----
> From: Mahfooz, Hamza <[email protected]>
> Sent: Monday, April 17, 2023 11:26 AM
> To: Koenig, Christian <[email protected]>; [email protected]
> Cc: [email protected]; Wentland, Harry <[email protected]>; Li, Sun peng (Leo) <[email protected]>; Siqueira, Rodrigo <[email protected]>; Deucher, Alexander <[email protected]>; Pan, Xinhui <[email protected]>; David Airlie <[email protected]>; Daniel Vetter <[email protected]>; Pillai, Aurabindo <[email protected]>; Zhuo, Qingqing (Lillian) <[email protected]>; Hans de Goede <[email protected]>; Wu, Hersen <[email protected]>; Wang, Chao-kai (Stylon) <[email protected]>; Tuikov, Luben <[email protected]>; [email protected]; [email protected]
> 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: [email protected]
>>>>> Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more
>>>>> flexible
>>>>> (v2)")
>>>>> Signed-off-by: Hamza Mahfooz <[email protected]>
>>>>> ---
>>>>>   .../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
--
Hamza

2023-04-17 17:39:37

by Alex Deucher

[permalink] [raw]
Subject: Re: [PATCH] drm/amd/display: fix flickering caused by S/G mode

On Mon, Apr 17, 2023 at 1:59 AM Christian König
<[email protected]> 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!

It doesn't exhibit when we allow only gtt or only vram, only when
switches between pools does it flicker.

Alex

>
> 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?
>
> Regards,
> Christian.
>
> >
> > Cc: [email protected]
> > Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more flexible (v2)")
> > Signed-off-by: Hamza Mahfooz <[email protected]>
> > ---
> > .../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);
>