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

[Quan, Evan]

Addressed this in the new patch which was just sent out.

Thanks,
Evan
> -----Original Message-----
> From: Kuehling, Felix
> Sent: 2018年12月14日 1:09
> To: amd-gfx at lists.freedesktop.org; Quan, Evan <Evan.Quan at amd.com>;
> Deucher, Alexander <Alexander.Deucher at amd.com>; Zeng, Oak
> <Oak.Zeng at amd.com>; Koenig, Christian <Christian.Koenig at amd.com>
> Subject: Re: [PATCH 2/4] drm/amdgpu: update the vm invalidation engine
> layout
> 
> 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;
> >