RE: [PATCH 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context when suspend

2017-05-31 Thread Deucher, Alexander 
> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Leo Liu
> Sent: Wednesday, May 31, 2017 5:01 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Leo
> Subject: [PATCH 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context
> when suspend
> 
> We are using PSP to resume firmware after suspend, and it is
> resumed at where it got suspended, so we'd better save the
> the context.
> 
> Signed-off-by: Leo Liu 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vce_v4_0.c   | 45
> +++--
>  2 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> index f5f35cc..f83e0c3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> @@ -34,6 +34,7 @@ struct amdgpu_vce {
>   struct amdgpu_bo*vcpu_bo;
>   uint64_tgpu_addr;
>   void*cpu_addr;
> + void*saved_bo;
>   unsignedfw_version;
>   unsignedfb_version;
>   atomic_thandles[AMDGPU_MAX_VCE_HANDLES];
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index ada2d0a..5229593 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -419,15 +419,19 @@ static int vce_v4_0_sw_init(void *handle)
> 
>   if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
>   const struct common_firmware_header *hdr;
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> +
> + adev->vce.saved_bo = kmalloc(size, GFP_KERNEL);
> + if (!adev->vce.saved_bo)
> + return -ENOMEM;
> +
>   hdr = (const struct common_firmware_header *)adev-
> >vce.fw->data;
>   adev-
> >firmware.ucode[AMDGPU_UCODE_ID_VCE].ucode_id =
> AMDGPU_UCODE_ID_VCE;
>   adev->firmware.ucode[AMDGPU_UCODE_ID_VCE].fw =
> adev->vce.fw;
>   adev->firmware.fw_size +=
>   ALIGN(le32_to_cpu(hdr->ucode_size_bytes),
> PAGE_SIZE);
>   DRM_INFO("PSP loading VCE firmware\n");
> - }
> -
> - if (adev->firmware.load_type != AMDGPU_FW_LOAD_PSP) {
> + } else {
>   r = amdgpu_vce_resume(adev);
>   if (r)
>   return r;
> @@ -466,6 +470,11 @@ static int vce_v4_0_sw_fini(void *handle)
>   /* free MM table */
>   amdgpu_virt_free_mm_table(adev);
> 
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + kfree(adev->vce.saved_bo);
> + adev->vce.saved_bo = NULL;
> + }
> +
>   r = amdgpu_vce_suspend(adev);
>   if (r)
>   return r;
> @@ -522,8 +531,18 @@ static int vce_v4_0_hw_fini(void *handle)
> 
>  static int vce_v4_0_suspend(void *handle)
>  {
> - int r;
>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
> +
> + if (adev->vce.vcpu_bo == NULL)
> + return 0;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + memcpy_fromio(adev->vce.saved_bo, ptr, size);
> + }
> 
>   r = vce_v4_0_hw_fini(adev);
>   if (r)
> @@ -534,12 +553,22 @@ static int vce_v4_0_suspend(void *handle)
> 
>  static int vce_v4_0_resume(void *handle)
>  {
> - int r;
>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
> 
> - r = amdgpu_vce_resume(adev);
> - if (r)
> - return r;
> + if (adev->vce.vcpu_bo == NULL)
> + return -EINVAL;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + memcpy_toio(ptr, adev->vce.saved_bo, size);
> + } else {
> + r = amdgpu_vce_resume(adev);
> + if (r)
> + return r;
> + }
> 
>   return vce_v4_0_hw_init(adev);
>  }
> --
> 2.7.4
> 
> ___
> 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 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context when suspend

2017-05-31 Thread Leo Liu 



On 05/31/2017 03:54 PM, Deucher, Alexander wrote:

> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Leo Liu
> Sent: Wednesday, May 31, 2017 3:28 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Leo
> Subject: [PATCH 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context
> when suspend
>
> We are using PSP to resume firmware after suspend, and it is
> resumed at where it got suspended, so we'd better save the
> the context.
>
> Signed-off-by: Leo Liu 

Patches 1, 2 are:
Reviewed-by: Alex Deucher 
A few comments below on patch 3.


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vce_v4_0.c   | 39
> -
>  2 files changed, 35 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> index c93f74a..5ce54cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> @@ -34,6 +34,7 @@ struct amdgpu_vce {
>struct amdgpu_bo*vcpu_bo;
>uint64_tgpu_addr;
>void*cpu_addr;
> + void*saved_bo;
>unsignedfw_version;
>unsignedfb_version;
>atomic_t handles[AMDGPU_MAX_VCE_HANDLES];
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index 0b7fcc1..2230a99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -522,8 +522,22 @@ static int vce_v4_0_hw_fini(void *handle)
>
>  static int vce_v4_0_suspend(void *handle)
>  {
> - int r;
>struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
> +
> + if (adev->vce.vcpu_bo == NULL)
> + return 0;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + adev->vce.saved_bo = kmalloc(size, GFP_KERNEL);

Might want to avoid malloc/free in the suspend/resume paths.  Just 
malloc/free the memory at sw_init/fini time.


Just waste a bit memory if it never goes to suspend, but sure, I will 
fix it in v2.


Thanks,
Leo



> + if (!adev->vce.saved_bo)
> + return -ENOMEM;
> +
> + memcpy_fromio(adev->vce.saved_bo, ptr, size);
> + }
>
>r = vce_v4_0_hw_fini(adev);
>if (r)
> @@ -534,12 +548,27 @@ static int vce_v4_0_suspend(void *handle)
>
>  static int vce_v4_0_resume(void *handle)
>  {
> - int r;
>struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
>
> - r = amdgpu_vce_resume(adev);
> - if (r)
> - return r;
> + if (adev->vce.vcpu_bo == NULL)
> + return -EINVAL;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + if (adev->vce.saved_bo != NULL) {
> + memcpy_toio(ptr, adev->vce.saved_bo, size);
> + kfree(adev->vce.saved_bo);
> + adev->vce.saved_bo = NULL;
> + } else

Kernel coding style; should be } else { ... } to match the chunk above.

> + memset_io(ptr, 0, size);
> + } else {
> + r = amdgpu_vce_resume(adev);
> + if (r)
> + return r;
> + }
>
>return vce_v4_0_hw_init(adev);
>  }
> --
> 2.7.4
>
> ___
> 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 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context when suspend

2017-05-31 Thread Deucher, Alexander 
> -Original Message-
> From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf
> Of Leo Liu
> Sent: Wednesday, May 31, 2017 3:28 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: Liu, Leo
> Subject: [PATCH 3/3] drm/amdgpu: add saved_bo to save vce 4.0 context
> when suspend
> 
> We are using PSP to resume firmware after suspend, and it is
> resumed at where it got suspended, so we'd better save the
> the context.
> 
> Signed-off-by: Leo Liu 

Patches 1, 2 are:
Reviewed-by: Alex Deucher 
A few comments below on patch 3.


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h |  1 +
>  drivers/gpu/drm/amd/amdgpu/vce_v4_0.c   | 39
> -
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> index c93f74a..5ce54cd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vce.h
> @@ -34,6 +34,7 @@ struct amdgpu_vce {
>   struct amdgpu_bo*vcpu_bo;
>   uint64_tgpu_addr;
>   void*cpu_addr;
> + void*saved_bo;
>   unsignedfw_version;
>   unsignedfb_version;
>   atomic_thandles[AMDGPU_MAX_VCE_HANDLES];
> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> index 0b7fcc1..2230a99 100644
> --- a/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v4_0.c
> @@ -522,8 +522,22 @@ static int vce_v4_0_hw_fini(void *handle)
> 
>  static int vce_v4_0_suspend(void *handle)
>  {
> - int r;
>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
> +
> + if (adev->vce.vcpu_bo == NULL)
> + return 0;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + adev->vce.saved_bo = kmalloc(size, GFP_KERNEL);

Might want to avoid malloc/free in the suspend/resume paths.  Just malloc/free 
the memory at sw_init/fini time.

> + if (!adev->vce.saved_bo)
> + return -ENOMEM;
> +
> + memcpy_fromio(adev->vce.saved_bo, ptr, size);
> + }
> 
>   r = vce_v4_0_hw_fini(adev);
>   if (r)
> @@ -534,12 +548,27 @@ static int vce_v4_0_suspend(void *handle)
> 
>  static int vce_v4_0_resume(void *handle)
>  {
> - int r;
>   struct amdgpu_device *adev = (struct amdgpu_device *)handle;
> + int r;
> 
> - r = amdgpu_vce_resume(adev);
> - if (r)
> - return r;
> + if (adev->vce.vcpu_bo == NULL)
> + return -EINVAL;
> +
> + if (adev->firmware.load_type == AMDGPU_FW_LOAD_PSP) {
> + unsigned size = amdgpu_bo_size(adev->vce.vcpu_bo);
> + void *ptr = adev->vce.cpu_addr;
> +
> + if (adev->vce.saved_bo != NULL) {
> + memcpy_toio(ptr, adev->vce.saved_bo, size);
> + kfree(adev->vce.saved_bo);
> + adev->vce.saved_bo = NULL;
> + } else

Kernel coding style; should be } else { ... } to match the chunk above.

> + memset_io(ptr, 0, size);
> + } else {
> + r = amdgpu_vce_resume(adev);
> + if (r)
> + return r;
> + }
> 
>   return vce_v4_0_hw_init(adev);
>  }
> --
> 2.7.4
> 
> ___
> 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