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

[Christian König]

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