[PATCH] drm/amdgpu: use 256 bit buffers for all wb allocations

[Christian König]

Am 28.07.2017 um 18:15 schrieb Alex Deucher:
> May waste a bit of memory, but simplifies the interface
> significantly.
>
> Signed-off-by: Alex Deucher <alexander.deucher at amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu.h        |  4 --
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 65 ------------------------------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c   | 65 ++++++++----------------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c     |  8 ++--
>   4 files changed, 20 insertions(+), 122 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> index ed1b688..c880851 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
> @@ -1190,10 +1190,6 @@ struct amdgpu_wb {
>   
>   int amdgpu_wb_get(struct amdgpu_device *adev, u32 *wb);
>   void amdgpu_wb_free(struct amdgpu_device *adev, u32 wb);
> -int amdgpu_wb_get_64bit(struct amdgpu_device *adev, u32 *wb);
> -int amdgpu_wb_get_256bit(struct amdgpu_device *adev, u32 *wb);
> -void amdgpu_wb_free_64bit(struct amdgpu_device *adev, u32 wb);
> -void amdgpu_wb_free_256bit(struct amdgpu_device *adev, u32 wb);
>   
>   void amdgpu_get_pcie_info(struct amdgpu_device *adev);
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index c2ddeb1..d422277 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -570,41 +570,6 @@ static int amdgpu_wb_init(struct amdgpu_device *adev)
>    */
>   int amdgpu_wb_get(struct amdgpu_device *adev, u32 *wb)
>   {
> -	unsigned long offset = find_first_zero_bit(adev->wb.used, adev->wb.num_wb);
> -	if (offset < adev->wb.num_wb) {
> -		__set_bit(offset, adev->wb.used);
> -		*wb = offset;
> -		return 0;
> -	} else {
> -		return -EINVAL;
> -	}
> -}
> -
> -/**
> - * amdgpu_wb_get_64bit - Allocate a wb entry
> - *
> - * @adev: amdgpu_device pointer
> - * @wb: wb index
> - *
> - * Allocate a wb slot for use by the driver (all asics).
> - * Returns 0 on success or -EINVAL on failure.
> - */
> -int amdgpu_wb_get_64bit(struct amdgpu_device *adev, u32 *wb)
> -{
> -	unsigned long offset = bitmap_find_next_zero_area_off(adev->wb.used,
> -				adev->wb.num_wb, 0, 2, 7, 0);
> -	if ((offset + 1) < adev->wb.num_wb) {
> -		__set_bit(offset, adev->wb.used);
> -		__set_bit(offset + 1, adev->wb.used);
> -		*wb = offset;
> -		return 0;
> -	} else {
> -		return -EINVAL;
> -	}
> -}
> -
> -int amdgpu_wb_get_256bit(struct amdgpu_device *adev, u32 *wb)
> -{
>   	int i = 0;
>   	unsigned long offset = bitmap_find_next_zero_area_off(adev->wb.used,
>   				adev->wb.num_wb, 0, 8, 63, 0);

Could be simplified to find_first_zero_bit() as well and then just 
multiply the resulting offset by 16.

Either way the patch looks good to me and is Reviewed-by: Christian 
König <christian.koenig at amd.com>.

Regards,
Christian.

> @@ -628,36 +593,6 @@ int amdgpu_wb_get_256bit(struct amdgpu_device *adev, u32 *wb)
>    */
>   void amdgpu_wb_free(struct amdgpu_device *adev, u32 wb)
>   {
> -	if (wb < adev->wb.num_wb)
> -		__clear_bit(wb, adev->wb.used);
> -}
> -
> -/**
> - * amdgpu_wb_free_64bit - Free a wb entry
> - *
> - * @adev: amdgpu_device pointer
> - * @wb: wb index
> - *
> - * Free a wb slot allocated for use by the driver (all asics)
> - */
> -void amdgpu_wb_free_64bit(struct amdgpu_device *adev, u32 wb)
> -{
> -	if ((wb + 1) < adev->wb.num_wb) {
> -		__clear_bit(wb, adev->wb.used);
> -		__clear_bit(wb + 1, adev->wb.used);
> -	}
> -}
> -
> -/**
> - * amdgpu_wb_free_256bit - Free a wb entry
> - *
> - * @adev: amdgpu_device pointer
> - * @wb: wb index
> - *
> - * Free a wb slot allocated for use by the driver (all asics)
> - */
> -void amdgpu_wb_free_256bit(struct amdgpu_device *adev, u32 wb)
> -{
>   	int i = 0;
>   
>   	if ((wb + 7) < adev->wb.num_wb)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> index 3874be8..704475674 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
> @@ -184,47 +184,22 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
>   			return r;
>   	}
>   
> -	if (ring->funcs->support_64bit_ptrs) {
> -		r = amdgpu_wb_get_64bit(adev, &ring->rptr_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring rptr_offs wb alloc failed\n", r);
> -			return r;
> -		}
> -
> -		r = amdgpu_wb_get_64bit(adev, &ring->wptr_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring wptr_offs wb alloc failed\n", r);
> -			return r;
> -		}
> -
> -	} else {
> -		r = amdgpu_wb_get(adev, &ring->rptr_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring rptr_offs wb alloc failed\n", r);
> -			return r;
> -		}
> -
> -		r = amdgpu_wb_get(adev, &ring->wptr_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring wptr_offs wb alloc failed\n", r);
> -			return r;
> -		}
> -
> +	r = amdgpu_wb_get(adev, &ring->rptr_offs);
> +	if (r) {
> +		dev_err(adev->dev, "(%d) ring rptr_offs wb alloc failed\n", r);
> +		return r;
>   	}
>   
> -	if (amdgpu_sriov_vf(adev) && ring->funcs->type == AMDGPU_RING_TYPE_GFX) {
> -		r = amdgpu_wb_get_256bit(adev, &ring->fence_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring fence_offs wb alloc failed\n", r);
> -			return r;
> -		}
> +	r = amdgpu_wb_get(adev, &ring->wptr_offs);
> +	if (r) {
> +		dev_err(adev->dev, "(%d) ring wptr_offs wb alloc failed\n", r);
> +		return r;
> +	}
>   
> -	} else {
> -		r = amdgpu_wb_get(adev, &ring->fence_offs);
> -		if (r) {
> -			dev_err(adev->dev, "(%d) ring fence_offs wb alloc failed\n", r);
> -			return r;
> -		}
> +	r = amdgpu_wb_get(adev, &ring->fence_offs);
> +	if (r) {
> +		dev_err(adev->dev, "(%d) ring fence_offs wb alloc failed\n", r);
> +		return r;
>   	}
>   
>   	r = amdgpu_wb_get(adev, &ring->cond_exe_offs);
> @@ -286,19 +261,11 @@ void amdgpu_ring_fini(struct amdgpu_ring *ring)
>   {
>   	ring->ready = false;
>   
> -	if (ring->funcs->support_64bit_ptrs) {
> -		amdgpu_wb_free_64bit(ring->adev, ring->rptr_offs);
> -		amdgpu_wb_free_64bit(ring->adev, ring->wptr_offs);
> -	} else {
> -		amdgpu_wb_free(ring->adev, ring->rptr_offs);
> -		amdgpu_wb_free(ring->adev, ring->wptr_offs);
> -	}
> +	amdgpu_wb_free(ring->adev, ring->rptr_offs);
> +	amdgpu_wb_free(ring->adev, ring->wptr_offs);
>   
>   	amdgpu_wb_free(ring->adev, ring->cond_exe_offs);
> -	if (amdgpu_sriov_vf(ring->adev) && ring->funcs->type == AMDGPU_RING_TYPE_GFX)
> -		amdgpu_wb_free_256bit(ring->adev, ring->fence_offs);
> -	else
> -		amdgpu_wb_free(ring->adev, ring->fence_offs);
> +	amdgpu_wb_free(ring->adev, ring->fence_offs);
>   
>   	amdgpu_bo_free_kernel(&ring->ring_obj,
>   			      &ring->gpu_addr,
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> index 5c24708..8273e63 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_0.c
> @@ -1278,8 +1278,8 @@ static int sdma_v4_0_sw_init(void *handle)
>   				     AMDGPU_SDMA_IRQ_TRAP1);
>   
>   		if (amdgpu_sriov_vf(adev)) {
> -			r = amdgpu_wb_get_64bit(adev,
> -						&adev->sdma.instance[i].poll_mem_offs);
> +			r = amdgpu_wb_get(adev,
> +					  &adev->sdma.instance[i].poll_mem_offs);
>   			if (r) {
>   				dev_err(adev->dev, "(%d) failed to allocate SDMA poll mem wb.\n", r);
>   				return r;
> @@ -1301,8 +1301,8 @@ static int sdma_v4_0_sw_fini(void *handle)
>   		amdgpu_ring_fini(&adev->sdma.instance[i].ring);
>   
>   		if (amdgpu_sriov_vf(adev))
> -			amdgpu_wb_free_64bit(adev,
> -					     adev->sdma.instance[i].poll_mem_offs);
> +			amdgpu_wb_free(adev,
> +				       adev->sdma.instance[i].poll_mem_offs);
>   	}
>   	return 0;
>   }