[PATCH 2/4] drm/amdgpu:new method to sync ce&de

[Christian König]

Well a perfect example of why we shouldn't take a look at what userspace 
does, but rather what the hardware can do when we design the IOCTLs.

I'm trying to raise awareness for this for quite a while now, but a lot 
of people seem to think when the UMD doesn't do something the kernel 
doesn't need to handle this situation.

Thanks for that info,
Christian.

Am 29.08.2016 um 10:28 schrieb Liu, Monk:
> But speaking with practice attitude: at least close source UMD OCL doesn't use CE at all, and I guess MESA either ...
>
> BR Monk
>
> -----Original Message-----
> From: Christian König [mailto:deathsimple at vodafone.de]
> Sent: Monday, August 29, 2016 4:25 PM
> To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
> Subject: Re: [PATCH 2/4] drm/amdgpu:new method to sync ce&de
>
> Hui what?
>
> It feels like a hundred times I asked if the compute engine has a CE as well, but so far the answer was always No. That would explain a whole bunch of problems we had with the compute rings as well.
>
> In this case the patch is good as it is, please just rebase it.
>
> Christian.
>
> Am 29.08.2016 um 10:14 schrieb Liu, Monk:
>> No, compute ring also can leverage constant engines, that's depend on
>> OCL umd behavior I just make sure KMD do nothing wrong
>>
>> BR Monk
>>
>> -----Original Message-----
>> From: Christian König [mailto:deathsimple at vodafone.de]
>> Sent: Monday, August 29, 2016 4:06 PM
>> To: Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org
>> Subject: Re: [PATCH 2/4] drm/amdgpu:new method to sync ce&de
>>
>> Am 29.08.2016 um 04:55 schrieb Monk Liu:
>>> CE & DE can have most up to 128dw as the gap between them so to sync
>>> CE nad DE we don't need double SWITCH_BUFFERs any more, which is
>>> urgly and harm performance, we only need insert 128NOP after VM flush
>>> to prevent CE vm fault.
>>>
>>> Change-Id: Ibec954ce4c817ad7d3bce89c2bcb95b6c6bb5411
>>> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
>> Looks good to me, but only the GFX engines have a CE. So syncing on the compute engines is pretty much pointless.
>>
>> So I suggest that you move this into the "usepfp" if branch as well.
>>
>> With that fixed the patch is Reviewed-by: Christian König <christian.koenig at amd.com>.
>>
>> Regards,
>> Christian.
>>
>>> ---
>>>     drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c | 18 ++++++------------
>>>     1 file changed, 6 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> index 26fced0..ce1e616 100755
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
>>> @@ -6005,14 +6005,6 @@ static void gfx_v8_0_ring_emit_pipeline_sync(struct amdgpu_ring *ring)
>>>     	amdgpu_ring_write(ring, seq);
>>>     	amdgpu_ring_write(ring, 0xffffffff);
>>>     	amdgpu_ring_write(ring, 4); /* poll interval */
>>> -
>>> -	if (usepfp) {
>>> -		/* synce CE with ME to prevent CE fetch CEIB before context switch done */
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -	}
>>>     }
>>>     
>>>     static void gfx_v8_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>>> @@
>>> -6020,6 +6012,9 @@ static void gfx_v8_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>>>     {
>>>     	int usepfp = (ring->type == AMDGPU_RING_TYPE_GFX);
>>>     
>>> +	/* GFX8 emits 128 dw nop to prevent DE do vm_flush before CE finish CEIB */
>>> +	amdgpu_ring_insert_nop(ring, 128);
>>> +
>>>     	amdgpu_ring_write(ring, PACKET3(PACKET3_WRITE_DATA, 3));
>>>     	amdgpu_ring_write(ring, (WRITE_DATA_ENGINE_SEL(usepfp) |
>>>     				 WRITE_DATA_DST_SEL(0)) |
>>> @@ -6059,11 +6054,10 @@ static void gfx_v8_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>>>     		/* sync PFP to ME, otherwise we might get invalid PFP reads */
>>>     		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME, 0));
>>>     		amdgpu_ring_write(ring, 0x0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> -		amdgpu_ring_write(ring, 0);
>>>     	}
>>> +
>>> +	/* GFX8 emits 128 dw nop to prevent CE access VM before vm_flush finish */
>>> +	amdgpu_ring_insert_nop(ring, 128);
>>>     }
>>>     
>>>     static u32 gfx_v8_0_ring_get_rptr_compute(struct amdgpu_ring
>>> *ring)
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx