BTW, I noticed the flush sequence in the kernel is wrong. The correct flush sequence should be: 1) EVENT_WRITE_EOP - CACHE_FLUSH_AND_INV_TS - write a dword to memory, but no fence/interrupt. 2) WAIT_REG_MEM on the dword to wait for idle before SURFACE_SYNC. 3) SURFACE_SYNC (TC, K$, I$) 4) Write CP_COHER_CNTL2. 5) EVENT_WRITE_EOP - BOTTOM_OF_PIPE_TS - write the fence with the interrupt. WAIT_REG_MEM wouldn't be needed if we were able to merge CACHE_FLUSH_AND_INV, SURFACE_SYNC, and CP_COHER_CNTL2 into one EOP event. The main issue with the current flush sequence in radeon and amdgpu is that it doesn't wait for idle before writing CP_COHER_CNTL2 and SURFACE_SYNC. So far we've been able to avoid the bug by waiting for idle in userspace IBs. Marek On Fri, May 26, 2017 at 5:47 PM, Marek Olšák <maraeo at gmail.com> wrote: > On Tue, May 9, 2017 at 2:13 PM, Nicolai Hähnle <nhaehnle at gmail.com> wrote: >> Hi all, >> >> I'm seeing some very strange errors on Verde with CPU readback from GART, >> and am pretty much out of ideas. Some help would be very much appreciated. >> >> The error manifests with the >> GL45-CTS.gtf32.GL3Tests.packed_pixels.packed_pixels_pbo test on amdgpu, >> but >> *not* on radeon. Here's what the test does: >> >> 1. Upload a texture. >> 2. Read the texture back via a shader that uses shader buffer writes to >> write data to a buffer that is allocated in GART. >> 3. The CPU then reads from the buffer -- and sometimes gets stale data. >> >> This sequence is repeated for many sub-tests. There are some sub-tests >> where >> the CPU reads stale data from the buffer, i.e. the shader writes simply >> don't make it to the CPU. The tests vary superficially, e.g. the first >> failing test is (almost?) always one where data is written in 16-bit words >> (but there are succeeding sub-tests with 16-bit writes as well). >> >> The bug is *not* a timing issue. Adding even a 1sec delay (sleep(1);) >> between the fence wait and the return of glMapBuffer does not fix the >> problem. The data must be stuck in a cache somewhere. >> >> Since the test runs okay with the radeon module, I tried some changes >> based >> on comparing the IB submit between radeon and amdgpu, and based on >> comparing >> register settings via scans obtained from umr. Some of the things I've >> tried: >> >> - Set HDP_MISC_CNTL.FLUSH_INVALIDATE_CACHE to 1 (both radeon and >> amdgpu/gfx9 >> set this) >> - Add SURFACE_SYNC packets preceded by setting CP_COHER_CNTL2 to the vmid >> (radeon does this) >> - Change gfx_v6_0_ring_emit_hdp_invalidate: select ME engine instead of >> PFP >> (which seems more logical, and is done by gfx7+), or remove the >> corresponding WRITE_DATA entirely >> >> None of these changes helped. >> >> What *does* help is adding an artificial wait. Specifically, I'm adding a >> sequence of >> >> - WRITE_DATA >> - CACHE_FLUSH_AND_INV_TS_EVENT (BOTTOM_OF_PIPE_TS has same behavior) >> - WAIT_REG_MEM >> >> as can be seen in the attached patch. This works around the problem, but >> it >> makes no sense: >> >> Adding the wait sequence *before* the SURFACE_SYNC in ring_emit_fence >> works >> around the problem. However(!) it does not actually cause the UMD to wait >> any longer than before. Without this change, the UMD immediately sees a >> signaled user fence (and never uses an ioctl to wait), and with this >> change, >> it *still* sees a signaled user fence. >> >> Also, note that the way I've hacked the change, the wait sequence is only >> added for the user fence emit (and I'm using a modified UMD to ensure that >> there is enough memory to be used by the added wait sequence). >> >> Adding the wait sequence *after* the SURFACE_SYNC *doesn't* work around >> the >> problem. >> >> So for whatever reason, the added wait sequence *before* the SURFACE_SYNC >> encourages some part of the GPU to flush the data from wherever it's >> stuck, >> and that's just really bizarre. There must be something really simple I'm >> missing, and any pointers would be appreciated. > > Have you tried this? > > diff --git a/src/gallium/drivers/radeonsi/si_hw_context.c > b/src/gallium/drivers/radeonsi/si_hw_context.c > index 92c09cb..e6ac0ba 100644 > --- a/src/gallium/drivers/radeonsi/si_hw_context.c > +++ b/src/gallium/drivers/radeonsi/si_hw_context.c > @@ -133,7 +133,8 @@ void si_context_gfx_flush(void *context, unsigned flags, > SI_CONTEXT_PS_PARTIAL_FLUSH; > > /* DRM 3.1.0 doesn't flush TC for VI correctly. */ > - if (ctx->b.chip_class == VI && ctx->b.screen->info.drm_minor <= 1) > + if ((ctx->b.chip_class == VI && ctx->b.screen->info.drm_minor <= 1) > || > + (ctx->b.chip_class == SI && ctx->b.screen->info.drm_major == 3)) > ctx->b.flags |= SI_CONTEXT_INV_GLOBAL_L2 | > SI_CONTEXT_INV_VMEM_L1; > > One more cache flush there shouldn't hurt. > > Also, Mesa uses PFP_SYNC_ME. It shouldn't be necessary, but it's worth a > try. > > Marek
