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

[Simon Ser <contact@emersion.fr>]

On Friday, August 26th, 2022 at 16:39, Alex Deucher <alexdeucher@gmail.com> wrote:

> On Fri, Aug 26, 2022 at 3:38 AM Simon Ser <contact@emersion.fr> wrote:
> >
> > On Thursday, August 25th, 2022 at 20:22, Alex Deucher <alexdeucher@gmail.com> wrote:
> >
> > > On Wed, Aug 24, 2022 at 11:09 AM Simon Ser contact@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.

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?