From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 8/8] drm/radeon: rework the VM code a bit more Date: Wed, 12 Sep 2012 14:08:16 +0200 Message-ID: <50507B30.2080900@vodafone.de> References: <1347372604-26557-1-git-send-email-deathsimple@vodafone.de> <1347372604-26557-8-git-send-email-deathsimple@vodafone.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 8EE0A9F79B for ; Wed, 12 Sep 2012 05:08:21 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 11.09.2012 18:11, Jerome Glisse wrote: > On Tue, Sep 11, 2012 at 10:10 AM, Christian K=F6nig > 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=F6nig >> --- >> 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/ra= deon.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_devic= e *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/rade= on/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 rade= on_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 =3D radeon_bo_size(bo), last_offset =3D 0; >> - unsigned last_pfn; >> + struct radeon_bo_va *bo_va; >> >> bo_va =3D kzalloc(sizeof(struct radeon_bo_va), GFP_KERNEL); >> if (bo_va =3D=3D NULL) { >> - return -ENOMEM; >> + return NULL; >> } >> bo_va->vm =3D vm; >> bo_va->bo =3D bo; >> - bo_va->soffset =3D offset; >> - bo_va->eoffset =3D offset + size; >> - bo_va->flags =3D flags; >> + bo_va->soffset =3D 0; >> + bo_va->eoffset =3D 0; >> + bo_va->flags =3D 0; >> bo_va->valid =3D false; >> + bo_va->ref_count =3D 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 >=3D bo_va->eoffset) { >> - kfree(bo_va); >> - return -EINVAL; >> - } >> >> - last_pfn =3D 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 =3D radeon_bo_size(bo_va->bo); >> + uint64_t eoffset, last_offset =3D 0; >> + struct radeon_vm *vm =3D 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 =3D soffset + size; >> + if (soffset >=3D eoffset) { >> + return -EINVAL; >> + } >> + >> + last_pfn =3D 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 =3D last_pfn =3D 0; >> } >> >> mutex_lock(&vm->mutex); >> @@ -758,24 +790,33 @@ int radeon_vm_bo_add(struct radeon_device *rdev, >> head =3D &vm->va; >> last_offset =3D 0; >> list_for_each_entry(tmp, &vm->va, vm_list) { >> - if (bo_va->soffset >=3D last_offset && bo_va->eoffset <= =3D tmp->soffset) { >> + if (bo_va =3D=3D tmp) { >> + /* skip over currently modified bo */ >> + continue; >> + } >> + >> + if (soffset >=3D last_offset && eoffset <=3D tmp->soffse= t) { >> /* bo can be added before this one */ >> break; >> } >> - if (bo_va->eoffset > tmp->soffset && bo_va->soffset < tm= p->eoffset) { >> + if (eoffset > tmp->soffset && soffset < tmp->eoffset) { >> /* bo and tmp overlap, invalid offset */ >> dev_err(rdev->dev, "bo %p va 0x%08X conflict wi= th (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 =3D tmp->eoffset; >> head =3D &tmp->vm_list; >> } >> - list_add(&bo_va->vm_list, head); >> - list_add_tail(&bo_va->bo_list, &bo->va); >> + >> + bo_va->soffset =3D soffset; >> + bo_va->eoffset =3D eoffset; >> + bo_va->flags =3D flags; >> + bo_va->valid =3D 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 *r= dev, >> 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 =3D=3D 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 =3D radeon_vm_bo_find(vm, bo); >> - if (bo_va =3D=3D NULL) >> - return 0; >> - >> mutex_lock(&rdev->vm_manager.lock); >> - mutex_lock(&vm->mutex); >> - r =3D radeon_vm_bo_update_pte(rdev, vm, bo, NULL); >> + mutex_lock(&bo_va->vm->mutex); >> + r =3D 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 =3D 0; >> @@ -1006,8 +1047,10 @@ int radeon_vm_init(struct radeon_device *rdev, st= ruct radeon_vm *vm) >> /* map the ib pool buffer at 0 in virtual address space, set >> * read only >> */ >> - r =3D radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo, RADEON_VA= _IB_OFFSET, >> - RADEON_VM_PAGE_READABLE | RADEON_VM_PAGE_SN= OOPED); >> + bo_va =3D radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo); >> + r =3D 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/radeo= n/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 =3D gem_to_radeon_bo(obj); >> + struct radeon_device *rdev =3D rbo->rdev; >> + struct radeon_fpriv *fpriv =3D file_priv->driver_priv; >> + struct radeon_vm *vm =3D &fpriv->vm; >> + struct radeon_bo_va *bo_va; >> + int r; >> + >> + if (rdev->family < CHIP_CAYMAN) { >> + return 0; >> + } >> + >> + r =3D radeon_bo_reserve(rbo, false); >> + if (r) { >> + return r; >> + } >> + >> + bo_va =3D radeon_vm_bo_find(vm, rbo); >> + if (!bo_va) { >> + bo_va =3D 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 =3D rbo->rdev; >> struct radeon_fpriv *fpriv =3D file_priv->driver_priv; >> struct radeon_vm *vm =3D &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 =3D radeon_vm_bo_find(vm, rbo); >> + if (bo_va) { >> + if (--bo_va->ref_count =3D=3D 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, vo= id *data, >> drm_gem_object_unreference_unlocked(gobj); >> return r; >> } >> + bo_va =3D radeon_vm_bo_find(&fpriv->vm, rbo); >> + if (!bo_va) { >> + args->operation =3D RADEON_VA_RESULT_ERROR; >> + drm_gem_object_unreference_unlocked(gobj); >> + return -ENOENT; >> + } >> + >> switch (args->operation) { >> case RADEON_VA_MAP: >> - bo_va =3D radeon_vm_bo_find(&fpriv->vm, rbo); >> - if (bo_va) { >> + if (bo_va->soffset) { >> args->operation =3D RADEON_VA_RESULT_VA_EXIST; >> args->offset =3D bo_va->soffset; >> goto out; >> } >> - r =3D radeon_vm_bo_add(rdev, &fpriv->vm, rbo, >> - args->offset, args->flags); >> + r =3D radeon_vm_bo_set_addr(rdev, bo_va, args->offset, a= rgs->flags); >> break; >> case RADEON_VA_UNMAP: >> - r =3D radeon_vm_bo_rmv(rdev, &fpriv->vm, rbo); >> + r =3D radeon_vm_bo_set_addr(rdev, bo_va, 0, 0); >> break; >> default: >> break; >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel