brndnmtthws / conky

Light-weight system monitor for X, Wayland (sort of), and other things, too
https://conky.cc
GNU General Public License v3.0
7.24k stars 620 forks source link

${nvidiabar mtrfreq[cur]} is always at 100% #1177

Closed z1atk0 closed 2 months ago

z1atk0 commented 2 years ago

Issue

An ${nvidiabar mtrfreq[cur]} is always drawn full, ie. at 100%. This is due to a logic bug in the get_nvidia_barval() function in src/nvidia.cc:

      case ATTR_FREQS_STRING:  // mtrfreq (calculate out of memfreqmax)
        if (strcmp(nvs->token, "memTransferRate") != 0) {
          // Just in case error for silly devs
          CRIT_ERR(nullptr, NULL,
                   "%s: attribute is 'ATTR_FREQS_STRING' but token is not "
                   "\"memTransferRate\" (arg: '%s')",
                   nvs->command, nvs->arg);
          return 0;
        }
        temp1 =
            get_nvidia_string_value(nvs->target, ATTR_FREQS_STRING, nvs->token,
                                    SEARCH_MAX, nvs->target_id, nvs->arg);
        temp2 = get_nvidia_string_value(nvs->target, ATTR_PERFMODES_STRING,
                                        (char *)"memTransferRatemax",
                                        SEARCH_MAX, nvs->target_id, nvs->arg);
        if (temp2 > temp1) temp1 = temp2;  // extra safe here
        value = ((float)temp1 * 100 / (float)temp2) + 0.5;
        break;

My GPU has three different performance levels 0, 1 and 2, with corresponding Graphics Clock & Memory Transfer Rate frequencies of 324 MHz & 648 MHz, 549 MHz & 3600 MHz, and 928 MHz & 5400 MHz, respectively: Screenshot from 2022-02-24 17-58-39

In terms of conky variable values, this corresponds to ${nvidia mtrfreqmin} = 648, ${nvidia mtrfreqmax} = 5400, and ${nvidia mtrfreq[cur]} being either 648, 3600 or 5400, depending on what performance mode the GPU is currently in. Now with this in mind, the logic bug immediately becomes obvious:

        temp1 =
            get_nvidia_string_value(nvs->target, ATTR_FREQS_STRING, nvs->token,
                                    SEARCH_MAX, nvs->target_id, nvs->arg);
        temp2 = get_nvidia_string_value(nvs->target, ATTR_PERFMODES_STRING,
                                        (char *)"memTransferRatemax",
                                        SEARCH_MAX, nvs->target_id, nvs->arg);
        if (temp2 > temp1) temp1 = temp2;  // extra safe here

temp1 gets initialized with the current MTR frequency (ie. 648, 3600 or 5400), and temp2 gets initialized with the maximum possible MTR frequency the card is capable of, ie. 5400. Therefore, the if() clause is always true (except when the card is actually running at 5400 MHz, but then temp1 and temp2 are already equal anyway), temp1 gets set to 5400, and the bar value is always 5400/5400 = 1.00 or 100%. Changing the if() clause to

        if (temp1 > temp2) temp1 = temp2;  // extra safe here

(which probably was the original intention anyway, I guess) fixes the problem.

Information

conky-1.12.2, X.Org-1.18.3, NVIDIA-Linux-x86-390.147, NVIDIA-libXNVCtrl-390.138, gcc-/g++-9.2.0 on Slackware-14.2 (32bit).

conky.text = [[
[...]
$font0 GPU: ${color grey}${nvidia temp}°C @ Mode ${nvidia perfmode}, Level ${nvidia perflevelcur} ${nvidiagraph temp 8,144 00ff00 ff0000 100 -t 0}
$color Util%:   $color Fan: ${color grey}${nvidia fanlevel}% @ ${nvidia fanspeed}rpm ${nvidiabar 6,0 fanlevel}
  ${color grey}GPU    ${nvidiabar 6,120 gpuutil}$color Freq (MHz):${color grey}
  MemBW  ${nvidiabar 6,120 membwutil} GPU ${nvidiabar 6,0 gpufreqcur}
  VEng   ${nvidiabar 6,120 videoutil} Mem ${nvidiabar 6,0 memfreqcur}
  PCIeBW ${nvidiabar 6,120 pcieutil} MTR ${nvidiabar 6,0 mtrfreqcur}
$color Mem: ${color grey}${nvidia mem} MB (${nvidia memutil}%) used, ${nvidia memavail} MB free, ${nvidia memtotal} MB total
  ${nvidiagraph memutil 8,378 00ff00 ff0000 100 -t 0}
[...]
]];
github-actions[bot] commented 1 year ago

This issue is stale because it has been open 365 days with no activity. Remove stale label or comment, or this issue will be closed in 30 days.

z1atk0 commented 1 year ago

Still present in conky-1.18.1.

github-actions[bot] commented 7 months ago

This issue is stale because it has been open 365 days with no activity. Remove stale label or comment, or this issue will be closed in 30 days.

z1atk0 commented 7 months ago

Still present in conky-1.19.8.

brndnmtthws commented 7 months ago

Would you like to open a PR for this?

z1atk0 commented 7 months ago

I'm not very git savvy, but I'll try & give it a go. Maybe I'll even manage before the next "issue is stale" warning comes around. :slightly_smiling_face:

w0j0pl commented 2 months ago

Is there still a need to change this? I can do PR with the proposed change if needed ;)

z1atk0 commented 2 months ago

From my point of view, yes, the problem is still present in the current release. I keep the patch around locally to fix this, that's why I still haven't got around yet to do the PR myself - whenever a new version is released, I just apply the patch and be done with it. Much easier for me than fooling around with PRs (and git in general, as I said :innocent:).

So yes, if you could do the PR it would be very much appreciated. :+1: :slightly_smiling_face:

w0j0pl commented 2 months ago

Would you like to open a PR for this?

I made a commit. It's my first ever commit to the project like that, so I don't know if I did everything correct. I didn't change a doc, because imo there is nothing to change. I also didn't test it, because tbh, I have no idea how should I do this, but as @z1atk0 said, he made this change by himself locally, so I guess, he tested this.

brndnmtthws commented 2 months ago

Should (hopefully) be fixed with #2018