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

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