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@xxxxxxxxxxxxxxxxxxxxx; Quan, Evan <Evan.Quan@xxxxxxx>;
> Deucher, Alexander <Alexander.Deucher@xxxxxxx>; Zeng, Oak
> <Oak.Zeng@xxxxxxx>; Koenig, Christian <Christian.Koenig@xxxxxxx>
> 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@xxxxxxx>
> > ---
> > 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;
> >
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
