From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> To: "Christian König" <christian.koenig@amd.com>, dri-devel@lists.freedesktop.org Cc: Alexander.Deucher@amd.com, tzimmermann@suse.de, amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs Date: Tue, 8 Dec 2020 13:44:42 -0500 [thread overview] Message-ID: <3ac4b80b-5eeb-fd80-c458-98539c075424@amd.com> (raw) In-Reply-To: <e1cd7521-a9ac-0830-8be9-c03838c151bc@amd.com> 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, >>> > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
WARNING: multiple messages have this Message-ID (diff)
From: Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> To: "Christian König" <christian.koenig@amd.com>, dri-devel@lists.freedesktop.org Cc: Alexander.Deucher@amd.com, tzimmermann@suse.de, amd-gfx@lists.freedesktop.org Subject: Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs Date: Tue, 8 Dec 2020 13:44:42 -0500 [thread overview] Message-ID: <3ac4b80b-5eeb-fd80-c458-98539c075424@amd.com> (raw) In-Reply-To: <e1cd7521-a9ac-0830-8be9-c03838c151bc@amd.com> 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, >>> > _______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
next prev parent reply other threads:[~2020-12-08 18:44 UTC|newest] Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-12-08 17:10 [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs Andrey Grodzovsky 2020-12-08 17:10 ` Andrey Grodzovsky 2020-12-08 17:23 ` Alex Deucher 2020-12-08 17:23 ` Alex Deucher 2020-12-08 17:36 ` Christian König 2020-12-08 17:36 ` Christian König 2020-12-08 18:26 ` Andrey Grodzovsky 2020-12-08 18:26 ` Andrey Grodzovsky 2020-12-08 18:29 ` Christian König 2020-12-08 18:29 ` Christian König 2020-12-08 18:44 ` Andrey Grodzovsky [this message] 2020-12-08 18:44 ` Andrey Grodzovsky 2020-12-08 18:47 ` Christian König 2020-12-08 18:47 ` Christian König 2020-12-08 18:52 ` Andrey Grodzovsky 2020-12-08 18:52 ` Andrey Grodzovsky 2020-12-08 19:01 ` Christian König 2020-12-08 19:01 ` Christian König 2020-12-08 19:11 ` Andrey Grodzovsky 2020-12-08 19:11 ` Andrey Grodzovsky 2020-12-08 19:16 ` Christian König 2020-12-08 19:16 ` Christian König
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=3ac4b80b-5eeb-fd80-c458-98539c075424@amd.com \ --to=andrey.grodzovsky@amd.com \ --cc=Alexander.Deucher@amd.com \ --cc=amd-gfx@lists.freedesktop.org \ --cc=christian.koenig@amd.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.