[PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

Totally untested but as long as read_sensor() has been recently
implemented for dpm based boards, amdgpu_sensors can now be
exposed.

Cc: Tom St Denis <tom.stdenis@amd.com>
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6f021e70f15f..1a8e3b9a2268 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3202,10 +3202,7 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
 	idx = *pos >> 2;
 
 	valuesize = sizeof(values);
-	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
-		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
-	else
-		return -EINVAL;
+	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
 
 	if (size > valuesize)
 		return -EINVAL;
-- 
2.11.1

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

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 13/02/17 07:23 PM, Samuel Pitoiset wrote:
> Totally untested but as long as read_sensor() has been recently
> implemented for dpm based boards, amdgpu_sensors can now be
> exposed.
>
> Cc: Tom St Denis <tom.stdenis@amd.com>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6f021e70f15f..1a8e3b9a2268 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3202,10 +3202,7 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>  	idx = *pos >> 2;
>
>  	valuesize = sizeof(values);
> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
> -		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
> -	else
> -		return -EINVAL;
> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
>
>  	if (size > valuesize)
>  		return -EINVAL;
>

NAK, this will oops the kernel if amdgpu.dpm=0 and on powerplay systems 
if powerplay=0.

Otherwise, it seems to work though the GPU_TEMP reads as "5000" on my 
Kaveri (meaning temp of 5C which isn't true).

I suspect the dpm code is reading the wrong register to get the temp but 
we can fix that in another change later on.

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

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

On 02/14/2017 03:04 PM, Tom St Denis wrote:
> On 13/02/17 07:23 PM, Samuel Pitoiset wrote:
>> Totally untested but as long as read_sensor() has been recently
>> implemented for dpm based boards, amdgpu_sensors can now be
>> exposed.
>>
>> Cc: Tom St Denis <tom.stdenis@amd.com>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 6f021e70f15f..1a8e3b9a2268 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3202,10 +3202,7 @@ static ssize_t
>> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>>      idx = *pos >> 2;
>>
>>      valuesize = sizeof(values);
>> -    if (adev->powerplay.pp_funcs &&
>> adev->powerplay.pp_funcs->read_sensor)
>> -        r =
>> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx,
>> &values[0], &valuesize);
>> -    else
>> -        return -EINVAL;
>> +    r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
>>
>>      if (size > valuesize)
>>          return -EINVAL;
>>
>
> NAK, this will oops the kernel if amdgpu.dpm=0 and on powerplay systems
> if powerplay=0.
>

The powerplay parameter has been removed recently.

commit a48203f576e7f708093f7a9d9dcc9ff7b067e559
Author: Rex Zhu <Rex.Zhu@amd.com>
Date:   Tue Dec 27 15:51:45 2016 +0800

     drm/amdgpu: delete dead module parameter:amdgpu_powerplay.

     Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
     Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
     Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

But I do agree for pre-powerplay chips.

> Otherwise, it seems to work though the GPU_TEMP reads as "5000" on my
> Kaveri (meaning temp of 5C which isn't true).
>
> I suspect the dpm code is reading the wrong register to get the temp but
> we can fix that in another change later on.

No idea. It was just a copy-n-paste. Maybe the initial code is buggy?

>
> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

Totally untested but as long as read_sensor() has been recently
implemented for dpm based boards, amdgpu_sensors can now be
exposed.

v2: - make sure read_sensor is not NULL on dpm chips
    - keep sanity check for powerplay chips

Cc: Tom St Denis <tom.stdenis@amd.com>
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6f021e70f15f..80821b436aeb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3204,6 +3204,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
 	valuesize = sizeof(values);
 	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
 		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
+	else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
+		r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
+						&valuesize);
 	else
 		return -EINVAL;
 
-- 
2.11.1

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

RE: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Samuel Pitoiset
> Sent: Tuesday, February 14, 2017 9:45 AM
> To: StDenis, Tom; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay chips
> 
> 
> 
> On 02/14/2017 03:04 PM, Tom St Denis wrote:
> > On 13/02/17 07:23 PM, Samuel Pitoiset wrote:
> >> Totally untested but as long as read_sensor() has been recently
> >> implemented for dpm based boards, amdgpu_sensors can now be
> >> exposed.
> >>
> >> Cc: Tom St Denis <tom.stdenis@amd.com>
> >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 6f021e70f15f..1a8e3b9a2268 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -3202,10 +3202,7 @@ static ssize_t
> >> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
> >>      idx = *pos >> 2;
> >>
> >>      valuesize = sizeof(values);
> >> -    if (adev->powerplay.pp_funcs &&
> >> adev->powerplay.pp_funcs->read_sensor)
> >> -        r =
> >> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle,
> idx,
> >> &values[0], &valuesize);
> >> -    else
> >> -        return -EINVAL;
> >> +    r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> >>
> >>      if (size > valuesize)
> >>          return -EINVAL;
> >>
> >
> > NAK, this will oops the kernel if amdgpu.dpm=0 and on powerplay systems
> > if powerplay=0.
> >
> 
> The powerplay parameter has been removed recently.
> 
> commit a48203f576e7f708093f7a9d9dcc9ff7b067e559
> Author: Rex Zhu <Rex.Zhu@amd.com>
> Date:   Tue Dec 27 15:51:45 2016 +0800
> 
>      drm/amdgpu: delete dead module parameter:amdgpu_powerplay.
> 
>      Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>      Signed-off-by: Rex Zhu <Rex.Zhu@amd.com>
>      Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
> 
> But I do agree for pre-powerplay chips.
> 
> > Otherwise, it seems to work though the GPU_TEMP reads as "5000" on my
> > Kaveri (meaning temp of 5C which isn't true).
> >
> > I suspect the dpm code is reading the wrong register to get the temp but
> > we can fix that in another change later on.
> 
> No idea. It was just a copy-n-paste. Maybe the initial code is buggy?

I don’t know that the GPU temp sensor on CI based APUs actually works properly.  I think the package temperature is exposed via the CPU thermals.

Alex

> 
> >
> > Tom
> _______________________________________________
> amd-gfx mailing list
> 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

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 10:08 AM, Samuel Pitoiset wrote:
> Totally untested but as long as read_sensor() has been recently
> implemented for dpm based boards, amdgpu_sensors can now be
> exposed.
>
> v2: - make sure read_sensor is not NULL on dpm chips
>     - keep sanity check for powerplay chips
>
> Cc: Tom St Denis <tom.stdenis@amd.com>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6f021e70f15f..80821b436aeb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3204,6 +3204,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>  	valuesize = sizeof(values);
>  	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
>  		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
> +	else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
> +		r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
> +						&valuesize);
>  	else
>  		return -EINVAL;
>
>

Sorry NAK again, even with dpm=0 those function pointers are set and you 
end up inside the dpm code trying to parse data structures that aren't 
initialized.

For instance, this oops on my Kaveri when trying to read SCLK.

[  729.616382] BUG: unable to handle kernel NULL pointer dereference at 
00000000000000c4
[  729.616467] IP: [<ffffffffc0e62d40>] kv_dpm_read_sensor+0xa0/0xd0 
[amdgpu]
[  729.616619] PGD 232c58067
[  729.616644] PUD 232c4f067
[  729.616670] PMD 0

[  729.616697] Oops: 0000 [#1] SMP
[  729.616726] Modules linked in: fuse amdkfd amd_iommu_v2 amdgpu 
i2c_algo_bit ttm drm_kms_helper drm xt_CHECKSUM iptable_mangle 
ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat 
nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge 
ebtable_filter ebtables ip6table_filter ip6_tables rpcsec_gss_krb5 nfsv4 
dns_resolver nfs fscache bnep vfat fat edac_mce_amd arc4 edac_core 
kvm_amd iwlmvm snd_hda_codec_realtek mac80211 kvm snd_hda_codec_generic 
snd_hda_codec_hdmi snd_hda_intel snd_hda_codec snd_hda_core btusb btrtl 
btbcm btintel iwlwifi bluetooth snd_hwdep cfg80211 snd_seq 
snd_seq_device snd_pcm irqbypass joydev snd_timer snd crct10dif_pclmul 
crc32_pclmul pcspkr ghash_clmulni_intel rfkill fam15h_power sp5100_tco 
k10temp soundcore acpi_cpufreq tpm_tis i2c_piix4 tpm_tis_core
[  729.617499]  shpchp video tpm nfsd auth_rpcgss nfs_acl lockd grace 
sunrpc ata_generic pata_acpi 8021q crc32c_intel garp stp llc serio_raw 
mrp pata_atiixp r8169 mii fjes
[  729.617672] CPU: 0 PID: 1417 Comm: umr Not tainted 4.9.0+ #4
[  729.617720] Hardware name: Gigabyte Technology Co., Ltd. To be filled 
by O.E.M./F2A88XN-WIFI, BIOS F5 04/22/2015
[  729.617802] task: ffff8e43ef37d880 task.stack: ffff9d7501594000
[  729.617852] RIP: 0010:[<ffffffffc0e62d40>]  [<ffffffffc0e62d40>] 
kv_dpm_read_sensor+0xa0/0xd0 [amdgpu]
[  729.618000] RSP: 0018:ffff9d7501597d60  EFLAGS: 00010246
[  729.618046] RAX: 0000000000000000 RBX: ffff9d7501597d98 RCX: 
0000000000000000
[  729.618104] RDX: 0000000000000000 RSI: 0000000000000286 RDI: 
0000000000000286
[  729.618162] RBP: ffff9d7501597d80 R08: 0000000000000000 R09: 
0000000000000000
[  729.618221] R10: 0000000000000000 R11: ffff8e43ef37d880 R12: 
ffff9d7501597d98
[  729.618285] R13: 0000000000000000 R14: ffff9d7501597d94 R15: 
0000000000000000
[  729.618344] FS:  00007f6525bfd700(0000) GS:ffff8e43fec00000(0000) 
knlGS:0000000000000000
[  729.618411] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[  729.618458] CR2: 00000000000000c4 CR3: 00000002261f6000 CR4: 
00000000000406f0
[  729.618515] Stack:
[  729.618535]  0000000000000004 00007ffe1e85ca00 ffff8e43f3162100 
0000000000000004
[  729.618607]  ffff9d7501597df8 ffffffffc0e2c637 000000401e85ca00 
ffff9d7501597f18
[  729.618676]  ffff9d7501597df8 ffffffffc0e2dc7d 0000000000005403 
0000000000000000
[  729.618747] Call Trace:
[  729.618826]  [<ffffffffc0e2c637>] 
amdgpu_debugfs_sensor_read+0xf7/0x120 [amdgpu]
[  729.618938]  [<ffffffffc0e2dc7d>] ? 
amdgpu_debugfs_regs_read+0x17d/0x220 [amdgpu]
[  729.619002]  [<ffffffff8f3433e4>] full_proxy_read+0x54/0x90
[  729.619049]  [<ffffffff8f24be67>] __vfs_read+0x37/0x150
[  729.619095]  [<ffffffff8f35e67b>] ? security_file_permission+0x9b/0xc0
[  729.621382]  [<ffffffff8f24cfe6>] vfs_read+0x96/0x130
[  729.622997]  [<ffffffff8f24e4d5>] SyS_read+0x55/0xc0
[  729.624540]  [<ffffffff8f8010b7>] entry_SYSCALL_64_fastpath+0x1a/0xa9
[  729.626139] Code: 24 31 c0 41 c7 06 04 00 00 00 41 5c 41 5d 41 5e 41 
5f 5d c3 c1 e8 10 83 e0 1f 83 f8 07 77 ad 48 8d 14 c5 00 00 00 00 5b 48 
29 c2 <41> 8b 84 95 c4 00 00 00 0f c8 41 89 04 24 41 c7 06 04 00 00 00

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

RE: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Samuel Pitoiset
> Sent: Monday, February 13, 2017 7:23 PM
> To: amd-gfx@lists.freedesktop.org
> Cc: StDenis, Tom; Samuel Pitoiset
> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay
> chips
> 
> Totally untested but as long as read_sensor() has been recently
> implemented for dpm based boards, amdgpu_sensors can now be
> exposed.
> 
> Cc: Tom St Denis <tom.stdenis@amd.com>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6f021e70f15f..1a8e3b9a2268 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3202,10 +3202,7 @@ static ssize_t
> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>  	idx = *pos >> 2;
> 
>  	valuesize = sizeof(values);
> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >read_sensor)
> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
> >powerplay.pp_handle, idx, &values[0], &valuesize);
> -	else
> -		return -EINVAL;
> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);

amdgpu_dpm is handled by both powerplay and non-powerplay so I think if we check for that here, we should be fine.  E.g.,
if (amdgpu_dpm != 0)
        r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);

Alex

> 
>  	if (size > valuesize)
>  		return -EINVAL;
> --
> 2.11.1
> 
> _______________________________________________
> amd-gfx mailing list
> 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

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 11:57 AM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Samuel Pitoiset
>> Sent: Monday, February 13, 2017 7:23 PM
>> To: amd-gfx@lists.freedesktop.org
>> Cc: StDenis, Tom; Samuel Pitoiset
>> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay
>> chips
>>
>> Totally untested but as long as read_sensor() has been recently
>> implemented for dpm based boards, amdgpu_sensors can now be
>> exposed.
>>
>> Cc: Tom St Denis <tom.stdenis@amd.com>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 6f021e70f15f..1a8e3b9a2268 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3202,10 +3202,7 @@ static ssize_t
>> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>>  	idx = *pos >> 2;
>>
>>  	valuesize = sizeof(values);
>> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
>>> read_sensor)
>> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
>>> powerplay.pp_handle, idx, &values[0], &valuesize);
>> -	else
>> -		return -EINVAL;
>> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
>
> amdgpu_dpm is handled by both powerplay and non-powerplay so I think if we check for that here, we should be fine.  E.g.,
> if (amdgpu_dpm != 0)
>         r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);

And a similar fix in the DRM ioctl too I suppose.

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

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

On 02/14/2017 05:41 PM, Tom St Denis wrote:
> On 14/02/17 10:08 AM, Samuel Pitoiset wrote:
>> Totally untested but as long as read_sensor() has been recently
>> implemented for dpm based boards, amdgpu_sensors can now be
>> exposed.
>>
>> v2: - make sure read_sensor is not NULL on dpm chips
>>     - keep sanity check for powerplay chips
>>
>> Cc: Tom St Denis <tom.stdenis@amd.com>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>>  1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 6f021e70f15f..80821b436aeb 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3204,6 +3204,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct
>> file *f, char __user *buf,
>>      valuesize = sizeof(values);
>>      if (adev->powerplay.pp_funcs &&
>> adev->powerplay.pp_funcs->read_sensor)
>>          r =
>> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx,
>> &values[0], &valuesize);
>> +    else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
>> +        r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
>> +                        &valuesize);
>>      else
>>          return -EINVAL;
>>
>>
>
> Sorry NAK again, even with dpm=0 those function pointers are set and you
> end up inside the dpm code trying to parse data structures that aren't
> initialized.

No worries. I thought that check was enough. Anyway, writing code 
without the hardware should be avoided. :)

Can you try the thing suggested by Alex? Because I will need to fix up 
the DRM ioctl codepath as well.

>
> For instance, this oops on my Kaveri when trying to read SCLK.
>
> [  729.616382] BUG: unable to handle kernel NULL pointer dereference at
> 00000000000000c4
> [  729.616467] IP: [<ffffffffc0e62d40>] kv_dpm_read_sensor+0xa0/0xd0
> [amdgpu]
> [  729.616619] PGD 232c58067
> [  729.616644] PUD 232c4f067
> [  729.616670] PMD 0
>
> [  729.616697] Oops: 0000 [#1] SMP
> [  729.616726] Modules linked in: fuse amdkfd amd_iommu_v2 amdgpu
> i2c_algo_bit ttm drm_kms_helper drm xt_CHECKSUM iptable_mangle
> ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_nat_ipv4 nf_nat
> nf_conntrack_ipv4 nf_defrag_ipv4 xt_conntrack nf_conntrack tun bridge
> ebtable_filter ebtables ip6table_filter ip6_tables rpcsec_gss_krb5 nfsv4
> dns_resolver nfs fscache bnep vfat fat edac_mce_amd arc4 edac_core
> kvm_amd iwlmvm snd_hda_codec_realtek mac80211 kvm snd_hda_codec_generic
> snd_hda_codec_hdmi snd_hda_intel snd_hda_codec snd_hda_core btusb btrtl
> btbcm btintel iwlwifi bluetooth snd_hwdep cfg80211 snd_seq
> snd_seq_device snd_pcm irqbypass joydev snd_timer snd crct10dif_pclmul
> crc32_pclmul pcspkr ghash_clmulni_intel rfkill fam15h_power sp5100_tco
> k10temp soundcore acpi_cpufreq tpm_tis i2c_piix4 tpm_tis_core
> [  729.617499]  shpchp video tpm nfsd auth_rpcgss nfs_acl lockd grace
> sunrpc ata_generic pata_acpi 8021q crc32c_intel garp stp llc serio_raw
> mrp pata_atiixp r8169 mii fjes
> [  729.617672] CPU: 0 PID: 1417 Comm: umr Not tainted 4.9.0+ #4
> [  729.617720] Hardware name: Gigabyte Technology Co., Ltd. To be filled
> by O.E.M./F2A88XN-WIFI, BIOS F5 04/22/2015
> [  729.617802] task: ffff8e43ef37d880 task.stack: ffff9d7501594000
> [  729.617852] RIP: 0010:[<ffffffffc0e62d40>]  [<ffffffffc0e62d40>]
> kv_dpm_read_sensor+0xa0/0xd0 [amdgpu]
> [  729.618000] RSP: 0018:ffff9d7501597d60  EFLAGS: 00010246
> [  729.618046] RAX: 0000000000000000 RBX: ffff9d7501597d98 RCX:
> 0000000000000000
> [  729.618104] RDX: 0000000000000000 RSI: 0000000000000286 RDI:
> 0000000000000286
> [  729.618162] RBP: ffff9d7501597d80 R08: 0000000000000000 R09:
> 0000000000000000
> [  729.618221] R10: 0000000000000000 R11: ffff8e43ef37d880 R12:
> ffff9d7501597d98
> [  729.618285] R13: 0000000000000000 R14: ffff9d7501597d94 R15:
> 0000000000000000
> [  729.618344] FS:  00007f6525bfd700(0000) GS:ffff8e43fec00000(0000)
> knlGS:0000000000000000
> [  729.618411] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [  729.618458] CR2: 00000000000000c4 CR3: 00000002261f6000 CR4:
> 00000000000406f0
> [  729.618515] Stack:
> [  729.618535]  0000000000000004 00007ffe1e85ca00 ffff8e43f3162100
> 0000000000000004
> [  729.618607]  ffff9d7501597df8 ffffffffc0e2c637 000000401e85ca00
> ffff9d7501597f18
> [  729.618676]  ffff9d7501597df8 ffffffffc0e2dc7d 0000000000005403
> 0000000000000000
> [  729.618747] Call Trace:
> [  729.618826]  [<ffffffffc0e2c637>]
> amdgpu_debugfs_sensor_read+0xf7/0x120 [amdgpu]
> [  729.618938]  [<ffffffffc0e2dc7d>] ?
> amdgpu_debugfs_regs_read+0x17d/0x220 [amdgpu]
> [  729.619002]  [<ffffffff8f3433e4>] full_proxy_read+0x54/0x90
> [  729.619049]  [<ffffffff8f24be67>] __vfs_read+0x37/0x150
> [  729.619095]  [<ffffffff8f35e67b>] ? security_file_permission+0x9b/0xc0
> [  729.621382]  [<ffffffff8f24cfe6>] vfs_read+0x96/0x130
> [  729.622997]  [<ffffffff8f24e4d5>] SyS_read+0x55/0xc0
> [  729.624540]  [<ffffffff8f8010b7>] entry_SYSCALL_64_fastpath+0x1a/0xa9
> [  729.626139] Code: 24 31 c0 41 c7 06 04 00 00 00 41 5c 41 5d 41 5e 41
> 5f 5d c3 c1 e8 10 83 e0 1f 83 f8 07 77 ad 48 8d 14 c5 00 00 00 00 5b 48
> 29 c2 <41> 8b 84 95 c4 00 00 00 0f c8 41 89 04 24 41 c7 06 04 00 00 00
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: StDenis, Tom
> Sent: Tuesday, February 14, 2017 11:58 AM
> To: Deucher, Alexander; 'Samuel Pitoiset'; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay chips
> 
> On 14/02/17 11:57 AM, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Samuel Pitoiset
> >> Sent: Monday, February 13, 2017 7:23 PM
> >> To: amd-gfx@lists.freedesktop.org
> >> Cc: StDenis, Tom; Samuel Pitoiset
> >> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay
> >> chips
> >>
> >> Totally untested but as long as read_sensor() has been recently
> >> implemented for dpm based boards, amdgpu_sensors can now be
> >> exposed.
> >>
> >> Cc: Tom St Denis <tom.stdenis@amd.com>
> >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 6f021e70f15f..1a8e3b9a2268 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -3202,10 +3202,7 @@ static ssize_t
> >> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
> >>  	idx = *pos >> 2;
> >>
> >>  	valuesize = sizeof(values);
> >> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >>> read_sensor)
> >> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
> >>> powerplay.pp_handle, idx, &values[0], &valuesize);
> >> -	else
> >> -		return -EINVAL;
> >> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> >
> > amdgpu_dpm is handled by both powerplay and non-powerplay so I think
> if we check for that here, we should be fine.  E.g.,
> > if (amdgpu_dpm != 0)
> >         r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> 
> And a similar fix in the DRM ioctl too I suppose.

Yes.

Alex

> 
> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: StDenis, Tom
> Sent: Tuesday, February 14, 2017 11:58 AM
> To: Deucher, Alexander; 'Samuel Pitoiset'; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay chips
> 
> On 14/02/17 11:57 AM, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Samuel Pitoiset
> >> Sent: Monday, February 13, 2017 7:23 PM
> >> To: amd-gfx@lists.freedesktop.org
> >> Cc: StDenis, Tom; Samuel Pitoiset
> >> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay
> >> chips
> >>
> >> Totally untested but as long as read_sensor() has been recently
> >> implemented for dpm based boards, amdgpu_sensors can now be
> >> exposed.
> >>
> >> Cc: Tom St Denis <tom.stdenis@amd.com>
> >> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> >> ---
> >>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
> >>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> index 6f021e70f15f..1a8e3b9a2268 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >> @@ -3202,10 +3202,7 @@ static ssize_t
> >> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
> >>  	idx = *pos >> 2;
> >>
> >>  	valuesize = sizeof(values);
> >> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >>> read_sensor)
> >> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
> >>> powerplay.pp_handle, idx, &values[0], &valuesize);
> >> -	else
> >> -		return -EINVAL;
> >> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> >
> > amdgpu_dpm is handled by both powerplay and non-powerplay so I think
> if we check for that here, we should be fine.  E.g.,
> > if (amdgpu_dpm != 0)
> >         r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> 
> And a similar fix in the DRM ioctl too I suppose.

We can just return an error if amdgpu_dpm ==0 in the ioctl.

Alex

> 
> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

On 02/14/2017 06:01 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: StDenis, Tom
>> Sent: Tuesday, February 14, 2017 11:58 AM
>> To: Deucher, Alexander; 'Samuel Pitoiset'; amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
>> powerplay chips
>>
>> On 14/02/17 11:57 AM, Deucher, Alexander wrote:
>>>> -----Original Message-----
>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
>> Behalf
>>>> Of Samuel Pitoiset
>>>> Sent: Monday, February 13, 2017 7:23 PM
>>>> To: amd-gfx@lists.freedesktop.org
>>>> Cc: StDenis, Tom; Samuel Pitoiset
>>>> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
>> powerplay
>>>> chips
>>>>
>>>> Totally untested but as long as read_sensor() has been recently
>>>> implemented for dpm based boards, amdgpu_sensors can now be
>>>> exposed.
>>>>
>>>> Cc: Tom St Denis <tom.stdenis@amd.com>
>>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>>> ---
>>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
>>>>  1 file changed, 1 insertion(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> index 6f021e70f15f..1a8e3b9a2268 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>>> @@ -3202,10 +3202,7 @@ static ssize_t
>>>> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>>>>  	idx = *pos >> 2;
>>>>
>>>>  	valuesize = sizeof(values);
>>>> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
>>>>> read_sensor)
>>>> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
>>>>> powerplay.pp_handle, idx, &values[0], &valuesize);
>>>> -	else
>>>> -		return -EINVAL;
>>>> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
>>>
>>> amdgpu_dpm is handled by both powerplay and non-powerplay so I think
>> if we check for that here, we should be fine.  E.g.,
>>> if (amdgpu_dpm != 0)
>>>         r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
>>
>> And a similar fix in the DRM ioctl too I suppose.
>
> We can just return an error if amdgpu_dpm ==0 in the ioctl.

-EINVAL?

>
> Alex
>
>>
>> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

RE: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: Samuel Pitoiset [mailto:samuel.pitoiset@gmail.com]
> Sent: Tuesday, February 14, 2017 12:04 PM
> To: Deucher, Alexander; StDenis, Tom; amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay chips
> 
> 
> 
> On 02/14/2017 06:01 PM, Deucher, Alexander wrote:
> >> -----Original Message-----
> >> From: StDenis, Tom
> >> Sent: Tuesday, February 14, 2017 11:58 AM
> >> To: Deucher, Alexander; 'Samuel Pitoiset'; amd-gfx@lists.freedesktop.org
> >> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> >> powerplay chips
> >>
> >> On 14/02/17 11:57 AM, Deucher, Alexander wrote:
> >>>> -----Original Message-----
> >>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> >> Behalf
> >>>> Of Samuel Pitoiset
> >>>> Sent: Monday, February 13, 2017 7:23 PM
> >>>> To: amd-gfx@lists.freedesktop.org
> >>>> Cc: StDenis, Tom; Samuel Pitoiset
> >>>> Subject: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> >> powerplay
> >>>> chips
> >>>>
> >>>> Totally untested but as long as read_sensor() has been recently
> >>>> implemented for dpm based boards, amdgpu_sensors can now be
> >>>> exposed.
> >>>>
> >>>> Cc: Tom St Denis <tom.stdenis@amd.com>
> >>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> >>>> ---
> >>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 5 +----
> >>>>  1 file changed, 1 insertion(+), 4 deletions(-)
> >>>>
> >>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> index 6f021e70f15f..1a8e3b9a2268 100644
> >>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> >>>> @@ -3202,10 +3202,7 @@ static ssize_t
> >>>> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
> >>>>  	idx = *pos >> 2;
> >>>>
> >>>>  	valuesize = sizeof(values);
> >>>> -	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs-
> >>>>> read_sensor)
> >>>> -		r = adev->powerplay.pp_funcs->read_sensor(adev-
> >>>>> powerplay.pp_handle, idx, &values[0], &valuesize);
> >>>> -	else
> >>>> -		return -EINVAL;
> >>>> +	r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> >>>
> >>> amdgpu_dpm is handled by both powerplay and non-powerplay so I
> think
> >> if we check for that here, we should be fine.  E.g.,
> >>> if (amdgpu_dpm != 0)
> >>>         r = amdgpu_dpm_read_sensor(adev, idx, &values[0], &valuesize);
> >>
> >> And a similar fix in the DRM ioctl too I suppose.
> >
> > We can just return an error if amdgpu_dpm ==0 in the ioctl.
> 
> -EINVAL?

Maybe -ENOENT?

Alex

> 
> >
> > Alex
> >
> >>
> >> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 12:01 PM, Samuel Pitoiset wrote:
>
>
> On 02/14/2017 05:41 PM, Tom St Denis wrote:
>> On 14/02/17 10:08 AM, Samuel Pitoiset wrote:
>>> Totally untested but as long as read_sensor() has been recently
>>> implemented for dpm based boards, amdgpu_sensors can now be
>>> exposed.
>>>
>>> v2: - make sure read_sensor is not NULL on dpm chips
>>>     - keep sanity check for powerplay chips
>>>
>>> Cc: Tom St Denis <tom.stdenis@amd.com>
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> index 6f021e70f15f..80821b436aeb 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>> @@ -3204,6 +3204,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct
>>> file *f, char __user *buf,
>>>      valuesize = sizeof(values);
>>>      if (adev->powerplay.pp_funcs &&
>>> adev->powerplay.pp_funcs->read_sensor)
>>>          r =
>>> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx,
>>> &values[0], &valuesize);
>>> +    else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
>>> +        r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
>>> +                        &valuesize);
>>>      else
>>>          return -EINVAL;
>>>
>>>
>>
>> Sorry NAK again, even with dpm=0 those function pointers are set and you
>> end up inside the dpm code trying to parse data structures that aren't
>> initialized.
>
> No worries. I thought that check was enough. Anyway, writing code
> without the hardware should be avoided. :)
>
> Can you try the thing suggested by Alex? Because I will need to fix up
> the DRM ioctl codepath as well.

Sure, I'm building a module with a amdgpu_dpm check near the top of the 
debugfs read function.

Tom

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

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 12:16 PM, Tom St Denis wrote:
> On 14/02/17 12:01 PM, Samuel Pitoiset wrote:
>> No worries. I thought that check was enough. Anyway, writing code
>> without the hardware should be avoided. :)
>>
>> Can you try the thing suggested by Alex? Because I will need to fix up
>> the DRM ioctl codepath as well.
>
> Sure, I'm building a module with a amdgpu_dpm check near the top of the
> debugfs read function.

It seems returning an error code causes it to do "bad things" on read

[root@kaveri linux]# git diff
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 80821b4..d63c443 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3198,6 +3198,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct 
file *f, char __user *buf,
         if (size & 3 || *pos & 0x3)
                 return -EINVAL;

+       if (amdgpu_dpm == 0)
+               return -EINVAL;
+
         /* convert offset to sensor number */
         idx = *pos >> 2;

That's what I applied on top of Samuel's three patches.

With DPM enabled it works fine obviously, with it disabled the entire 
system hangs.  I think probably because the VFS spins trying to read but 
never finishes...

Any ideas?

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

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 12:52 PM, Tom St Denis wrote:
> On 14/02/17 12:16 PM, Tom St Denis wrote:
>> On 14/02/17 12:01 PM, Samuel Pitoiset wrote:
>>> No worries. I thought that check was enough. Anyway, writing code
>>> without the hardware should be avoided. :)
>>>
>>> Can you try the thing suggested by Alex? Because I will need to fix up
>>> the DRM ioctl codepath as well.
>>
>> Sure, I'm building a module with a amdgpu_dpm check near the top of the
>> debugfs read function.
>
> It seems returning an error code causes it to do "bad things" on read
>
> [root@kaveri linux]# git diff
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 80821b4..d63c443 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3198,6 +3198,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct
> file *f, char __user *buf,
>         if (size & 3 || *pos & 0x3)
>                 return -EINVAL;
>
> +       if (amdgpu_dpm == 0)
> +               return -EINVAL;
> +
>         /* convert offset to sensor number */
>         idx = *pos >> 2;
>
> That's what I applied on top of Samuel's three patches.
>
> With DPM enabled it works fine obviously, with it disabled the entire
> system hangs.  I think probably because the VFS spins trying to read but
> never finishes...

Nevermind, the issue is umr reading UVD registers when dpm=0 on KV. 
Without sampling UVD the error works properly.

So add those three lines to your patch (and similar in the ioctl) you 
should be fine.

Cheers,
To
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH v2] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 12:52 PM, Tom St Denis wrote:
> On 14/02/17 12:16 PM, Tom St Denis wrote:
>> On 14/02/17 12:01 PM, Samuel Pitoiset wrote:
>>> No worries. I thought that check was enough. Anyway, writing code
>>> without the hardware should be avoided. :)
>>>
>>> Can you try the thing suggested by Alex? Because I will need to fix up
>>> the DRM ioctl codepath as well.
>>
>> Sure, I'm building a module with a amdgpu_dpm check near the top of the
>> debugfs read function.
>
> It seems returning an error code causes it to do "bad things" on read
>
> [root@kaveri linux]# git diff
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 80821b4..d63c443 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3198,6 +3198,9 @@ static ssize_t amdgpu_debugfs_sensor_read(struct
> file *f, char __user *buf,
>         if (size & 3 || *pos & 0x3)
>                 return -EINVAL;
>
> +       if (amdgpu_dpm == 0)
> +               return -EINVAL;
> +
>         /* convert offset to sensor number */
>         idx = *pos >> 2;
>
> That's what I applied on top of Samuel's three patches.
>
> With DPM enabled it works fine obviously, with it disabled the entire
> system hangs.  I think probably because the VFS spins trying to read but
> never finishes...

Nevermind, the issue is umr reading UVD registers when dpm=0 on KV. 
Without sampling UVD the error works properly.

So add those three lines to your patch (and similar in the ioctl) you 
should be fine.

Cheers,
Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Michel Dänzer]

On 15/02/17 01:17 AM, Deucher, Alexander wrote:
>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On Behalf
>> Of Samuel Pitoiset
>>
>>> Otherwise, it seems to work though the GPU_TEMP reads as "5000" on my
>>> Kaveri (meaning temp of 5C which isn't true).
>>>
>>> I suspect the dpm code is reading the wrong register to get the temp but
>>> we can fix that in another change later on.
>>
>> No idea. It was just a copy-n-paste. Maybe the initial code is buggy?
> 
> I don’t know that the GPU temp sensor on CI based APUs actually works
> properly.  I think the package temperature is exposed via the CPU
> thermals.

Seems to work fine with radeon on my two Kaveri laptops, FWIW.

On my desktop Kaveri, unplausibly low values (around 9-10C) are reported
at idle for both GPU and CPU. With load, both go up to around 30C, so it
seems like the values make sense in a relative sense but not in an
absolute one.


-- 
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/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Deucher, Alexander]

> -----Original Message-----
> From: Michel Dänzer [mailto:michel@daenzer.net]
> Sent: Tuesday, February 14, 2017 9:52 PM
> To: Deucher, Alexander; 'Samuel Pitoiset'; StDenis, Tom
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
> powerplay chips
> 
> On 15/02/17 01:17 AM, Deucher, Alexander wrote:
> >> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
> Behalf
> >> Of Samuel Pitoiset
> >>
> >>> Otherwise, it seems to work though the GPU_TEMP reads as "5000" on
> my
> >>> Kaveri (meaning temp of 5C which isn't true).
> >>>
> >>> I suspect the dpm code is reading the wrong register to get the temp but
> >>> we can fix that in another change later on.
> >>
> >> No idea. It was just a copy-n-paste. Maybe the initial code is buggy?
> >
> > I don’t know that the GPU temp sensor on CI based APUs actually works
> > properly.  I think the package temperature is exposed via the CPU
> > thermals.
> 
> Seems to work fine with radeon on my two Kaveri laptops, FWIW.
> 
> On my desktop Kaveri, unplausibly low values (around 9-10C) are reported
> at idle for both GPU and CPU. With load, both go up to around 30C, so it
> seems like the values make sense in a relative sense but not in an
> absolute one.

I think  I vaguely recall that it might actually be power rather than temperature.  Need to check with the SMU guys.

Alex

> 
> 
> --
> 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/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 14/02/17 09:56 PM, Deucher, Alexander wrote:
>> -----Original Message-----
>> From: Michel Dänzer [mailto:michel@daenzer.net]
>> Sent: Tuesday, February 14, 2017 9:52 PM
>> To: Deucher, Alexander; 'Samuel Pitoiset'; StDenis, Tom
>> Cc: amd-gfx@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
>> powerplay chips
>>
>> On 15/02/17 01:17 AM, Deucher, Alexander wrote:
>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
>> Behalf
>>>> Of Samuel Pitoiset
>>>>
>>>>> Otherwise, it seems to work though the GPU_TEMP reads as "5000" on
>> my
>>>>> Kaveri (meaning temp of 5C which isn't true).
>>>>>
>>>>> I suspect the dpm code is reading the wrong register to get the temp but
>>>>> we can fix that in another change later on.
>>>>
>>>> No idea. It was just a copy-n-paste. Maybe the initial code is buggy?
>>>
>>> I don’t know that the GPU temp sensor on CI based APUs actually works
>>> properly.  I think the package temperature is exposed via the CPU
>>> thermals.
>>
>> Seems to work fine with radeon on my two Kaveri laptops, FWIW.
>>
>> On my desktop Kaveri, unplausibly low values (around 9-10C) are reported
>> at idle for both GPU and CPU. With load, both go up to around 30C, so it
>> seems like the values make sense in a relative sense but not in an
>> absolute one.
>
> I think  I vaguely recall that it might actually be power rather than temperature.  Need to check with the SMU guys.


On my retail kaveri (with latest bios...) it seems to track with load 
but isn't really accurate (it'll jump double digits back and forth)

Eitherway though if Samuel updates the patches to include a check for 
amdgpu_drm we should be able to commit them.

I've already created a patch for umr to read them, just waiting on the 
kernel side to land.

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

Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

On 02/15/2017 04:01 PM, Tom St Denis wrote:
> On 14/02/17 09:56 PM, Deucher, Alexander wrote:
>>> -----Original Message-----
>>> From: Michel Dänzer [mailto:michel@daenzer.net]
>>> Sent: Tuesday, February 14, 2017 9:52 PM
>>> To: Deucher, Alexander; 'Samuel Pitoiset'; StDenis, Tom
>>> Cc: amd-gfx@lists.freedesktop.org
>>> Subject: Re: [PATCH] drm/amdgpu: expose amdgpu_sensors on pre-
>>> powerplay chips
>>>
>>> On 15/02/17 01:17 AM, Deucher, Alexander wrote:
>>>>> From: amd-gfx [mailto:amd-gfx-bounces@lists.freedesktop.org] On
>>> Behalf
>>>>> Of Samuel Pitoiset
>>>>>
>>>>>> Otherwise, it seems to work though the GPU_TEMP reads as "5000" on
>>> my
>>>>>> Kaveri (meaning temp of 5C which isn't true).
>>>>>>
>>>>>> I suspect the dpm code is reading the wrong register to get the
>>>>>> temp but
>>>>>> we can fix that in another change later on.
>>>>>
>>>>> No idea. It was just a copy-n-paste. Maybe the initial code is buggy?
>>>>
>>>> I don’t know that the GPU temp sensor on CI based APUs actually works
>>>> properly.  I think the package temperature is exposed via the CPU
>>>> thermals.
>>>
>>> Seems to work fine with radeon on my two Kaveri laptops, FWIW.
>>>
>>> On my desktop Kaveri, unplausibly low values (around 9-10C) are reported
>>> at idle for both GPU and CPU. With load, both go up to around 30C, so it
>>> seems like the values make sense in a relative sense but not in an
>>> absolute one.
>>
>> I think  I vaguely recall that it might actually be power rather than
>> temperature.  Need to check with the SMU guys.
>
>
> On my retail kaveri (with latest bios...) it seems to track with load
> but isn't really accurate (it'll jump double digits back and forth)
>
> Eitherway though if Samuel updates the patches to include a check for
> amdgpu_drm we should be able to commit them.

Sure, I will send a v3.

>
> I've already created a patch for umr to read them, just waiting on the
> kernel side to land.
>
> Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH v3] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

read_sensor() has been recently implemented for dpm based boards
which means amdgpu_sensors can now be exposed.

v2: - make sure read_sensor is not NULL on dpm chips
    - keep sanity check for powerplay chips
v3: - make sure amdgpu_dpm != 0

Cc: Tom St Denis <tom.stdenis@amd.com>
Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index 6f021e70f15f..d63c44383660 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -3198,12 +3198,18 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
 	if (size & 3 || *pos & 0x3)
 		return -EINVAL;
 
+	if (amdgpu_dpm == 0)
+		return -EINVAL;
+
 	/* convert offset to sensor number */
 	idx = *pos >> 2;
 
 	valuesize = sizeof(values);
 	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
 		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
+	else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
+		r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
+						&valuesize);
 	else
 		return -EINVAL;
 
-- 
2.11.1

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

Re: [PATCH v3] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Tom St Denis]

On 15/02/17 01:32 PM, Samuel Pitoiset wrote:
> read_sensor() has been recently implemented for dpm based boards
> which means amdgpu_sensors can now be exposed.
>
> v2: - make sure read_sensor is not NULL on dpm chips
>     - keep sanity check for powerplay chips
> v3: - make sure amdgpu_dpm != 0
>
> Cc: Tom St Denis <tom.stdenis@amd.com>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>

Cool, you can throw my R-b on this and the first patch (which adds the 
sensors).  Christian/Alex/etc can RB the DRM part.

Reviewed-by: Tom St Denis <tom.stdenis@amd.com>

Thanks,
Tom


> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> index 6f021e70f15f..d63c44383660 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
> @@ -3198,12 +3198,18 @@ static ssize_t amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>  	if (size & 3 || *pos & 0x3)
>  		return -EINVAL;
>
> +	if (amdgpu_dpm == 0)
> +		return -EINVAL;
> +
>  	/* convert offset to sensor number */
>  	idx = *pos >> 2;
>
>  	valuesize = sizeof(values);
>  	if (adev->powerplay.pp_funcs && adev->powerplay.pp_funcs->read_sensor)
>  		r = adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx, &values[0], &valuesize);
> +	else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
> +		r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
> +						&valuesize);
>  	else
>  		return -EINVAL;
>
>

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

Re: [PATCH v3] drm/amdgpu: expose amdgpu_sensors on pre-powerplay chips

[Samuel Pitoiset]

On 02/15/2017 07:34 PM, Tom St Denis wrote:
> On 15/02/17 01:32 PM, Samuel Pitoiset wrote:
>> read_sensor() has been recently implemented for dpm based boards
>> which means amdgpu_sensors can now be exposed.
>>
>> v2: - make sure read_sensor is not NULL on dpm chips
>>     - keep sanity check for powerplay chips
>> v3: - make sure amdgpu_dpm != 0
>>
>> Cc: Tom St Denis <tom.stdenis@amd.com>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>
> Cool, you can throw my R-b on this and the first patch (which adds the
> sensors).  Christian/Alex/etc can RB the DRM part.
>
> Reviewed-by: Tom St Denis <tom.stdenis@amd.com>

Thanks.

I'm improving the DRM part to expose more stuff (not only the clocks/temp).

>
> Thanks,
> Tom
>
>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 6f021e70f15f..d63c44383660 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -3198,12 +3198,18 @@ static ssize_t
>> amdgpu_debugfs_sensor_read(struct file *f, char __user *buf,
>>      if (size & 3 || *pos & 0x3)
>>          return -EINVAL;
>>
>> +    if (amdgpu_dpm == 0)
>> +        return -EINVAL;
>> +
>>      /* convert offset to sensor number */
>>      idx = *pos >> 2;
>>
>>      valuesize = sizeof(values);
>>      if (adev->powerplay.pp_funcs &&
>> adev->powerplay.pp_funcs->read_sensor)
>>          r =
>> adev->powerplay.pp_funcs->read_sensor(adev->powerplay.pp_handle, idx,
>> &values[0], &valuesize);
>> +    else if (adev->pm.funcs && adev->pm.funcs->read_sensor)
>> +        r = adev->pm.funcs->read_sensor(adev, idx, &values[0],
>> +                        &valuesize);
>>      else
>>          return -EINVAL;
>>
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx