[PATCH 2/2] drm/amdgpu: expose AMDGPU_GEM_CREATE_VRAM_CLEARED to user space

[Christian König]

Am 22.07.2016 um 04:10 schrieb Flora Cui:
> Change-Id: I256178afa18c1a433fe60d8656d1c5cc5d55cf2f
> Signed-off-by: Flora Cui <Flora.Cui at amd.com>
> Reviewed-by: Chunming Zhou <David1.Zhou at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 23 +++++++++++++++++++++++
>   include/uapi/drm/amdgpu_drm.h              |  2 ++
>   2 files changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6f0873c..974fd26 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -281,6 +281,29 @@ int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>   
>   	trace_amdgpu_bo_create(bo);

I would move the trace after the clear operation.

>   
> +	if (flags & AMDGPU_GEM_CREATE_VRAM_CLEARED &&
> +	    bo->tbo.mem.placement & TTM_PL_FLAG_VRAM) {
> +		struct fence *fence;
> +
> +		/* make sure module init routine finishes since it requires gpu_scheduler */
> +		if (!adev->accel_working) {
> +			DRM_INFO("amdgpu: acceleration disabled, skipping clear op\n");
> +			return 0;

We should return -EBUSY here when that doesn't work instead of simply 
ignoring the request.

> +		}
> +
> +		r = amdgpu_bo_reserve(bo, false);
> +		if (unlikely(r != 0)) {
> +			DRM_INFO("amdgpu: reserve bo fail, skipping clear op\n");
> +			return 0;

Same here. When something goes wrong userspace should be signaled by 
this and not the functionality just dropped.

Userspace can still make the request without the flag if that is a 
critical code path.

> +		}

As discussed with David, you need to validate the buffer here before 
call amdgpu_fill_buffer() to make sure that it is actually in GPU 
accessible memory.

Regards,
Christian.

> +		amdgpu_fill_buffer(bo, 0, bo->tbo.resv, &fence);
> +		amdgpu_bo_fence(bo, fence, false);
> +		amdgpu_bo_unreserve(bo);
> +		fence_put(bo->tbo.moving);
> +		bo->tbo.moving = fence_get(fence);
> +		fence_put(fence);
> +	}
> +
>   	return 0;
>   }
>   
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index 946f238..8df3816 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -75,6 +75,8 @@
>   #define AMDGPU_GEM_CREATE_NO_CPU_ACCESS		(1 << 1)
>   /* Flag that USWC attributes should be used for GTT */
>   #define AMDGPU_GEM_CREATE_CPU_GTT_USWC		(1 << 2)
> +/* Flag that the memory should be in VRAM and cleared */
> +#define AMDGPU_GEM_CREATE_VRAM_CLEARED		(1 << 3)
>   
>   struct drm_amdgpu_gem_create_in  {
>   	/** the requested memory size */