Re: [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,
>>  >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, 0x, 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, 0x, 0x, 0x) 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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

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,
>  >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, 0x, 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, 0x, 0x, 0x) 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?


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

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[StDenis, Tom]

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,
 >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 <<

I still don't get why this is a reason to hit pause on the patch(es) though.  
They're root only, you can still not use * if you don't want to and furthermore 
I'm already using the write debugfs content (to debug waveforms).

Tom



From: Michel Dänzer <michel.daen...@mailbox.org>
Sent: Tuesday, October 11, 2016 20:34
To: StDenis, Tom; Christian König
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/amdgpu: Allow broadcast on debugfs read (v2)

On 11/10/16 09:32 PM, StDenis, Tom wrote:
> It's used by the UMD though they read from 0/*/* when reading the
> RASTER_CONFIG registers (which may be a bug...)

We should probably clarify what userspace is trying to do there, and
whether the hardware actually does that.


--
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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Michel Dänzer]

On 11/10/16 09:32 PM, StDenis, Tom wrote:
> It's used by the UMD though they read from 0/*/* when reading the
> RASTER_CONFIG registers (which may be a bug...)

We should probably clarify what userspace is trying to do there, and
whether the hardware actually does that.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[StDenis, Tom]

It's used by the UMD though they read from 0/*/* when reading the RASTER_CONFIG 
registers (which may be a bug...)


I only added it because the previous interface was either */*/* or x/y/z (where 
none are *).  Now it supports arbitrary bank selections.


Tom



From: Christian König <deathsim...@vodafone.de>
Sent: Tuesday, October 11, 2016 08:30
To: StDenis, Tom; Michel Dänzer
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/amd/amdgpu: Allow broadcast on debugfs read (v2)

I briefly remember reading in some hardware doc that this actually has a 
defined behavior.

It was something about applying either bit wise "and" or "or" on the resulting 
bits and returning that to the reader.

This way you can check with a single broadcast read if an error occurred or if 
any of the instances are busy.

But if you don't actually use it I suggest to not implement this.

Regards,
Christian.

Am 11.10.2016 um 12:26 schrieb StDenis, Tom:

That actually makes sense.  It came up because mmPA_SC_RASTER_CONFIG (and _1) 
are read by libdrm with a selection of 0/*/* for se/sh/instance so I wanted to 
be able to reproduce that via the debugfs interface for testing.


But ya, if you use the broadcast bit who are you reading from then ... hmm..


Tom



From: amd-gfx 
<amd-gfx-boun...@lists.freedesktop.org><mailto:amd-gfx-boun...@lists.freedesktop.org>
 on behalf of Michel Dänzer 
<michel.daen...@mailbox.org><mailto:michel.daen...@mailbox.org>
Sent: Monday, October 10, 2016 23:52
To: Tom St Denis
Cc: amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
Subject: Re: [PATCH] drm/amd/amdgpu: Allow broadcast on debugfs read (v2)

On 09/10/16 08:58 PM, Tom St Denis wrote:
> Allow any of the se/sh/instance fields to be
> specified as a broadcast by submitting 0x3FF.
>
> (v2) Fix broadcast range checking
>
> Signed-off-by: Tom St Denis <tom.stde...@amd.com><mailto:tom.stde...@amd.com>

I'm not sure how a "broadcast read" is supposed to work? I can only see
the register specs talking about broadcast in connection with writes.


--
Earthling Michel Dänzer   |   http://www.amd.com
Graphics, Processors and Immersive VR Solutions | AMD <http://www.amd.com/>
www.amd.com<http://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
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
amd-gfx Info Page - 
lists.freedesktop.org<https://lists.freedesktop.org/mailman/listinfo/amd-gfx>
lists.freedesktop.org
To see the collection of prior postings to the list, visit the amd-gfx 
Archives. Using amd-gfx: To post a message to all the list members, send email 
...






___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org<mailto:amd-gfx@lists.freedesktop.org>
https://lists.freedesktop.org/mailman/listinfo/amd-gfx


___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

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

[Tom St Denis]

Allow any of the se/sh/instance fields to be
specified as a broadcast by submitting 0x3FF.

(v2) Fix broadcast range checking

Signed-off-by: Tom St Denis 
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 4217e754f99e..04c4aee70452 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -2574,6 +2574,13 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, 
char __user *buf,
se_bank = (*pos >> 24) & 0x3FF;
sh_bank = (*pos >> 34) & 0x3FF;
instance_bank = (*pos >> 44) & 0x3FF;
+
+   if (se_bank == 0x3FF)
+   se_bank = 0x;
+   if (sh_bank == 0x3FF)
+   sh_bank = 0x;
+   if (instance_bank == 0x3FF)
+   instance_bank = 0x;
use_bank = 1;
} else {
use_bank = 0;
@@ -2582,8 +2589,8 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, 
char __user *buf,
*pos &= 0x3;
 
if (use_bank) {
-   if (sh_bank >= adev->gfx.config.max_sh_per_se ||
-   se_bank >= adev->gfx.config.max_shader_engines)
+   if ((sh_bank != 0x && sh_bank >= 
adev->gfx.config.max_sh_per_se) ||
+   (se_bank != 0x && se_bank >= 
adev->gfx.config.max_shader_engines))
return -EINVAL;
mutex_lock(>grbm_idx_mutex);
amdgpu_gfx_select_se_sh(adev, se_bank,
-- 
2.10.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx