[amdgpu] PowerPlay table parsing requires an extra field

[valpackett]

The pp_od_clk_voltage sysctl is described here (see also e.g. WattmanGTK code). Setting a P-state works like this on Linux:

#   "s state clock voltage"
# echo "s 0 300 750" > /sys/class/drm/card0/device/pp_od_clk_voltage

However, something in the parsing goes wrong on our end when trying to set the corresponding sysctl to that string: (I added extra debug output with values)

amdgpu: [powerplay] invalid clock voltage input: i=0 + 3 > size=2 || input[i]=1090 >= count=4

It's seeing 2 fields instead of 3, using the value of the 3rd field as 2nd…

So currently, an extra blank field is required:

sysctl sys.device.drmn0.pp_od_clk_voltage="m _ 3 1090 1000"

Maybe our strsep doesn't do the same thing as the Linux one or something…

[hselasky]

static ssize_t amdgpu_set_pp_od_clk_voltage(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) {

Can you add some prints in amdgpu_set_pp_od_clk_voltage() to print the parameter array in question like this: uint32_t xx; for (xx = 0; xx != parameter_size; xx++) printf("parameter %d = %d\n", (int)xx, (int)parameter[xx]); Right after the while (temp_str[0]) loop ?

[valpackett]

tmp_str = 'm 3 1090 1000
'
[ifedit] tmp_str = 'm 3 1090 1000
'
[isspace] tmp_str = '1090 1000
'
parameter 0 = 1090
parameter 1 = 1000

whoops, the second field actually gets chopped off by

    while (isspace(*++tmp_str));

[valpackett]

OH right if isspace from ctype.h is used, it would expand to

((*++tmp_str) == ' ' || ((*++tmp_str) >= '\t' && (*++tmp_str) <= '\r'))

[hselasky]

I wonder if we should fix this upstream!

[hselasky]

Hi, Can you test this kernel patch?

https://reviews.freebsd.org/D19694

--HPS

[valpackett]

Yes, that works, thanks!

[hselasky]

https://svnweb.freebsd.org/changeset/base/345499 Will land in 11- and 12- soon.