The broadcast read was required to attempt to debug a pro-stack problem which I believe the "cache gca config" patches are meant to mitigate. The broadcast write support is required to select CUs for wave readings. libdrm performs a broadcast read when reading raster config data so apparently that's a thing.
Since it's a debugfs interface any stability problems a broadcast read may or may not cause isn't really a concern for day to day operations. FWIW I've never yet seen an issue with broadcast reading raster config registers.
But to put it simply, the patch was part of a series that was blocking other work and there wasn't sufficient cause to NAK it.
Tom
________________________________
From: Michel Dänzer <michel@xxxxxxxxxxx>
Sent: Tuesday, October 18, 2016 23:49
To: StDenis, Tom; Christian König
Cc: amd-gfx at lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/amdgpu: Allow broadcast on debugfs read (v2)
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
Graphics, Processors and Immersive VR Solutions | AMD <http://www.amd.com/>
www.amd.com
Explore a wide range of innovative next generation computing processors, graphics, and Immersive VR solutions by Advanced Micro Devices (AMD). Visit AMD.com now!
Libre software enthusiast | Mesa and X developer
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20161019/8aabf248/attachment-0001.html>
