GART write flush error on SI w/ amdgpu

[Christian König]

Am 20.06.2017 um 12:34 schrieb Marek Olšák:
> 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.

Well not waiting for idle between IBs is an explicit requirement, 
because it is rather bad for performance to do so.

David Zhou, Monk and I worked quite a lot on this to avoid both possible 
hazard and performance drop.

Christian.

>
> 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
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx