Re: [PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-06 Thread Yong Zhao 

Hi Alex,

As far as I know, we never tested suspend/resume on the setting you 
mentioned. Theoretically it should work.


When I read the code now, I was wondering whether we should stop kfd 
before amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If 
that's not needed, it may make more sense to stick to the previous 
design which kept the kfd suspend/resume inside your IP block 
suspend/resume.


Regards,

Yong


On 2017-07-06 05:06 PM, Alex Deucher wrote:

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling  wrote:

From: Yong Zhao 

Signed-off-by: Yong Zhao 
Reviewed-by: Felix Kuehling 

Does this work properly for multiple GPUs?  E.g., if one is suspended
and another is not?  E.g., PX laptops where we runtime suspend the
dGPU while the APU is still running.

Alex


---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
  drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
  2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5b1220f..bc69b9c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -56,6 +56,8 @@
  #include 
  #include "amdgpu_vf_error.h"

+#include "amdgpu_amdkfd.h"
+
  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");

@@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
suspend, bool fbcon)
 drm_modeset_unlock_all(dev);
 }

+   amdgpu_amdkfd_suspend(adev);
+
 /* unpin the front buffers and cursors */
 list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
 struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
@@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool 
resume, bool fbcon)
 }
 }
 }
+   r = amdgpu_amdkfd_resume(adev);
+   if (r)
+   return r;

 /* blat the mode back in */
 if (fbcon) {
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index 6ce9f80..00639bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
  {
 struct amdgpu_device *adev = (struct amdgpu_device *)handle;

-   amdgpu_amdkfd_suspend(adev);
-
 return cik_common_hw_fini(adev);
  }

  static int cik_common_resume(void *handle)
  {
-   int r;
 struct amdgpu_device *adev = (struct amdgpu_device *)handle;

-   r = cik_common_hw_init(adev);
-   if (r)
-   return r;
-
-   return amdgpu_amdkfd_resume(adev);
+   return cik_common_hw_init(adev);
  }

  static bool cik_common_is_idle(void *handle)
--
1.9.1

___
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 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-06 Thread Yong Zhao 



On 2017-07-06 05:44 PM, Alex Deucher wrote:

On Thu, Jul 6, 2017 at 5:33 PM, Yong Zhao  wrote:

Hi Alex,

As far as I know, we never tested suspend/resume on the setting you
mentioned. Theoretically it should work.

Are the kfd s/r entry points global or per GPU?  If you have two GPUs
and you suspend one, will it suspend the entire kfd?  I'm fine with
the change, it's no worse than the current situation.  Mostly just
curious.
kfd s/r is per GPU. If we suspend only one out of two GPUs, the other 
one will keep working.



When I read the code now, I was wondering whether we should stop kfd before
amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If that's not
needed, it may make more sense to stick to the previous design which kept
the kfd suspend/resume inside your IP block suspend/resume.

I think it makes more sense to put the kfd calls in the common device
s/r code rather than in the soc specific ip functions.  Change is:
Reviewed-by: Alex Deucher 



Regards,

Yong



On 2017-07-06 05:06 PM, Alex Deucher wrote:

On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling 
wrote:

From: Yong Zhao 

Signed-off-by: Yong Zhao 
Reviewed-by: Felix Kuehling 

Does this work properly for multiple GPUs?  E.g., if one is suspended
and another is not?  E.g., PX laptops where we runtime suspend the
dGPU while the APU is still running.

Alex


---
   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
   drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
   2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5b1220f..bc69b9c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -56,6 +56,8 @@
   #include 
   #include "amdgpu_vf_error.h"

+#include "amdgpu_amdkfd.h"
+
   MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
   MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");

@@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev,
bool suspend, bool fbcon)
  drm_modeset_unlock_all(dev);
  }

+   amdgpu_amdkfd_suspend(adev);
+
  /* unpin the front buffers and cursors */
  list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
  struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
@@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev,
bool resume, bool fbcon)
  }
  }
  }
+   r = amdgpu_amdkfd_resume(adev);
+   if (r)
+   return r;

  /* blat the mode back in */
  if (fbcon) {
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
b/drivers/gpu/drm/amd/amdgpu/cik.c
index 6ce9f80..00639bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
   {
  struct amdgpu_device *adev = (struct amdgpu_device *)handle;

-   amdgpu_amdkfd_suspend(adev);
-
  return cik_common_hw_fini(adev);
   }

   static int cik_common_resume(void *handle)
   {
-   int r;
  struct amdgpu_device *adev = (struct amdgpu_device *)handle;

-   r = cik_common_hw_init(adev);
-   if (r)
-   return r;
-
-   return amdgpu_amdkfd_resume(adev);
+   return cik_common_hw_init(adev);
   }

   static bool cik_common_is_idle(void *handle)
--
1.9.1

___
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 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-06 Thread Alex Deucher 
On Thu, Jul 6, 2017 at 5:33 PM, Yong Zhao  wrote:
> Hi Alex,
>
> As far as I know, we never tested suspend/resume on the setting you
> mentioned. Theoretically it should work.

Are the kfd s/r entry points global or per GPU?  If you have two GPUs
and you suspend one, will it suspend the entire kfd?  I'm fine with
the change, it's no worse than the current situation.  Mostly just
curious.

>
> When I read the code now, I was wondering whether we should stop kfd before
> amdgpu_bo_evict_vram() and amdgpu_fence_driver_suspend(). If that's not
> needed, it may make more sense to stick to the previous design which kept
> the kfd suspend/resume inside your IP block suspend/resume.

I think it makes more sense to put the kfd calls in the common device
s/r code rather than in the soc specific ip functions.  Change is:
Reviewed-by: Alex Deucher 


>
> Regards,
>
> Yong
>
>
>
> On 2017-07-06 05:06 PM, Alex Deucher wrote:
>>
>> On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling 
>> wrote:
>>>
>>> From: Yong Zhao 
>>>
>>> Signed-off-by: Yong Zhao 
>>> Reviewed-by: Felix Kuehling 
>>
>> Does this work properly for multiple GPUs?  E.g., if one is suspended
>> and another is not?  E.g., PX laptops where we runtime suspend the
>> dGPU while the APU is still running.
>>
>> Alex
>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
>>>   drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
>>>   2 files changed, 8 insertions(+), 8 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 5b1220f..bc69b9c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -56,6 +56,8 @@
>>>   #include 
>>>   #include "amdgpu_vf_error.h"
>>>
>>> +#include "amdgpu_amdkfd.h"
>>> +
>>>   MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>>>   MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>>>
>>> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev,
>>> bool suspend, bool fbcon)
>>>  drm_modeset_unlock_all(dev);
>>>  }
>>>
>>> +   amdgpu_amdkfd_suspend(adev);
>>> +
>>>  /* unpin the front buffers and cursors */
>>>  list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
>>>  struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
>>> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev,
>>> bool resume, bool fbcon)
>>>  }
>>>  }
>>>  }
>>> +   r = amdgpu_amdkfd_resume(adev);
>>> +   if (r)
>>> +   return r;
>>>
>>>  /* blat the mode back in */
>>>  if (fbcon) {
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> index 6ce9f80..00639bf 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
>>> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>>>   {
>>>  struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>
>>> -   amdgpu_amdkfd_suspend(adev);
>>> -
>>>  return cik_common_hw_fini(adev);
>>>   }
>>>
>>>   static int cik_common_resume(void *handle)
>>>   {
>>> -   int r;
>>>  struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>>>
>>> -   r = cik_common_hw_init(adev);
>>> -   if (r)
>>> -   return r;
>>> -
>>> -   return amdgpu_amdkfd_resume(adev);
>>> +   return cik_common_hw_init(adev);
>>>   }
>>>
>>>   static bool cik_common_is_idle(void *handle)
>>> --
>>> 1.9.1
>>>
>>> ___
>>> 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 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-06 Thread Alex Deucher 
On Mon, Jul 3, 2017 at 5:11 PM, Felix Kuehling  wrote:
> From: Yong Zhao 
>
> Signed-off-by: Yong Zhao 
> Reviewed-by: Felix Kuehling 

Does this work properly for multiple GPUs?  E.g., if one is suspended
and another is not?  E.g., PX laptops where we runtime suspend the
dGPU while the APU is still running.

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
>  drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b1220f..bc69b9c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -56,6 +56,8 @@
>  #include 
>  #include "amdgpu_vf_error.h"
>
> +#include "amdgpu_amdkfd.h"
> +
>  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>
> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
> suspend, bool fbcon)
> drm_modeset_unlock_all(dev);
> }
>
> +   amdgpu_amdkfd_suspend(adev);
> +
> /* unpin the front buffers and cursors */
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool 
> resume, bool fbcon)
> }
> }
> }
> +   r = amdgpu_amdkfd_resume(adev);
> +   if (r)
> +   return r;
>
> /* blat the mode back in */
> if (fbcon) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c 
> b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 6ce9f80..00639bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>  {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -   amdgpu_amdkfd_suspend(adev);
> -
> return cik_common_hw_fini(adev);
>  }
>
>  static int cik_common_resume(void *handle)
>  {
> -   int r;
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -   r = cik_common_hw_init(adev);
> -   if (r)
> -   return r;
> -
> -   return amdgpu_amdkfd_resume(adev);
> +   return cik_common_hw_init(adev);
>  }
>
>  static bool cik_common_is_idle(void *handle)
> --
> 1.9.1
>
> ___
> 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 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-03 Thread Oded Gabbay 
On Tue, Jul 4, 2017 at 12:11 AM, Felix Kuehling  wrote:
> From: Yong Zhao 
>
> Signed-off-by: Yong Zhao 
> Reviewed-by: Felix Kuehling 
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
>  drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
>  2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 5b1220f..bc69b9c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -56,6 +56,8 @@
>  #include 
>  #include "amdgpu_vf_error.h"
>
> +#include "amdgpu_amdkfd.h"
> +
>  MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
>  MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
>
> @@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
> suspend, bool fbcon)
> drm_modeset_unlock_all(dev);
> }
>
> +   amdgpu_amdkfd_suspend(adev);
> +
> /* unpin the front buffers and cursors */
> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
> @@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool 
> resume, bool fbcon)
> }
> }
> }
> +   r = amdgpu_amdkfd_resume(adev);
> +   if (r)
> +   return r;
>
> /* blat the mode back in */
> if (fbcon) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c 
> b/drivers/gpu/drm/amd/amdgpu/cik.c
> index 6ce9f80..00639bf 100644
> --- a/drivers/gpu/drm/amd/amdgpu/cik.c
> +++ b/drivers/gpu/drm/amd/amdgpu/cik.c
> @@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
>  {
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -   amdgpu_amdkfd_suspend(adev);
> -
> return cik_common_hw_fini(adev);
>  }
>
>  static int cik_common_resume(void *handle)
>  {
> -   int r;
> struct amdgpu_device *adev = (struct amdgpu_device *)handle;
>
> -   r = cik_common_hw_init(adev);
> -   if (r)
> -   return r;
> -
> -   return amdgpu_amdkfd_resume(adev);
> +   return cik_common_hw_init(adev);
>  }
>
>  static bool cik_common_is_idle(void *handle)
> --
> 1.9.1
>
> ___
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx

This patch is:
Reviewed-by: Oded Gabbay 
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 06/12] drm/amdgpu: Correctly establish the suspend/resume hook for amdkfd

2017-07-03 Thread Felix Kuehling 
From: Yong Zhao 

Signed-off-by: Yong Zhao 
Reviewed-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 7 +++
 drivers/gpu/drm/amd/amdgpu/cik.c   | 9 +
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 5b1220f..bc69b9c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -56,6 +56,8 @@
 #include 
 #include "amdgpu_vf_error.h"
 
+#include "amdgpu_amdkfd.h"
+
 MODULE_FIRMWARE("amdgpu/vega10_gpu_info.bin");
 MODULE_FIRMWARE("amdgpu/raven_gpu_info.bin");
 
@@ -2397,6 +2399,8 @@ int amdgpu_device_suspend(struct drm_device *dev, bool 
suspend, bool fbcon)
drm_modeset_unlock_all(dev);
}
 
+   amdgpu_amdkfd_suspend(adev);
+
/* unpin the front buffers and cursors */
list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
struct amdgpu_crtc *amdgpu_crtc = to_amdgpu_crtc(crtc);
@@ -2537,6 +2541,9 @@ int amdgpu_device_resume(struct drm_device *dev, bool 
resume, bool fbcon)
}
}
}
+   r = amdgpu_amdkfd_resume(adev);
+   if (r)
+   return r;
 
/* blat the mode back in */
if (fbcon) {
diff --git a/drivers/gpu/drm/amd/amdgpu/cik.c b/drivers/gpu/drm/amd/amdgpu/cik.c
index 6ce9f80..00639bf 100644
--- a/drivers/gpu/drm/amd/amdgpu/cik.c
+++ b/drivers/gpu/drm/amd/amdgpu/cik.c
@@ -1825,21 +1825,14 @@ static int cik_common_suspend(void *handle)
 {
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-   amdgpu_amdkfd_suspend(adev);
-
return cik_common_hw_fini(adev);
 }
 
 static int cik_common_resume(void *handle)
 {
-   int r;
struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-   r = cik_common_hw_init(adev);
-   if (r)
-   return r;
-
-   return amdgpu_amdkfd_resume(adev);
+   return cik_common_hw_init(adev);
 }
 
 static bool cik_common_is_idle(void *handle)
-- 
1.9.1

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