Skip to content
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

Dell XPS 15 9530 #881

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Dell XPS 15 9530 #881

wants to merge 4 commits into from

Conversation

bashfulrobot
Copy link

@bashfulrobot bashfulrobot commented Mar 3, 2024

I am just beginning to create support for a Dell XPS 15 9530.

I am not yet done with all the code, but I wanted to start the PR to ensure my work meets the needed standards. I am basing off the 9520 and have already been testing by leveraging the common components directly in my local config.

My first question:

The common/GPU/Nvidia component seems to overlap with what is in the wiki for Nvidia. If a person adds the Nvidia configuration from this repo, is that all the base configuration a consumer needs, or would they still need to follow the WIKI? I want to clarify that in the readme for this model. (It always confuses me with my other machines that consume from here.) For example, I will see people manually create the offload scripts in their config, but I know the Nvidia component will also do that.

Thank you.

- Add new file `dell/xps/15-9530/README.wiki`
- Specify tested hardware
- Add notes section
- Add NVIDIA Offload section

[dell/xps/15-9530/README.wiki]
- Add a new file `dell/xps/15-9530/README.wiki`
- Specify the tested hardware:
CPU, RAM, HDD, Screen, Graphics, Input
- Add a section for notes
- Add a section for NVIDIA Offload
@bashfulrobot bashfulrobot marked this pull request as draft March 4, 2024 05:32
reader and Touchpad preference in Gnome Settings

[dell/xps/15-9530/README.wiki]
- Added documentation for Goodix fingerprint reader
- Noted that the fingerprint reader has only been
tested on Gnome
- Mentioned that a reboot is required for
fingerprint options to appear in Gnome Settings
- Registered fingerprints in the Users module in
Gnome Settings
- Set the password as the login method to unlock
the keyring
- Enabled the preference to disable the Touchpad
while typing
- Recommended disabling the Touchpad during typing
showing an example using Home-Manager and `dconf`
- Added new `default.nix` file in `dell/xps/15-9530/fingerprint` directory
- Enabled `services.fprintd` service in the configuration
- Set up `tod` driver for fingerprint scanner
- Allowed password authentication for initial password login
- Configured fingerprint authentication for GDM login screen

[dell/xps/15-9530/fingerprint/default.nix]
- Add a new file `default.nix` in the `dell/xps/15-9530/fingerprint` directory
- Enable the `services.fprintd` service in the configuration
- Set up the `tod` driver for the fingerprint scanner
- Configured fingerprint authentication for the initial password login
- Configure fingerprint authentication for the GDM login screen
- Added `default.nix` file for Dell XPS 15-9530 configuration
- Imported necessary files for CPU, laptop, SSD, and fingerprint
- Enabled thermald service
- Configured iwlwifi module with power saving option
- Removed commented out code for `disable_11ax` option
- Added `default.nix` file for NVIDIA configuration in Dell XPS 15-9530
- Imported `default.nix` file from parent directory and NVIDIA prime configuration file
- Configured `hardware.nvidia.prime` with Bus IDs of Intel and NVIDIA GPUs

[dell/xps/15-9530/default.nix]
- Add a new file `dell/xps/15-9530/default.nix`
- Import the following files: `../../../common/cpu/intel`, `../../../common/pc/laptop`, `../../../common/pc/laptop/ssd`, and `./fingerprint`
- Enable thermald service
- Add configuration for iwlwifi module with `options iwlwifi power_save=1`
- Remove commented out code for `disable_11ax` option
[dell/xps/15-9530/nvidia/default.nix]
- Added a new file `default.nix` in the directory `dell/xps/15-9530/nvidia/`
- The file `default.nix` includes two imports: `../default.nix` and `../../../../common/gpu/nvidia/prime.nix`
- Added a configuration for `hardware.nvidia.prime` which includes the Bus IDs of the Intel and NVIDIA GPUs.
@bashfulrobot
Copy link
Author

I have become blocked on this item (troubleshooting Nvidia). I don't understand how to merge the settings properly to build out properly for this repo. I will build this out as a traditional config for now, and if another person has a 9530 (and a better experience getting the Nvidia working correctly), I will pick this back up.

I defer to the community whether we should leave this open or close it out.


# Used to allow a password login on first login as an alternative to just a fingerprint
security.pam.services.login.fprintAuth = false;
# similarly to how other distributions handle the fingerprinting login
Copy link
Member

Choose a reason for hiding this comment

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

It's not obvious to me just now how this is different from what NixOS is currently doing.

Copy link
Author

Choose a reason for hiding this comment

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

I likely grabbed that when I was referencing other configuration examples. I should remove it.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I just tested, and this is how it affects the behaviour.

Without that line, you cannot enter a password when first logging in. It is grayed out. You "have" to use the fingerprint. But then it errors out. After the error, you can enter the password. It very much feels like a broken flow and could be confusing to some users.

By adding this, you remove the failed fingerprint error. You can then use the expected login flows (fingerprint or password). Obviously, if you log in via fingerprint (first login), you will still have to enter your keychain password at some point.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. This is than generally broken in nixpkgs, right?

Copy link
Member

Choose a reason for hiding this comment

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

I will try to reproduce this behavior on a different laptop with Gnome.

Copy link

Choose a reason for hiding this comment

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

I tested this behaviour with KDE Plasma 6 (SDDM), NixOS 24.05 on a XPS 15 9520.

This setting disables the possibility to login using a fingerprint.

The default behaviour, using only services.fprintd.enable = true; requires an interaction from the fingerprint reader after a password has been provided, and then checks whether either is valid: I can provide an invalid or empty password (just press Enter) followed by a valid fingerprint reading to login, or a valid password and any fingerprint reading.

This default behaviour seems better than disabling the fingerprint by default.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think if we want the fingerprint behaviour to be changed, we should upstream to nixpkgs. This is not directly hardware related

# Used to allow a password login on first login as an alternative to just a fingerprint
security.pam.services.login.fprintAuth = false;
# similarly to how other distributions handle the fingerprinting login
security.pam.services.gdm-fingerprint =
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
security.pam.services.gdm-fingerprint =
security.pam.services.gdm-fingerprint = lib.mkDefault

Copy link
Contributor

Choose a reason for hiding this comment

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

Just by glancing at this, I cannot tell what it does, but it does not look hardware related.

Comment on lines +9 to +12
tod = {
enable = true;
driver = pkgs.libfprint-2-tod1-goodix;
};
Copy link

Choose a reason for hiding this comment

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

Is this section required on the 9530 ? The service works find on 9520 without specifying the tod.

@lasandell
Copy link

I just tried to test this, but the module needs to be added to the flake.nix of this repo to make it usable from flakes-based nixos configs.

Copy link
Contributor

@Lyndeno Lyndeno left a comment

Choose a reason for hiding this comment

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

I believe the settings modifying the default fingerprint behaviour should be removed as it is not hardware related.

If the default behaviour is not good, more benefit will be realised by upstreaming to nixpkgs.

@hoh
Copy link

hoh commented Jul 2, 2024

Related to NixOS/nixpkgs#239770 and NixOS/nixpkgs#171136

@tonybutt
Copy link

anything I can do to get this resolved and merged?

@bashfulrobot
Copy link
Author

I had been blocked by the Nvidia drivers. I couldn't get it going in way up until very recently.

I had to add initrd.kernelModules = [ "nvidia" "i915" "nvidia_modeset" "nvidia_drm" ]; to get it all working. I need to come back and have a look at this PR and clean it up with everything that I've done in my individual repo.

@tonybutt
Copy link

I have to support nixos on these 9530s and I would like to get this finished up and resolved so I can use it without pinning to the pr. Anything I can do to test for you?

@tonybutt
Copy link

We are using the 9520 config and it works ok.

@kuflierl
Copy link

kuflierl commented Oct 12, 2024

Everyone seems to be mentioning the Variation with an RTX4000 series card. What about the Intel Arc GPU variation or the Intel Xe graphics variation?
Also: we might need to rename 9530 to 9530-2023 due to Dell's ... questionable choice in naming. There is a laptop with the same name from the year 2013.

@Mic92
Copy link
Member

Mic92 commented Oct 31, 2024

@tonybutt you can also make a new pull request that doesn't have the fingerprint module. This should be rather uncontroversial.

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.

7 participants