[PATCH 2/4] drm/amdgpu: update the vm invalidation engine layout

[Kuehling, Felix]

Some nit-picks inline.

On 2018-12-12 11:09 p.m., Evan Quan wrote:
> We need new invalidation engine layout due to new SDMA page
> queues added.
>
> Change-Id: I2f3861689bffb9828c9eae744a7a0de4963ac2b6
> Signed-off-by: Evan Quan <evan.quan at amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 47 ++++++++++++++-------------
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.h | 10 ++++++
>  2 files changed, 35 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index dc4dadc3575c..092a4d111557 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -719,37 +719,40 @@ static bool gmc_v9_0_keep_stolen_memory(struct amdgpu_device *adev)
>  	}
>  }
>  
> -static int gmc_v9_0_late_init(void *handle)
> +static int gmc_v9_0_allocate_vm_inv_eng(struct amdgpu_device *adev)

The function returns int. But only ever returns 0 and the caller ignores
the return value.


>  {
> -	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> -	/*
> -	 * The latest engine allocation on gfx9 is:
> -	 * Engine 0, 1: idle
> -	 * Engine 2, 3: firmware
> -	 * Engine 4~13: amdgpu ring, subject to change when ring number changes
> -	 * Engine 14~15: idle
> -	 * Engine 16: kfd tlb invalidation
> -	 * Engine 17: Gart flushes
> -	 */
> -	unsigned vm_inv_eng[AMDGPU_MAX_VMHUBS] = { 4, 4 };
> +	struct amdgpu_ring *ring;
> +	unsigned vm_inv_engs[AMDGPU_MAX_VMHUBS] =
> +		{GFXHUB_FREE_VM_INV_ENGS_BITMAP, MMHUB_FREE_VM_INV_ENGS_BITMAP};
>  	unsigned i;
> -	int r;
> +	unsigned vmhub, inv_eng;
>  
> -	if (!gmc_v9_0_keep_stolen_memory(adev))
> -		amdgpu_bo_late_init(adev);
> +	for (i = 0; i < adev->num_rings; ++i) {
> +		ring = adev->rings[i];
> +		vmhub = ring->funcs->vmhub;
> +
> +		inv_eng = ffs(vm_inv_engs[vmhub]);
> +		BUG_ON(!inv_eng);

Adding new BUG_ONs is discouraged. checkpatch.pl always warns about
these. Errors that can be handled more gracefully shouldn't use a
BUG_ON. E.g. use a WARN_ON and return an error code.


>  
> -	for(i = 0; i < adev->num_rings; ++i) {
> -		struct amdgpu_ring *ring = adev->rings[i];
> -		unsigned vmhub = ring->funcs->vmhub;
> +		ring->vm_inv_eng = inv_eng - 1;
> +		change_bit(inv_eng - 1, (unsigned long *)(&vm_inv_engs[vmhub]));
>  
> -		ring->vm_inv_eng = vm_inv_eng[vmhub]++;
>  		dev_info(adev->dev, "ring %s uses VM inv eng %u on hub %u\n",
>  			 ring->name, ring->vm_inv_eng, ring->funcs->vmhub);
>  	}
>  
> -	/* Engine 16 is used for KFD and 17 for GART flushes */
> -	for(i = 0; i < AMDGPU_MAX_VMHUBS; ++i)
> -		BUG_ON(vm_inv_eng[i] > 16);
> +	return 0;

This is the only return statement. Maybe this could be a void function.
Unless you decide to turn the BUG_ON into a WARN with an error return.


> +}
> +
> +static int gmc_v9_0_late_init(void *handle)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> +	int r;
> +
> +	if (!gmc_v9_0_keep_stolen_memory(adev))
> +		amdgpu_bo_late_init(adev);
> +
> +	gmc_v9_0_allocate_vm_inv_eng(adev);

You're ignoring the return value. Either, make the function void, or
handle potential error returns.

Regards,
  Felix


>  
>  	if (adev->asic_type == CHIP_VEGA10 && !amdgpu_sriov_vf(adev)) {
>  		r = gmc_v9_0_ecc_available(adev);
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.h b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.h
> index b030ca5ea107..5c8deac65580 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.h
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.h
> @@ -24,6 +24,16 @@
>  #ifndef __GMC_V9_0_H__
>  #define __GMC_V9_0_H__
>  
> +	/*
> +	 * The latest engine allocation on gfx9 is:
> +	 * Engine 2, 3: firmware
> +	 * Engine 0, 1, 4~16: amdgpu ring,
> +	 *                    subject to change when ring number changes
> +	 * Engine 17: Gart flushes
> +	 */
> +#define GFXHUB_FREE_VM_INV_ENGS_BITMAP		0x1FFF3
> +#define MMHUB_FREE_VM_INV_ENGS_BITMAP		0x1FFF3
> +
>  extern const struct amd_ip_funcs gmc_v9_0_ip_funcs;
>  extern const struct amdgpu_ip_block_version gmc_v9_0_ip_block;
>