<html> <head> <meta content="text/html; charset=UTF-8" http-equiv="Content-Type"> </head> <body text="#000000" bgcolor="#FFFFFF"> <div class="moz-cite-prefix">On 2017年04月17日 22:54, Xie, AlexBin wrote: </div> <blockquote cite="mid:MWHPR1201MB0045DE55E81D0FF5CE76671FF2060@MWHPR1201MB0045.namprd12.prod.outlook.com" type="cite"> <meta http-equiv="Content-Type" content="text/html; charset=UTF-8"> <style type="text/css" style="display:none;"></style> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr"> 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) </div> </blockquote> Ah, this is a wired wrapper for ttm_bo_reserve. <blockquote cite="mid:MWHPR1201MB0045DE55E81D0FF5CE76671FF2060@MWHPR1201MB0045.namprd12.prod.outlook.com" type="cite"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr"> 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. </div> </blockquote> 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 <blockquote cite="mid:MWHPR1201MB0045DE55E81D0FF5CE76671FF2060@MWHPR1201MB0045.namprd12.prod.outlook.com" type="cite"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;" dir="ltr"> Thanks, <div>Alex Bin Xie <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Zhou, David(ChunMing) Sent: Friday, April 14, 2017 12:00 AM To: Xie, AlexBin; <a class="moz-txt-link-abbreviated" href="mailto:amd-gfx@lists.freedesktop.org">amd-gfx@lists.freedesktop.org</a> Subject: Re: [PATCH] dmr/amdgpu: Fix wrongly unref of BO <div> </div> </div> </div> <div class="PlainText"> 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 <a class="moz-txt-link-rfc2396E" href="mailto:AlexBin.Xie@amd.com"><AlexBin.Xie@amd.com></a> > --- > 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); > } > > /** </div> </div> </div> </div> </blockquote> </body> </html>