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 19:13:36 +0200 Message-ID: <5050C2C0.9070100@vodafone.de> References: <1347372604-26557-1-git-send-email-deathsimple@vodafone.de> <1347372604-26557-8-git-send-email-deathsimple@vodafone.de> <50507B30.2080900@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 1FA629E796 for ; Wed, 12 Sep 2012 10:13:39 -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 12.09.2012 15:54, Jerome Glisse wrote: > On Wed, Sep 12, 2012 at 8:08 AM, Christian K=F6nig > wrote: >> 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 usesp= ace >> 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 wou= ld >> also eliminate the refcounting nouveau is currently doing. >> >> Feel free to choose one, I just have implemented number 1 because that w= as >> 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=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/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_dev= ice >>>> *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 =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->soff= set) { >>>> /* 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 =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 >>>> *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 =3D=3D NU= LL)) >>>> 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, >>>> struct 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_SNOOPED); >>>> + 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/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_f= ile >>>> *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, >>>> void *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_EXIS= T; >>>> 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, >>>> args->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 >>