[PATCH] drm/amd/amdgpu: Fix amdgpu_debugfs_gpr_read debugfs entry

[Tom St Denis]

On 04/11/2018 08:17 AM, Christian König wrote:
> Am 11.04.2018 um 14:03 schrieb Tom St Denis:
>>
>>
>> On 04/11/2018 07:58 AM, Christian König wrote:
>>>>
>>>>   -    if (size & 3 || *pos & 3)
>>>> +    if (size & 3 || size > (4 * AMDGPU_DEBUGFS_MAX_SGPR_READ)) 
>>>
>>> I think checking the position alignment here is still necessary, 
>>> cause we can't read from not dw boundaries don't we?
>>
>> The index is a dword index as fed into SQ_IND_INDEX (offset => start 
>> => regno as you trace from the debugfs entry to 
>> gfx_v8_0_read_wave_sgprs to wave_read_regs (or analogues...)).
>>
>> SQ_IND_INDEX doesn't take a byte offset but a dword offset, for instance:
>>
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP0 0x026c
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP1 0x026d
>> include/asic_reg/gc/gc_9_0_offset.h:#define ixSQ_WAVE_TTMP2 0x026e
>>
>>
>> The current way for instance would prohibit reading (directly) 
>> SQ_WAVE_TTMP1.
>>
>> I agree it's not really how a typical file device works but it's not a 
>> typical file device :-).  It's assumed every read would be preceded by 
>> a seek to set the higher order bits anyways.
> 
> So pos=0 is one dw and pos=1 is another one? If that's the case then 
> that would be a bug since pos is by definition a byte offset.
> 
> I suggest to keep the limitation and instead fix how pos is interpreted 
> instead.

It would break copies of umr out there already (in ways that won't be 
obvious and therefore result in lost time/etc).   Since umr is probably 
the only user of this it's not really a big deal.  Ideally, everyone 
uses the latest umr each day :-) but that's not realistically the case.

I agree that had I written this today I would keep the file offset 
consistent with the byte offset into the register file (shift down by 4 
before calling the callback).

Since you need to seek to a very specific address to use this device 
it's not the sort of thing that someone would cat and then expect 
sensible output anyways.

Tom