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

Christian König deathsimple at vodafone.de
Wed Sep 12 10:13:36 PDT 2012


On 12.09.2012 15:54, Jerome Glisse wrote:
> On Wed, Sep 12, 2012 at 8:08 AM, Christian König
> <deathsimple at vodafone.de> wrote:
>> 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.
> 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 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