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

app: boards: Enable Probe for LNL #8982

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

gbernatxintel
Copy link
Contributor

Enables extraction and injection probes for LNL platform.
NOTE: this commit does NOT enable probe log backend.

@sofci
Copy link
Collaborator

sofci commented Mar 25, 2024

Can one of the admins verify this patch?

reply test this please to run this test once

@@ -70,6 +70,8 @@ CONFIG_WINSTREAM_CONSOLE=n

CONFIG_INTEL_ADSP_IPC=y

CONFIG_PROBE=y
Copy link
Member

@plbossart plbossart Mar 25, 2024

Choose a reason for hiding this comment

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

config PROBE
bool "Probes enabled"
default y if CAVS

Should this be CAVS | ACE so that all platforms have this by default?

config PROBE_DMA_MAX
int "Maximum injection probe DMAs attached"

The name of the Kconfig is very misleading, consider renaming to make it self-explanatory that this is for INJECTION only.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm fine with these handled in separate PRs. Especially with vendor defaults, I wonder if it would be better to have some common definition files e.g. for Intel SOF platforms. The PROBE is really a generic feature not tied to hardware in anyway, so having the defaults set based on HW capabilities is a bit misleading. This is really about which feature Intel SOF builds want to have enabled.

@@ -70,6 +70,8 @@ CONFIG_WINSTREAM_CONSOLE=n

CONFIG_INTEL_ADSP_IPC=y

CONFIG_PROBE=y
Copy link
Member

Choose a reason for hiding this comment

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

commit title: boards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, thank you fixed

Copy link
Collaborator

Choose a reason for hiding this comment

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

btw, the commit still has the typo (updating the pull request title won't change the commit).

@gbernatxintel gbernatxintel changed the title app: boadrds: Enable Probe for LNL app: boards: Enable Probe for LNL Mar 26, 2024
Copy link
Collaborator

@kv2019i kv2019i left a comment

Choose a reason for hiding this comment

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

Thanks @gbernatxintel !

Copy link
Contributor

@tmleman tmleman left a comment

Choose a reason for hiding this comment

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

For me, this is not a blocker. But please remove the 'X' from your name, the comma is also unnecessary. In my opinion, it's clearest when it's just the first and last name.

Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

Enables extraction and injection probes for LNL platform.
NOTE: this commit does NOT enable probe log backend.

Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>
@gbernatxintel
Copy link
Contributor Author

For me, this is not a blocker. But please remove the 'X' from your name, the comma is also unnecessary. In my opinion, it's clearest when it's just the first and last name.

Signed-off-by: Grzegorz Bernat <grzegorzx.bernat@intel.com>

Thank you, I fixed it.

@kv2019i
Copy link
Collaborator

kv2019i commented Mar 28, 2024

Just pending on required CI to pass, otherwise ready to merge.

@wszypelt
Copy link

wszypelt commented Apr 2, 2024

SOFCI TEST

@kv2019i kv2019i merged commit 78831db into thesofproject:main Apr 2, 2024
44 of 45 checks 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.

7 participants