Re: [PATCH] drm/amdgpu: protect ring overrun

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

Am 23.04.20 um 11:06 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.
>
> v2: fix coding stype and remove abs operation

One nit pick below, with that fixed the patch is Reviewed-by: Christian 
König <christian.koenig@xxxxxxx>

> Signed-off-by: Yintian Tao <yttao@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c | 10 +++++++++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c   | 22 ++++++++++++++++++----
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h  |  3 ++-
>   drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c  |  8 +++++++-
>   drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c    |  1 -
>   drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c     |  1 -
>   drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c     | 14 +++++++++++---
>   drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c    |  8 +++++++-
>   drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c     |  8 +++++++-
>   9 files changed, 61 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> index 7531527067df..397bd5fa77cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_fence.c
> @@ -192,14 +192,22 @@ int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **f,
>    * Used For polling fence.
>    * Returns 0 on success, -ENOMEM on failure.
>    */
> -int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s)
> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s,
> +			      uint32_t timeout)
>   {
>   	uint32_t seq;
> +	signed long r = 0;

Please drop the initialization of r here. That is usually seen as rather 
bad style because it prevents the compiler from raising an warning when 
this really isn't initialized.

Regards,
Christian.

>   
>   	if (!s)
>   		return -EINVAL;
>   
>   	seq = ++ring->fence_drv.sync_seq;
> +	r = amdgpu_fence_wait_polling(ring,
> +				      seq - ring->fence_drv.num_fences_mask,
> +				      timeout);
> +	if (r < 1)
> +		return -ETIMEDOUT;
> +
>   	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..0103acc57474 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -675,13 +675,15 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   
>   	spin_lock_irqsave(&kiq->ring_lock, flags);
>   	if (amdgpu_device_wb_get(adev, &reg_val_offs)) {
> -		spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   		pr_err("critical bug! too many kiq readers\n");
> -		goto failed_kiq_read;
> +		goto failed_unlock;
>   	}
>   	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, MAX_KIQ_REG_WAIT);
> +	if (r)
> +		goto failed_undo;
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> @@ -712,7 +714,13 @@ uint32_t amdgpu_kiq_rreg(struct amdgpu_device *adev, uint32_t reg)
>   	amdgpu_device_wb_free(adev, reg_val_offs);
>   	return value;
>   
> +failed_undo:
> +	amdgpu_ring_undo(ring);
> +failed_unlock:
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq_read:
> +	if (reg_val_offs)
> +		amdgpu_device_wb_free(adev, reg_val_offs);
>   	pr_err("failed to read reg:%x\n", reg);
>   	return ~0;
>   }
> @@ -730,7 +738,10 @@ 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, MAX_KIQ_REG_WAIT);
> +	if (r)
> +		goto failed_undo;
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> @@ -759,6 +770,9 @@ void amdgpu_kiq_wreg(struct amdgpu_device *adev, uint32_t reg, uint32_t v)
>   
>   	return;
>   
> +failed_undo:
> +	amdgpu_ring_undo(ring);
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq_write:
>   	pr_err("failed to write reg:%x\n", reg);
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> index 137d3d2b46e8..be218754629a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.h
> @@ -118,7 +118,8 @@ void amdgpu_fence_driver_suspend(struct amdgpu_device *adev);
>   void amdgpu_fence_driver_resume(struct amdgpu_device *adev);
>   int amdgpu_fence_emit(struct amdgpu_ring *ring, struct dma_fence **fence,
>   		      unsigned flags);
> -int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s);
> +int amdgpu_fence_emit_polling(struct amdgpu_ring *ring, uint32_t *s,
> +			      uint32_t timeout);
>   bool amdgpu_fence_process(struct amdgpu_ring *ring);
>   int amdgpu_fence_wait_empty(struct amdgpu_ring *ring);
>   signed long amdgpu_fence_wait_polling(struct amdgpu_ring *ring,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> index 8c10084f44ef..cbbb8d02535a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_virt.c
> @@ -60,7 +60,10 @@ 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, MAX_KIQ_REG_WAIT);
> +	if (r)
> +		goto failed_undo;
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> @@ -82,6 +85,9 @@ void amdgpu_virt_kiq_reg_write_reg_wait(struct amdgpu_device *adev,
>   
>   	return;
>   
> +failed_undo:
> +	amdgpu_ring_undo(ring);
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq:
>   	pr_err("failed to write reg %x wait reg %x\n", reg0, reg1);
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> index 8a579ce8757d..2b6d7687f4cb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v10_0.c
> @@ -7605,7 +7605,6 @@ static void gfx_v10_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg,
>   				     uint32_t reg_val_offs)
>   {
>   	struct amdgpu_device *adev = ring->adev;
> -	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4));
>   	amdgpu_ring_write(ring, 0 |	/* src: register*/
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> index 6c56cedce4c3..6ae78b9e9551 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
> @@ -6393,7 +6393,6 @@ static void gfx_v8_0_ring_emit_rreg(struct amdgpu_ring *ring, uint32_t reg,
>   				    uint32_t reg_val_offs)
>   {
>   	struct amdgpu_device *adev = ring->adev;
> -	struct amdgpu_kiq *kiq = &adev->gfx.kiq;
>   
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4));
>   	amdgpu_ring_write(ring, 0 |	/* src: register*/
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> index 09aa5f509bd2..be05ab221b16 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
> @@ -4054,9 +4054,8 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev)
>   
>   	spin_lock_irqsave(&kiq->ring_lock, flags);
>   	if (amdgpu_device_wb_get(adev, &reg_val_offs)) {
> -		spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   		pr_err("critical bug! too many kiq readers\n");
> -		goto failed_kiq_read;
> +		goto failed_unlock;
>   	}
>   	amdgpu_ring_alloc(ring, 32);
>   	amdgpu_ring_write(ring, PACKET3(PACKET3_COPY_DATA, 4));
> @@ -4070,7 +4069,10 @@ 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, MAX_KIQ_REG_WAIT);
> +	if (r)
> +		goto failed_undo;
> +
>   	amdgpu_ring_commit(ring);
>   	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   
> @@ -4102,7 +4104,13 @@ static uint64_t gfx_v9_0_kiq_read_clock(struct amdgpu_device *adev)
>   	amdgpu_device_wb_free(adev, reg_val_offs);
>   	return value;
>   
> +failed_undo:
> +	amdgpu_ring_undo(ring);
> +failed_unlock:
> +	spin_unlock_irqrestore(&kiq->ring_lock, flags);
>   failed_kiq_read:
> +	if (reg_val_offs)
> +		amdgpu_device_wb_free(adev, reg_val_offs);
>   	pr_err("failed to read gpu clock\n");
>   	return ~0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v10_0.c
> index 30b75d79efdb..b120f9160f13 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, MAX_KIQ_REG_WAIT);
> +		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..0a6026308343 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, MAX_KIQ_REG_WAIT);
> +		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);

_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx