[PATCH 1/8] drm/radeon: fix VA range check

[PATCH 1/8] drm/radeon: fix VA range check

[Christian König]

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;
 		}
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/8] drm/radeon: fix VA overlap check

[Christian König]

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,
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/8] drm/radeon: fix VA overlap check

[Jerome Glisse]

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

[PATCH 3/8] drm/radeon: move IB pool to 1MB offset

[Christian König]

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);
 	}
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/8] drm/radeon: move IB pool to 1MB offset

[Jerome Glisse]

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

[PATCH 4/8] drm/radeon: move and rename radeon_bo_va function

[Christian König]

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
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/8] drm/radeon: move and rename radeon_bo_va function

[Jerome Glisse]

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

[PATCH 5/8] drm/radeon: let bo_reserve take no_intr instead of no_wait param

[Christian König]

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)
 {
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 5/8] drm/radeon: let bo_reserve take no_intr instead of no_wait param

[Jerome Glisse]

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

[PATCH 6/8] drm/radeon: fix gem_close_object handling

[Christian König]

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);
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 6/8] drm/radeon: fix gem_close_object handling

[Jerome Glisse]

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

[PATCH 7/8] drm/radeon: remove radeon_bo_clear_va

[Christian König]

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);
 }
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 7/8] drm/radeon: remove radeon_bo_clear_va

[Jerome Glisse]

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

Re: [PATCH 7/8] drm/radeon: remove radeon_bo_clear_va

[Christian König]

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

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

[Christian König]

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;
-- 
1.7.9.5

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jerome Glisse]

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

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

[Christian König]

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);
>>   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

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

[Jerome Glisse]

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:
> 1. Remember how many times a handle was opened in a vm context, that is what
> this patch does.
>
> 2. Instead of removing the mapping on closing the handle we change usespace
> to remove the mapping separately.
>
> 3. Change GEM to only call the open/close callbacks when the handle is
> opened and closed for the first/last time in a process context, that would
> also eliminate the refcounting nouveau is currently doing.
>
> Feel free to choose one, I just have implemented number 1 because that was
> the simplest way to go (for me).
>
> Cheers,
> Christian.

I am all ok for the refcounting part but i don't think the always add
a va to bo is a good idea. It just add more code for no good reason.

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);
>>>   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
>
>

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

[Christian König]

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:
>> 1. Remember how many times a handle was opened in a vm context, that is what
>> this patch does.
>>
>> 2. Instead of removing the mapping on closing the handle we change usespace
>> to remove the mapping separately.
>>
>> 3. Change GEM to only call the open/close callbacks when the handle is
>> opened and closed for the first/last time in a process context, that would
>> also eliminate the refcounting nouveau is currently doing.
>>
>> Feel free to choose one, I just have implemented number 1 because that was
>> the simplest way to go (for me).
>>
>> Cheers,
>> Christian.
> I am all ok for the refcounting part but i don't think the always add
> a va to bo is a good idea. It just add more code for no good reason.
Always adding a va to a bo is only an issue on cayman, on SI and further 
we will need a va for each bo anyway.

The problem with not adding a va is that I just need something to 
actually count the number of references in.

So there isn't so much of an alternative to adding the va on opening the 
handle.

Cheers,
Christian.

>
> Cheers,
> Jerome
>
>>>> Signed-off-by: Christian König <deathsimple@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
>>

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

[Jerome Glisse]

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:
>>> 1. Remember how many times a handle was opened in a vm context, that is
>>> what
>>> this patch does.
>>>
>>> 2. Instead of removing the mapping on closing the handle we change
>>> usespace
>>> to remove the mapping separately.
>>>
>>> 3. Change GEM to only call the open/close callbacks when the handle is
>>> opened and closed for the first/last time in a process context, that
>>> would
>>> also eliminate the refcounting nouveau is currently doing.
>>>
>>> Feel free to choose one, I just have implemented number 1 because that
>>> was
>>> the simplest way to go (for me).
>>>
>>> Cheers,
>>> Christian.
>>
>> I am all ok for the refcounting part but i don't think the always add
>> a va to bo is a good idea. It just add more code for no good reason.
>
> Always adding a va to a bo is only an issue on cayman, on SI and further we
> will need a va for each bo anyway.
>
> The problem with not adding a va is that I just need something to actually
> count the number of references in.
>
> So there isn't so much of an alternative to adding the va on opening the
> handle.
>
> Cheers,
> Christian.

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);
>>>>>    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
>>>
>>>
>

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

[Christian König]

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:
>> 1. Remember how many times a handle was opened in a vm context, that is what
>> this patch does.
>>
>> 2. Instead of removing the mapping on closing the handle we change usespace
>> to remove the mapping separately.
>>
>> 3. Change GEM to only call the open/close callbacks when the handle is
>> opened and closed for the first/last time in a process context, that would
>> also eliminate the refcounting nouveau is currently doing.
>>
>> Feel free to choose one, I just have implemented number 1 because that was
>> the simplest way to go (for me).
>>
>> Cheers,
>> Christian.
> I am all ok for the refcounting part but i don't think the always add
> a va to bo is a good idea. It just add more code for no good reason.
Always adding a va to a bo is only an issue on cayman, on SI and further 
we will need a va for each bo anyway.

The problem with not adding a va is that I just need something to 
actually count the number of references in.

So there isn't so much of an alternative to adding the va on opening the 
handle.

Cheers,
Christian.

>
> Cheers,
> Jerome
>
>>>> Signed-off-by: Christian König <deathsimple@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
>>

Re: [PATCH 1/8] drm/radeon: fix VA range check

[Jerome Glisse]

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