The end offset is exclusive not inclusive.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index d7bd46b..614e31a 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -732,7 +732,7 @@ 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->soffset >= last_offset && bo_va->eoffset <= tmp->soffset) { /* bo can be added before this one */ break; }
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 614e31a..5694421 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -736,7 +736,7 @@ int radeon_vm_bo_add(struct radeon_device *rdev, /* bo can be added before this one */ break; } - if (bo_va->soffset >= tmp->soffset && bo_va->soffset < tmp->eoffset) { + if (bo_va->eoffset > tmp->soffset && bo_va->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,
On Tue, Sep 11, 2012 at 10:09 AM, Christian König deathsimple@vodafone.de wrote:
Signed-off-by: Christian König deathsimple@vodafone.de
Reviewed-by: Jerome Glisse jglisse@redhat.com
drivers/gpu/drm/radeon/radeon_gart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 614e31a..5694421 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -736,7 +736,7 @@ int radeon_vm_bo_add(struct radeon_device *rdev, /* bo can be added before this one */ break; }
if (bo_va->soffset >= tmp->soffset && bo_va->soffset < tmp->eoffset) {
if (bo_va->eoffset > tmp->soffset && bo_va->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,-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Even GPUs can have a null pointer dereference, so move the IB pool to another offset to catch those.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_gart.c | 2 +- drivers/gpu/drm/radeon/radeon_ring.c | 6 +++--- 3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index d48bd30..55f17f9 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -123,6 +123,7 @@ extern int radeon_lockup_timeout; #define CAYMAN_RING_TYPE_CP2_INDEX 2
/* hardcode those limit for now */ +#define RADEON_VA_IB_OFFSET (1 << 20) #define RADEON_VA_RESERVED_SIZE (8 << 20) #define RADEON_IB_VM_MAX_SIZE (64 << 10)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 5694421..1b1c001 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -980,7 +980,7 @@ 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, 0, + r = radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo, RADEON_VA_IB_OFFSET, RADEON_VM_PAGE_READABLE | RADEON_VM_PAGE_SNOOPED); return r; } diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 993cf71..d90b0bc 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -79,10 +79,10 @@ int radeon_ib_get(struct radeon_device *rdev, int ring, ib->ptr = radeon_sa_bo_cpu_addr(ib->sa_bo); ib->vm = vm; if (vm) { - /* ib pool is bind at 0 in virtual address space, - * so gpu_addr is the offset inside the pool bo + /* ib pool is bound at RADEON_VA_IB_OFFSET in virtual address + * space and soffset is the offset inside the pool bo */ - ib->gpu_addr = ib->sa_bo->soffset; + ib->gpu_addr = ib->sa_bo->soffset + RADEON_VA_IB_OFFSET; } else { ib->gpu_addr = radeon_sa_bo_gpu_addr(ib->sa_bo); }
On Tue, Sep 11, 2012 at 10:09 AM, Christian König deathsimple@vodafone.de wrote:
Even GPUs can have a null pointer dereference, so move the IB pool to another offset to catch those.
Reviewed-by: Jerome Glisse jglisse@redhat.com
Signed-off-by: Christian König deathsimple@vodafone.de
drivers/gpu/drm/radeon/radeon.h | 1 + drivers/gpu/drm/radeon/radeon_gart.c | 2 +- drivers/gpu/drm/radeon/radeon_ring.c | 6 +++--- 3 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index d48bd30..55f17f9 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -123,6 +123,7 @@ extern int radeon_lockup_timeout; #define CAYMAN_RING_TYPE_CP2_INDEX 2
/* hardcode those limit for now */ +#define RADEON_VA_IB_OFFSET (1 << 20) #define RADEON_VA_RESERVED_SIZE (8 << 20) #define RADEON_IB_VM_MAX_SIZE (64 << 10)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 5694421..1b1c001 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -980,7 +980,7 @@ 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, 0,
r = radeon_vm_bo_add(rdev, vm, rdev->ring_tmp_bo.bo, RADEON_VA_IB_OFFSET, RADEON_VM_PAGE_READABLE | RADEON_VM_PAGE_SNOOPED); return r;} diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 993cf71..d90b0bc 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -79,10 +79,10 @@ int radeon_ib_get(struct radeon_device *rdev, int ring, ib->ptr = radeon_sa_bo_cpu_addr(ib->sa_bo); ib->vm = vm; if (vm) {
/* ib pool is bind at 0 in virtual address space,* so gpu_addr is the offset inside the pool bo
/* ib pool is bound at RADEON_VA_IB_OFFSET in virtual address* space and soffset is the offset inside the pool bo */
ib->gpu_addr = ib->sa_bo->soffset;
ib->gpu_addr = ib->sa_bo->soffset + RADEON_VA_IB_OFFSET; } else { ib->gpu_addr = radeon_sa_bo_gpu_addr(ib->sa_bo); }-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
It doesn't really belong into the object functions, also rename it to avoid collisions with struct radeon_bo_va.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon.h | 2 ++ drivers/gpu/drm/radeon/radeon_gart.c | 34 ++++++++++++++++++++++++++++---- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_object.c | 13 ------------ drivers/gpu/drm/radeon/radeon_object.h | 2 -- 5 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 55f17f9..8cca1d2 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -1846,6 +1846,8 @@ int radeon_vm_bo_update_pte(struct radeon_device *rdev, struct ttm_mem_reg *mem); 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, diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 1b1c001..2c59491 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -662,7 +662,31 @@ void radeon_vm_fence(struct radeon_device *rdev, vm->fence = radeon_fence_ref(fence); }
-/* object have to be reserved */ +/** + * radeon_vm_bo_find - find the bo_va for a specific vm & bo + * + * @vm: requested vm + * @bo: requested buffer object + * + * Find @bo inside the requested vm (cayman+). + * Search inside the @bos vm list for the requested vm + * Returns the found bo_va or NULL if none is found + * + * Object has to be reserved! + */ +struct radeon_bo_va *radeon_vm_bo_find(struct radeon_vm *vm, + struct radeon_bo *bo) +{ + struct radeon_bo_va *bo_va; + + list_for_each_entry(bo_va, &bo->va, bo_list) { + if (bo_va->vm == vm) { + return bo_va; + } + } + return NULL; +} + /** * radeon_vm_bo_add - add a bo to a specific vm * @@ -676,6 +700,8 @@ void radeon_vm_fence(struct radeon_device *rdev, * 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. + * + * Object has to be reserved! */ int radeon_vm_bo_add(struct radeon_device *rdev, struct radeon_vm *vm, @@ -823,7 +849,7 @@ int radeon_vm_bo_update_pte(struct radeon_device *rdev, if (vm->sa_bo == NULL) return 0;
- bo_va = radeon_bo_va(bo, vm); + bo_va = radeon_vm_bo_find(vm, bo); if (bo_va == NULL) { dev_err(rdev->dev, "bo %p not in vm %p\n", bo, vm); return -EINVAL; @@ -912,7 +938,7 @@ int radeon_vm_bo_rmv(struct radeon_device *rdev, struct radeon_bo_va *bo_va; int r;
- bo_va = radeon_bo_va(bo, vm); + bo_va = radeon_vm_bo_find(vm, bo); if (bo_va == NULL) return 0;
@@ -1009,7 +1035,7 @@ void radeon_vm_fini(struct radeon_device *rdev, struct radeon_vm *vm) */ r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false); if (!r) { - bo_va = radeon_bo_va(rdev->ring_tmp_bo.bo, vm); + bo_va = radeon_vm_bo_find(vm, rdev->ring_tmp_bo.bo); list_del_init(&bo_va->bo_list); list_del_init(&bo_va->vm_list); radeon_bo_unreserve(rdev->ring_tmp_bo.bo); diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 1b57b00..6cac5cc 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -461,7 +461,7 @@ int radeon_gem_va_ioctl(struct drm_device *dev, void *data, } switch (args->operation) { case RADEON_VA_MAP: - bo_va = radeon_bo_va(rbo, &fpriv->vm); + bo_va = radeon_vm_bo_find(&fpriv->vm, rbo); if (bo_va) { args->operation = RADEON_VA_RESULT_VA_EXIST; args->offset = bo_va->soffset; diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 9024e72..2844e0b 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -646,16 +646,3 @@ int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait) } return 0; } - -/* object have to be reserved */ -struct radeon_bo_va *radeon_bo_va(struct radeon_bo *rbo, struct radeon_vm *vm) -{ - struct radeon_bo_va *bo_va; - - list_for_each_entry(bo_va, &rbo->va, bo_list) { - if (bo_va->vm == vm) { - return bo_va; - } - } - return NULL; -} diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 17fb99f..2aaf6e3 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -141,8 +141,6 @@ extern void radeon_bo_move_notify(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem); extern int radeon_bo_fault_reserve_notify(struct ttm_buffer_object *bo); extern int radeon_bo_get_surface_reg(struct radeon_bo *bo); -extern struct radeon_bo_va *radeon_bo_va(struct radeon_bo *rbo, - struct radeon_vm *vm);
/* * sub allocation
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@vodafone.de wrote:
It doesn't really belong into the object functions, also rename it to avoid collisions with struct radeon_bo_va.
Signed-off-by: Christian König deathsimple@vodafone.de
Reviewed-by: Jerome Glisse jglisse@redhat.com
drivers/gpu/drm/radeon/radeon.h | 2 ++ drivers/gpu/drm/radeon/radeon_gart.c | 34 ++++++++++++++++++++++++++++---- drivers/gpu/drm/radeon/radeon_gem.c | 2 +- drivers/gpu/drm/radeon/radeon_object.c | 13 ------------ drivers/gpu/drm/radeon/radeon_object.h | 2 -- 5 files changed, 33 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 55f17f9..8cca1d2 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -1846,6 +1846,8 @@ int radeon_vm_bo_update_pte(struct radeon_device *rdev, struct ttm_mem_reg *mem); 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, diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index 1b1c001..2c59491 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -662,7 +662,31 @@ void radeon_vm_fence(struct radeon_device *rdev, vm->fence = radeon_fence_ref(fence); }
-/* object have to be reserved */ +/**
- radeon_vm_bo_find - find the bo_va for a specific vm & bo
- @vm: requested vm
- @bo: requested buffer object
- Find @bo inside the requested vm (cayman+).
- Search inside the @bos vm list for the requested vm
- Returns the found bo_va or NULL if none is found
- Object has to be reserved!
- */
+struct radeon_bo_va *radeon_vm_bo_find(struct radeon_vm *vm,
struct radeon_bo *bo)+{
struct radeon_bo_va *bo_va;list_for_each_entry(bo_va, &bo->va, bo_list) {if (bo_va->vm == vm) {return bo_va;}}return NULL;+}
/**
- radeon_vm_bo_add - add a bo to a specific vm
@@ -676,6 +700,8 @@ void radeon_vm_fence(struct radeon_device *rdev,
- 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.
*/
- Object has to be reserved!
int radeon_vm_bo_add(struct radeon_device *rdev, struct radeon_vm *vm, @@ -823,7 +849,7 @@ int radeon_vm_bo_update_pte(struct radeon_device *rdev, if (vm->sa_bo == NULL) return 0;
bo_va = radeon_bo_va(bo, vm);
bo_va = radeon_vm_bo_find(vm, bo); if (bo_va == NULL) { dev_err(rdev->dev, "bo %p not in vm %p\n", bo, vm); return -EINVAL;@@ -912,7 +938,7 @@ int radeon_vm_bo_rmv(struct radeon_device *rdev, struct radeon_bo_va *bo_va; int r;
bo_va = radeon_bo_va(bo, vm);
bo_va = radeon_vm_bo_find(vm, bo); if (bo_va == NULL) return 0;@@ -1009,7 +1035,7 @@ void radeon_vm_fini(struct radeon_device *rdev, struct radeon_vm *vm) */ r = radeon_bo_reserve(rdev->ring_tmp_bo.bo, false); if (!r) {
bo_va = radeon_bo_va(rdev->ring_tmp_bo.bo, vm);
bo_va = radeon_vm_bo_find(vm, rdev->ring_tmp_bo.bo); list_del_init(&bo_va->bo_list); list_del_init(&bo_va->vm_list); radeon_bo_unreserve(rdev->ring_tmp_bo.bo);diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 1b57b00..6cac5cc 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -461,7 +461,7 @@ int radeon_gem_va_ioctl(struct drm_device *dev, void *data, } switch (args->operation) { case RADEON_VA_MAP:
bo_va = radeon_bo_va(rbo, &fpriv->vm);
bo_va = radeon_vm_bo_find(&fpriv->vm, rbo); if (bo_va) { args->operation = RADEON_VA_RESULT_VA_EXIST; args->offset = bo_va->soffset;diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 9024e72..2844e0b 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -646,16 +646,3 @@ int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait) } return 0; }
-/* object have to be reserved */ -struct radeon_bo_va *radeon_bo_va(struct radeon_bo *rbo, struct radeon_vm *vm) -{
struct radeon_bo_va *bo_va;list_for_each_entry(bo_va, &rbo->va, bo_list) {if (bo_va->vm == vm) {return bo_va;}}return NULL;-} diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 17fb99f..2aaf6e3 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -141,8 +141,6 @@ extern void radeon_bo_move_notify(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem); extern int radeon_bo_fault_reserve_notify(struct ttm_buffer_object *bo); extern int radeon_bo_get_surface_reg(struct radeon_bo *bo); -extern struct radeon_bo_va *radeon_bo_va(struct radeon_bo *rbo,
struct radeon_vm *vm);/*
- sub allocation
-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
The no_wait param isn't used anywhere, and actually isn't very usefull at all.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_object.c | 7 +++---- drivers/gpu/drm/radeon/radeon_object.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 2844e0b..8d23b7e 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -627,18 +627,17 @@ int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type, bool no_wait) /** * radeon_bo_reserve - reserve bo * @bo: bo structure - * @no_wait: don't sleep while trying to reserve (return -EBUSY) + * @no_intr: don't return -ERESTARTSYS on pending signal * * Returns: - * -EBUSY: buffer is busy and @no_wait is true * -ERESTARTSYS: A wait for the buffer to become unreserved was interrupted by * a signal. Release all buffer reservations and return to user-space. */ -int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait) +int radeon_bo_reserve(struct radeon_bo *bo, bool no_intr) { int r;
- r = ttm_bo_reserve(&bo->tbo, true, no_wait, false, 0); + r = ttm_bo_reserve(&bo->tbo, !no_intr, false, false, 0); if (unlikely(r != 0)) { if (r != -ERESTARTSYS) dev_err(bo->rdev->dev, "%p reserve failed\n", bo); diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 2aaf6e3..93cd491 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -52,7 +52,7 @@ static inline unsigned radeon_mem_type_to_domain(u32 mem_type) return 0; }
-int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait); +int radeon_bo_reserve(struct radeon_bo *bo, bool no_intr);
static inline void radeon_bo_unreserve(struct radeon_bo *bo) {
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@vodafone.de wrote:
The no_wait param isn't used anywhere, and actually isn't very usefull at all.
Signed-off-by: Christian König deathsimple@vodafone.de
Reviewed-by: Jerome Glisse
drivers/gpu/drm/radeon/radeon_object.c | 7 +++---- drivers/gpu/drm/radeon/radeon_object.h | 2 +- 2 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 2844e0b..8d23b7e 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -627,18 +627,17 @@ int radeon_bo_wait(struct radeon_bo *bo, u32 *mem_type, bool no_wait) /**
- radeon_bo_reserve - reserve bo
- @bo: bo structure
- @no_wait: don't sleep while trying to reserve (return -EBUSY)
- @no_intr: don't return -ERESTARTSYS on pending signal
- Returns:
*/
- -EBUSY: buffer is busy and @no_wait is true
- -ERESTARTSYS: A wait for the buffer to become unreserved was interrupted by
- a signal. Release all buffer reservations and return to user-space.
-int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait) +int radeon_bo_reserve(struct radeon_bo *bo, bool no_intr) { int r;
r = ttm_bo_reserve(&bo->tbo, true, no_wait, false, 0);
r = ttm_bo_reserve(&bo->tbo, !no_intr, false, false, 0); if (unlikely(r != 0)) { if (r != -ERESTARTSYS) dev_err(bo->rdev->dev, "%p reserve failed\n", bo);diff --git a/drivers/gpu/drm/radeon/radeon_object.h b/drivers/gpu/drm/radeon/radeon_object.h index 2aaf6e3..93cd491 100644 --- a/drivers/gpu/drm/radeon/radeon_object.h +++ b/drivers/gpu/drm/radeon/radeon_object.h @@ -52,7 +52,7 @@ static inline unsigned radeon_mem_type_to_domain(u32 mem_type) return 0; }
-int radeon_bo_reserve(struct radeon_bo *bo, bool no_wait); +int radeon_bo_reserve(struct radeon_bo *bo, bool no_intr);
static inline void radeon_bo_unreserve(struct radeon_bo *bo) { -- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
Make the reserve non interruptible.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_gem.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 6cac5cc..cfad667 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -134,13 +134,16 @@ 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; + int r;
if (rdev->family < CHIP_CAYMAN) { return; }
- if (radeon_bo_reserve(rbo, false)) { - dev_err(rdev->dev, "leaking bo va because we fail to reserve bo\n"); + r = radeon_bo_reserve(rbo, true); + if (r) { + dev_err(rdev->dev, "leaking bo va because " + "we fail to reserve bo (%d)\n", r); return; } radeon_vm_bo_rmv(rdev, vm, rbo);
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@vodafone.de wrote:
Make the reserve non interruptible.
Signed-off-by: Christian König deathsimple@vodafone.de
Reviewed-by: Jerome Glisse
drivers/gpu/drm/radeon/radeon_gem.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c b/drivers/gpu/drm/radeon/radeon_gem.c index 6cac5cc..cfad667 100644 --- a/drivers/gpu/drm/radeon/radeon_gem.c +++ b/drivers/gpu/drm/radeon/radeon_gem.c @@ -134,13 +134,16 @@ 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;
int r; if (rdev->family < CHIP_CAYMAN) { return; }
if (radeon_bo_reserve(rbo, false)) {dev_err(rdev->dev, "leaking bo va because we fail to reserve bo\n");
r = radeon_bo_reserve(rbo, true);if (r) {dev_err(rdev->dev, "leaking bo va because ""we fail to reserve bo (%d)\n", r); return; } radeon_vm_bo_rmv(rdev, vm, rbo);-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
It is unnecessary when we remove the va in drm_close.
Signed-off-by: Christian König deathsimple@vodafone.de --- drivers/gpu/drm/radeon/radeon_object.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 8d23b7e..d210fe5 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -46,16 +46,6 @@ static void radeon_bo_clear_surface_reg(struct radeon_bo *bo); * function are calling it. */
-void radeon_bo_clear_va(struct radeon_bo *bo) -{ - struct radeon_bo_va *bo_va, *tmp; - - list_for_each_entry_safe(bo_va, tmp, &bo->va, bo_list) { - /* remove from all vm address space */ - radeon_vm_bo_rmv(bo->rdev, bo_va->vm, bo); - } -} - static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct radeon_bo *bo; @@ -65,7 +55,6 @@ static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_clear_surface_reg(bo); - radeon_bo_clear_va(bo); drm_gem_object_release(&bo->gem_base); kfree(bo); }
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@vodafone.de wrote:
It is unnecessary when we remove the va in drm_close.
Signed-off-by: Christian König deathsimple@vodafone.de
NAK there is case for which drm_close is not call like ib pool and other iirc. This clear va is really a safety net.
drivers/gpu/drm/radeon/radeon_object.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 8d23b7e..d210fe5 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -46,16 +46,6 @@ static void radeon_bo_clear_surface_reg(struct radeon_bo *bo);
- function are calling it.
*/
-void radeon_bo_clear_va(struct radeon_bo *bo) -{
struct radeon_bo_va *bo_va, *tmp;list_for_each_entry_safe(bo_va, tmp, &bo->va, bo_list) {/* remove from all vm address space */radeon_vm_bo_rmv(bo->rdev, bo_va->vm, bo);}-}
static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct radeon_bo *bo; @@ -65,7 +55,6 @@ static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_clear_surface_reg(bo);
radeon_bo_clear_va(bo); drm_gem_object_release(&bo->gem_base); kfree(bo);}
1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 11.09.2012 18:12, Jerome Glisse wrote:
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@vodafone.de wrote:
It is unnecessary when we remove the va in drm_close.
Signed-off-by: Christian König deathsimple@vodafone.de
NAK there is case for which drm_close is not call like ib pool and other iirc. This clear va is really a safety net.
Ah, ok that makes sense. Sorry I was just a bit confused about that.
Christian.
drivers/gpu/drm/radeon/radeon_object.c | 11 ----------- 1 file changed, 11 deletions(-)
diff --git a/drivers/gpu/drm/radeon/radeon_object.c b/drivers/gpu/drm/radeon/radeon_object.c index 8d23b7e..d210fe5 100644 --- a/drivers/gpu/drm/radeon/radeon_object.c +++ b/drivers/gpu/drm/radeon/radeon_object.c @@ -46,16 +46,6 @@ static void radeon_bo_clear_surface_reg(struct radeon_bo *bo);
- function are calling it.
*/
-void radeon_bo_clear_va(struct radeon_bo *bo) -{
struct radeon_bo_va *bo_va, *tmp;list_for_each_entry_safe(bo_va, tmp, &bo->va, bo_list) {/* remove from all vm address space */radeon_vm_bo_rmv(bo->rdev, bo_va->vm, bo);}-}
- static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) { struct radeon_bo *bo;
@@ -65,7 +55,6 @@ static void radeon_ttm_bo_destroy(struct ttm_buffer_object *tbo) list_del_init(&bo->list); mutex_unlock(&bo->rdev->gem.mutex); radeon_bo_clear_surface_reg(bo);
radeon_bo_clear_va(bo); drm_gem_object_release(&bo->gem_base); kfree(bo);-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
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.
Signed-off-by: Christian König deathsimple@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; + + /* 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;
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@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.
Signed-off-by: Christian König deathsimple@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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 11.09.2012 18:11, Jerome Glisse wrote:
On Tue, Sep 11, 2012 at 10:10 AM, Christian König deathsimple@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@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);
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
*/ int radeon_vm_bo_rmv(struct radeon_device *rdev,
- remove the ptes for @bo_va in the page table.
- Returns 0 for success.
- Object have to be reserved!
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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 12, 2012 at 8:08 AM, Christian König deathsimple@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@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:
- Remember how many times a handle was opened in a vm context, that is what
this patch does.
- Instead of removing the mapping on closing the handle we change usespace
to remove the mapping separately.
- 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@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);
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 conflictwith (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
*/ int radeon_vm_bo_rmv(struct radeon_device *rdev,
- remove the ptes for @bo_va in the page table.
- Returns 0 for success.
- Object have to be reserved!
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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12.09.2012 15:54, Jerome Glisse wrote:
On Wed, Sep 12, 2012 at 8:08 AM, Christian König deathsimple@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@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:
- Remember how many times a handle was opened in a vm context, that is what
this patch does.
- Instead of removing the mapping on closing the handle we change usespace
to remove the mapping separately.
- 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@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);
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 conflictwith (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
int radeon_vm_bo_rmv(struct radeon_device *rdev,
- remove the ptes for @bo_va in the page table.
*/
- Returns 0 for success.
- Object have to be reserved!
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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Wed, Sep 12, 2012 at 1:13 PM, Christian König deathsimple@vodafone.de wrote:
On 12.09.2012 15:54, Jerome Glisse wrote:
On Wed, Sep 12, 2012 at 8:08 AM, Christian König deathsimple@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@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:
- Remember how many times a handle was opened in a vm context, that is
what this patch does.
- Instead of removing the mapping on closing the handle we change
usespace to remove the mapping separately.
- 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.
Ok fair enough
Cheers, Jerome
Cheers, Jerome
Signed-off-by: Christian König deathsimple@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);
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 conflictwith (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
int radeon_vm_bo_rmv(struct radeon_device *rdev,
- remove the ptes for @bo_va in the page table.
*/
- Returns 0 for success.
- Object have to be reserved!
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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 12.09.2012 15:54, Jerome Glisse wrote:
On Wed, Sep 12, 2012 at 8:08 AM, Christian König deathsimple@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@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:
- Remember how many times a handle was opened in a vm context, that is what
this patch does.
- Instead of removing the mapping on closing the handle we change usespace
to remove the mapping separately.
- 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@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);
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 conflictwith (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
int radeon_vm_bo_rmv(struct radeon_device *rdev,
- remove the ptes for @bo_va in the page table.
*/
- Returns 0 for success.
- Object have to be reserved!
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@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Tue, Sep 11, 2012 at 10:09 AM, Christian König deathsimple@vodafone.de wrote:
The end offset is exclusive not inclusive.
Signed-off-by: Christian König deathsimple@vodafone.de
Reviewed-by: Jerome Glisse jglisse@redhat.com
drivers/gpu/drm/radeon/radeon_gart.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gart.c b/drivers/gpu/drm/radeon/radeon_gart.c index d7bd46b..614e31a 100644 --- a/drivers/gpu/drm/radeon/radeon_gart.c +++ b/drivers/gpu/drm/radeon/radeon_gart.c @@ -732,7 +732,7 @@ 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->soffset >= last_offset && bo_va->eoffset <= tmp->soffset) { /* bo can be added before this one */ break; }-- 1.7.9.5
dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel
dri-devel@lists.freedesktop.org
-
Christian König -
Jerome Glisse