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.
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,