Re: [PATCH 1/2] drm/amdgpu: modify invalidate semaphore limit in gmc9

2019-12-10 Thread Christian König

Am 10.12.19 um 17:20 schrieb Changfeng.Zhu:

From: changzhu 

It may fail to load guest driver in round 2 or cause Xstart problem
when using invalidate semaphore for SRIOV or picasso. So it needs avoid
using invalidate semaphore for SRIOV and picasso.

Change-Id: I806f8e99ec97be84e6aed0f5c499a53b1931b490
Signed-off-by: changzhu 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 47 +++
  1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 601667246a1c..552fd7f3fec4 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -412,6 +412,27 @@ static uint32_t gmc_v9_0_get_invalidate_req(unsigned int 
vmid,
return req;
  }
  
+/**

+ * gmc_v9_0_use_invalidate_semaphore - judge whether to use semaphore
+ *
+ * @adev: amdgpu_device pointer
+ * @vmhub: vmhub type
+ *
+ */
+static bool gmc_v9_0_use_invalidate_semaphore(struct amdgpu_device *adev,
+  uint32_t vmhub)
+{
+   if ((vmhub == AMDGPU_MMHUB_0 ||
+vmhub == AMDGPU_MMHUB_1) &&
+   (!amdgpu_sriov_vf(adev)) &&
+   (!(adev->asic_type == CHIP_RAVEN &&
+  adev->rev_id < 0x8 &&
+  adev->pdev->device == 0x15d8)))
+   return true;
+   else
+   return false;


Probably better to not use an if here but instead just return the value. 
E.g. something like this:


return (vmhub == AMD. || ) &&
    


+}
+
  /*
   * GART
   * VMID 0 is the physical GPU addresses as used by the kernel.
@@ -434,6 +455,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
*adev, uint32_t vmid,
const unsigned eng = 17;
u32 j, tmp;
struct amdgpu_vmhub *hub;
+   bool value = gmc_v9_0_use_invalidate_semaphore(adev, vmhub);


Please use a more meaningful name for the variable, something like 
use_semaphore.


And move that to the beginning of the declaration.

Christian.

  
  	BUG_ON(vmhub >= adev->num_vmhubs);
  
@@ -464,11 +486,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device *adev, uint32_t vmid,

 */
  
  	/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */

-   if ((vmhub == AMDGPU_MMHUB_0 ||
-vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8))) {
+   if (value) {
for (j = 0; j < adev->usec_timeout; j++) {
/* a read return value of 1 means semaphore acuqire */
tmp = RREG32_NO_KIQ(hub->vm_inv_eng0_sem + eng);
@@ -498,11 +516,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
*adev, uint32_t vmid,
}
  
  	/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */

-   if ((vmhub == AMDGPU_MMHUB_0 ||
-vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/*
 * add semaphore release after invalidation,
 * write with 0 means semaphore release
@@ -524,6 +538,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
struct amdgpu_vmhub *hub = >vmhub[ring->funcs->vmhub];
uint32_t req = gmc_v9_0_get_invalidate_req(vmid, 0);
unsigned eng = ring->vm_inv_eng;
+   bool value = gmc_v9_0_use_invalidate_semaphore(ring->adev, 
ring->funcs->vmhub);
  
  	/*

 * It may lose gpuvm invalidate acknowldege state across power-gating
@@ -533,11 +548,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
 */
  
  	/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */

-   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
-ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/* a read return value of 1 means semaphore acuqire */
amdgpu_ring_emit_reg_wait(ring,
  hub->vm_inv_eng0_sem + eng, 0x1, 0x1);
@@ -553,11 +564,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
req, 1 << vmid);
  
  	/* TODO: It needs to continue working on debugging with semaphore for GFXHUB as well. */

-   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
-ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/*
 * add semaphore release after invalidation,
  

Re: [PATCH 1/2] drm/amdgpu: modify invalidate semaphore limit in gmc9

2019-12-10 Thread Alex Deucher
On Tue, Dec 10, 2019 at 11:21 AM Changfeng.Zhu  wrote:
>
> From: changzhu 
>
> It may fail to load guest driver in round 2 or cause Xstart problem
> when using invalidate semaphore for SRIOV or picasso. So it needs avoid
> using invalidate semaphore for SRIOV and picasso.
>
> Change-Id: I806f8e99ec97be84e6aed0f5c499a53b1931b490
> Signed-off-by: changzhu 
> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 47 +++
>  1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> index 601667246a1c..552fd7f3fec4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
> @@ -412,6 +412,27 @@ static uint32_t gmc_v9_0_get_invalidate_req(unsigned int 
> vmid,
> return req;
>  }
>
> +/**
> + * gmc_v9_0_use_invalidate_semaphore - judge whether to use semaphore
> + *
> + * @adev: amdgpu_device pointer
> + * @vmhub: vmhub type
> + *
> + */
> +static bool gmc_v9_0_use_invalidate_semaphore(struct amdgpu_device *adev,
> +  uint32_t vmhub)
> +{
> +   if ((vmhub == AMDGPU_MMHUB_0 ||
> +vmhub == AMDGPU_MMHUB_1) &&
> +   (!amdgpu_sriov_vf(adev)) &&
> +   (!(adev->asic_type == CHIP_RAVEN &&
> +  adev->rev_id < 0x8 &&
> +  adev->pdev->device == 0x15d8)))
> +   return true;
> +   else
> +   return false;
> +}
> +
>  /*
>   * GART
>   * VMID 0 is the physical GPU addresses as used by the kernel.
> @@ -434,6 +455,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
> *adev, uint32_t vmid,
> const unsigned eng = 17;
> u32 j, tmp;
> struct amdgpu_vmhub *hub;
> +   bool value = gmc_v9_0_use_invalidate_semaphore(adev, vmhub);

Call this use_semaphore or something like that rather than value.
Same comment to the instances below as well.  With that fixed,
Reviewed-by: Alex Deucher 

>
> BUG_ON(vmhub >= adev->num_vmhubs);
>
> @@ -464,11 +486,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
> *adev, uint32_t vmid,
>  */
>
> /* TODO: It needs to continue working on debugging with semaphore for 
> GFXHUB as well. */
> -   if ((vmhub == AMDGPU_MMHUB_0 ||
> -vmhub == AMDGPU_MMHUB_1) &&
> -   (!(adev->asic_type == CHIP_RAVEN &&
> -  adev->rev_id < 0x8 &&
> -  adev->pdev->device == 0x15d8))) {
> +   if (value) {
> for (j = 0; j < adev->usec_timeout; j++) {
> /* a read return value of 1 means semaphore acuqire */
> tmp = RREG32_NO_KIQ(hub->vm_inv_eng0_sem + eng);
> @@ -498,11 +516,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
> *adev, uint32_t vmid,
> }
>
> /* TODO: It needs to continue working on debugging with semaphore for 
> GFXHUB as well. */
> -   if ((vmhub == AMDGPU_MMHUB_0 ||
> -vmhub == AMDGPU_MMHUB_1) &&
> -   (!(adev->asic_type == CHIP_RAVEN &&
> -  adev->rev_id < 0x8 &&
> -  adev->pdev->device == 0x15d8)))
> +   if (value)
> /*
>  * add semaphore release after invalidation,
>  * write with 0 means semaphore release
> @@ -524,6 +538,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
> amdgpu_ring *ring,
> struct amdgpu_vmhub *hub = >vmhub[ring->funcs->vmhub];
> uint32_t req = gmc_v9_0_get_invalidate_req(vmid, 0);
> unsigned eng = ring->vm_inv_eng;
> +   bool value = gmc_v9_0_use_invalidate_semaphore(ring->adev, 
> ring->funcs->vmhub);
>
> /*
>  * It may lose gpuvm invalidate acknowldege state across power-gating
> @@ -533,11 +548,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
> amdgpu_ring *ring,
>  */
>
> /* TODO: It needs to continue working on debugging with semaphore for 
> GFXHUB as well. */
> -   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
> -ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
> -   (!(adev->asic_type == CHIP_RAVEN &&
> -  adev->rev_id < 0x8 &&
> -  adev->pdev->device == 0x15d8)))
> +   if (value)
> /* a read return value of 1 means semaphore acuqire */
> amdgpu_ring_emit_reg_wait(ring,
>   hub->vm_inv_eng0_sem + eng, 0x1, 
> 0x1);
> @@ -553,11 +564,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
> amdgpu_ring *ring,
> req, 1 << vmid);
>
> /* TODO: It needs to continue working on debugging with semaphore for 
> GFXHUB as well. */
> -   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
> -ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
> -   (!(adev->asic_type == CHIP_RAVEN &&
> -  adev->rev_id < 0x8 &&
> -  adev->pdev->device == 0x15d8)))

[PATCH 1/2] drm/amdgpu: modify invalidate semaphore limit in gmc9

2019-12-10 Thread Changfeng.Zhu
From: changzhu 

It may fail to load guest driver in round 2 or cause Xstart problem
when using invalidate semaphore for SRIOV or picasso. So it needs avoid
using invalidate semaphore for SRIOV and picasso.

Change-Id: I806f8e99ec97be84e6aed0f5c499a53b1931b490
Signed-off-by: changzhu 
---
 drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c | 47 +++
 1 file changed, 27 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
index 601667246a1c..552fd7f3fec4 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v9_0.c
@@ -412,6 +412,27 @@ static uint32_t gmc_v9_0_get_invalidate_req(unsigned int 
vmid,
return req;
 }
 
+/**
+ * gmc_v9_0_use_invalidate_semaphore - judge whether to use semaphore
+ *
+ * @adev: amdgpu_device pointer
+ * @vmhub: vmhub type
+ *
+ */
+static bool gmc_v9_0_use_invalidate_semaphore(struct amdgpu_device *adev,
+  uint32_t vmhub)
+{
+   if ((vmhub == AMDGPU_MMHUB_0 ||
+vmhub == AMDGPU_MMHUB_1) &&
+   (!amdgpu_sriov_vf(adev)) &&
+   (!(adev->asic_type == CHIP_RAVEN &&
+  adev->rev_id < 0x8 &&
+  adev->pdev->device == 0x15d8)))
+   return true;
+   else
+   return false;
+}
+
 /*
  * GART
  * VMID 0 is the physical GPU addresses as used by the kernel.
@@ -434,6 +455,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
*adev, uint32_t vmid,
const unsigned eng = 17;
u32 j, tmp;
struct amdgpu_vmhub *hub;
+   bool value = gmc_v9_0_use_invalidate_semaphore(adev, vmhub);
 
BUG_ON(vmhub >= adev->num_vmhubs);
 
@@ -464,11 +486,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
*adev, uint32_t vmid,
 */
 
/* TODO: It needs to continue working on debugging with semaphore for 
GFXHUB as well. */
-   if ((vmhub == AMDGPU_MMHUB_0 ||
-vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8))) {
+   if (value) {
for (j = 0; j < adev->usec_timeout; j++) {
/* a read return value of 1 means semaphore acuqire */
tmp = RREG32_NO_KIQ(hub->vm_inv_eng0_sem + eng);
@@ -498,11 +516,7 @@ static void gmc_v9_0_flush_gpu_tlb(struct amdgpu_device 
*adev, uint32_t vmid,
}
 
/* TODO: It needs to continue working on debugging with semaphore for 
GFXHUB as well. */
-   if ((vmhub == AMDGPU_MMHUB_0 ||
-vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/*
 * add semaphore release after invalidation,
 * write with 0 means semaphore release
@@ -524,6 +538,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
struct amdgpu_vmhub *hub = >vmhub[ring->funcs->vmhub];
uint32_t req = gmc_v9_0_get_invalidate_req(vmid, 0);
unsigned eng = ring->vm_inv_eng;
+   bool value = gmc_v9_0_use_invalidate_semaphore(ring->adev, 
ring->funcs->vmhub);
 
/*
 * It may lose gpuvm invalidate acknowldege state across power-gating
@@ -533,11 +548,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
 */
 
/* TODO: It needs to continue working on debugging with semaphore for 
GFXHUB as well. */
-   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
-ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/* a read return value of 1 means semaphore acuqire */
amdgpu_ring_emit_reg_wait(ring,
  hub->vm_inv_eng0_sem + eng, 0x1, 0x1);
@@ -553,11 +564,7 @@ static uint64_t gmc_v9_0_emit_flush_gpu_tlb(struct 
amdgpu_ring *ring,
req, 1 << vmid);
 
/* TODO: It needs to continue working on debugging with semaphore for 
GFXHUB as well. */
-   if ((ring->funcs->vmhub == AMDGPU_MMHUB_0 ||
-ring->funcs->vmhub == AMDGPU_MMHUB_1) &&
-   (!(adev->asic_type == CHIP_RAVEN &&
-  adev->rev_id < 0x8 &&
-  adev->pdev->device == 0x15d8)))
+   if (value)
/*
 * add semaphore release after invalidation,
 * write with 0 means semaphore release
-- 
2.17.1

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