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

Submitted by Zhang, Jerry(Junwei) on Sept. 7, 2016, 2:43 p.m.

Details

Message ID 1473259386-4230-1-git-send-email-Jerry.Zhang@amd.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Zhang, Jerry(Junwei) Sept. 7, 2016, 2:43 p.m.
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(+)

Patch hide | download patch | download mbox

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

Comments

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

>