[amdgpu_linux] Add widget type #115
Conversation
| _data["{gpu_usage}"] = "N/A" | ||
| end | ||
|
|
||
| if _mem == nil then |
There was a problem hiding this comment.
My Lua is rusty now but I suppose _mem == nil should always be true here.
There was a problem hiding this comment.
yeah, I wanted to put that variable in a closure based on the assumption that the max vram wouldn’t change on subsequent calls, but atm it’s a local variable which means it’s called every time, so it could be optimized there, I admit I’m not a lua expert either, how would you do that, define that variable outside of the function ? In practice I don’t notice a negative performance effect but I don’t update the associated widget as often as the performance graph.
There was a problem hiding this comment.
I wanted to put that variable in a closure based on the assumption that the max vram wouldn’t change on subsequent calls, [...], how would you do that, define that variable outside of the function ?
Yes, some other widgets also do it.
widgets/amdgpu_linux.lua
Outdated
| f = io.open(amdgpu .. "/mem_info_vram_used", "r") | ||
| if f then | ||
| for line in f:lines() do | ||
| _data["{mem_usage}"] = line/_mem*100 |
There was a problem hiding this comment.
I have a feeling line and _mem should be checked to be numbers instead of propagating an attempt to div error to naughtification.
There was a problem hiding this comment.
Looking at the specs, it seems mem_info_vram_total and mem_info_vram_used should exist together, but at least one should be checked with tonumber. They should also consist of only one line, thus f:read'*l' can be preferred for better communicating the intention.
|
Thanks for the review, I didn’t use everything that the open source amdgpu driver provides, but yeah, a comment pointing to this documentation would probably be helpful : https://www.kernel.org/doc/html/v5.9/gpu/amdgpu.html |
|
@McSinyx here is what I came up with, I checked with strace now mem_info_vram_total is only read once, I checked for numbers and prevent division by zero and added a link to the documentation. I missed your comment about using f:read, just modified that as well. |
- declare _mem outside of function so that total ram is checked only once (performance improvement) - use string.match and tonumber and check for division by zero - add link to kernel doc
|
😅 I failed my own deadline but happy new year nevertheless 🎆 |
|
thank you man, happy new year! |
Hi guys,
I’ve had this vicious widget I made about 2 years ago, I use both of the metrics in my config, one with a progressbar and the other with a graph, it works quite well for me. It’s been a long time since I last contributed to vicious (12 years!), hope you guys like this one !
Merry Christmas,
Regards,