Enabling probe feature in AMD Platforms #8634

DINESHKUMARK1 · 2023-12-15T12:27:15Z

Enabling probe feature on Vangogh and Rembrandt Platforms.

sofci · 2023-12-15T12:28:11Z

Can one of the admins verify this patch?

reply test this please to run this test once

kv2019i · 2023-12-15T14:12:25Z

@DINESHKUMARK1 Can you check the CI first, lot of build errors across multiple targets.

dbaluta · 2023-12-18T14:44:25Z

src/drivers/amd/common/acp_dma.c

@@ -22,10 +22,15 @@
 #include <stddef.h>


Can you please add more details about what 'probe' functionality is?

Is the ability to get some statistics/logs via compress interface?

Hi,
This is a debug feature to probe the data in between components of pipeline, for more information find the below link

https://thesofproject.github.io/latest/developer_guides/debugability/probes/index.html

@dbaluta its already used on Intel platforms. Kernel parts also upstream.

src/drivers/amd/common/acp_dma.c

lyakh · 2023-12-19T14:21:24Z

src/drivers/amd/common/acp_dma.c

+ if (probe_pos >= (PROBE_BUFFER_WATERMARK)) {
+ io_reg_write((PU_REGISTER_BASE +
+ ACP_FUTURE_REG_ACLK_0),
+ (PROBE_UPDATE_POS_MASK | probe_pos_update));


3 instances of superfluous parentheses on 4 lines

lyakh · 2023-12-19T14:23:57Z

src/lib/notifier.c

@@ -109,6 +109,9 @@ void notifier_unregister(void *receiver, void *caller, enum notify_id type)
 (!caller || handle->caller == caller)) {
 if (!--handle->num_registrations) {
 list_item_del(&handle->list);
+ #ifdef CONFIG_PROBE
+ list_item_del(&notify->list[NOTIFIER_ID_BUFFER_PRODUCE]);
+ #endif


Presumably this would be a bug fix. Are calls to

notifier_unregister(_probe, dev->cb, NOTIFIER_ID_BUFFER_PRODUCE);

in probe.c not enough?

If this is indeed needed, then please move #ifdef / #endif to position 0, and reduce indentation of list_item_del()

lyakh · 2023-12-19T14:38:38Z

src/drivers/amd/vangogh/acp_dma.c

+ dma_cfg->wr_size = 0;
+ else
+ dma_cfg->wr_size = dma_cfg->size;
+ }


wrong indentation

btian1 · 2023-12-20T05:33:48Z

src/ipc/dma-copy.c

@@ -184,8 +184,6 @@ int dma_copy_new(struct dma_copy *dc)
 return 0;
 }

-#if CONFIG_DMA_GW
-


this config is not AMD specific, if AMD platform want to use this, better to add this config to AMD build?

Enabling CONFIG_DMA_GW will bring in many other API's which will cause failures in DMA operations and are not required on AMD platform, but dma_copy_set_stream_tag is required for enabling DMA channel based on stream tag, That is the reason to remove CONFIG_DMA_GW for this API, I believe this won't affect non AMD platforms.

yeah, this is fine - its mostly generic if anyone is using a DMA tag to identify a DMA channel.

btian1 · 2023-12-20T05:35:29Z

src/drivers/amd/common/acp_dma.c

@@ -49,6 +54,7 @@ void dma_config_descriptor(uint32_t dscr_start_idx, uint32_t dscr_count,
 static struct dma_chan_data *acp_dma_channel_get(struct dma *dma,
 unsigned int req_chan)
 {
+ uint32_t *p = NULL;


uint32_t *p = NULL; -=-> uint32_t *p;

btian1 · 2023-12-20T05:36:12Z

src/drivers/amd/common/acp_dma.c

@@ -73,6 +79,18 @@ static struct dma_chan_data *acp_dma_channel_get(struct dma *dma,
 acp_dma_chan->config[req_chan].rd_size = 0;
 acp_dma_chan->config[req_chan].wr_size = 0;
 acp_dma_chan->config[req_chan].size = 0;
+ acp_dma_chan->config[req_chan].probe_channel = 0xFF;
+ p = dma->priv_data;
+ if (p) {


may be can delete p, directly use dma->priv_data

lgirdwood

LGTM, just 1 question on notifier.

lgirdwood · 2023-12-20T14:25:46Z

src/ipc/dma-copy.c

@@ -184,8 +184,6 @@ int dma_copy_new(struct dma_copy *dc)
 return 0;
 }

-#if CONFIG_DMA_GW
-


yeah, this is fine - its mostly generic if anyone is using a DMA tag to identify a DMA channel.

lgirdwood · 2023-12-20T14:27:38Z

src/lib/notifier.c

@@ -109,6 +109,9 @@ void notifier_unregister(void *receiver, void *caller, enum notify_id type)
 (!caller || handle->caller == caller)) {
 if (!--handle->num_registrations) {
 list_item_del(&handle->list);
+#ifdef CONFIG_PROBE


oh, can you confirm we have the opposite list_add() call for this notifier using the same #ifdef CONFIG_PROBE kconfig ?

only for list_item_del(&notify->list[NOTIFIER_ID_BUFFER_PRODUCE]) is used as it is a bug for probe buffer in delete case and avoid side effects/issues in regular usecases

There is probably a real bug here, but this does not look a correct fix. The probe code does notify unregister for NOTIFIER_ID_BUFFER_PRODUCE and NOTIFIER_ID_BUFFER_FREE, so the unconditional list_item_del seems out of place here.

Quick look at the API means we need to call notifier_unregister() and pass in NOTIFIER_ID_BUFFER_PRODUCE as type.

Updated the patch with proper fix and removed the macro config_probe.

lgirdwood · 2023-12-20T14:28:50Z

src/drivers/amd/common/acp_dma.c

@@ -22,10 +22,15 @@
 #include <stddef.h>


@dbaluta its already used on Intel platforms. Kernel parts also upstream.

kv2019i

The series looks mostly good, but the git commit prefixes are not correct and the bugfix in "deregister NOTIFIER_ID_BUFFER_PRODUCE" does not look correct. I won-t leave a -1 as I'm OOO next week, but I think these needs to be addressed before merge.

src/probe/probe.c

kv2019i · 2023-12-21T13:28:53Z

src/lib/notifier.c

@@ -109,6 +109,9 @@ void notifier_unregister(void *receiver, void *caller, enum notify_id type)
 (!caller || handle->caller == caller)) {
 if (!--handle->num_registrations) {
 list_item_del(&handle->list);
+#ifdef CONFIG_PROBE


There is probably a real bug here, but this does not look a correct fix. The probe code does notify unregister for NOTIFIER_ID_BUFFER_PRODUCE and NOTIFIER_ID_BUFFER_FREE, so the unconditional list_item_del seems out of place here.

local dma_sg_config is not required, struct probe_dma_ext holds the DMA config segments. proper fix for notifier_unregister to remove the probe point functionality. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Remove CONFIG_DMA_GW macro to use dma_copy_set_stream_tag function to get dma channel on AMD platforms for probe. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Add ACP_FUTURE_REG_ACLK_0 Register in AMD/Renoir Platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Add ACP_FUTURE_REG_ACLK_0 Register in AMD/Acp_6_3 Platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Add probe functionality in AMD/rembrandt platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Add probe functionality in AMD/vangogh platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

Add probe functionality in AMD platforms. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

kv2019i

Thanks @DINESHKUMARK1 , looks good now!

kv2019i · 2024-01-02T12:54:58Z

SOFCI TEST

kv2019i · 2024-01-02T12:55:24Z

@wszypelt Can you check the quickbuild fails, 5 days with no results..?

wszypelt · 2024-01-03T07:45:18Z

@kv2019i It seems that the request was not queued, anyway I turned it on manually, the results should be available within an hour

DINESHKUMARK1 requested review from bardliao, marcinszkudlinski, pblaszko, bhiregoudar, sunilkumardommati, lgirdwood, plbossart, mmaka1, lbetlej, dbaluta and kv2019i as code owners December 15, 2023 12:27

dbaluta reviewed Dec 18, 2023

View reviewed changes

lyakh reviewed Dec 19, 2023

View reviewed changes

btian1 reviewed Dec 20, 2023

View reviewed changes

lgirdwood reviewed Dec 20, 2023

View reviewed changes

bhiregoudar approved these changes Dec 21, 2023

View reviewed changes

kv2019i reviewed Dec 21, 2023

View reviewed changes

DINESHKUMARK1 added 7 commits December 28, 2023 18:28

probe: small fixes in probe functionality.

a1b8cbc

local dma_sg_config is not required, struct probe_dma_ext holds the DMA config segments. proper fix for notifier_unregister to remove the probe point functionality. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

dma-copy: remove CONFIG_DMA_GW macro.

00411bd

Remove CONFIG_DMA_GW macro to use dma_copy_set_stream_tag function to get dma channel on AMD platforms for probe. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

amd: add ACP_FUTURE_REG_ACLK_0 Register in AMD/Renoir Platform.

82792c1

Add ACP_FUTURE_REG_ACLK_0 Register in AMD/Renoir Platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

amd: add ACP_FUTURE_REG_ACLK_0 Register in AMD/Acp_6_3 Platform.

1829eef

Add ACP_FUTURE_REG_ACLK_0 Register in AMD/Acp_6_3 Platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

amd: add probe functionality in AMD/rembrandt platform.

60acfae

Add probe functionality in AMD/rembrandt platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

amd: add probe functionality in AMD/vangogh platform.

0211ca5

Add probe functionality in AMD/vangogh platform. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

amd: add probe functionality in AMD platforms.

58bd139

Add probe functionality in AMD platforms. Signed-off-by: Kalva, DineshKumar <dineshkumar.kalva@amd.com>

kv2019i approved these changes Jan 2, 2024

View reviewed changes

kv2019i merged commit cae1c3a into thesofproject:main Jan 3, 2024
43 of 44 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enabling probe feature in AMD Platforms #8634

Enabling probe feature in AMD Platforms #8634

DINESHKUMARK1 commented Dec 15, 2023

sofci commented Dec 15, 2023

kv2019i commented Dec 15, 2023

dbaluta Dec 18, 2023

DINESHKUMARK1 Dec 19, 2023 •

edited

Loading

lgirdwood Dec 20, 2023

lyakh Dec 19, 2023

lyakh Dec 19, 2023

lyakh Dec 19, 2023

btian1 Dec 20, 2023

DINESHKUMARK1 Dec 20, 2023

lgirdwood Dec 20, 2023

btian1 Dec 20, 2023

btian1 Dec 20, 2023

lgirdwood left a comment

lgirdwood Dec 20, 2023

lgirdwood Dec 20, 2023

DINESHKUMARK1 Dec 21, 2023 •

edited

Loading

kv2019i Dec 21, 2023

lgirdwood Dec 21, 2023

DINESHKUMARK1 Dec 28, 2023

lgirdwood Dec 20, 2023

kv2019i left a comment

kv2019i Dec 21, 2023

kv2019i left a comment

kv2019i commented Jan 2, 2024

kv2019i commented Jan 2, 2024

wszypelt commented Jan 3, 2024

Enabling probe feature in AMD Platforms #8634

Enabling probe feature in AMD Platforms #8634

Conversation

DINESHKUMARK1 commented Dec 15, 2023

sofci commented Dec 15, 2023

kv2019i commented Dec 15, 2023

Choose a reason for hiding this comment

DINESHKUMARK1 Dec 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgirdwood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DINESHKUMARK1 Dec 21, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kv2019i left a comment

Choose a reason for hiding this comment

kv2019i commented Jan 2, 2024

kv2019i commented Jan 2, 2024

wszypelt commented Jan 3, 2024

DINESHKUMARK1 Dec 19, 2023 •

edited

Loading

DINESHKUMARK1 Dec 21, 2023 •

edited

Loading