| Message ID | 20240620145238.25295-2-tzimmermann@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: Convert to ttm_bo_vmap() | expand |
Am 20.06.24 um 16:44 schrieb Thomas Zimmermann: > Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both > require the caller to hold the GEM reservation lock, which is not the > case while releasing a buffer object. Hence, push a possible call to > unmap out from the buffer-object release code. Warn if a buffer object > with mapped pages is supposed to be released. Yeah, I've looked into this a while ago as well and that unfortunately won't work like this. Amdgpu also uses ttm_bo_kmap() on user allocations, so the amdgpu_bo_kunmap() in amdgpu_bo_destroy() is a must have. On the other hand I'm pretty sure that calling ttm_bo_vunmap() without holding the reservation lock is ok in this situation. After all it's guaranteed that nobody else is having a reference to the BO any more. Regards, Christian. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > index a1b7438c43dc8..d58b11ea0ead5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c > @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo) > { > struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo); > > - amdgpu_bo_kunmap(bo); > + /* > + * BO memory pages should be unmapped at this point. Call > + * amdgpu_bo_kunmap() before releasing the BO. > + */ > + if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo)) > + amdgpu_bo_kunmap(bo); > > if (bo->tbo.base.import_attach) > drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg); > @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr, > WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend); > > if (likely(amdgpu_bo_reserve(*bo, true) == 0)) { > - if (cpu_addr) > - amdgpu_bo_kunmap(*bo); > - > + amdgpu_bo_kunmap(*bo); > amdgpu_bo_unpin(*bo); > amdgpu_bo_unreserve(*bo); > }
Hi Am 20.06.24 um 17:50 schrieb Christian König: > Am 20.06.24 um 16:44 schrieb Thomas Zimmermann: >> Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both >> require the caller to hold the GEM reservation lock, which is not the >> case while releasing a buffer object. Hence, push a possible call to >> unmap out from the buffer-object release code. Warn if a buffer object >> with mapped pages is supposed to be released. > > Yeah, I've looked into this a while ago as well and that unfortunately > won't work like this. > > Amdgpu also uses ttm_bo_kmap() on user allocations, so the > amdgpu_bo_kunmap() in amdgpu_bo_destroy() is a must have. Is there a testcase (igt-gpu-tools ?) that runs this code? I've tested these patches by booting and running a 3d game under X11. But I didn't expect that to fully cover all cases. Best regards Thomas > > On the other hand I'm pretty sure that calling ttm_bo_vunmap() without > holding the reservation lock is ok in this situation. > > After all it's guaranteed that nobody else is having a reference to > the BO any more. > > Regards, > Christian. > >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++---- >> 1 file changed, 7 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> index a1b7438c43dc8..d58b11ea0ead5 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >> @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct >> ttm_buffer_object *tbo) >> { >> struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo); >> - amdgpu_bo_kunmap(bo); >> + /* >> + * BO memory pages should be unmapped at this point. Call >> + * amdgpu_bo_kunmap() before releasing the BO. >> + */ >> + if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo)) >> + amdgpu_bo_kunmap(bo); >> if (bo->tbo.base.import_attach) >> drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg); >> @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, >> u64 *gpu_addr, >> WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend); >> if (likely(amdgpu_bo_reserve(*bo, true) == 0)) { >> - if (cpu_addr) >> - amdgpu_bo_kunmap(*bo); >> - >> + amdgpu_bo_kunmap(*bo); >> amdgpu_bo_unpin(*bo); >> amdgpu_bo_unreserve(*bo); >> } >
Am 21.06.24 um 09:32 schrieb Thomas Zimmermann: > Hi > > Am 20.06.24 um 17:50 schrieb Christian König: >> Am 20.06.24 um 16:44 schrieb Thomas Zimmermann: >>> Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both >>> require the caller to hold the GEM reservation lock, which is not the >>> case while releasing a buffer object. Hence, push a possible call to >>> unmap out from the buffer-object release code. Warn if a buffer object >>> with mapped pages is supposed to be released. >> >> Yeah, I've looked into this a while ago as well and that >> unfortunately won't work like this. >> >> Amdgpu also uses ttm_bo_kmap() on user allocations, so the >> amdgpu_bo_kunmap() in amdgpu_bo_destroy() is a must have. > > Is there a testcase (igt-gpu-tools ?) that runs this code? I've > tested these patches by booting and running a 3d game under X11. But I > didn't expect that to fully cover all cases. You need a hardware generation and use case which needs patching or inspection of IBs. Video decoding on old SI or CIK hardware generation should probably do the trick. Regards, Christian. > > Best regards > Thomas > >> >> On the other hand I'm pretty sure that calling ttm_bo_vunmap() >> without holding the reservation lock is ok in this situation. >> >> After all it's guaranteed that nobody else is having a reference to >> the BO any more. >> >> Regards, >> Christian. >> >>> >>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++---- >>> 1 file changed, 7 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >>> index a1b7438c43dc8..d58b11ea0ead5 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c >>> @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct >>> ttm_buffer_object *tbo) >>> { >>> struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo); >>> - amdgpu_bo_kunmap(bo); >>> + /* >>> + * BO memory pages should be unmapped at this point. Call >>> + * amdgpu_bo_kunmap() before releasing the BO. >>> + */ >>> + if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo)) >>> + amdgpu_bo_kunmap(bo); >>> if (bo->tbo.base.import_attach) >>> drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg); >>> @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo >>> **bo, u64 *gpu_addr, >>> WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend); >>> if (likely(amdgpu_bo_reserve(*bo, true) == 0)) { >>> - if (cpu_addr) >>> - amdgpu_bo_kunmap(*bo); >>> - >>> + amdgpu_bo_kunmap(*bo); >>> amdgpu_bo_unpin(*bo); >>> amdgpu_bo_unreserve(*bo); >>> } >> >
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index a1b7438c43dc8..d58b11ea0ead5 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -58,7 +58,12 @@ static void amdgpu_bo_destroy(struct ttm_buffer_object *tbo) { struct amdgpu_bo *bo = ttm_to_amdgpu_bo(tbo); - amdgpu_bo_kunmap(bo); + /* + * BO memory pages should be unmapped at this point. Call + * amdgpu_bo_kunmap() before releasing the BO. + */ + if (drm_WARN_ON_ONCE(bo->tbo.base.dev, bo->kmap.bo)) + amdgpu_bo_kunmap(bo); if (bo->tbo.base.import_attach) drm_prime_gem_destroy(&bo->tbo.base, bo->tbo.sg); @@ -450,9 +455,7 @@ void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr, WARN_ON(amdgpu_ttm_adev((*bo)->tbo.bdev)->in_suspend); if (likely(amdgpu_bo_reserve(*bo, true) == 0)) { - if (cpu_addr) - amdgpu_bo_kunmap(*bo); - + amdgpu_bo_kunmap(*bo); amdgpu_bo_unpin(*bo); amdgpu_bo_unreserve(*bo); }
Prepares for using ttm_bo_vmap() and ttm_bo_vunmap() in amdgpu. Both require the caller to hold the GEM reservation lock, which is not the case while releasing a buffer object. Hence, push a possible call to unmap out from the buffer-object release code. Warn if a buffer object with mapped pages is supposed to be released. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> --- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)