[PATCH 08/22] drm/amdgpu: cleanups VCE/UVD ib test part

[Christian König]

Am 26.02.2018 um 06:18 schrieb Monk Liu:
> 1)amdgpu_vce_get_create_msg is only used in ib test so
> no reason no to use a static routine for it, and add a
> timeout parameter for it.
>
> 2)fence handling of MM's ib test part is a little messy
> clean it make it easier to read
>
> Change-Id: Ic9bfd9971457600266096e114210e84ce9b4347d
> Signed-off-by: Monk Liu <Monk.Liu at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c | 43 ++++++++++++++++++---------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h |  2 --
>   2 files changed, 23 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> index 0877fc39..a829350 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.c
> @@ -405,8 +405,8 @@ void amdgpu_vce_free_handles(struct amdgpu_device *adev, struct drm_file *filp)
>    *
>    * Open up a stream for HW test
>    */
> -int amdgpu_vce_get_create_msg(struct amdgpu_ring *ring, uint32_t handle,
> -			      struct dma_fence **fence)
> +static int amdgpu_vce_get_create_msg(struct amdgpu_ring *ring, uint32_t handle,
> +			      struct dma_fence **fence, long timeout)

Looks good to me, but you could go a step further and make 
amdgpu_vce_get_destroy_msg static as well.

Additional to that you can also drop the fence parameter from 
amdgpu_vce_get_create_msg.

>   {
>   	const unsigned ib_size_dw = 1024;
>   	struct amdgpu_job *job;
> @@ -461,19 +461,25 @@ int amdgpu_vce_get_create_msg(struct amdgpu_ring *ring, uint32_t handle,
>   		ib->ptr[i] = 0x0;
>   
>   	r = amdgpu_ib_schedule(ring, 1, ib, NULL, &f);
> -	job->fence = dma_fence_get(f);
>   	if (r)
> +		return r;
> +	r = dma_fence_wait_timeout(f, false, timeout);
> +	if (r == 0) {
> +		DRM_ERROR("amdgpu: VCE IB test get_create_msg timed out.\n");
> +		r = -ETIMEDOUT;

NAK, waiting for the create message alone can crash the VCE firmware on 
older firmware versions.

You need to send create plus destroy or encode message before you can wait.

>   		goto err;
> -
> -	amdgpu_job_free(job);
> -	if (fence)
> -		*fence = dma_fence_get(f);
> -	dma_fence_put(f);
> -	return 0;
> +	} else if (r < 0) {
> +		DRM_ERROR("amdgpu: VCE IB test get_create_msg fence wait failed (%ld).\n", r);
> +		goto err;
> +	} else {
> +		r = 0;
> +	}

That looks like you are leaking job here now.

Probably best if you split the patch and make the functions static first 
and then add the rest in another patch.

Regards,
Christian.

>   
>   err:
> -	amdgpu_job_free(job);
> +	amdgpu_ib_free(ring->adev, ib, NULL);
> +	dma_fence_put(f);
>   	return r;
> +
>   }
>   
>   /**
> @@ -523,7 +529,7 @@ int amdgpu_vce_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
>   		ib->ptr[i] = 0x0;
>   
>   	if (direct) {
> -		r = amdgpu_ib_schedule(ring, 1, ib, NULL, &f);
> +		r = amdgpu_ib_schedule(ring, 1, ib, NULL, fence);
>   		job->fence = dma_fence_get(f);
>   		if (r)
>   			goto err;
> @@ -531,14 +537,11 @@ int amdgpu_vce_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
>   		amdgpu_job_free(job);
>   	} else {
>   		r = amdgpu_job_submit(job, ring, &ring->adev->vce.entity,
> -				      AMDGPU_FENCE_OWNER_UNDEFINED, &f);
> +				      AMDGPU_FENCE_OWNER_UNDEFINED, fence);
>   		if (r)
>   			goto err;
>   	}
>   
> -	if (fence)
> -		*fence = dma_fence_get(f);
> -	dma_fence_put(f);
>   	return 0;
>   
>   err:
> @@ -1075,13 +1078,13 @@ int amdgpu_vce_ring_test_ring(struct amdgpu_ring *ring)
>   int amdgpu_vce_ring_test_ib(struct amdgpu_ring *ring, long timeout)
>   {
>   	struct dma_fence *fence = NULL;
> -	long r;
> +	long r = 0;
>   
>   	/* skip vce ring1/2 ib test for now, since it's not reliable */
>   	if (ring != &ring->adev->vce.ring[0])
>   		return 0;
>   
> -	r = amdgpu_vce_get_create_msg(ring, 1, NULL);
> +	r = amdgpu_vce_get_create_msg(ring, 1, NULL, timeout);
>   	if (r) {
>   		DRM_ERROR("amdgpu: failed to get create msg (%ld).\n", r);
>   		goto error;
> @@ -1095,12 +1098,12 @@ int amdgpu_vce_ring_test_ib(struct amdgpu_ring *ring, long timeout)
>   
>   	r = dma_fence_wait_timeout(fence, false, timeout);
>   	if (r == 0) {
> -		DRM_ERROR("amdgpu: IB test timed out.\n");
> +		DRM_ERROR("amdgpu: IB test get_destory_msg timed out.\n");
>   		r = -ETIMEDOUT;
>   	} else if (r < 0) {
> -		DRM_ERROR("amdgpu: fence wait failed (%ld).\n", r);
> +		DRM_ERROR("amdgpu: IB test get_destory_msg fence wait failed (%ld).\n", r);
>   	} else {
> -		DRM_DEBUG("ib test on ring %d succeeded\n", ring->idx);
> +		DRM_DEBUG("ib test get_destory_msg on ring %d succeeded\n", ring->idx);
>   		r = 0;
>   	}
>   error:
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> index 7178126..60cb657 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> @@ -57,8 +57,6 @@ int amdgpu_vce_sw_init(struct amdgpu_device *adev, unsigned long size);
>   int amdgpu_vce_sw_fini(struct amdgpu_device *adev);
>   int amdgpu_vce_suspend(struct amdgpu_device *adev);
>   int amdgpu_vce_resume(struct amdgpu_device *adev);
> -int amdgpu_vce_get_create_msg(struct amdgpu_ring *ring, uint32_t handle,
> -			      struct dma_fence **fence);
>   int amdgpu_vce_get_destroy_msg(struct amdgpu_ring *ring, uint32_t handle,
>   			       bool direct, struct dma_fence **fence);
>   void amdgpu_vce_free_handles(struct amdgpu_device *adev, struct drm_file *filp);