[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 at vodafone.de]
> Sent: Thursday, September 08, 2016 0:19
> To: Zhang, Jerry; amd-gfx at 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 at 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 at 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);
>