drm/amdgpu: Don't need to call csb_vram_unpin

Submitted by Deng, Emily on May 27, 2019, 8:41 a.m.

Details

Message ID 1558946487-18034-1-git-send-email-Emily.Deng@amd.com
State New
Headers show
Series "drm/amdgpu: Don't need to call csb_vram_unpin" ( rev: 2 ) in AMD X.Org drivers

Not browsing as part of any series.

Commit Message

Deng, Emily May 27, 2019, 8:41 a.m.
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(-)

Patch hide | download patch | download mbox

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);
 
 	return 0;
 }

Comments

Reviewed-by: Evan Quan <evan.quan@amd.com>


> -----Original Message-----

> From: amd-gfx <amd-gfx-bounces@lists.freedesktop.org> On Behalf Of

> Emily Deng

> Sent: Monday, May 27, 2019 4:41 PM

> To: amd-gfx@lists.freedesktop.org

> Cc: Deng, Emily <Emily.Deng@amd.com>

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

> 

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

> 

>  	return 0;

>  }

> --

> 2.7.4

> 

> _______________________________________________

> amd-gfx mailing list

> amd-gfx@lists.freedesktop.org

> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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;
>   }
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
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.

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
>-----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.
>

>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
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
>-----Original Message-----

>From: Koenig, Christian <Christian.Koenig@amd.com>

>Sent: Tuesday, May 28, 2019 3:43 PM

>To: Deng, Emily <Emily.Deng@amd.com>; Quan, Evan

><Evan.Quan@amd.com>; amd-gfx@lists.freedesktop.org

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

>

>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.

Thanks for your good suggestion.  Will revert this patch, and submit another patch.

Best wishes
Emily Deng
>

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