[PATCH 3/3] drm/amdgpu: Use polling for KIQ read/write register

[Christian König]

Am 30.06.2017 um 17:54 schrieb Shaoyun Liu:
> Change-Id: I87762bfc9903401ac06892bed10efa1767c15025
> Signed-off-by: Shaoyun Liu <Shaoyun.Liu at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c | 47 +++++++++++++++++++++++---------
>   1 file changed, 34 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index a65e76c..06ef893 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -116,24 +116,33 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   {
>   	signed long r;
>   	uint32_t val;
> -	struct dma_fence *f;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   	struct amdgpu_ring *ring = &kiq->ring;
> +	unsigned long end_jiffies;
> +	uint32_t seq;
> +	volatile uint32_t *f;
>   
>   	BUG_ON(!ring->funcs->emit_rreg);
>   
>   	spin_lock(&kiq->ring_lock);
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_rreg(ring, reg);
> -	amdgpu_fence_emit(ring, &f);
> +	f = amdgpu_fence_drv_cpu_addr(ring);
> +	*f = 0;

Wow, wait a second. Messing with the fence counter here is a clear 
no-go. That could even crash the interrupt handler.

You need to add proper wrap around handling instead of setting the value 
to zero.

> +	seq = ++ring->fence_drv.sync_seq;

What I meant with moving the functionality into the fence code was 
increasing the counter and emitting the fence write.

> +	amdgpu_ring_emit_fence(ring, amdgpu_fence_drv_gpu_addr(ring), seq, 0);
>   	amdgpu_ring_commit(ring);
>   	spin_unlock(&kiq->ring_lock);
>   
> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
> -	dma_fence_put(f);
> -	if (r < 1) {
> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> -		return ~0;
> +	end_jiffies = (MAX_KIQ_REG_WAIT * HZ / 1000) + jiffies;
> +	while (true) {
> +		if (*f >= seq)
> +			break;
> +		if (time_after(jiffies, end_jiffies)) {
> +			DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> +			return ~0;
> +		}
> +		cpu_relax();
>   	}

That waiting should go into the fence code as well.

Regards,
Christian.

>   
>   	val = adev->wb.wb[adev->virt.reg_val_offs];
> @@ -144,23 +153,35 @@ uint32_t amdgpu_virt_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   void amdgpu_virt_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>   {
>   	signed long r;
> -	struct dma_fence *f;
>   	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   	struct amdgpu_ring *ring = &kiq->ring;
> +	unsigned long end_jiffies;
> +	uint32_t seq;
> +	volatile uint32_t *f;
>   
>   	BUG_ON(!ring->funcs->emit_wreg);
>   
>   	spin_lock(&kiq->ring_lock);
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_wreg(ring, reg, v);
> -	amdgpu_fence_emit(ring, &f);
> +	f = amdgpu_fence_drv_cpu_addr(ring);
> +	*f = 0;
> +	seq = ++ring->fence_drv.sync_seq;
> +	amdgpu_ring_emit_fence(ring, amdgpu_fence_drv_gpu_addr(ring), seq, 0);
>   	amdgpu_ring_commit(ring);
>   	spin_unlock(&kiq->ring_lock);
>   
> -	r = dma_fence_wait_timeout(f, false, msecs_to_jiffies(MAX_KIQ_REG_WAIT));
> -	if (r < 1)
> -		DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> -	dma_fence_put(f);
> +	end_jiffies = (MAX_KIQ_REG_WAIT * HZ / 1000) + jiffies;
> +	while (true) {
> +		if (*f >= seq)
> +			break;
> +		if (time_after(jiffies, end_jiffies)) {
> +			DRM_ERROR("wait for kiq fence error: %ld.\n", r);
> +			return;
> +		}
> +		cpu_relax();
> +	}
> +
>   }
>   
>   /**