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.

Sign up for GitHub

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

sof-firmware: 1.7 -> 1.8 #130884

Closed
wants to merge 1 commit into from
Closed

sof-firmware: 1.7 -> 1.8 #130884

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkgs/os-specific/linux/firmware/sof-firmware/default.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,18 +3,19 @@
with lib;
stdenv.mkDerivation rec {
pname = "sof-firmware";
version = "1.7";
version = "1.8";

src = fetchFromGitHub {
owner = "thesofproject";
repo = "sof-bin";
rev = "v${version}";
sha256 = "sha256-Z0Z4HLsIIuW8E1kFNhAECmzj1HkJVfbEw13B8V7PZLk=";
sha256 = "sha256-NPbzDXZoZVWriV3klemX59ACnAlb357A5V/GbmzshyA=";
};

dontFixup = true; # binaries must not be stripped or patchelfed

installPhase = ''
cd v${version}.x
Copy link
Member

Choose a reason for hiding this comment

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

Starting with v1.8 upstream has a new install script: https://github.com/thesofproject/sof-bin/blob/v1.8/install.sh
Any chance we could use this?

Copy link
Contributor

Choose a reason for hiding this comment

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

The script was introduced in 1.7. As stated in the README:

You don't have to use install.sh, you can use any recursive copy of your preference. This is all what install.sh does

Having said that, we might as well use it? This seems to work:

      installPhase = ''    
        mkdir -p $out/lib/firmware/intel/    
        mkdir -p $out/bin/    
        export FW_DEST=$out/lib/firmware/intel    
        export TOOLS_DEST=$out/bin    
        ./install.sh v1.8.x/v1.8    
      '';

The script installs some additional tools, not sure what they're for. We'll need a fixup phase that strips and patches these, but not the SOF binaries.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh and rsync is needed as a build input.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually stripping and patching the firmware binaries doesn't seem to make a difference. Maybe it's just unnecessary?

If we want the tools to be thrown in with the firmware they should be added to systemPackages too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@evenbrenden why dont you take this change over then since you seem to have thoughts about how it shoudl be done.

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO using the install script is a nice practice, but I think it's fine to merge this PR as is.

As for the tools, I'm really not sure if we should add them, just noticed them now. Either way that could be done in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

The track record of the SOF install script is also pretty bad. They manage to break the script with every other release. It was me, who switched the installPhase to just perform a recursive copy.

Copy link
Member

Choose a reason for hiding this comment

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

For copying two files the script is quite big and uses rsync which we don't need.

mkdir -p $out/lib/firmware/intel/
cp -a sof-v${version} $out/lib/firmware/intel/sof
cp -a sof-tplg-v${version} $out/lib/firmware/intel/sof-tplg
Comment on lines +18 to 21
Copy link
Member

Choose a reason for hiding this comment

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

After further consideration I don't think it makes sense to use the upstream install script. However, I think it is worthwhile to address the review comments left by the bot:

Suggested change
cd v${version}.x
mkdir -p $out/lib/firmware/intel/
cp -a sof-v${version} $out/lib/firmware/intel/sof
cp -a sof-tplg-v${version} $out/lib/firmware/intel/sof-tplg
runHook preInstall
cd v${version}.x
mkdir -p $out/lib/firmware/intel/
cp -a sof-v${version} $out/lib/firmware/intel/sof
cp -a sof-tplg-v${version} $out/lib/firmware/intel/sof-tplg
runHook postInstall

Expand Down