* [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 17:10 ` Andrey Grodzovsky
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 17:10 UTC (permalink / raw)
To: dri-devel
Cc: Alexander.Deucher, ckoenig.leichtzumerken, tzimmermann, amd-gfx,
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.
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,
--
2.7.4
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 17:10 ` Andrey Grodzovsky
@ 2020-12-08 17:23 ` Alex Deucher
-1 siblings, 0 replies; 22+ messages in thread
From: Alex Deucher @ 2020-12-08 17:23 UTC (permalink / raw)
To: Andrey Grodzovsky
Cc: Deucher, Alexander, Christian König, amd-gfx list,
Thomas Zimmermann, Maling list - DRI developers
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 17:23 ` Alex Deucher
0 siblings, 0 replies; 22+ messages in thread
From: Alex Deucher @ 2020-12-08 17:23 UTC (permalink / raw)
To: Andrey Grodzovsky
Cc: Deucher, Alexander, Christian König, amd-gfx list,
Thomas Zimmermann, Maling list - DRI developers
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 17:10 ` Andrey Grodzovsky
@ 2020-12-08 17:36 ` Christian König
-1 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 17:36 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 17:36 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 17:36 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 17:36 ` Christian König
@ 2020-12-08 18:26 ` Andrey Grodzovsky
-1 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:26 UTC (permalink / raw)
To: christian.koenig, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 18:26 ` Andrey Grodzovsky
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:26 UTC (permalink / raw)
To: christian.koenig, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 18:26 ` Andrey Grodzovsky
@ 2020-12-08 18:29 ` Christian König
-1 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 18:29 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 18:29 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 18:29 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 18:29 ` Christian König
@ 2020-12-08 18:44 ` Andrey Grodzovsky
-1 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:44 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 18:44 ` Andrey Grodzovsky
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:44 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 18:44 ` Andrey Grodzovsky
@ 2020-12-08 18:47 ` Christian König
-1 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 18:47 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 18:47 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 18:47 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 18:47 ` Christian König
@ 2020-12-08 18:52 ` Andrey Grodzovsky
-1 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:52 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 18:52 ` Andrey Grodzovsky
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 18:52 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 18:52 ` Andrey Grodzovsky
@ 2020-12-08 19:01 ` Christian König
-1 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 19:01 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>>
>>>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 19:01 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 19:01 UTC (permalink / raw)
To: Andrey Grodzovsky, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>>
>>>>
>>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 19:01 ` Christian König
@ 2020-12-08 19:11 ` Andrey Grodzovsky
-1 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 19:11 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>>>
>>>>>
>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 19:11 ` Andrey Grodzovsky
0 siblings, 0 replies; 22+ messages in thread
From: Andrey Grodzovsky @ 2020-12-08 19:11 UTC (permalink / raw)
To: Christian König, dri-devel; +Cc: Alexander.Deucher, tzimmermann, amd-gfx
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,
>>>>>>>
>>>>>
>>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
2020-12-08 19:11 ` Andrey Grodzovsky
@ 2020-12-08 19:16 ` Christian König
-1 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 19:16 UTC (permalink / raw)
To: Andrey Grodzovsky, Christian König, dri-devel
Cc: Alexander.Deucher, amd-gfx, tzimmermann
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
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs
@ 2020-12-08 19:16 ` Christian König
0 siblings, 0 replies; 22+ messages in thread
From: Christian König @ 2020-12-08 19:16 UTC (permalink / raw)
To: Andrey Grodzovsky, Christian König, dri-devel
Cc: Alexander.Deucher, amd-gfx, tzimmermann
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
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 22+ messages in thread