On 10/11/17 01:20 PM, Christian König wrote: > Am 10.11.2017 um 19:03 schrieb Tom St Denis: >> 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 <tom.stdenis at amd.com> >> --- >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 2 +- >>  1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> index c1f1b8f15395..cc59020d5874 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c >> @@ -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. 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
