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
