[PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Alex Deucher]

Looks like fallout from a rebase or merge.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 45259b5..c2f70be 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
 
 	r = amdgpu_suspend(adev);
 
-	/* evict remaining vram memory */
-	amdgpu_bo_evict_vram(adev);
-
 	pci_save_state(dev->pdev);
 	if (suspend) {
 		/* Shut down the device */
-- 
2.5.5

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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Dave Airlie]

On 6 October 2016 at 05:42, Alex Deucher <alexdeucher@gmail.com> wrote:
> Looks like fallout from a rebase or merge.

Nearly sure we do it twice on purpose, at least radeon does it as well.

so NAK without a better explaination.

Dave.

>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 45259b5..c2f70be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>
>         r = amdgpu_suspend(adev);
>
> -       /* evict remaining vram memory */
> -       amdgpu_bo_evict_vram(adev);
> -
>         pci_save_state(dev->pdev);
>         if (suspend) {
>                 /* Shut down the device */
> --
> 2.5.5
>
> _______________________________________________
> 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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Tom St Denis]

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

I proposed to remove that when running into SDMA job out-of-order issues
and Christian K very strongly NAK'ed that (a couple of times).

So I suspect he'll do it again.

Tom


On Wed, Oct 5, 2016 at 8:10 PM Dave Airlie <airlied-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> On 6 October 2016 at 05:42, Alex Deucher <alexdeucher-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> > Looks like fallout from a rebase or merge.
>
> Nearly sure we do it twice on purpose, at least radeon does it as well.
>
> so NAK without a better explaination.
>
> Dave.
>
> >
> > Signed-off-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 45259b5..c2f70be 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct drm_device *dev,
> bool suspend, bool fbcon)
> >
> >         r = amdgpu_suspend(adev);
> >
> > -       /* evict remaining vram memory */
> > -       amdgpu_bo_evict_vram(adev);
> > -
> >         pci_save_state(dev->pdev);
> >         if (suspend) {
> >                 /* Shut down the device */
> > --
> > 2.5.5
> >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3724 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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Christian König]

Am 05.10.2016 um 21:42 schrieb Alex Deucher:
> Looks like fallout from a rebase or merge.
>
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

NAK, this is done on purpose and is for evicting the GART table using 
memcpy.

Tom wanted to remove that as well and it makes resume then fail horrible 
on dGPUs.

We should probably add a comment why this is necessary.

Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>   1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 45259b5..c2f70be 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct drm_device *dev, bool suspend, bool fbcon)
>   
>   	r = amdgpu_suspend(adev);
>   
> -	/* evict remaining vram memory */
> -	amdgpu_bo_evict_vram(adev);
> -
>   	pci_save_state(dev->pdev);
>   	if (suspend) {
>   		/* Shut down the device */


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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Tom St Denis]

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

Yup that's what I said in another part of the thread :-)

i'm a trailblazer of bad ideas at times :-) hehehehe

Tom

On Sun, Oct 9, 2016 at 11:13 AM Christian König <deathsimple@vodafone.de>
wrote:

> Am 05.10.2016 um 21:42 schrieb Alex Deucher:
> > Looks like fallout from a rebase or merge.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org>
>
> NAK, this is done on purpose and is for evicting the GART table using
> memcpy.
>
> Tom wanted to remove that as well and it makes resume then fail horrible
> on dGPUs.
>
> We should probably add a comment why this is necessary.
>
> Christian.
>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
> >   1 file changed, 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > index 45259b5..c2f70be 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> > @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct drm_device *dev,
> bool suspend, bool fbcon)
> >
> >       r = amdgpu_suspend(adev);
> >
> > -     /* evict remaining vram memory */
> > -     amdgpu_bo_evict_vram(adev);
> > -
> >       pci_save_state(dev->pdev);
> >       if (suspend) {
> >               /* Shut down the device */
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>

[-- Attachment #1.2: Type: text/html, Size: 3114 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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Christian König]

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

> i'm a trailblazer of bad ideas at times :-) hehehehe
Well, I know this because I had the same bad idea earlier and spend a 
lot of time bisecting why it broke :)

Christian.

Am 09.10.2016 um 19:54 schrieb Tom St Denis:
> Yup that's what I said in another part of the thread :-)
>
> i'm a trailblazer of bad ideas at times :-) hehehehe
>
> Tom
>
> On Sun, Oct 9, 2016 at 11:13 AM Christian König 
> <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>
>     Am 05.10.2016 um 21:42 schrieb Alex Deucher:
>     > Looks like fallout from a rebase or merge.
>     >
>     > Signed-off-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org
>     <mailto:alexander.deucher-5C7GfCeVMHo@public.gmane.org>>
>
>     NAK, this is done on purpose and is for evicting the GART table using
>     memcpy.
>
>     Tom wanted to remove that as well and it makes resume then fail
>     horrible
>     on dGPUs.
>
>     We should probably add a comment why this is necessary.
>
>     Christian.
>
>     > ---
>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>     >   1 file changed, 3 deletions(-)
>     >
>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > index 45259b5..c2f70be 100644
>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>     > @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct
>     drm_device *dev, bool suspend, bool fbcon)
>     >
>     >       r = amdgpu_suspend(adev);
>     >
>     > -     /* evict remaining vram memory */
>     > -     amdgpu_bo_evict_vram(adev);
>     > -
>     >       pci_save_state(dev->pdev);
>     >       if (suspend) {
>     >               /* Shut down the device */
>
>
>     _______________________________________________
>     amd-gfx mailing list
>     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 4716 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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[zhoucm1]

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

We can make it dGpu specific if APU has no problem.

Regards,
David Zhou

On 2016年10月10日 15:09, Christian König wrote:
>> i'm a trailblazer of bad ideas at times :-) hehehehe
> Well, I know this because I had the same bad idea earlier and spend a 
> lot of time bisecting why it broke :)
>
> Christian.
>
> Am 09.10.2016 um 19:54 schrieb Tom St Denis:
>> Yup that's what I said in another part of the thread :-)
>>
>> i'm a trailblazer of bad ideas at times :-) hehehehe
>>
>> Tom
>>
>> On Sun, Oct 9, 2016 at 11:13 AM Christian König 
>> <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>>
>>     Am 05.10.2016 um 21:42 schrieb Alex Deucher:
>>     > Looks like fallout from a rebase or merge.
>>     >
>>     > Signed-off-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org
>>     <mailto:alexander.deucher-5C7GfCeVMHo@public.gmane.org>>
>>
>>     NAK, this is done on purpose and is for evicting the GART table using
>>     memcpy.
>>
>>     Tom wanted to remove that as well and it makes resume then fail
>>     horrible
>>     on dGPUs.
>>
>>     We should probably add a comment why this is necessary.
>>
>>     Christian.
>>
>>     > ---
>>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>>     >   1 file changed, 3 deletions(-)
>>     >
>>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > index 45259b5..c2f70be 100644
>>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct
>>     drm_device *dev, bool suspend, bool fbcon)
>>     >
>>     >       r = amdgpu_suspend(adev);
>>     >
>>     > -     /* evict remaining vram memory */
>>     > -     amdgpu_bo_evict_vram(adev);
>>     > -
>>     >       pci_save_state(dev->pdev);
>>     >       if (suspend) {
>>     >               /* Shut down the device */
>>
>>
>>     _______________________________________________
>>     amd-gfx mailing list
>>     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


[-- Attachment #1.2: Type: text/html, Size: 5901 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

Re: [PATCH] drm/amdgpu: remove duplicate evict_vram in suspend path

[Christian König]

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

APUs have a problem with that as well when we try to suspend to disk 
because the kernel has no idea that he needs to backup the stolen VRAM 
as well.

I discussed that with Alex internally as well and our conclusion is that 
we could work on this, but it's probably not worth it and potentially 
causes more trouble than it fixes.

It's probably a better idea to give the suspend/resume and eviction code 
some more testing and hardening and add comments here and there why we 
do things like we do them.

Regards,
Christian.

Am 10.10.2016 um 09:11 schrieb zhoucm1:
> We can make it dGpu specific if APU has no problem.
>
> Regards,
> David Zhou
>
> On 2016年10月10日 15:09, Christian König wrote:
>>> i'm a trailblazer of bad ideas at times :-) hehehehe
>> Well, I know this because I had the same bad idea earlier and spend a 
>> lot of time bisecting why it broke :)
>>
>> Christian.
>>
>> Am 09.10.2016 um 19:54 schrieb Tom St Denis:
>>> Yup that's what I said in another part of the thread :-)
>>>
>>> i'm a trailblazer of bad ideas at times :-) hehehehe
>>>
>>> Tom
>>>
>>> On Sun, Oct 9, 2016 at 11:13 AM Christian König 
>>> <deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org <mailto:deathsimple-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>> wrote:
>>>
>>>     Am 05.10.2016 um 21:42 schrieb Alex Deucher:
>>>     > Looks like fallout from a rebase or merge.
>>>     >
>>>     > Signed-off-by: Alex Deucher <alexander.deucher-5C7GfCeVMHo@public.gmane.org
>>>     <mailto:alexander.deucher-5C7GfCeVMHo@public.gmane.org>>
>>>
>>>     NAK, this is done on purpose and is for evicting the GART table
>>>     using
>>>     memcpy.
>>>
>>>     Tom wanted to remove that as well and it makes resume then fail
>>>     horrible
>>>     on dGPUs.
>>>
>>>     We should probably add a comment why this is necessary.
>>>
>>>     Christian.
>>>
>>>     > ---
>>>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 ---
>>>     >   1 file changed, 3 deletions(-)
>>>     >
>>>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > index 45259b5..c2f70be 100644
>>>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>     > @@ -1956,9 +1956,6 @@ int amdgpu_device_suspend(struct
>>>     drm_device *dev, bool suspend, bool fbcon)
>>>     >
>>>     >       r = amdgpu_suspend(adev);
>>>     >
>>>     > -     /* evict remaining vram memory */
>>>     > -     amdgpu_bo_evict_vram(adev);
>>>     > -
>>>     >       pci_save_state(dev->pdev);
>>>     >       if (suspend) {
>>>     >               /* Shut down the device */
>>>
>>>
>>>     _______________________________________________
>>>     amd-gfx mailing list
>>>     amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org <mailto:amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org>
>>>     https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>>>
>>
>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org
>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
>


[-- Attachment #1.2: Type: text/html, Size: 7155 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