| Message ID | 1607447432-28982-1-git-send-email-andrey.grodzovsky@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs | expand |
On Tue, Dec 8, 2020 at 12:10 PM Andrey Grodzovsky <andrey.grodzovsky@amd.com> wrote: > > 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. > > This fixes null ptr regression casued by commit > d693def drm: Remove obsolete GEM and PRIME callbacks from struct drm_driver > Fixes: d693def4fd1c ("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; > } > > + Unrelated whitespace change. > /** > * 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) Maybe call this amdgpu_gem_object_do_create() for consistency with amdgpu_object.c and other areas of the code. > +{ > + 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, > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
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. 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,
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. 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. 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, >
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. > 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. 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, >>
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 ? > >> 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. 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, >>> >
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. > > 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, >>>> >>
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 ? 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, >>>>> >>> >
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, >>>>>> >>>> >>
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. 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, >>>>>>> >>>>> >>> >
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
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,
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. 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(-)