On Tuesday, August 30th, 2022 at 16:06, Alex Deucher <alexdeucher@xxxxxxxxx> wrote: > On Tue, Aug 30, 2022 at 3:08 AM Simon Ser contact@xxxxxxxxxxx wrote: > > > On Friday, August 26th, 2022 at 16:39, Alex Deucher alexdeucher@xxxxxxxxx wrote: > > > > > 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. > > > > 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"? 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?
