[PATCH] drm/amd/amdgpu: Allow broadcast on debugfs read (v2)

[Michel Dänzer]

On 13/10/16 04:20 PM, Michel Dänzer wrote:
> On 13/10/16 12:39 AM, StDenis, Tom wrote:
>> It comes from amdgpu_query_gpu_info_init()
>>
>>
>>         for (i = 0; i < (int)dev->info.num_shader_engines; i++) {
>>                 unsigned instance = (i << AMDGPU_INFO_MMR_SE_INDEX_SHIFT) |
>>                                     (*AMDGPU_INFO_MMR_SH_INDEX_MASK*<<
>>                                      AMDGPU_INFO_MMR_SH_INDEX_SHIFT);
>>
>>                 r = amdgpu_read_mm_registers(dev, 0x263d, 1, instance, 0,
>>                                              &dev->info.backend_disable[i]);
>>
>> This effectively reads from 0/* where the kernel adds the instance of *
>> so it's 0/*/*.  That line was last changed  by Alex
>>
>> *0936139536380* (Alex Deucher  2015-04-20 12:04:22 -0400 174)          
>>                       (AMDGPU_INFO_MMR_SH_INDEX_MASK <<
> 
> As a side note, following that to the end in the kernel code, I noticed
> an interesting minor difference between the AMDGPU_INFO_READ_MMR_REG
> functionality used by this code and the debugfs interface:
> 
> With AMDGPU_INFO_READ_MMR_REG, the effect is that
> amdgpu_asic_read_register() doesn't call amdgpu_gfx_select_se_sh() at
> all before reading the register, so the read is performed from whichever
> SH instance is currently selected.
> 
> Whereas with this patch, amdgpu_debugfs_regs_read() calls
> amdgpu_gfx_select_se_sh(adev, se_bank, 0xFFFFFFFF, instance_bank) before
> the register read, which translates to only the SH_BROADCAST_WRITES bit
> being set for the SH instance index.
> 
> The end result should be the same though, since
> amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff) is
> normally called after every register read.
> 
> 
>> I still don't get why this is a reason to hit pause on the patch(es)
>> though.
> 
> At the very least, it should be documented in an appropriate place
> (commit log and/or code, or any other place where the debugfs interface
> semantics are documented) what actually happens when passing all ones
> for the SE/SH index. Does the hardware ignore the *_BROADCAST_WRITES bit
> for reads, so they're performed from instance 0, or does it combine the
> values from all instances with logical and/or?

I'm not sure how to interpret the fact that this patch has landed
without any changes or followups.

FWIW, I'm still interested in (pointers to) information about what the
libdrm_amdgpu code above expects and what the hardware does for reads
with the broadcast bit enabled, from anyone.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer