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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxx>
---
  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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux