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

[scarthgap] add Jetson Tegra w/ Jetpack 5.x support #398

Conversation

zach-welch-aquabyte
Copy link
Contributor

This branch works well on our custom Orin NX carrier board running a snapshot of scarthgap (circa 5.0.2). It includes a few cherry-picked commits from Josef's branch.

@TheYoctoJester TheYoctoJester self-requested a review August 9, 2024 07:45
Copy link
Contributor

@TheYoctoJester TheYoctoJester left a comment

Choose a reason for hiding this comment

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

Hi @zach-welch-aquabyte,

Thanks a lot for assembling this. Two things should be changed, though:

  • the meta-mender-tegra-jetpack4 layer is not used anymore, so it should be removed.
  • with the JP4 support gone, the content of meta-mender-tegra-jetpack5 should be moved back to meta-mender-tegra.

Do you want to adjust this, or would like me to? I can prepare a PR on top of your branch.

@zach-welch-aquabyte
Copy link
Contributor Author

zach-welch-aquabyte commented Aug 9, 2024 via email

@zach-welch-aquabyte zach-welch-aquabyte force-pushed the scarthgap-tegra-jetpack5-v3 branch from 22888c9 to e17bd59 Compare August 9, 2024 22:42
@zach-welch-aquabyte
Copy link
Contributor Author

I have rebased the branch to pick the meta-mender-tegra-jetpack5 sublayer from kirkstone directly into meta-mender-tegra. As promised, I also improved the metadata, so the ChangeLog looks more readable and complete.

During that review, I noticed that I unintentionally had squashed an unrelated change into an early commit. I have broken that out separately as the WIP patch near the end of the series. I have posted it here as-is for feedback about how to fix that last (known) issue.

@TheYoctoJester TheYoctoJester force-pushed the scarthgap-tegra-jetpack5-v3 branch from e17bd59 to 814bfd2 Compare August 11, 2024 12:22
@TheYoctoJester
Copy link
Contributor

Hi @zach-welch-aquabyte, great! I have already added my signed commits (required for me as part of Mender), and the rest looks good to me. Only one last nitpick is, e1017d7 doesn't have a Signed-off-by. Once you add that its good to merge 👍

version: 14
includes:
- kas/include/mender-full.yml
- kas/include/tegra-jetpack6.yml
Copy link
Contributor

Choose a reason for hiding this comment

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

This file implies compatibility with JP 6. Do you know if this was ever tested?
For me Bitbake is complaining that it cannot apply linux-tegra_%.bbappend as the base recipe was renamed to linux-jammy-nvidia-tegra.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have only tested with JP5. I suspect you might be the first to try it with JP6.

Does it build if you rename the bbappend file? While I am not certain how to resolve the renamed recipe issue correctly, it would be interesting to hear how much more work remains to be done, as I eventually plan to give JP6 a go as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

It builds, but there are still some other fixes needed to work properly. Though most of them seem to be compatible with JP 6.
I'm still in the process of figuring out all required changes :D

Copy link
Contributor

Choose a reason for hiding this comment

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

@apbr I'll go ahead and merge it as-is then, to reduce the amount of things which need to be carried along for JP6. To me its nicer having a state in-branch which already builds and "just" needs some more polish, instead of having to keep a whole separate state externally for even working on JP6.

@zach-welch-aquabyte zach-welch-aquabyte force-pushed the scarthgap-tegra-jetpack5-v3 branch from 814bfd2 to 15b4c97 Compare August 14, 2024 21:23
@zach-welch-aquabyte
Copy link
Contributor Author

Per @madisongh's suggestion, I resolved the pending issue with e1017d7 in this PR by introducing the new global l4t_version.bbclass in meta-tegra, submitted as OE4T/meta-tegra#1658.

While that PR needs to be merged before this one, I have pushed my latest branch here to unblock that possibility.

@zach-welch-aquabyte zach-welch-aquabyte force-pushed the scarthgap-tegra-jetpack5-v3 branch 2 times, most recently from f0dda3a to 40c6b63 Compare August 14, 2024 21:31
@TheYoctoJester
Copy link
Contributor

TheYoctoJester commented Aug 16, 2024 via email

@apbr
Copy link
Contributor

apbr commented Aug 16, 2024

@apbr so we keep this open until sorted out? Or what strategy should we apply?

Good question, There are some incompatibilities I noticed that might be difficult to resolve (See Gitter/Matrix chat). I'm not sure how to proceed with those.
So not sure it makes sense to wait. Alternatively this could be merged as JP 5 only? What do you think?

@TheYoctoJester
Copy link
Contributor

TheYoctoJester commented Aug 16, 2024 via email

Copy link
Contributor

@apbr apbr left a comment

Choose a reason for hiding this comment

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

I added some more comments.
(I know, you probably didn't write most of the changes in question,)

@zach-welch-aquabyte zach-welch-aquabyte force-pushed the scarthgap-tegra-jetpack5-v3 branch from 7bdc04e to 8aa19ce Compare August 16, 2024 20:17
This reflects the state of Tegra Jetpack5 as on the
kirkstone branch by commit 8280a10
as state for forward porting to scarthgap.

Changelog: Title
Ticket: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Ticket: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
This recipe was created in meta-tegra from tegra-bootfiles for scarthgap, so
this overlay remains functionally identical to its kirkstone cousin.

However, this updated version now saves the original layouts in WORKDIR,
rather than overwriting them.  It also eliminates a little duplicated code.

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
The `setup-nv-boot-control.service` unit creates a `RuntimeDirectory`,
so use that path.

Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
The previous commit adjusts this service to use the RuntimeDirectory
created by systemd, so this custom directory is no longer required.

Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
@TheYoctoJester TheYoctoJester force-pushed the scarthgap-tegra-jetpack5-v3 branch from 8aa19ce to c4948cd Compare August 18, 2024 12:31
Copy link
Contributor

@TheYoctoJester TheYoctoJester left a comment

Choose a reason for hiding this comment

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

Hi @zach-welch-aquabyte, gave it a review on mostly the formal things I can reasonably comment on. I can also fix those and push, if you'd like me to.

@@ -1,5 +1,5 @@
header:
version: 11
version: 14
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed from this branch

@@ -1,5 +1,5 @@
header:
version: 11
version: 14
Copy link
Contributor

Choose a reason for hiding this comment

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

should be removed from this branch

@@ -20,7 +20,7 @@ repos:

meta-mender:
url: https://github.com/mendersoftware/meta-mender.git
commit: 4302cb71a7f8a0ffb09f47e41f4ce1df3e812419
commit: 21b8bd76e92391665ad2cf52fa1def65ba97c7a2
Copy link
Contributor

Choose a reason for hiding this comment

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

needs bump to e8b6b3e so your PR on persistent machine IDs is already merged

version: 14

repos:
meta-mender:
Copy link
Contributor

Choose a reason for hiding this comment

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

meta-mender can be removed then here

This layer depends on:

```
URI: https://github.com/madisongh/meta-tegra.git
Copy link
Contributor

Choose a reason for hiding this comment

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

```
URI: https://github.com/madisongh/meta-tegra.git
layers: meta-tegra
branch: kirkstone
Copy link
Contributor

Choose a reason for hiding this comment

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

should be scarthgap, respectively scarthgap-l4t-r35.x

```
URI: https://github.com/mendersoftware/meta-mender.git
layers: meta-mender-core
branch: kirkstone
Copy link
Contributor

Choose a reason for hiding this comment

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

should be scarthgap

@@ -0,0 +1,46 @@
#!/bin/sh
set -e
echo "$(mender show-artifact): $(basename "$0") was called!" >&2
Copy link
Contributor

Choose a reason for hiding this comment

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

@zach-welch-aquabyte Maybe a bit unrelated, but on my device/image the binary is only available as mender-update and not as mender. Do you have any idea, why that could be the case?

Copy link
Contributor

Choose a reason for hiding this comment

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

@apbr starting in Mender Client 4.0, the binary is split into mender-auth and mender-update. So unless additional configuration is applied, this script should actually refer to mender-update.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, I proposed a change: zach-welch-aquabyte#1 (Includes also a first step towards compatibility with JP 6)

Copy link
Contributor

Choose a reason for hiding this comment

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

As this PR is now merged, I created a PR directly here: #402

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
l4t_bsp.bbclass became recipe-scoped for scarthgap, so
l4t_version.bbclass was created to provide the L4T_VERSION
variable at global scope.

Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
The upstream recipe now handles this.

Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
This recipe is no longer relevant with Jetpack 5.x, as it only supports
tegra194 and tegra234.

Changelog: Title
Issue: None

Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
@zach-welch-aquabyte zach-welch-aquabyte force-pushed the scarthgap-tegra-jetpack5-v3 branch from c4948cd to 337f0c8 Compare August 21, 2024 20:21
@mender-test-bot
Copy link

Merging these commits will result in the following changelog entries:

Changelogs

meta-mender-community (scarthgap-tegra-jetpack5-v3)

New changes in meta-mender-community since scarthgap:

Bug Fixes
  • drop vestigial jetpack5 layer suffix
Features
  • meta-mender-tegra: add scarthgap compatibility
  • pick meta-mender-tegra-jetpack5 from kirkstone
Other
  • kas: add tegra-jetpack5 based builds
  • kas: clean up tegra-{jetpack5/jetpack6}
  • tegra-nv-boot-control-config: drop obsolete overlay
  • image_types_mender_tegra: make a recipe class
  • nv_update_verifier.sh: fix string equality test
  • redundant-boot-overrides: drop obsolete recipe
  • setup-nv-boot-control: no longer needed
  • tegra-bup-payload: removed from meta-tegra
  • tegra-mender-setup: use l4t_version instead of l4t_bsp
  • tegra-nv-boot-control-config: use service runtime path

@TheYoctoJester TheYoctoJester force-pushed the scarthgap-tegra-jetpack5-v3 branch from 337f0c8 to 894a6dd Compare August 21, 2024 20:34
zach-welch-aquabyte and others added 3 commits August 21, 2024 22:37
Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Port over the kas includes as well as the boards from
kirkstone:
- jetson-agx-orin-devkit.yml
- jetson-agx-xavier-devkit.yml
- jetson-orin-nano-devkit.yml

Changelog: Title
Ticket: None

Signed-off-by: Josef Holzmayr <jester@theyoctojester.info>
Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
Changelog: Title
Ticket: None

Signed-off-by: Josef Holzmayr <jester@theyoctojester.info>
Signed-off-by: Zachary T Welch <zach@aquabyte.ai>
@TheYoctoJester TheYoctoJester force-pushed the scarthgap-tegra-jetpack5-v3 branch from 894a6dd to 0d2cc0d Compare August 21, 2024 20:39
@TheYoctoJester TheYoctoJester merged commit 4c1c2c1 into mendersoftware:scarthgap Aug 21, 2024
1 check passed
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.

4 participants