drm/amd/amdgpu: Fix wave mask in amdgpu_debugfs_wave_read()

The bottom two bits of the simd value were being put into the upper bits of the wave value which was likely working due to the bits being ignored (or aliased).

Eitherway, now we mask it correctly.

Signed-off-by Tom St Denis <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0

index c1f1b8f15395..cc59020d5874 100644

Am 10.11.2017 um 19:03 schrieb Tom St Denis:

Quoted 16 lines

The bottom two bits of the simd value were being put into the upper bits of the wave value which was likely working due to the bits being ignored (or aliased).

Eitherway, now we mask it correctly.

Signed-off-by Tom St Denis <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0

index c1f1b8f15395..cc59020d5874 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c static ssize_t amdgpu_debugfs_wave_read(struct · line 3771

Quoted 7 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +1 -1

@@ -3766,7 +3766,7 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
 file *f, char __user *buf,
       se = ((*pos >> 7) & 0xFF);
       sh = ((*pos >> 15) & 0xFF);
       cu = ((*pos >> 23) & 0xFF);
-    wave = ((*pos >> 31) & 0xFF);
+    wave = ((*pos >> 31) & 0x3F);
       simd = ((*pos >> 37) & 0xFF);

Have you considered using GENMASK_ULL instead of fiddling with the hex masks manually?

E.g. that would look like this

se = (*pos & GENMASK_ULL(14, 7)) >> 7; sh = (*pos & GENMASK_ULL(22, 15)) >> 15; cu = (*pos & GENMASK_ULL(30, 23)) >> 23; wave = (*pos & GENMASK_ULL(36, 31)) >> 31; simd = (*pos & GENMASK_ULL(44, 37)) >> 37;

if you ask me makes it totally obvious what is happening here and avoids such typos.

Might be that we even have some GENMASK_ULL + shift macro somewhere in the kernel, but I don't know of anything of hand.

Christian.

Quoted 35 lines

Am 10.11.2017 um 19:03 schrieb Tom St Denis:

Quoted 16 lines

The bottom two bits of the simd value were being put into the upper bits of the wave value which was likely working due to the bits being ignored (or aliased).

Eitherway, now we mask it correctly.

Signed-off-by Tom St Denis <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0

index c1f1b8f15395..cc59020d5874 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c static ssize_t amdgpu_debugfs_wave_read(struct · line 3771

Quoted 7 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +1 -1

@@ -3766,7 +3766,7 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
 file *f, char __user *buf,
       se = ((*pos >> 7) & 0xFF);
       sh = ((*pos >> 15) & 0xFF);
       cu = ((*pos >> 23) & 0xFF);
-    wave = ((*pos >> 31) & 0xFF);
+    wave = ((*pos >> 31) & 0x3F);
       simd = ((*pos >> 37) & 0xFF);

Have you considered using GENMASK_ULL instead of fiddling with the hex masks manually?

E.g. that would look like this

se = (*pos & GENMASK_ULL(14, 7)) >> 7; sh = (*pos & GENMASK_ULL(22, 15)) >> 15; cu = (*pos & GENMASK_ULL(30, 23)) >> 23; wave = (*pos & GENMASK_ULL(36, 31)) >> 31; simd = (*pos & GENMASK_ULL(44, 37)) >> 37;

Quoted 5 lines

if you ask me makes it totally obvious what is happening here and avoids such typos.

Might be that we even have some GENMASK_ULL + shift macro somewhere in the kernel, but I don't know of anything of hand.

Christian.

I'll take a gander it's a good suggestion.  Though this could apply to 3 other functions in the debugfs side.  Maybe do a fix patch first and then another patch to switch all 4 functions over to a cleaner approach (using GENMASK_ULL/etc)?

Tom
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Am 10.11.2017 um 19:29 schrieb Tom St Denis:

Quoted 38 lines · Christian König

Quoted 37 lines

Am 10.11.2017 um 19:03 schrieb Tom St Denis:

Quoted 16 lines

The bottom two bits of the simd value were being put into the upper bits of the wave value which was likely working due to the bits being ignored (or aliased).

Eitherway, now we mask it correctly.

Signed-off-by Tom St Denis <[email protected]>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +-

  1 file changed, 1 insertion(+), 1 deletion(-)

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +0 -0

index c1f1b8f15395..cc59020d5874 100644

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c static ssize_t amdgpu_debugfs_wave_read(struct · line 3771

Quoted 7 lines

drivers/gpu/drm/amd/amdgpu/amdgpu_device.c +1 -1

@@ -3766,7 +3766,7 @@ static ssize_t amdgpu_debugfs_wave_read(struct 
 file *f, char __user *buf,
       se = ((*pos >> 7) & 0xFF);
       sh = ((*pos >> 15) & 0xFF);
       cu = ((*pos >> 23) & 0xFF);
-    wave = ((*pos >> 31) & 0xFF);
+    wave = ((*pos >> 31) & 0x3F);
       simd = ((*pos >> 37) & 0xFF);

Have you considered using GENMASK_ULL instead of fiddling with the hex masks manually?

E.g. that would look like this

se = (*pos & GENMASK_ULL(14, 7)) >> 7; sh = (*pos & GENMASK_ULL(22, 15)) >> 15; cu = (*pos & GENMASK_ULL(30, 23)) >> 23; wave = (*pos & GENMASK_ULL(36, 31)) >> 31; simd = (*pos & GENMASK_ULL(44, 37)) >> 37;

if you ask me makes it totally obvious what is happening here and avoids such typos.

Quoted 6 lines

Quoted 3 lines

Might be that we even have some GENMASK_ULL + shift macro somewhere in the kernel, but I don't know of anything of hand.

Christian.

I'll take a gander it's a good suggestion.  Though this could apply to 3 other functions in the debugfs side.  Maybe do a fix patch first and then another patch to switch all 4 functions over to a cleaner approach (using GENMASK_ULL/etc)?

For kernel work I would usually prefer a patch which fixes the original issue and then another one to clean up the coding and move to GENMASK_ULL, but it is such a minor issue that I probably don't care much.