[PATCH 8/8] drm/radeon: rework the VM code a bit more
Christian König
deathsimple at vodafone.de
Wed Sep 12 05:08:16 PDT 2012
On 11.09.2012 18:11, Jerome Glisse wrote:
> On Tue, Sep 11, 2012 at 10:10 AM, Christian König
> <deathsimple at vodafone.de> 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.
>
>> Signed-off-by: Christian König <deathsimple at vodafone.de>
>> ---
>> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
More information about the dri-devel
mailing list