Re: [PATCH] drm/amdgpu: Don't need to call csb_vram_unpin

From: "Koenig, Christian" <Christian.Koenig-5C7GfCeVMHo@public.gmane.org>
To: "Deng, Emily" <Emily.Deng-5C7GfCeVMHo@public.gmane.org>,
	"Quan, Evan" <Evan.Quan-5C7GfCeVMHo@public.gmane.org>,
	"amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org"
	<amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
Subject: Re: [PATCH] drm/amdgpu: Don't need to call csb_vram_unpin
Date: Tue, 28 May 2019 07:43:27 +0000	[thread overview]
Message-ID: <daf490f4-49fd-3688-84ff-5dc47eaa535c@amd.com> (raw)
In-Reply-To: <MWHPR12MB132646027A5B99282AB6557A8F1E0-Gy0DoCVfaSVaj5rVPFIlogdYzm3356FpvxpqHgZTriW3zl9H0oFU5g@public.gmane.org>

Am 28.05.19 um 09:38 schrieb Deng, Emily:
>> -----Original Message-----
>> From: Koenig, Christian <Christian.Koenig@amd.com>
>> Sent: Tuesday, May 28, 2019 3:04 PM
>> To: Quan, Evan <Evan.Quan@amd.com>; Deng, Emily
>> <Emily.Deng@amd.com>; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: Don't need to call csb_vram_unpin
>>
>> Ok in this case the patch is a NAK.
>>
>> The correct solution is to stop using amdgpu_bo_free_kernel in
>> gfx_v9_0_sw_fini.
> So we just lead the memory leak here and not destroy the bo? I don't think it is correct.

Oh, no. That's not what I meant.

We should stop using amdgpu_bo_free_kernel and instead use amdgpu_bo_free!

Sorry for not being clear here,
Christian.

>> BTW: Are we using the kernel pointer somewhere? Cause that one became
>> completely invalid because of patch "drm/amdgpu: pin the csb buffer on hw
>> init".
>>
>> Christian.
>>
>> Am 28.05.19 um 03:42 schrieb Quan, Evan:
>>> The original unpin in hw_fini was introduced by
>>> https://lists.freedesktop.org/archives/amd-gfx/2018-July/023681.html
>>>
>>> Evan
>>>> -----Original Message-----
>>>> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of
>>>> Christian K?nig
>>>> Sent: Monday, May 27, 2019 7:02 PM
>>>> To: Deng, Emily <Emily.Deng@amd.com>; amd-gfx@lists.freedesktop.org
>>>> Subject: Re: [PATCH] drm/amdgpu: Don't need to call csb_vram_unpin
>>>>
>>>> Am 27.05.19 um 10:41 schrieb Emily Deng:
>>>>> As it will destroy clear_state_obj, and also will unpin it in the
>>>>> gfx_v9_0_sw_fini, so don't need to call csb_vram unpin in
>>>>> gfx_v9_0_hw_fini, or it will have unpin warning.
>>>>>
>>>>> v2: For suspend, still need to do unpin
>>>>>
>>>>> Signed-off-by: Emily Deng <Emily.Deng@amd.com>
>>>>> ---
>>>>>     drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c | 3 ++-
>>>>>     1 file changed, 2 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>>>> index 5eb70e8..5b1ff48 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v9_0.c
>>>>> @@ -3395,7 +3395,8 @@ static int gfx_v9_0_hw_fini(void *handle)
>>>>>     	gfx_v9_0_cp_enable(adev, false);
>>>>>     	adev->gfx.rlc.funcs->stop(adev);
>>>>>
>>>>> -	gfx_v9_0_csb_vram_unpin(adev);
>>>>> +	if (adev->in_suspend)
>>>>> +		gfx_v9_0_csb_vram_unpin(adev);
>>>> That doesn't looks like a good idea to me.
>>>>
>>>> Why do we have unpin both in the sw_fini as well as the hw_fini code
>> paths?
>>>> Regards,
>>>> Christian.
>>>>
>>>>>     	return 0;
>>>>>     }
>>>> _______________________________________________
>>>> amd-gfx mailing list
>>>> amd-gfx@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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