[PATCH] drm/amdgpu: protect ring overrun

[Christian König]

Am 23.04.20 um 06:22 schrieb Yintian Tao:
> Wait for the oldest sequence on the ring
> to be signaled in order to make sure there
> will be no command overrun.

One technical problem and a few style suggestions below. Apart from that 
looks good to me.

>
> Signed-off-by: Yintian Tao <yttao at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c |  7 +++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c   | 17 +++++++++++++++--
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c  |  8 +++++++-
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c     |  9 ++++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c    |  8 +++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c     |  8 +++++++-
>   6 files changed, 51 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 7531527067df..5462ea83d8b2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -200,6 +200,13 @@ int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
>   		return -EINVAL;
>   
>   	seq = ++ring->fence_drv.sync_seq;
> +	if ((abs(seq - ring->fence_drv.num_fences_mask) >
> +	    ring->fence_drv.num_fences_mask) &&

That is a rather bad idea and won't work. The sequence is a 32bit value 
and supposed to wrap around. Just dropping the extra check should do it 
should work.

> +	    (amdgpu_fence_wait_polling(ring,
> +				       seq - ring->fence_drv.num_fences_mask,
> +				       MAX_KIQ_REG_WAIT) < 1))

Maybe we should make the timeout a parameter here.

And it is usually better to use the style like this:

r = amdgpu_fence....
if (r < 1)
     .....

> +		 return -ETIME;

We usually use -ETIMEDOUT because this is not really bound to a specific 
timer.

> +
>   	amdgpu_ring_emit_fence(ring, ring->fence_drv.gpu_addr,
>   			       seq, 0);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index a721b0e0ff69..7087333681f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -681,7 +681,14 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   	}
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_rreg(ring, reg, reg_val_offs);
> -	amdgpu_fence_emit_polling(ring, &seq);
> +	r = amdgpu_fence_emit_polling(ring, &seq);
> +	if (r) {
> +		amdgpu_ring_undo(ring);
> +		amdgpu_device_wb_free(adev, reg_val_offs);
> +		spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +		goto failed_kiq_read;
> +	}
> +

If we already have goto style error handling we should probably just add 
a new label to handle it.

Same for the other cases below where you already have a goto.

Regards,
Christian.

>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> @@ -730,7 +737,13 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>   	spin_lock_irqsave(&kiq->ring_lock, flags);
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_wreg(ring, reg, v);
> -	amdgpu_fence_emit_polling(ring, &seq);
> +	r = amdgpu_fence_emit_polling(ring, &seq);
> +	if (r) {
> +		amdgpu_ring_undo(ring);
> +		spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +		goto failed_kiq_write;
> +	}
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 8c10084f44ef..12d181ac7e78 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -60,7 +60,13 @@ void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device *adev,
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_emit_reg_write_reg_wait(ring, reg0, reg1,
>   					    ref, mask);
> -	amdgpu_fence_emit_polling(ring, &seq);
> +	r = amdgpu_fence_emit_polling(ring, &seq);
> +	if (r) {
> +		amdgpu_ring_undo(ring);
> +		spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +		goto failed_kiq;
> +	}
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 5b1549f167b0..650b7a67d3bc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -4068,7 +4068,14 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev)
>   				reg_val_offs * 4));
>   	amdgpu_ring_write(ring, upper_32_bits(adev->wb.gpu_addr +
>   				reg_val_offs * 4));
> -	amdgpu_fence_emit_polling(ring, &seq);
> +	r = amdgpu_fence_emit_polling(ring, &seq);
> +	if (r) {
> +		amdgpu_ring_undo(ring);
> +		amdgpu_device_wb_free(adev, reg_val_offs);
> +		spin_unlock_irqrestore(&kiq->ring_lock, flags);
> +		goto failed_kiq_read;
> +	}
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> index 30b75d79efdb..71430f2a2374 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> @@ -427,7 +427,13 @@ static int gmc_v10_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
>   		amdgpu_ring_alloc(ring, kiq->pmf->invalidate_tlbs_size + 8);
>   		kiq->pmf->kiq_invalidate_tlbs(ring,
>   					pasid, flush_type, all_hub);
> -		amdgpu_fence_emit_polling(ring, &seq);
> +		r = amdgpu_fence_emit_polling(ring, &seq);
> +		if (r) {
> +			amdgpu_ring_undo(ring);
> +			spin_unlock(&kiq->ring_lock);
> +			return -ETIME;
> +		}
> +
>   		amdgpu_ring_commit(ring);
>   		spin_unlock(&adev->gfx.kiq.ring_lock);
>   		r = amdgpu_fence_wait_polling(ring, seq, adev->usec_timeout);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index fecdbc471983..a42a95675e2b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -621,7 +621,13 @@ static int gmc_v9_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,
>   						      pasid, 2, all_hub);
>   		kiq->pmf->kiq_invalidate_tlbs(ring,
>   					pasid, flush_type, all_hub);
> -		amdgpu_fence_emit_polling(ring, &seq);
> +		r = amdgpu_fence_emit_polling(ring, &seq);
> +		if (r) {
> +			amdgpu_ring_undo(ring);
> +			spin_unlock(&kiq->ring_lock);
> +			return -ETIME;
> +		}
> +
>   		amdgpu_ring_commit(ring);
>   		spin_unlock(&adev->gfx.kiq.ring_lock);
>   		r = amdgpu_fence_wait_polling(ring, seq, adev->usec_timeout);