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

[Christian König <christian.koenig@xxxxxxx>]

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@xxxxxxxxxxxxxxx
> > > > > Fixes: 81d0bcf99009 ("drm/amdgpu: make display pinning more 
> > > > > flexible (v2)")
> > > > > Signed-off-by: Hamza Mahfooz <hamza.mahfooz@xxxxxxx>
> > > > > ---
> > > > >   .../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);