Skip to content

Add Lenovo ThinkPad P14s Intel Gen 2 #1509

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

borja-rojo-ilvento
Copy link

Description of changes

Added a configuration for the Lenovo ThinkPad P14s Gen 2 (which is my current daily driver). Seems it just needed a PCI bus adjustment. It uses an NVidia T500 and Intel Xe graphics. It mimics the implementations for the Gen 3 and Gen 5 models otherwise.

Things done
  • Tested the changes in your own NixOS Configuration

  • Tested the changes end-to-end by using your fork of nixos-hardware and
    importing it via <nixos-hardware> or Flake input

  • Ran nix run ./tests#run . and passes

@borja-rojo-ilvento borja-rojo-ilvento marked this pull request as draft June 20, 2025 23:37
@borja-rojo-ilvento
Copy link
Author

Found the issue. It was an omission of the common/gpu/intel/tiger-lake module, which I believe will include the relevant media drivers.

@borja-rojo-ilvento borja-rojo-ilvento marked this pull request as ready for review June 22, 2025 21:36
@borja-rojo-ilvento
Copy link
Author

@mergify queue

Copy link
Contributor

mergify bot commented Jun 22, 2025

queue

❌ Command disallowed due to command restrictions in the Mergify configuration.

  • sender-permission >= write

@Mic92 Mic92 force-pushed the lenovo-thinkpad-p14s-intel-gen2 branch 2 times, most recently from 81d46f2 to eb74272 Compare July 1, 2025 14:28
imports = [
../../../../../common/gpu/nvidia/prime.nix
../../../../../common/gpu/nvidia/turing
../../../../../common/cpu/intel/tiger-lake
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the cpu profile also includes gpu modules. I had it in my memories that the "modesettings" driver was included by default in xserver? That's why I dropped it for now. However please correct me on that in which case I would add this rather to the intel gpu profile than here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, so I have done testing on this previous, and just double checked the effect of including or excluding services.xserver.videoDrivers = [ "modesetting" ]; with your other changes. We can indeed exclude the redundant tiger-lake gpu module.

Originally, I found that excluding it caused the computer to hang when changing into suspend mode. After lots of research, I understand that this has to do with the hand-off failing on systems with a discrete GPU, such as thing one. This is also noted in the wiki, here.

A quick rg \"modesetting\' call in the repo yeilds:

raspberry-pi/4/modesetting.nix
109:      "modesetting" # Prefer the modesetting driver in X11

raspberry-pi/5/default.nix
24:    Driver "modesetting"

common/gpu/amd/default.nix
6:    services.xserver.videoDrivers = lib.mkDefault [ "modesetting" ];

lenovo/thinkpad/p14s/intel/gen2/default.nix
19:  services.xserver.videoDrivers = [ "modesetting" ];

So, it turns out that this isn't actually set. Ostensibly, this 'should be set at needed, implicitly', but reading the wiki, it seems that the 'modesetting' functionality requires different drivers depending on what CPU you have, either AMD or Intel. This means that the common/gpu/nvidia/prime.nix module which defines PRIME Offload needs to detect which CPU type you have, and include the correct driver accordingly. Or, a different solution that is CPU aware which I am not well-versed enough to design and implement...

I believe, that although services.xserver.videoDrivers does include "modesetting", as per this options search, the common/gpu/nvidia/default.nix module overrides this with services.xserver.videoDrivers = lib.mkDefault [ "nvidia" ];, so that the default now excludes "modesetting" and "fbdev".

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a follow-up, I'm going to see what happens if I change services.xserver.videoDriver = mkDefault [ "nvidia" ]; to services.xserver.videoDriver = [ "nvidia" ]; and see if Nix will just add the "nvidia" drivers, so I don't need to 'add back' the "modesetting" drivers.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That didn't seem to work. Suspend still breaks.

I am adding back in the "modesetting" driver. I'll add the link to the Wiki in the upcoming README for clarification.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Probably a lot of our hybrid configurations are wrong than. I never had a device dual intel/nvidia so far.

../.
];

# For suspending to RAM to work, set Config -> Power -> Sleep State to "Linux S3" in EFI.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could be a candidate for a README... but not a mandatory change for now.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A README in the some directory as this module? I wouldn't think this would fit in the general README haha

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. We have per model READMEs usually containing information/quirks that cannot be automated away.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make one. I think it's good form.

@borja-rojo-ilvento borja-rojo-ilvento force-pushed the lenovo-thinkpad-p14s-intel-gen2 branch from eb74272 to c7cf8e8 Compare July 1, 2025 16:20
Copy link
Author

@borja-rojo-ilvento borja-rojo-ilvento left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the next iteration, and includes back the services.xserver.videoDrivers = [ "modesetting" ]; addition.

../.
];

# For suspending to RAM to work, set Config -> Power -> Sleep State to "Linux S3" in EFI.
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll make one. I think it's good form.

imports = [
../../../../../common/gpu/nvidia/prime.nix
../../../../../common/gpu/nvidia/turing
../../../../../common/cpu/intel/tiger-lake
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That didn't seem to work. Suspend still breaks.

I am adding back in the "modesetting" driver. I'll add the link to the Wiki in the upcoming README for clarification.

@borja-rojo-ilvento
Copy link
Author

Bump.

Are we OK to accept this as-is?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants