BeardOverflow / msi-ec

GNU General Public License v2.0
134 stars 41 forks source link

MSI Delta 15 support added, install instruction update to dkms-install #107

Closed peterwolf4 closed 5 months ago

peterwolf4 commented 5 months ago

-- Additional description

glpnk commented 5 months ago

Other looks good. All features work?

Source #41

Look like 15CKEMS1.108 is single mentioned version in #41

glpnk commented 5 months ago

@teackot What you think about installation guide? Maybe split it for 2 guides before 6.2 and after, with dkms? Or backports could contain msi-ec module?

glpnk commented 5 months ago

@peterwolf4 Thanks. Anyway, we need to wait for Teackot's suggestions about updating the guide and merging. So you can update this PR later

peterwolf4 commented 5 months ago

@teackot What you think about installation guide? Maybe split it for 2 guides before 6.2 and after, with dkms? Or backports could contain msi-ec module?

I think that would be very helpful, especially for beginner who are working their way into this topic. Maybe update to include a check if the system uses dkms, also if msi-ec is already running could be added.. Then again, if users are not comfortable with the low level basics it might be better to not have them contribute eagerly and play around with it too much? In the end I also have little experience and still was able to pool all contributions from the open issue and it works great; so I'd always emphasize making contribution as easy as possible to gather as much info about msi laptops as possible. Maybe readme should include a search command to see if msi-ec is already installed with the current kernel, thinking about something simple along the lines of lsmod | grep msi_ec; checking if dkms is available with which dkms or if its known from which kernel version on dkms is supported maybe just hint towards uname -a for a manual check. Lets see how it'll all come together in the end :)

glpnk commented 5 months ago

I think the simplest way to check for in-kernel module by using modinfo msi-ec. If module is signed (and secure boot is off), then used built-in module. If version is returned - it's custom.

Usage of dkms is more versatile solution, because it reinstalls module after updates.

New guide is good

teackot commented 5 months ago

DKMS method would work in any case for every user, right? It is also preferable because the user doesn't need to rebuild the driver manually every kernel update.

How about adding a quick description in the beginning of the github installation section telling the user that DKMS method is probably what they're looking for but they can use regular installation method if they wish and if they don't have msi-ec bundled with the kernel.

Then the prerequisites section listing the dependencies including (optional) DKMS

Then the "Using DKMS" section, and finally the classic method (no dkms)

glpnk commented 5 months ago

DKMS is pretty old, so I think it may be included in every Linux distro.

because the user doesn't need to rebuild the driver manually every kernel update

Technically yes, but sometimes it just breaks (maybe because I use Xanmod and modules require GCC-13, when system use 12 version. Or they just binary incompatible, which is weird). Maybe we need to improve build scripts.

Then the prerequisites section listing the dependencies including (optional) DKMS

With a note that DKMS is already present, in many cases

teackot commented 5 months ago

DKMS is pretty old, so I think it may be included in every Linux distro.

It actually isn't. It is included in Ubuntu (and derivatives) but not in Fedora, Manjaro or Debian

peterwolf4 commented 5 months ago

In any case, I think it'd be best to aid users towards using DKMS, simply because the sudo make install error that appears when trying to replace a running msi-ec module instance may be misleading, see my older comment about the modprob Operation not supported error. Also in this context: the kernel error message (sudo dmesg) was misleading imo, simply stating: firmware 'not supported', which hints towards the wrong direction (since I added the exact firmware nr, so it technically was supported already, just couldn't replace the currently running msi-ec without unloading it before, I assume). Then using dkms-install which I found on another issue on here worked without any other changes, so I could imagine quite a few users could get lost on the way and stop trying if they do not find the eagerness to search through open & closed issues.

I will update this fork to recommend the dkms-install first, and we'll see how readable it will be in the end :)

peterwolf4 commented 5 months ago

I think the simplest way to check for in-kernel module by using modinfo msi-ec. If module is signed (and secure boot is off), then used built-in module. If version is returned - it's custom.

I now sticked with lsmod | grep msi_ec since it shows only modules which are currently running. Since eg. Arch kernel comes bundled with msi-ec, modinfo msi-ec still showed results after I removed msi-ec from the current modules with modprobe -r msi-ec, hence I think using lsmod is more reliable overall, even though the output is less detailed. I tried to make the installs as verbose as necessary from my pov, esp. since I just recently joined the project I think these additional inputs would've cleared a lot of issues I've had on my way. Looking forward to your feedback! @teackot @glpnk

teackot commented 5 months ago

The make install method won't work if the module is present in the system, no matter if it is running or not, because the module bundled with the kernel has a greater priority and you won't be able to load the makeinstalled module

DKMS, for some reason, has a priority over the in-tree modules - that's why it works

peterwolf4 commented 5 months ago

mh interesting, I tried it and with the combination of removing the previous kernel module & I had (now) no issues to install using sudo make install.. So you mean it should fail on that step already if I understand you right?

teackot commented 5 months ago

It should fail on the modprobe msi-ec step

peterwolf4 commented 5 months ago

okay.. I will try to verify including a couple of reboots to make sure previous configurations are not somehow still loaded

peterwolf4 commented 5 months ago

okay didn't took nearly as long as anticipated, you are right; not sure why it did before, probably due to missing reboots. Any other tips to improve the install description?

glpnk commented 5 months ago

DKMS, for some reason, has a priority over the in-tree modules - that's why it works

DKMS just archive built-in version and replace it with custom. You can manually replace built-in module with custom. But it placed in different directory, DKMS just place custom module in /extra not something like /platform/x86/???

teackot commented 5 months ago

What do you think of separating the instruction into "building" and "installing" sections for the sake of readability, or is this an overkill?

Something like this:

Screenshot from 2024-01-31 22-49-30

peterwolf4 commented 5 months ago

Honestly, love it! Just noticed you'll need to fix the typo dirver in the first line :) Reads a little nicer because now the subsectioning of dkms and no dkms is much clearer separated, compared to before imo.

glpnk commented 5 months ago

Maybe DKMS not require building step, now I check this after kernel update

Also, we need to remove E1592IMS from Summit E16 Flip A12UCT / A12MT (E1592IMS, 1592EMS1) (in readme). And possible add info about firmware namings from my #98 analysis if it's necessary.

teackot commented 5 months ago

Maybe DKMS not require building step, now I check this after kernel update

Actually, you're right (just tested it too). DKMS (re)builds the module automatically, that's the whole point of DKMS

peterwolf4 commented 5 months ago

then we can move the make step into the old, non dkms description. Also I will update your suggestion @glpnk and link the discussion into the readme for more insight on this, somehow missed your comprehensive overview so far (since it was linked nowhere?).

teackot commented 5 months ago

Maybe DKMS not require building step, now I check this after kernel update

Screenshot from 2024-01-31 23-09-53

peterwolf4 commented 5 months ago

Are you sure it will run without installing system wide package dependencies? Honestly quite doubtful right now but might be wrong (and hard to test) as I definitely have less expertise :)

glpnk commented 5 months ago

I've tried to hardcode gcc-13 in makefile to fix the issue with broken module after kernel update. But for me is simpler to run this every time when it stops working:

sudo make dkms-install CC=gcc-13

@peterwolf4: and link the discussion into the readme

I think this is unnecessary, because this discussion contain many unstructured info

peterwolf4 commented 5 months ago

At the very least cloning the repo should be above, so I'd suggest keeping 1. and 2. in place, then split into recommended dkms install, & push the make command downwards into classic install

teackot commented 5 months ago

Are you sure it will run without installing system wide package dependencies

Oh, I didn't consider that

peterwolf4 commented 5 months ago

@teackot I gave you write permission already btw. so you should be able to commit the readme to my fork, then we can continue to implement @glpnk update on the device naming. I still think we should at least link your quite in depth discussion post somewhere down there to have more awareness on it for everyone eager to contribute to the project.

teackot commented 5 months ago

I gave you write permission already btw.

Got it

Thoughts?:

Screenshot from 2024-01-31 23-26-26

peterwolf4 commented 5 months ago

@teackot looking very good now imo, if you want you can also implement the suggested change from @glpnk already? Else I can do it after you commited the new installs to my fork :)

Also, we need to remove E1592IMS from Summit E16 Flip A12UCT / A12MT (E1592IMS, 1592EMS1) (in readme). And possible add info about firmware namings from my #98 analysis if it's necessary.

teackot commented 5 months ago

README.md update: changed Summit E16 Flip ec fw description

A couple of seconds ahead of me =)

glpnk commented 5 months ago

I think that info about firmware naming needs to be part of wiki and link to it mentioned in Readme

peterwolf4 commented 5 months ago

Great, thanks for your work @teackot & @glpnk ! Really loving the project. Do you know by chance how to customize maximum usage? eg. predetermined msi profiles seem to be defined in available........ but where are these profiles comming from? Used to take corectrl but it still struggles with the new 6.7 kernel for now, so I just thought I'd ask now that we're in contact. Also, is there any help regarding customizing the advanced fan curve? Was missing some info on that so it may be useful to include it on the readme as well, just a thought (I know Mcontrol center may be able to do it once they start prioritizing msi-ec?)

peterwolf4 commented 5 months ago

I think that info about firmware naming needs to be part of wiki and link to it mentioned in Readme

Long time @glpnk : I added you to to be able to directly contribute your thoughts to this fork :) Maybe summarize your most critical takes under a new section: 'what to read into if you want to contribute'? I am, honestly, not anywhere near your expertise on EC related things, but getting an overview and setting up documentation neatly is quite my thing. So if you want just contribute your hot takes and I can try to implement them into the readme as well, however you like!

teackot commented 5 months ago

but where are these profiles comming from?

They come from the MSI official app. Each entry of available...... is a state that can be achieved by changing different settings in the app.

peterwolf4 commented 5 months ago

They come from the MSI official app. Each entry of available...... is a state that can be achieved by changing different settings in the app.

Ughh just what I feared... so I'd actually have to reinstall windows to make new settings available I guess? Funny how linux (wannabe) chads like me would go any length into learning completely low level hardware related info but reinstalling windows is still like a safeword to me. Maybe someone else will be willing to provide better tuned advanced fancurves (they are imo not fast enough, laptop still gets too hot, basically just as good / copy paste of the auto ones..) Thanks though, know this was rather off topic! :)

teackot commented 5 months ago

Also, is there any help regarding customizing the advanced fan curve?

We have a discussion here: #15. I think we still haven't even decided how to expose it to userspace

glpnk commented 5 months ago

Fan curve requires research about Linux hwmon API related to fan control.

@peterwolf4 Thanks, but I don't know what to summarize. Now I'm trying to analyze EC from DSDT code side by using ImHex. But for now, I can assume that exists at least 2 EC maps. But I don't know how App knows about available features. At the same time, these 2 maps used on different hardware sets and looks kinda standard.

glpnk commented 5 months ago

My laptop is overheating on video play, so I just set CPU Governor to Conservative and max frequency equal base clock frequency

peterwolf4 commented 5 months ago

I see, if you want any help regarding this (outside of the need for a dual boot windows setup), I'd be more than willing to help with whatever I can provide! I think it'd make the module much more viable, esp for tools such as MControlCenter (though I might be mistaken ofc), but let alone for powerusers who just want to script their own settings: it'd be heaven. :) @teackot Btw. I do remember some cli tools who handled user given fan control exactly as you suggested in that issue (4. method), I think it would be easiest for both ends (assuming its easiest to implement). Don't remember how the tool was called sadly, else I'd forward it. @glpnk Thanks for the hint, maybe I will read into it a little tomorrow. Maybe we should then wait with another readme update and accept this fork as finished? Afterall the main reason for it to exist in the first palce is also finished, we could fork another version to discuss the implementation of more help regarding ec fw naming etc once you feel comfortable adding. I will for now just include a small link to the discussion thread to raise awareness, guess that'd be a fine solution for now.

Also, couldn't find a reliable way to underclock / undervolt my cpu or gpu since the new kernel is not well supported apparently. Any tools you'd recommend to set max frequency? I tried pairing shift mode eco with auto fan but still gets too hot afer ~30 mins of spiked load (gaming with vulkan).

glpnk commented 5 months ago

[[[Ahhh ImHex again SegFaulted during video recording...]]]

Screencast from 01.02.2024 00:24:26.webm

Basically, DSDT contain description for some registries. But many settings exported as byte that can contain different settings. But CPU Shift is controlled inside DSDT/SSDT tables

teackot commented 5 months ago

Also, couldn't find a reliable way to underclock / undervolt my cpu or gpu since the new kernel is not well supported apparently.

For undervolting, I've been using intel-undervolt (not using it right now though)

glpnk commented 5 months ago

I think we're done with this fork.

I use cpupower-gui https://github.com/vagnum08/cpupower-gui for underclocking, and AMD-Pstate driver in amd_pstate=passive

teackot commented 5 months ago

You can always make another PR, so yeah, I think we can merge this

peterwolf4 commented 5 months ago

Nice, was already adding this last commit; before I read your reply. Check if you both align with it but I think emphasizing help always is a good hint that anybody can start into such projects :) Just added links for wiki and the discussion. Otherwise I am also done with this fork, also thanks for the headsup on the underclocking! I will look into it in the future.

peterwolf4 commented 5 months ago

Nice, work & thanks for your time! Hope to hear from you both in the future.

teackot commented 5 months ago

Great job!