Am 08.12.20 um 20:11 schrieb Andrey Grodzovsky:
On 12/8/20 2:01 PM, Christian König wrote:
Am 08.12.20 um 19:52 schrieb Andrey Grodzovsky:
On 12/8/20 1:47 PM, Christian König wrote:
Am 08.12.20 um 19:44 schrieb Andrey Grodzovsky:
On 12/8/20 1:29 PM, Christian König wrote:
Am 08.12.20 um 19:26 schrieb Andrey Grodzovsky: > > On 12/8/20 12:36 PM, Christian König wrote: >> Am 08.12.20 um 18:10 schrieb Andrey Grodzovsky: >>> For BOs imported from outside of amdgpu, setting of >>> amdgpu_gem_object_funcs >>> was missing in amdgpu_dma_buf_create_obj. Fix by refactoring >>> BO creation >>> and amdgpu_gem_object_funcs setting into single function called >>> from both code paths. >> >> Can you outline why we can't use amdgpu_gem_object_create() >> directly? >> >> I mean we have a bit of extra error handling in there and we >> need to grab the resv lock and set the domains after creation, >> but that shouldn't matter and I don't see why that should not >> work. > > > On top of what you mentioned you also have > bp.domain/bp.preferred_domain being set differently so you need > to add another > argument to amdgpu_gem_object_create to reflect this difference > which clutters even more the already cluttered argument list.
That should be outside of the call to amdgpu_gem_object_create(), similar to how it is outside of the amdgpu_bo_create currently.
So you agree we have to add another argument to amdgpu_gem_object_create (e.g. u32 preferred_domain) which will be 0 for amdgpu_dma_buf_create_obj and equal to initial_domain for all the code path currently calling amdgpu_gem_object_create ?
No, I just don't see the need for that. We need to overwrite the preferred domain after the function call anyway.
DMA-buf imports are created with the initial domain CPU and then get that overwritten to GTT after creation.
> Regarding the extra error handling - you have the 'retry' dance > in amdgpu_gem_object_create which jumps back to the middle of > amdgpu_bo_param > initialization but you don't have it in > amdgpu_dma_buf_create_obj which also complicates the reuse of > amdgpu_gem_object_create as is.
Regarding the extra error handling, that kicks in only when AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED is specified as flags or AMDGPU_GEM_DOMAIN_VRAM as initial domain. Neither is the case here.
Yes, still, it makes me a bit uncomfortable relying on internal implementation details of an API function I call to do the thing I expect.
Yeah, ok that is a rather good argument.
Christian.
So should I just keep it as is or you think it's still would be more beneficial to unify them the way you propose ?
Maybe we should move the error handling into amdgpu_gem_create_ioctl() anyway.
We don't really want that handling in the userptr stuff and for the call from amdgpufb_create_pinned_object() that is actually a bug!
E.g. for the fb emulation we can't fall back from VRAM to GTT like in the create ioctl.
What about amdgpu_mode_dumb_create, seems like there GTT domain is also relevant and so the error handling would be needed there too.
Nope, same thing as with the fb emulation:
domain = amdgpu_bo_get_preferred_pin_domain(adev, amdgpu_display_supported_domains(adev, flags));
This BO is created for scanout, falling back to GTT because we don't have enough memory or clearing the CPU access flag is most likely a bug here.
Christian.
Andrey
Christian.
Andrey
Andrey
Christian.
> > Andrey > > >> >> Thanks, >> Christian. >> >>> >>> This fixes null ptr regression casued by commit >>> d693def drm: Remove obsolete GEM and PRIME callbacks from >>> struct drm_driver >>> >>> Signed-off-by: Andrey Grodzovsky andrey.grodzovsky@amd.com >>> --- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 13 ++++++------- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 22 >>> +++++++++++++++++----- >>> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h | 5 +++++ >>> 3 files changed, 28 insertions(+), 12 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> index e5919ef..da4d0ab 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> @@ -405,6 +405,7 @@ struct dma_buf >>> *amdgpu_gem_prime_export(struct drm_gem_object *gobj, >>> return buf; >>> } >>> + >>> /** >>> * amdgpu_dma_buf_create_obj - create BO for DMA-buf import >>> * >>> @@ -424,7 +425,7 @@ amdgpu_dma_buf_create_obj(struct >>> drm_device *dev, struct dma_buf *dma_buf) >>> struct amdgpu_device *adev = drm_to_adev(dev); >>> struct amdgpu_bo *bo; >>> struct amdgpu_bo_param bp; >>> - int ret; >>> + struct drm_gem_object *obj; >>> memset(&bp, 0, sizeof(bp)); >>> bp.size = dma_buf->size; >>> @@ -434,21 +435,19 @@ amdgpu_dma_buf_create_obj(struct >>> drm_device *dev, struct dma_buf *dma_buf) >>> bp.type = ttm_bo_type_sg; >>> bp.resv = resv; >>> dma_resv_lock(resv, NULL); >>> - ret = amdgpu_bo_create(adev, &bp, &bo); >>> - if (ret) >>> + obj = amdgpu_gem_object_create_raw(adev, &bp); >>> + if (IS_ERR(obj)) >>> goto error; >>> + bo = gem_to_amdgpu_bo(obj); >>> bo->allowed_domains = AMDGPU_GEM_DOMAIN_GTT; >>> bo->preferred_domains = AMDGPU_GEM_DOMAIN_GTT; >>> if (dma_buf->ops != &amdgpu_dmabuf_ops) >>> bo->prime_shared_count = 1; >>> - dma_resv_unlock(resv); >>> - return &bo->tbo.base; >>> - >>> error: >>> dma_resv_unlock(resv); >>> - return ERR_PTR(ret); >>> + return obj; >>> } >>> /** >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> index c9f94fb..5f22ce6 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c >>> @@ -52,13 +52,26 @@ static void amdgpu_gem_object_free(struct >>> drm_gem_object *gobj) >>> } >>> } >>> +struct drm_gem_object *amdgpu_gem_object_create_raw(struct >>> amdgpu_device *adev, >>> + struct amdgpu_bo_param *bp) >>> +{ >>> + struct amdgpu_bo *bo; >>> + int r; >>> + >>> + r = amdgpu_bo_create(adev, bp, &bo); >>> + if (r) >>> + return ERR_PTR(r); >>> + >>> + bo->tbo.base.funcs = &amdgpu_gem_object_funcs; >>> + return &bo->tbo.base; >>> +} >>> + >>> int amdgpu_gem_object_create(struct amdgpu_device *adev, >>> unsigned long size, >>> int alignment, u32 initial_domain, >>> u64 flags, enum ttm_bo_type type, >>> struct dma_resv *resv, >>> struct drm_gem_object **obj) >>> { >>> - struct amdgpu_bo *bo; >>> struct amdgpu_bo_param bp; >>> int r; >>> @@ -73,8 +86,9 @@ int amdgpu_gem_object_create(struct >>> amdgpu_device *adev, unsigned long size, >>> retry: >>> bp.flags = flags; >>> bp.domain = initial_domain; >>> - r = amdgpu_bo_create(adev, &bp, &bo); >>> - if (r) { >>> + *obj = amdgpu_gem_object_create_raw(adev, &bp); >>> + if (IS_ERR(*obj)) { >>> + r = PTR_ERR(*obj); >>> if (r != -ERESTARTSYS) { >>> if (flags & >>> AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED) { >>> flags &= >>> ~AMDGPU_GEM_CREATE_CPU_ACCESS_REQUIRED; >>> @@ -90,8 +104,6 @@ int amdgpu_gem_object_create(struct >>> amdgpu_device *adev, unsigned long size, >>> } >>> return r; >>> } >>> - *obj = &bo->tbo.base; >>> - (*obj)->funcs = &amdgpu_gem_object_funcs; >>> return 0; >>> } >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h >>> index 637bf51..a6b90d3 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.h >>> @@ -38,12 +38,17 @@ unsigned long amdgpu_gem_timeout(uint64_t >>> timeout_ns); >>> /* >>> * GEM objects. >>> */ >>> + >>> +struct amdgpu_bo_param; >>> + >>> void amdgpu_gem_force_release(struct amdgpu_device *adev); >>> int amdgpu_gem_object_create(struct amdgpu_device *adev, >>> unsigned long size, >>> int alignment, u32 initial_domain, >>> u64 flags, enum ttm_bo_type type, >>> struct dma_resv *resv, >>> struct drm_gem_object **obj); >>> +struct drm_gem_object *amdgpu_gem_object_create_raw(struct >>> amdgpu_device *adev, >>> + struct amdgpu_bo_param *bp); >>> int amdgpu_mode_dumb_create(struct drm_file *file_priv, >>> struct drm_device *dev, >>
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx