[PATCH 4/4] amd/display: indicate support for atomic async page-flips on DCN

[Alex Deucher]

On Tue, Aug 30, 2022 at 10:24 AM Simon Ser <contact at emersion.fr> wrote:
>
> On Tuesday, August 30th, 2022 at 16:06, Alex Deucher <alexdeucher at gmail.com> wrote:
>
> > On Tue, Aug 30, 2022 at 3:08 AM Simon Ser contact at emersion.fr wrote:
> >
> > > On Friday, August 26th, 2022 at 16:39, Alex Deucher alexdeucher at gmail.com wrote:
> > >
> > > > On Fri, Aug 26, 2022 at 3:38 AM Simon Ser contact at emersion.fr wrote:
> > > >
> > > > > On Thursday, August 25th, 2022 at 20:22, Alex Deucher alexdeucher at gmail.com wrote:
> > > > >
> > > > > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact at emersion.fr wrote:
> > > > > >
> > > > > > > amdgpu_dm_commit_planes already sets the flip_immediate flag for
> > > > > > > async page-flips. This flag is used to set the UNP_FLIP_CONTROL
> > > > > > > register. Thus, no additional change is required to handle async
> > > > > > > page-flips with the atomic uAPI.
> > > > > > >
> > > > > > > Note, async page-flips are still unsupported on DCE with the atomic
> > > > > > > uAPI. The mode_set_base callbacks unconditionally set the
> > > > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN field of the GRPH_FLIP_CONTROL
> > > > > > > register to 0, which disables async page-flips.
> > > > > >
> > > > > > Can you elaborate a bit on this? We don't use hsync flips at all, even
> > > > > > in non-atomic, as far as I recall. The hardware can also do immediate
> > > > > > flips which take effect as soon as you update the base address
> > > > > > register which is what we use for async updates today IIRC.
> > > > >
> > > > > When user-space performs a page-flip with the legacy KMS uAPI on DCE
> > > > > ASICs, amdgpu_display_crtc_page_flip_target() is called. This function
> > > > > checks for the DRM_MODE_PAGE_FLIP_ASYNC flag, sets work->async, which
> > > > > is then passed as an argument to adev->mode_info.funcs->page_flip() by
> > > > > amdgpu_display_flip_work_func(). Looking at an implementation, for
> > > > > instance dce_v10_0_page_flip(), the async flag is used to set that
> > > > > GRPH_FLIP_CONTROL register:
> > > > >
> > > > > /* flip at hsync for async, default is vsync */
> > > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, async ? 1 : 0);
> > > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > >
> > > > > I don't know how the hardware works, but I assumed it would be
> > > > > necessary to do the same in the atomic uAPI code-path as well. However
> > > > > dce_v10_0_crtc_do_set_base() has this code block:
> > > > >
> > > > > /* Make sure surface address is updated at vertical blank rather than
> > > > > * horizontal blank
> > > > > */
> > > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > > GRPH_SURFACE_UPDATE_H_RETRACE_EN, 0);
> > > > > WREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset, tmp);
> > > > >
> > > > > Which unconditionally sets that same register.
> > > > >
> > > > > Either way, it's not a very big deal for this patch series, DCE and DCN
> > > > > are separate, DCE can be sorted out separately.
> > > > >
> > > > > Am I completely mistaken here?
> > > >
> > > > I checked the code and it looks like only DCE11 and newer support
> > > > immediate flips. E.g.,
> > > >
> > > > /* flip immediate for async, default is vsync */
> > > > tmp = RREG32(mmGRPH_FLIP_CONTROL + amdgpu_crtc->crtc_offset);
> > > > tmp = REG_SET_FIELD(tmp, GRPH_FLIP_CONTROL,
> > > > GRPH_SURFACE_UPDATE_IMMEDIATE_EN, async ? 1 : 0);
> > > >
> > > > in dce_v11_0.c.
> > > >
> > > > Either way, the non-DC display code is not atomic anyway, so I don't
> > > > think this is an issue. We still support async flips via the
> > > > non-atomic API. I agree this is not blocking for the patch series,
> > > > just thinking out loud mostly.
> > >
> > > Michel pointed out that DC can drive both DCN and DCE. This was a
> > > misunderstanding on my end, I thought DC could only drive DCN. I'll reword the
> > > commit message to refer to DC instead of DCN.
> > >
> > > This begs the question, should we bother to set the
> > > atomic_async_page_flip_not_supported flag on non-atomic drivers? I've just
> > > slapped the flag everywhere for simplicity's sake, but maybe it would make more
> > > sense to just set it for atomic-capable drivers. Especially if the long-term
> > > goal is to convert all atomic drivers to support async flips and eventually
> > > remove atomic_async_page_flip_not_supported.
> >
> > yeah, I think we can drop the flag for non-atomic. amdgpu at least
> > already supports async flips.
> >
> > > Thanks for the hint regarding DCE10. It sounds like it may be worthwhile to
> > > unset drm_mode_config.async_page_flip on DCE10 and earlier, to indicate to
> > > user-space that async page-flips are not supported on these ASICs? Right now it
> > > seems like we indicate that we support them, and then ignore the ASYNC_FLIP
> > > flag?
> >
> > Async flips work fine with the current code. I think I did the
> > initial implementation on DCE10. We set
> > GRPH_SURFACE_UPDATE_H_RETRACE_EN dynamically in dce_v10_0_page_flip()
> > based on the type of flip selected.
>
> Hm, can you elaborate on the difference between "immediate flip" (as in
> UNP_FLIP_CONTROL) and GRPH_SURFACE_UPDATE_H_RETRACE_EN? What are their
> relationship with KMS's concept of "async flips"?

The display surface registers are double buffered.  The default is for
the swap to take place during vblank.  However, you can select
different behavior via the GRPH_FLIP_CONTROL register.  On DCE10 and
older you can set GRPH_SURFACE_UPDATE_H_RETRACE_EN to select swapping
at hsync.  On DCE11 and newer, you can set
GRPH_SURFACE_UPDATE_IMMEDIATE_EN which causes the swap to immediately
(IIRC as soon as GRPH_PRIMARY_SURFACE_ADDRESS is written).

>
> Also you said earlier:
>
> > We don't use hsync flips at all, even in non-atomic, as far as I recall.
>
> Is "hsync flip" controlled by GRPH_SURFACE_UPDATE_H_RETRACE_EN, or is it
> something else entirely?

Yes, GRPH_SURFACE_UPDATE_H_RETRACE_EN.  We use hsync swaps on older
DCE parts that don't support immediate swaps.

Alex