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