[PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
Christian König
christian.koenig at amd.com
Tue Dec 8 18:47:45 UTC 2020
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 at 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,
>>>>
>>
More information about the dri-devel
mailing list