| Message ID | 20240604160503.43359-7-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/6] drm/amdgpu: cleanup MES command submission | expand |
I was waiting for some replies elsewhere on this thread. Anwyay.. for the below, because I don't understand how come an important fix like this is not garnering more attention: On 04/06/2024 17:05, Christian König wrote: > From: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Since you pretty much changed my logic you could also "demote" me to Reported-by:. In which case I could theoretically even provide an r-b. :) > > Currently the driver appears to be thinking that it will be attempting to > re-validate the evicted buffers on the next submission if they are not in > their preferred placement. > > That however appears not to be true for the very common case of buffers > with allowed placements of VRAM+GTT. Simply because the check can only > detect if the current placement is *none* of the preferred ones, happily > leaving VRAM+GTT buffers in the GTT placement "forever". > > Fix it by extending the VRAM+GTT special case to the re-validation logic. > > v2: re-work the criteria to consider if something is in it's preferred > placement or not and also disable the handling on APUs. > > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 4e2391c83d7c..1a470dafa93d 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1242,15 +1242,15 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va, > return r; > } > > - /* If the BO is not in its preferred location add it back to > - * the evicted list so that it gets validated again on the > - * next command submission. > - */ > if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv) { > - uint32_t mem_type = bo->tbo.resource->mem_type; > - > - if (!(bo->preferred_domains & > - amdgpu_mem_type_to_domain(mem_type))) > + /* > + * If the preferred location is VRAM but we placed it into GTT > + * add it back to the evicted list so that it gets validated > + * again on the next command submission. > + */ > + if (!(adev->flags & AMD_IS_APU) && > + bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM && > + bo->tbo.resource->mem_type != TTM_PL_VRAM) I think this logic works to fix discrete and APU apart that I don't like the special casing things so much. Which is perhaps a consequence of too many dynamic games with placements. :( That said, my version also had a little bit of special casing so perhaps the battle for clean design has been lost. So ack from me for this version. Not sure it is good to merge it though without also fixing the migration throttling and that is a more difficult one (the part where I was awaiting a reply). Regards, Tvrtko > amdgpu_vm_bo_evicted(&bo_va->base); > else > amdgpu_vm_bo_idle(&bo_va->base);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index 4e2391c83d7c..1a470dafa93d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -1242,15 +1242,15 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, struct amdgpu_bo_va *bo_va, return r; } - /* If the BO is not in its preferred location add it back to - * the evicted list so that it gets validated again on the - * next command submission. - */ if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv) { - uint32_t mem_type = bo->tbo.resource->mem_type; - - if (!(bo->preferred_domains & - amdgpu_mem_type_to_domain(mem_type))) + /* + * If the preferred location is VRAM but we placed it into GTT + * add it back to the evicted list so that it gets validated + * again on the next command submission. + */ + if (!(adev->flags & AMD_IS_APU) && + bo->preferred_domains & AMDGPU_GEM_DOMAIN_VRAM && + bo->tbo.resource->mem_type != TTM_PL_VRAM) amdgpu_vm_bo_evicted(&bo_va->base); else amdgpu_vm_bo_idle(&bo_va->base);