Re: [PATCH 8/8] drm/radeon: rework the VM code a bit more

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

 

On 12.09.2012 15:54, Jerome Glisse wrote:

On Wed, Sep 12, 2012 at 8:08 AM, Christian König
<deathsimple@xxxxxxxxxxx> wrote:

On 11.09.2012 18:11, Jerome Glisse wrote:

On Tue, Sep 11, 2012 at 10:10 AM, Christian König
<deathsimple@xxxxxxxxxxx> wrote:

Roughly based on how nouveau is handling it. Instead of
adding the bo_va when the address is set add the bo_va
when the handle is opened, but set the address to zero
until userspace tells us where to place it.

This fixes another bunch of problems with glamor.

What exactly are the fix ? I don't see why this change is needed. It
make things more complicated in my view.

Applications can open GEM Handles multiple times, so what happens is that
(for example) the DDX creates an BO to back a pixmap, hands that over to
GLAMOR and than closes the handle again (a bit later and also not all the
times).

In the end the kernel complains that bo x isn't in vm y, what makes sense
cause the DDX has removed the mapping by closing the handle. What we don't
have in this picture is that the handle was opened two times, once for
creation and once for handing it over to GLAMOR.

I see three possible solutions to that problem:
1. Remember how many times a handle was opened in a vm context, that is what
this patch does.

2. Instead of removing the mapping on closing the handle we change usespace
to remove the mapping separately.

3. Change GEM to only call the open/close callbacks when the handle is
opened and closed for the first/last time in a process context, that would
also eliminate the refcounting nouveau is currently doing.

Feel free to choose one, I just have implemented number 1 because that was
the simplest way to go (for me).

Cheers,
Christian.

I am all ok for the refcounting part but i don't think the always add
a va to bo is a good idea. It just add more code for no good reason.
Always adding a va to a bo is only an issue on cayman, on SI and further we will need a va for each bo anyway.
The problem with not adding a va is that I just need something to actually count the number of references in.
So there isn't so much of an alternative to adding the va on opening the handle. 

Cheers,
Christian.



Cheers,
Jerome


Signed-off-by: Christian König <deathsimple@xxxxxxxxxxx>
---
   drivers/gpu/drm/radeon/radeon.h      |   30 ++++---
   drivers/gpu/drm/radeon/radeon_gart.c |  147
++++++++++++++++++++++------------
   drivers/gpu/drm/radeon/radeon_gem.c  |   47 +++++++++--
   3 files changed, 153 insertions(+), 71 deletions(-)

diff --git a/drivers/gpu/drm/radeon/radeon.h
b/drivers/gpu/drm/radeon/radeon.h
index 8cca1d2..4d67f0f 100644
--- a/drivers/gpu/drm/radeon/radeon.h
+++ b/drivers/gpu/drm/radeon/radeon.h
@@ -292,17 +292,20 @@ struct radeon_mman {

   /* bo virtual address in a specific vm */
   struct radeon_bo_va {
-       /* bo list is protected by bo being reserved */
+       /* protected by bo being reserved */
          struct list_head                bo_list;
-       /* vm list is protected by vm mutex */
-       struct list_head                vm_list;
-       /* constant after initialization */
-       struct radeon_vm                *vm;
-       struct radeon_bo                *bo;
          uint64_t                        soffset;
          uint64_t                        eoffset;
          uint32_t                        flags;
          bool                            valid;
+       unsigned                        ref_count;

unsigned refcount is a recipe for failure, if somehow you over unref
then you va will stay alive forever and this overunref will probably
go unnoticed.


+
+       /* protected by vm mutex */
+       struct list_head                vm_list;
+
+       /* constant after initialization */
+       struct radeon_vm                *vm;
+       struct radeon_bo                *bo;
   };

   struct radeon_bo {
@@ -1848,14 +1851,15 @@ void radeon_vm_bo_invalidate(struct radeon_device
*rdev,
                               struct radeon_bo *bo);
   struct radeon_bo_va *radeon_vm_bo_find(struct radeon_vm *vm,
                                         struct radeon_bo *bo);
-int radeon_vm_bo_add(struct radeon_device *rdev,
-                    struct radeon_vm *vm,
-                    struct radeon_bo *bo,
-                    uint64_t offset,
-                    uint32_t flags);
+struct radeon_bo_va *radeon_vm_bo_add(struct radeon_device *rdev,
+                                     struct radeon_vm *vm,
+                                     struct radeon_bo *bo);
+int radeon_vm_bo_set_addr(struct radeon_device *rdev,
+                         struct radeon_bo_va *bo_va,
+                         uint64_t offset,
+                         uint32_t flags);
   int radeon_vm_bo_rmv(struct radeon_device *rdev,
-                    struct radeon_vm *vm,
-                    struct radeon_bo *bo);
+                    struct radeon_bo_va *bo_va);

   /* audio */
   void r600_audio_update_hdmi(struct work_struct *work);
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c
b/drivers/gpu/drm/radeon/radeon_gart.c
index 2c59491..2f28ff3 100644
--- a/drivers/gpu/drm/radeon/radeon_gart.c
+++ b/drivers/gpu/drm/radeon/radeon_gart.c
@@ -693,51 +693,83 @@ struct radeon_bo_va *radeon_vm_bo_find(struct
radeon_vm *vm,
    * @rdev: radeon_device pointer
    * @vm: requested vm
    * @bo: radeon buffer object
- * @offset: requested offset of the buffer in the VM address space
- * @flags: attributes of pages (read/write/valid/etc.)
    *
    * Add @bo into the requested vm (cayman+).
- * Add @bo to the list of bos associated with the vm and validate
- * the offset requested within the vm address space.
- * Returns 0 for success, error for failure.
+ * Add @bo to the list of bos associated with the vm
+ * Returns newly added bo_va or NULL for failure
    *
    * Object has to be reserved!
    */
-int radeon_vm_bo_add(struct radeon_device *rdev,
-                    struct radeon_vm *vm,
-                    struct radeon_bo *bo,
-                    uint64_t offset,
-                    uint32_t flags)
+struct radeon_bo_va *radeon_vm_bo_add(struct radeon_device *rdev,
+                                     struct radeon_vm *vm,
+                                     struct radeon_bo *bo)
   {
-       struct radeon_bo_va *bo_va, *tmp;
-       struct list_head *head;
-       uint64_t size = radeon_bo_size(bo), last_offset = 0;
-       unsigned last_pfn;
+       struct radeon_bo_va *bo_va;

          bo_va = kzalloc(sizeof(struct radeon_bo_va), GFP_KERNEL);
          if (bo_va == NULL) {
-               return -ENOMEM;
+               return NULL;
          }
          bo_va->vm = vm;
          bo_va->bo = bo;
-       bo_va->soffset = offset;
-       bo_va->eoffset = offset + size;
-       bo_va->flags = flags;
+       bo_va->soffset = 0;
+       bo_va->eoffset = 0;
+       bo_va->flags = 0;
          bo_va->valid = false;
+       bo_va->ref_count = 1;
          INIT_LIST_HEAD(&bo_va->bo_list);
          INIT_LIST_HEAD(&bo_va->vm_list);
-       /* make sure object fit at this offset */
-       if (bo_va->soffset >= bo_va->eoffset) {
-               kfree(bo_va);
-               return -EINVAL;
-       }

-       last_pfn = bo_va->eoffset / RADEON_GPU_PAGE_SIZE;
-       if (last_pfn > rdev->vm_manager.max_pfn) {
-               kfree(bo_va);
-               dev_err(rdev->dev, "va above limit (0x%08X > 0x%08X)\n",
-                       last_pfn, rdev->vm_manager.max_pfn);
-               return -EINVAL;
+       mutex_lock(&vm->mutex);
+       list_add(&bo_va->vm_list, &vm->va);
+       list_add_tail(&bo_va->bo_list, &bo->va);
+       mutex_unlock(&vm->mutex);
+
+       return bo_va;
+}
+
+/**
+ * radeon_vm_bo_set_addr - set bos virtual address inside a vm
+ *
+ * @rdev: radeon_device pointer
+ * @bo_va: bo_va to store the address
+ * @soffset: requested offset of the buffer in the VM address space
+ * @flags: attributes of pages (read/write/valid/etc.)
+ *
+ * Set offset of @bo_va (cayman+).
+ * Validate and set the offset requested within the vm address space.
+ * Returns 0 for success, error for failure.
+ *
+ * Object has to be reserved!
+ */
+int radeon_vm_bo_set_addr(struct radeon_device *rdev,
+                         struct radeon_bo_va *bo_va,
+                         uint64_t soffset,
+                         uint32_t flags)
+{
+       uint64_t size = radeon_bo_size(bo_va->bo);
+       uint64_t eoffset, last_offset = 0;
+       struct radeon_vm *vm = bo_va->vm;
+       struct radeon_bo_va *tmp;
+       struct list_head *head;
+       unsigned last_pfn;
+
+       if (soffset) {
+               /* make sure object fit at this offset */
+               eoffset = soffset + size;
+               if (soffset >= eoffset) {
+                       return -EINVAL;
+               }
+
+               last_pfn = eoffset / RADEON_GPU_PAGE_SIZE;
+               if (last_pfn > rdev->vm_manager.max_pfn) {
+                       dev_err(rdev->dev, "va above limit (0x%08X >
0x%08X)\n",
+                               last_pfn, rdev->vm_manager.max_pfn);
+                       return -EINVAL;
+               }
+
+       } else {
+               eoffset = last_pfn = 0;
          }

          mutex_lock(&vm->mutex);
@@ -758,24 +790,33 @@ int radeon_vm_bo_add(struct radeon_device *rdev,
          head = &vm->va;
          last_offset = 0;
          list_for_each_entry(tmp, &vm->va, vm_list) {
-               if (bo_va->soffset >= last_offset && bo_va->eoffset <=
tmp->soffset) {
+               if (bo_va == tmp) {
+                       /* skip over currently modified bo */
+                       continue;
+               }
+
+               if (soffset >= last_offset && eoffset <= tmp->soffset) {
                          /* bo can be added before this one */
                          break;
                  }
-               if (bo_va->eoffset > tmp->soffset && bo_va->soffset <
tmp->eoffset) {
+               if (eoffset > tmp->soffset && soffset < tmp->eoffset) {
                          /* bo and tmp overlap, invalid offset */
                          dev_err(rdev->dev, "bo %p va 0x%08X conflict
with (bo %p 0x%08X 0x%08X)\n",
-                               bo, (unsigned)bo_va->soffset, tmp->bo,
+                               bo_va->bo, (unsigned)bo_va->soffset,
tmp->bo,
                                  (unsigned)tmp->soffset,
(unsigned)tmp->eoffset);
-                       kfree(bo_va);
                          mutex_unlock(&vm->mutex);
                          return -EINVAL;
                  }
                  last_offset = tmp->eoffset;
                  head = &tmp->vm_list;
          }
-       list_add(&bo_va->vm_list, head);
-       list_add_tail(&bo_va->bo_list, &bo->va);
+
+       bo_va->soffset = soffset;
+       bo_va->eoffset = eoffset;
+       bo_va->flags = flags;
+       bo_va->valid = false;
+       list_move(&bo_va->vm_list, head);
+
          mutex_unlock(&vm->mutex);
          return 0;
   }
@@ -855,6 +896,12 @@ int radeon_vm_bo_update_pte(struct radeon_device
*rdev,
                  return -EINVAL;
          }

+       if (!bo_va->soffset) {
+               dev_err(rdev->dev, "bo %p don't has a mapping in vm
%p\n",
+                       bo, vm);
+               return -EINVAL;
+       }
+
          if ((bo_va->valid && mem) || (!bo_va->valid && mem == NULL))
                  return 0;

@@ -921,33 +968,26 @@ int radeon_vm_bo_update_pte(struct radeon_device
*rdev,
    * radeon_vm_bo_rmv - remove a bo to a specific vm
    *
    * @rdev: radeon_device pointer
- * @vm: requested vm
- * @bo: radeon buffer object
+ * @bo_va: requested bo_va
    *
- * Remove @bo from the requested vm (cayman+).
- * Remove @bo from the list of bos associated with the vm and
- * remove the ptes for @bo in the page table.
+ * Remove @bo_va->bo from the requested vm (cayman+).
+ * Remove @bo_va->bo from the list of bos associated with the bo_va->vm
and
+ * remove the ptes for @bo_va in the page table.
    * Returns 0 for success.
    *
    * Object have to be reserved!
    */
   int radeon_vm_bo_rmv(struct radeon_device *rdev,
-                    struct radeon_vm *vm,
-                    struct radeon_bo *bo)
+                    struct radeon_bo_va *bo_va)
   {
-       struct radeon_bo_va *bo_va;
          int r;

-       bo_va = radeon_vm_bo_find(vm, bo);
-       if (bo_va == NULL)
-               return 0;
-
          mutex_lock(&rdev->vm_manager.lock);
-       mutex_lock(&vm->mutex);
-       r = radeon_vm_bo_update_pte(rdev, vm, bo, NULL);
+       mutex_lock(&bo_va->vm->mutex);
+       r = radeon_vm_bo_update_pte(rdev, bo_va->vm, bo_va->bo, NULL);
          mutex_unlock(&rdev->vm_manager.lock);
          list_del(&bo_va->vm_list);
-       mutex_unlock(&vm->mutex);
+       mutex_unlock(&bo_va->vm->mutex);
          list_del(&bo_va->bo_list);

          kfree(bo_va);
@@ -987,6 +1027,7 @@ void radeon_vm_bo_invalidate(struct radeon_device
*rdev,
    */
   int radeon_vm_init(struct radeon_device *rdev, struct radeon_vm *vm)
   {
+       struct radeon_bo_va *bo_va;
          int r;

          vm->id = 0;
@@ -1006,8 +1047,10 @@ int radeon_vm_init(struct radeon_device *rdev,
struct radeon_vm *vm)
          /* map the ib pool buffer at 0 in virtual address space, set
           * read only
           */
-       r = radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo,
RADEON_VA_IB_OFFSET,
-                            RADEON_VM_PAGE_READABLE |
RADEON_VM_PAGE_SNOOPED);
+       bo_va = radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo);
+       r = radeon_vm_bo_set_addr(rdev, bo_va, RADEON_VA_IB_OFFSET,
+                                 RADEON_VM_PAGE_READABLE |
+                                 RADEON_VM_PAGE_SNOOPED);
          return r;
   }

diff --git a/drivers/gpu/drm/radeon/radeon_gem.c
b/drivers/gpu/drm/radeon/radeon_gem.c
index cfad667..6579bef 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -124,6 +124,30 @@ void radeon_gem_fini(struct radeon_device *rdev)
    */
   int radeon_gem_object_open(struct drm_gem_object *obj, struct drm_file
*file_priv)
   {
+       struct radeon_bo *rbo = gem_to_radeon_bo(obj);
+       struct radeon_device *rdev = rbo->rdev;
+       struct radeon_fpriv *fpriv = file_priv->driver_priv;
+       struct radeon_vm *vm = &fpriv->vm;
+       struct radeon_bo_va *bo_va;
+       int r;
+
+       if (rdev->family < CHIP_CAYMAN) {
+               return 0;
+       }
+
+       r = radeon_bo_reserve(rbo, false);
+       if (r) {
+               return r;
+       }
+
+       bo_va = radeon_vm_bo_find(vm, rbo);
+       if (!bo_va) {
+               bo_va = radeon_vm_bo_add(rdev, vm, rbo);
+       } else {
+               ++bo_va->ref_count;
+       }
+       radeon_bo_unreserve(rbo);
+
          return 0;
   }

@@ -134,6 +158,7 @@ void radeon_gem_object_close(struct drm_gem_object
*obj,
          struct radeon_device *rdev = rbo->rdev;
          struct radeon_fpriv *fpriv = file_priv->driver_priv;
          struct radeon_vm *vm = &fpriv->vm;
+       struct radeon_bo_va *bo_va;
          int r;

          if (rdev->family < CHIP_CAYMAN) {
@@ -146,7 +171,12 @@ void radeon_gem_object_close(struct drm_gem_object
*obj,
                          "we fail to reserve bo (%d)\n", r);
                  return;
          }
-       radeon_vm_bo_rmv(rdev, vm, rbo);
+       bo_va = radeon_vm_bo_find(vm, rbo);
+       if (bo_va) {
+               if (--bo_va->ref_count == 0) {
+                       radeon_vm_bo_rmv(rdev, bo_va);
+               }
+       }
          radeon_bo_unreserve(rbo);
   }

@@ -462,19 +492,24 @@ int radeon_gem_va_ioctl(struct drm_device *dev,
void *data,
                  drm_gem_object_unreference_unlocked(gobj);
                  return r;
          }
+       bo_va = radeon_vm_bo_find(&fpriv->vm, rbo);
+       if (!bo_va) {
+               args->operation = RADEON_VA_RESULT_ERROR;
+               drm_gem_object_unreference_unlocked(gobj);
+               return -ENOENT;
+       }
+
          switch (args->operation) {
          case RADEON_VA_MAP:
-               bo_va = radeon_vm_bo_find(&fpriv->vm, rbo);
-               if (bo_va) {
+               if (bo_va->soffset) {
                          args->operation = RADEON_VA_RESULT_VA_EXIST;
                          args->offset = bo_va->soffset;
                          goto out;
                  }
-               r = radeon_vm_bo_add(rdev, &fpriv->vm, rbo,
-                                    args->offset, args->flags);
+               r = radeon_vm_bo_set_addr(rdev, bo_va, args->offset,
args->flags);
                  break;
          case RADEON_VA_UNMAP:
-               r = radeon_vm_bo_rmv(rdev, &fpriv->vm, rbo);
+               r = radeon_vm_bo_set_addr(rdev, bo_va, 0, 0);
                  break;
          default:
                  break;
--
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel




_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://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