Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO

[zhoucm1 <david1.zhou-5C7GfCeVMHo@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2562 bytes --]



On 2017年04月17日 22:54, Xie, AlexBin wrote:
>
> Hi David,
>
>
> Thanks for the comments. However, please have look at 
> amdgpu_bo_reserve definition.
>
> static inline int amdgpu_bo_reserve(struct amdgpu_bo *bo, bool no_intr)
>
Ah, this is a wired wrapper for ttm_bo_reserve.

>
> When we call this function like the following:
>
>          r = amdgpu_bo_reserve(adev->vram_scratch.robj, false);
> The false means interruptible.
>
>
> On the other hand,  when amdgpu_bo_reserve function return error, why 
> do we unref BO without kunmap and unpin the BO? This is wrong 
> implementation when amdgpu_bo_reserve return any error.
>
Yeah, I see your mean, it's in driver un-loading, How about changing to 
no interruptible? Your patch will make a memleak if bo_reserve fails, 
but it seems not matter. I have no strong preference.

Regards,
David Zhou
>
>
> Thanks,
> Alex Bin Xie
>
> ------------------------------------------------------------------------
> *From:* Zhou, David(ChunMing)
> *Sent:* Friday, April 14, 2017 12:00 AM
> *To:* Xie, AlexBin; amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> *Subject:* Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO
>
>
> On 2017年04月14日 05:34, Alex Xie wrote:
> > According to comment of amdgpu_bo_reserve, amdgpu_bo_reserve
> > can return with -ERESTARTSYS. When this function was interrupted
> > by a signal, BO should not be unref. Otherwise the BO might be
> > released while is kmapped and pinned, or BO MIGHT be deref
> > multiple times, etc.
>          r = amdgpu_bo_reserve(adev->vram_scratch.robj, false);
> we have specified interruptible to false, so -ERESTARTSYS isn't possible
> here.
>
> Thanks,
> David Zhou
> >
> > Change-Id: If76071a768950a0d3ad9d5da7fcae04881807621
> > Signed-off-by: Alex Xie <AlexBin.Xie-5C7GfCeVMHo@public.gmane.org>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 53996e3..1dcc2d1 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -355,8 +355,8 @@ static void amdgpu_vram_scratch_fini(struct 
> amdgpu_device *adev)
> > amdgpu_bo_kunmap(adev->vram_scratch.robj);
> > amdgpu_bo_unpin(adev->vram_scratch.robj);
> > amdgpu_bo_unreserve(adev->vram_scratch.robj);
> > + amdgpu_bo_unref(&adev->vram_scratch.robj);
> >        }
> > - amdgpu_bo_unref(&adev->vram_scratch.robj);
> >   }
> >
> >   /**
>


[-- Attachment #1.2: Type: text/html, Size: 8196 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

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