Am 10.11.2017 um 19:29 schrieb Tom St Denis: > 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)? 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. Christian. > > Tom > _______________________________________________ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
