[PATCH 1/2] drm/amdgpu: add a new helper to free a BO in kernel allocations

[PATCH 1/2] drm/amdgpu: add a new helper to free a BO in kernel allocations

[Junwei Zhang]

Free the BO allocated by amdgpu_bo_create_kernel()

Change-Id: I1c7248cfb00bcc51fb86d89760522ca36095cdd2
Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 26 ++++++++++++++++++++++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 ++
 2 files changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 53e4ac5..81dd4f1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -299,6 +299,32 @@ error_free:
 	return r;
 }
 
+/**
+ * amdgpu_bo_free_kernel - free BO for kernel use
+ *
+ * @bo: amdgpu BO to free
+ *
+ * unmaps and unpin a BO for kernel internal use.
+ */
+void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
+			   void **cpu_addr)
+{
+	if (*bo == NULL)
+		return;
+
+	if (likely(amdgpu_bo_reserve(*bo, false) == 0)) {
+		amdgpu_bo_kunmap(*bo);
+		amdgpu_bo_unpin(*bo);
+		amdgpu_bo_unreserve(*bo);
+	}
+	amdgpu_bo_unref(bo);
+
+	if (gpu_addr)
+		*gpu_addr = 0;
+	if (cpu_addr)
+		*cpu_addr = NULL;
+}
+
 int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
 				unsigned long size, int byte_align,
 				bool kernel, u32 domain, u64 flags,
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 72be7d0..c3459fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -132,6 +132,8 @@ int amdgpu_bo_create_kernel(struct amdgpu_device *adev,
 			    unsigned long size, int align,
 			    u32 domain, struct amdgpu_bo **bo_ptr,
 			    u64 *gpu_addr, void **cpu_addr);
+void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
+			   void **cpu_addr);
 int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr);
 void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
 struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/2] drm/amdgpu: free the BO in kernel by helper amdgpu_bo_free_kernel()

[Junwei Zhang]

Change-Id: Iba545d9f9dc1a921b3b727f1c531ee221af153eb
Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c   | 16 +++-------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c | 19 ++++---------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c  | 15 +++------------
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c    |  6 +++---
 drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c    |  6 +++---
 5 files changed, 16 insertions(+), 46 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
index 5ebb3f4..3ab4c65 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ih.c
@@ -119,8 +119,6 @@ int amdgpu_ih_ring_init(struct amdgpu_device *adev, unsigned ring_size,
  */
 void amdgpu_ih_ring_fini(struct amdgpu_device *adev)
 {
-	int r;
-
 	if (adev->irq.ih.use_bus_addr) {
 		if (adev->irq.ih.ring) {
 			/* add 8 bytes for the rptr/wptr shadows and
@@ -132,17 +130,9 @@ void amdgpu_ih_ring_fini(struct amdgpu_device *adev)
 			adev->irq.ih.ring = NULL;
 		}
 	} else {
-		if (adev->irq.ih.ring_obj) {
-			r = amdgpu_bo_reserve(adev->irq.ih.ring_obj, false);
-			if (likely(r == 0)) {
-				amdgpu_bo_kunmap(adev->irq.ih.ring_obj);
-				amdgpu_bo_unpin(adev->irq.ih.ring_obj);
-				amdgpu_bo_unreserve(adev->irq.ih.ring_obj);
-			}
-			amdgpu_bo_unref(&adev->irq.ih.ring_obj);
-			adev->irq.ih.ring = NULL;
-			adev->irq.ih.ring_obj = NULL;
-		}
+		amdgpu_bo_free_kernel(&adev->irq.ih.ring_obj,
+				      &adev->irq.ih.gpu_addr,
+				      (void **)&adev->irq.ih.ring);
 		amdgpu_wb_free(adev, adev->irq.ih.wptr_offs);
 		amdgpu_wb_free(adev, adev->irq.ih.rptr_offs);
 	}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
index 550dfbd..179078e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ring.c
@@ -252,28 +252,17 @@ int amdgpu_ring_init(struct amdgpu_device *adev, struct amdgpu_ring *ring,
  */
 void amdgpu_ring_fini(struct amdgpu_ring *ring)
 {
-	int r;
-	struct amdgpu_bo *ring_obj;
-
-	ring_obj = ring->ring_obj;
 	ring->ready = false;
-	ring->ring = NULL;
-	ring->ring_obj = NULL;
 
 	amdgpu_wb_free(ring->adev, ring->cond_exe_offs);
 	amdgpu_wb_free(ring->adev, ring->fence_offs);
 	amdgpu_wb_free(ring->adev, ring->rptr_offs);
 	amdgpu_wb_free(ring->adev, ring->wptr_offs);
 
-	if (ring_obj) {
-		r = amdgpu_bo_reserve(ring_obj, false);
-		if (likely(r == 0)) {
-			amdgpu_bo_kunmap(ring_obj);
-			amdgpu_bo_unpin(ring_obj);
-			amdgpu_bo_unreserve(ring_obj);
-		}
-		amdgpu_bo_unref(&ring_obj);
-	}
+	amdgpu_bo_free_kernel(&ring->ring_obj,
+			      &ring->gpu_addr,
+			      (void **)&ring->ring);
+
 	amdgpu_debugfs_ring_fini(ring);
 }
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index bf59354..eca1e79 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -249,22 +249,13 @@ int amdgpu_uvd_sw_init(struct amdgpu_device *adev)
 
 int amdgpu_uvd_sw_fini(struct amdgpu_device *adev)
 {
-	int r;
-
 	kfree(adev->uvd.saved_bo);
 
 	amd_sched_entity_fini(&adev->uvd.ring.sched, &adev->uvd.entity);
 
-	if (adev->uvd.vcpu_bo) {
-		r = amdgpu_bo_reserve(adev->uvd.vcpu_bo, false);
-		if (!r) {
-			amdgpu_bo_kunmap(adev->uvd.vcpu_bo);
-			amdgpu_bo_unpin(adev->uvd.vcpu_bo);
-			amdgpu_bo_unreserve(adev->uvd.vcpu_bo);
-		}
-
-		amdgpu_bo_unref(&adev->uvd.vcpu_bo);
-	}
+	amdgpu_bo_free_kernel(&adev->uvd.vcpu_bo,
+			      &adev->uvd.gpu_addr,
+			      (void **)&adev->uvd.cpu_addr);
 
 	amdgpu_ring_fini(&adev->uvd.ring);
 
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 605efcc..d356056 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -4518,9 +4518,9 @@ static int gfx_v7_0_sw_fini(void *handle)
 	int i;
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	amdgpu_bo_unref(&adev->gds.oa_gfx_bo);
-	amdgpu_bo_unref(&adev->gds.gws_gfx_bo);
-	amdgpu_bo_unref(&adev->gds.gds_gfx_bo);
+	amdgpu_bo_free_kernel(&adev->gds.oa_gfx_bo, NULL, NULL);
+	amdgpu_bo_free_kernel(&adev->gds.gws_gfx_bo, NULL, NULL);
+	amdgpu_bo_free_kernel(&adev->gds.gds_gfx_bo, NULL, NULL);
 
 	for (i = 0; i < adev->gfx.num_gfx_rings; i++)
 		amdgpu_ring_fini(&adev->gfx.gfx_ring[i]);
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
index 663a3ec..55b0c71 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v8_0.c
@@ -2324,9 +2324,9 @@ static int gfx_v8_0_sw_fini(void *handle)
 	int i;
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	amdgpu_bo_unref(&adev->gds.oa_gfx_bo);
-	amdgpu_bo_unref(&adev->gds.gws_gfx_bo);
-	amdgpu_bo_unref(&adev->gds.gds_gfx_bo);
+	amdgpu_bo_free_kernel(&adev->gds.oa_gfx_bo, NULL, NULL);
+	amdgpu_bo_free_kernel(&adev->gds.gws_gfx_bo, NULL, NULL);
+	amdgpu_bo_free_kernel(&adev->gds.gds_gfx_bo, NULL, NULL);
 
 	for (i = 0; i < adev->gfx.num_gfx_rings; i++)
 		amdgpu_ring_fini(&adev->gfx.gfx_ring[i]);
-- 
1.9.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/2] drm/amdgpu: add a new helper to free a BO in kernel allocations

[Christian König]

Am 07.09.2016 um 16:43 schrieb Junwei Zhang:
> Free the BO allocated by amdgpu_bo_create_kernel()
>
> Change-Id: I1c7248cfb00bcc51fb86d89760522ca36095cdd2
> Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>

Really nice cleanup, but one comment below.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 26 ++++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 ++
>   2 files changed, 28 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 53e4ac5..81dd4f1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -299,6 +299,32 @@ error_free:
>   	return r;
>   }
>   
> +/**
> + * amdgpu_bo_free_kernel - free BO for kernel use
> + *
> + * @bo: amdgpu BO to free
> + *
> + * unmaps and unpin a BO for kernel internal use.
> + */
> +void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
> +			   void **cpu_addr)
> +{
> +	if (*bo == NULL)
> +		return;
> +
> +	if (likely(amdgpu_bo_reserve(*bo, false) == 0)) {
> +		amdgpu_bo_kunmap(*bo);

We only kmap it in amdgpu_bo_create_kernel() when there was a CPU 
address given.

I think we should mirror that here or otherwise might run into problems 
calling kunmap() more often than kmap().

With that fixed both patches are Reviewed-by: Christian König 
<christian.koenig@amd.com>.

Regards,
Christian.

> +		amdgpu_bo_unpin(*bo);
> +		amdgpu_bo_unreserve(*bo);
> +	}
> +	amdgpu_bo_unref(bo);
> +
> +	if (gpu_addr)
> +		*gpu_addr = 0;
> +	if (cpu_addr)
> +		*cpu_addr = NULL;
> +}
> +
>   int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
>   				unsigned long size, int byte_align,
>   				bool kernel, u32 domain, u64 flags,
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 72be7d0..c3459fc 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -132,6 +132,8 @@ int amdgpu_bo_create_kernel(struct amdgpu_device *adev,
>   			    unsigned long size, int align,
>   			    u32 domain, struct amdgpu_bo **bo_ptr,
>   			    u64 *gpu_addr, void **cpu_addr);
> +void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
> +			   void **cpu_addr);
>   int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr);
>   void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
>   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH 1/2] drm/amdgpu: add a new helper to free a BO in kernel allocations

[Zhang, Jerry]

Hi Christian,

> > +	if (likely(amdgpu_bo_reserve(*bo, false) == 0)) {
> > +		amdgpu_bo_kunmap(*bo);
> 
> We only kmap it in amdgpu_bo_create_kernel() when there was a CPU address
> given.
> 
> I think we should mirror that here or otherwise might run into problems calling
> kunmap() more often than kmap().
Thanks for your comments.
I consider it too when creating the patch.

Actually looking into the amdgpu_bo_kunmap(), it will check if the bo is really mapped by if(bo->kptr == NULL).
But with the judgment of cpu address, it looks obvious in code.

I will update it and commit these patches.

Regards,
Jerry (Junwei Zhang)

SRDC SW Development
AMD Shanghai
_____________________________________


> -----Original Message-----
> From: Christian König [mailto:deathsimple@vodafone.de]
> Sent: Thursday, September 08, 2016 0:19
> To: Zhang, Jerry; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH 1/2] drm/amdgpu: add a new helper to free a BO in kernel
> allocations
> 
> Am 07.09.2016 um 16:43 schrieb Junwei Zhang:
> > Free the BO allocated by amdgpu_bo_create_kernel()
> >
> > Change-Id: I1c7248cfb00bcc51fb86d89760522ca36095cdd2
> > Signed-off-by: Junwei Zhang <Jerry.Zhang@amd.com>
> 
> Really nice cleanup, but one comment below.
> 
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 26
> ++++++++++++++++++++++++++
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 ++
> >   2 files changed, 28 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > index 53e4ac5..81dd4f1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> > @@ -299,6 +299,32 @@ error_free:
> >   	return r;
> >   }
> >
> > +/**
> > + * amdgpu_bo_free_kernel - free BO for kernel use
> > + *
> > + * @bo: amdgpu BO to free
> > + *
> > + * unmaps and unpin a BO for kernel internal use.
> > + */
> > +void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
> > +			   void **cpu_addr)
> > +{
> > +	if (*bo == NULL)
> > +		return;
> > +
> > +	if (likely(amdgpu_bo_reserve(*bo, false) == 0)) {
> > +		amdgpu_bo_kunmap(*bo);
> 
> We only kmap it in amdgpu_bo_create_kernel() when there was a CPU address
> given.
> 
> I think we should mirror that here or otherwise might run into problems calling
> kunmap() more often than kmap().
> 
> With that fixed both patches are Reviewed-by: Christian König
> <christian.koenig@amd.com>.
> 
> Regards,
> Christian.
> 
> > +		amdgpu_bo_unpin(*bo);
> > +		amdgpu_bo_unreserve(*bo);
> > +	}
> > +	amdgpu_bo_unref(bo);
> > +
> > +	if (gpu_addr)
> > +		*gpu_addr = 0;
> > +	if (cpu_addr)
> > +		*cpu_addr = NULL;
> > +}
> > +
> >   int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
> >   				unsigned long size, int byte_align,
> >   				bool kernel, u32 domain, u64 flags, diff --git
> > a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > index 72be7d0..c3459fc 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> > @@ -132,6 +132,8 @@ int amdgpu_bo_create_kernel(struct amdgpu_device
> *adev,
> >   			    unsigned long size, int align,
> >   			    u32 domain, struct amdgpu_bo **bo_ptr,
> >   			    u64 *gpu_addr, void **cpu_addr);
> > +void amdgpu_bo_free_kernel(struct amdgpu_bo **bo, u64 *gpu_addr,
> > +			   void **cpu_addr);
> >   int amdgpu_bo_kmap(struct amdgpu_bo *bo, void **ptr);
> >   void amdgpu_bo_kunmap(struct amdgpu_bo *bo);
> >   struct amdgpu_bo *amdgpu_bo_ref(struct amdgpu_bo *bo);
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx