Re: [PATCH] drm/amdgpu: Initialise drm_gem_object_funcs for imported BOs

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

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.grodzov...@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