[PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-19 Thread Christian König 
Testing for reset is pointless since the reset can start right after the
test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of VMIDs
to invalidate instead of just restting the first one.

Signed-off-by: Christian König 
Reviewed-by: Alex Deucher 
Reviewed-by: Shashank Sharma 
Acked-by: Felix Kuehling 
---
 drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
 #include "amdgpu_ucode.h"
 #include "amdgpu_amdkfd.h"
 #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
 
 #include "bif/bif_4_1_d.h"
 #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
uint16_t pasid, uint32_t flush_type,
bool all_hub, uint32_t inst)
 {
+   u32 mask = 0x0;
int vmid;
-   unsigned int tmp;
 
-   if (amdgpu_in_reset(adev))
-   return -EIO;
+   if(!down_read_trylock(>reset_domain->sem))
+   return 0;
 
for (vmid = 1; vmid < 16; vmid++) {
+   u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
 
-   tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-   WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-   RREG32(mmVM_INVALIDATE_RESPONSE);
-   break;
-   }
+   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+   mask |= 1 << vmid;
}
 
+   WREG32(mmVM_INVALIDATE_REQUEST, mask);
+   RREG32(mmVM_INVALIDATE_RESPONSE);
+   up_read(>reset_domain->sem);
return 0;
 }
 
-- 
2.34.1

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-08 Thread Felix Kuehling 
I think you mean "VMIDs to invalidate", not "VMIDs to reset". With that 
fixed, the patch is


Acked-by: Felix Kuehling 


On 2023-09-05 02:04, Christian König wrote:

Testing for reset is pointless since the reset can start right after the
test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of VMIDs
to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
  #include "amdgpu_ucode.h"
  #include "amdgpu_amdkfd.h"
  #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
  
  #include "bif/bif_4_1_d.h"

  #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
uint16_t pasid, uint32_t flush_type,
bool all_hub, uint32_t inst)
  {
+   u32 mask = 0x0;
int vmid;
-   unsigned int tmp;
  
-	if (amdgpu_in_reset(adev))

-   return -EIO;
+   if(!down_read_trylock(>reset_domain->sem))
+   return 0;
  
  	for (vmid = 1; vmid < 16; vmid++) {

+   u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  
-		tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);

if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-   WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-   RREG32(mmVM_INVALIDATE_RESPONSE);
-   break;
-   }
+   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+   mask |= 1 << vmid;
}
  
+	WREG32(mmVM_INVALIDATE_REQUEST, mask);

+   RREG32(mmVM_INVALIDATE_RESPONSE);
+   up_read(>reset_domain->sem);
return 0;
  }

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-07 Thread Shashank Sharma 



On 07/09/2023 08:57, Christian König wrote:

Am 06.09.23 um 16:35 schrieb Shashank Sharma:


On 06/09/2023 16:25, Shashank Sharma wrote:


On 05/09/2023 08:04, Christian König wrote:
Testing for reset is pointless since the reset can start right 
after the

test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of 
VMIDs

to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c

index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
  #include "amdgpu_ucode.h"
  #include "amdgpu_amdkfd.h"
  #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
    #include "bif/bif_4_1_d.h"
  #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int 
gmc_v7_0_flush_gpu_tlb_pasid(struct amdgpu_device *adev,

  uint16_t pasid, uint32_t flush_type,
  bool all_hub, uint32_t inst)
  {
+    u32 mask = 0x0;
  int vmid;
-    unsigned int tmp;
  -    if (amdgpu_in_reset(adev))
-    return -EIO;
+ if(!down_read_trylock(>reset_domain->sem))
+    return 0;
    for (vmid = 1; vmid < 16; vmid++) {
+    u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  -    tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-    WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-    RREG32(mmVM_INVALIDATE_RESPONSE);
-    break;
-    }
+    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+    mask |= 1 << vmid;


I am a bit concerned here about the change in code, in the previous 
code we were writing the 'first match out of 16' of tmp and of mask 
and programming the registers with (1 << vmid), whereas in new code 
set we are writing the 'last match out of 16' of vmid. Is that 
intentional or expected ?



With last, I mean all matching bits until last :)


Take a closer look :)

The bits are ORed together for each VMID which has the matching pasid.



Agree, I saw that the previous code was programming only the first 
matching VMID (and then break the loop), but and then I reiterated the 
commit message and realized that it was the bug and this patch is fixing 
it :).


Please feel free to use: Reviewed-by: Shashank Sharma 



- Shashank



Christian.


- Shashank


  }
  +    WREG32(mmVM_INVALIDATE_REQUEST, mask);
+    RREG32(mmVM_INVALIDATE_RESPONSE);
+    up_read(>reset_domain->sem);
  return 0;
  }

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-07 Thread Christian König 

Am 06.09.23 um 16:35 schrieb Shashank Sharma:


On 06/09/2023 16:25, Shashank Sharma wrote:


On 05/09/2023 08:04, Christian König wrote:
Testing for reset is pointless since the reset can start right after 
the

test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of 
VMIDs

to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c

index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
  #include "amdgpu_ucode.h"
  #include "amdgpu_amdkfd.h"
  #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
    #include "bif/bif_4_1_d.h"
  #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,

  uint16_t pasid, uint32_t flush_type,
  bool all_hub, uint32_t inst)
  {
+    u32 mask = 0x0;
  int vmid;
-    unsigned int tmp;
  -    if (amdgpu_in_reset(adev))
-    return -EIO;
+ if(!down_read_trylock(>reset_domain->sem))
+    return 0;
    for (vmid = 1; vmid < 16; vmid++) {
+    u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  -    tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-    WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-    RREG32(mmVM_INVALIDATE_RESPONSE);
-    break;
-    }
+    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+    mask |= 1 << vmid;


I am a bit concerned here about the change in code, in the previous 
code we were writing the 'first match out of 16' of tmp and of mask 
and programming the registers with (1 << vmid), whereas in new code 
set we are writing the 'last match out of 16' of vmid. Is that 
intentional or expected ?



With last, I mean all matching bits until last :)


Take a closer look :)

The bits are ORed together for each VMID which has the matching pasid.

Christian.


- Shashank


  }
  +    WREG32(mmVM_INVALIDATE_REQUEST, mask);
+    RREG32(mmVM_INVALIDATE_RESPONSE);
+    up_read(>reset_domain->sem);
  return 0;
  }

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-06 Thread Shashank Sharma 



On 06/09/2023 16:25, Shashank Sharma wrote:


On 05/09/2023 08:04, Christian König wrote:

Testing for reset is pointless since the reset can start right after the
test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of VMIDs
to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c

index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
  #include "amdgpu_ucode.h"
  #include "amdgpu_amdkfd.h"
  #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
    #include "bif/bif_4_1_d.h"
  #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,

  uint16_t pasid, uint32_t flush_type,
  bool all_hub, uint32_t inst)
  {
+    u32 mask = 0x0;
  int vmid;
-    unsigned int tmp;
  -    if (amdgpu_in_reset(adev))
-    return -EIO;
+    if(!down_read_trylock(>reset_domain->sem))
+    return 0;
    for (vmid = 1; vmid < 16; vmid++) {
+    u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  -    tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-    WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-    RREG32(mmVM_INVALIDATE_RESPONSE);
-    break;
-    }
+    (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+    mask |= 1 << vmid;


I am a bit concerned here about the change in code, in the previous 
code we were writing the 'first match out of 16' of tmp and of mask 
and programming the registers with (1 << vmid), whereas in new code 
set we are writing the 'last match out of 16' of vmid. Is that 
intentional or expected ?



With last, I mean all matching bits until last :)

- Shashank


  }
  +    WREG32(mmVM_INVALIDATE_REQUEST, mask);
+    RREG32(mmVM_INVALIDATE_RESPONSE);
+    up_read(>reset_domain->sem);
  return 0;
  }

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-06 Thread Shashank Sharma 



On 05/09/2023 08:04, Christian König wrote:

Testing for reset is pointless since the reset can start right after the
test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of VMIDs
to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
  1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
  #include "amdgpu_ucode.h"
  #include "amdgpu_amdkfd.h"
  #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
  
  #include "bif/bif_4_1_d.h"

  #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
uint16_t pasid, uint32_t flush_type,
bool all_hub, uint32_t inst)
  {
+   u32 mask = 0x0;
int vmid;
-   unsigned int tmp;
  
-	if (amdgpu_in_reset(adev))

-   return -EIO;
+   if(!down_read_trylock(>reset_domain->sem))
+   return 0;
  
  	for (vmid = 1; vmid < 16; vmid++) {

+   u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
  
-		tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);

if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-   WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-   RREG32(mmVM_INVALIDATE_RESPONSE);
-   break;
-   }
+   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+   mask |= 1 << vmid;


I am a bit concerned here about the change in code, in the previous code 
we were writing the 'first match out of 16' of tmp and of mask and 
programming the registers with (1 << vmid), whereas in new code set we 
are writing the 'last match out of 16' of vmid. Is that intentional or 
expected ?


- Shashank


}
  
+	WREG32(mmVM_INVALIDATE_REQUEST, mask);

+   RREG32(mmVM_INVALIDATE_RESPONSE);
+   up_read(>reset_domain->sem);
return 0;
  }

Re: [PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-05 Thread Alex Deucher 
On Tue, Sep 5, 2023 at 7:30 AM Christian König
 wrote:
>
> Testing for reset is pointless since the reset can start right after the
> test. Grab the reset semaphore instead.
>
> The same PASID can be used by more than once VMID, build a mask of VMIDs
> to reset instead of just restting the first one.
>
> Signed-off-by: Christian König 

Reviewed-by: Alex Deucher 

> ---
>  drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
>  1 file changed, 10 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
> b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> index 6a6929ac2748..9e19a752f94b 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
> @@ -33,6 +33,7 @@
>  #include "amdgpu_ucode.h"
>  #include "amdgpu_amdkfd.h"
>  #include "amdgpu_gem.h"
> +#include "amdgpu_reset.h"
>
>  #include "bif/bif_4_1_d.h"
>  #include "bif/bif_4_1_sh_mask.h"
> @@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
> amdgpu_device *adev,
> uint16_t pasid, uint32_t flush_type,
> bool all_hub, uint32_t inst)
>  {
> +   u32 mask = 0x0;
> int vmid;
> -   unsigned int tmp;
>
> -   if (amdgpu_in_reset(adev))
> -   return -EIO;
> +   if(!down_read_trylock(>reset_domain->sem))
> +   return 0;
>
> for (vmid = 1; vmid < 16; vmid++) {
> +   u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
>
> -   tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
> if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
> -   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) 
> {
> -   WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
> -   RREG32(mmVM_INVALIDATE_RESPONSE);
> -   break;
> -   }
> +   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
> +   mask |= 1 << vmid;
> }
>
> +   WREG32(mmVM_INVALIDATE_REQUEST, mask);
> +   RREG32(mmVM_INVALIDATE_RESPONSE);
> +   up_read(>reset_domain->sem);
> return 0;
>  }
>
> --
> 2.34.1
>

[PATCH 04/11] drm/amdgpu: fix and cleanup gmc_v7_0_flush_gpu_tlb_pasid

2023-09-05 Thread Christian König 
Testing for reset is pointless since the reset can start right after the
test. Grab the reset semaphore instead.

The same PASID can be used by more than once VMID, build a mask of VMIDs
to reset instead of just restting the first one.

Signed-off-by: Christian König 
---
 drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c | 19 ++-
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c 
b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
index 6a6929ac2748..9e19a752f94b 100644
--- a/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gmc_v7_0.c
@@ -33,6 +33,7 @@
 #include "amdgpu_ucode.h"
 #include "amdgpu_amdkfd.h"
 #include "amdgpu_gem.h"
+#include "amdgpu_reset.h"
 
 #include "bif/bif_4_1_d.h"
 #include "bif/bif_4_1_sh_mask.h"
@@ -426,23 +427,23 @@ static int gmc_v7_0_flush_gpu_tlb_pasid(struct 
amdgpu_device *adev,
uint16_t pasid, uint32_t flush_type,
bool all_hub, uint32_t inst)
 {
+   u32 mask = 0x0;
int vmid;
-   unsigned int tmp;
 
-   if (amdgpu_in_reset(adev))
-   return -EIO;
+   if(!down_read_trylock(>reset_domain->sem))
+   return 0;
 
for (vmid = 1; vmid < 16; vmid++) {
+   u32 tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
 
-   tmp = RREG32(mmATC_VMID0_PASID_MAPPING + vmid);
if ((tmp & ATC_VMID0_PASID_MAPPING__VALID_MASK) &&
-   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid) {
-   WREG32(mmVM_INVALIDATE_REQUEST, 1 << vmid);
-   RREG32(mmVM_INVALIDATE_RESPONSE);
-   break;
-   }
+   (tmp & ATC_VMID0_PASID_MAPPING__PASID_MASK) == pasid)
+   mask |= 1 << vmid;
}
 
+   WREG32(mmVM_INVALIDATE_REQUEST, mask);
+   RREG32(mmVM_INVALIDATE_RESPONSE);
+   up_read(>reset_domain->sem);
return 0;
 }
 
-- 
2.34.1