EDGEML-8777: Support firmware log buffer #328

VinitAmd · 2024-12-16T13:21:18Z

[Why]
Log buffer support is required to enhance debugging support on NPU stack

[How]

allocate log buffer and share with firmware via 'start event trace' message Config start_event_trace request param, send via mgmt channel. Handle the send buffer resp.
receive notifications about log buffer fullness via interrupt from firmware. Handle channel interrupt process read FW lg buffer and update log metadata head/tail.

Signed-off-by: Vinit [email protected]

AMDGithubSCIMAdmin · 2024-12-16T13:23:04Z

Can one of the admins verify this patch?

mamin506

"__packed" is missing for the request and response structs.

src/driver/amdxdna/aie2_msg_priv.h

mamin506 · 2025-01-09T00:54:00Z

ok to test

VinitAmd · 2025-01-09T04:54:31Z

=== 3 tests failed on Linux_npu_tests run.
ipu_suspend : aie2_state_write:NPU suspend \xf8,\x94\x84\xff\xff\xff\xff\xb8%n\x84\xff\xff\xff\xff\x08\x1c̅\xff\xff\xff\xff failed
ipu_resume : aie2_state_write: NPU resume \xff\xf8,\x94\x84\xff\xff\xff\xff(\xbcw\x8eW\xa8\xff\xff\x08\x1c̅\xff\xff\xff\xff failed
ipu_check_header_hash
Already ticket is raised for FW team FWDEV-103771 .

mamin506 · 2025-01-10T00:16:14Z

@VinitAmd , please fix the pipeline failure. The driver is not able to load. Please try your driver on a NPU1 machine. Please contact @vengutta18 for the machine if you don't have one.

mamin506

I'm not expect any changes for amdxdna_mailbox* files for this feature.
Please cleanup the amdxdna_mailbox* files.

After this is fix, I will review other code.
Please follow https://github.com/amd/xdna-driver?tab=readme-ov-file#checkpatch to setup your workspace.
The coding style needs to pass checkpatch.pl test.

src/driver/amdxdna/amdxdna_mailbox_helper.h

src/driver/amdxdna/amdxdna_mailbox.h

src/driver/amdxdna/amdxdna_mailbox.c

VinitAmd · 2025-01-10T05:18:16Z

I'm not expect any changes for amdxdna_mailbox* files for this feature. Please cleanup the amdxdna_mailbox* files.

After this is fix, I will review other code. Please follow https://github.com/amd/xdna-driver?tab=readme-ov-file#checkpatch to setup your workspace. The coding style needs to pass checkpatch.pl test.

Thanks, I was looking for this info

VinitAmd · 2025-01-13T18:36:28Z

I'm not expect any changes for amdxdna_mailbox* files for this feature. Please cleanup the amdxdna_mailbox* files.
After this is fix, I will review other code. Please follow https://github.com/amd/xdna-driver?tab=readme-ov-file#checkpatch to setup your workspace. The coding style needs to pass checkpatch.pl test.

Thanks, I was looking for this info

Code formatting done with checkpatch.pl

src/driver/amdxdna/aie2_msg_priv.h

mamin506 · 2025-01-16T16:37:15Z

retest this please

VinitAmd · 2025-01-22T10:23:37Z

trigger jenkins

mamin506 · 2025-01-24T17:27:12Z

ok to test

mamin506 · 2025-01-24T17:34:07Z

retest this please

amd-akshatah · 2025-01-24T18:24:00Z

ok to test

amd-akshatah · 2025-01-24T18:25:28Z

retest this please

sreedharamd · 2025-01-27T04:51:25Z

ok to test

amd-akshatah · 2025-01-27T21:49:36Z

retest this please

vengutta18 · 2025-01-28T07:45:21Z

src/driver/amdxdna/aie2_debugfs.c

@@ -263,6 +263,45 @@ static int aie2_dpm_level_get(struct seq_file *m, void *unused)

 AIE2_DBGFS_FOPS(dpm_level, aie2_dpm_level_get, aie2_dpm_level_set);

+//write debufs for event trace


Kernel style comments please.

vengutta18 · 2025-01-28T07:48:01Z

src/driver/amdxdna/aie2_pci.h

@@ -250,11 +250,13 @@ struct amdxdna_dev_hdl {
 	u32				npuclk_freq;
 	u32				hclk_freq;
 	bool				force_preempt_enabled;
+	uint32_t			event_trace_enabled;


can it be bool?

There may be a option to enable event trace but disable log printing for debug purpose , so kept it uint32.
if we want all the time encode log too added to dmesg, it can be bool

vengutta18 · 2025-01-28T07:54:05Z

src/driver/amdxdna/aie2_event_trace.c

+#include "amdxdna_trace.h"
+#include "amdxdna_mailbox.h"
+
+uint8_t g_fwLogBuf[TRACE_EVENT_BUFFER_SIZE];


Please allocate this using kzalloc and clean up when it's no longer needed. Also variable name to fw_log_buf

vengutta18 · 2025-01-28T07:55:27Z

src/driver/amdxdna/aie2_event_trace.c

+
+static void clear_event_trace_msix(struct amdxdna_dev_hdl *ndev)
+{
+	// Clear the log buffer interrupt


comments ditto, kernel style.

vengutta18 · 2025-01-28T07:57:32Z

src/driver/amdxdna/aie2_pci.h

+int aie2_start_event_trace(struct amdxdna_dev_hdl *ndev, dma_addr_t addr, u32 size);
+void aie2_set_trace_timestamp(struct amdxdna_dev_hdl *ndev, uint32_t *resp);
+void aie2_assign_event_trace_state(struct amdxdna_dev_hdl *ndev, uint32_t state);
+int aie2_is_event_trace_supported_on_dev(struct amdxdna_dev_hdl *ndev);


Do we need all the functions here, only keep the functions that are used by other files,rest of them make it static inside the aie2_event_trace.c

vengutta18 · 2025-01-28T08:18:37Z

src/driver/amdxdna/aie2_event_trace.c

+
+		if (ret) {
+			XDNA_ERR(ndev->xdna, "Send start event trace failed, ret %d", ret);
+			// Currently this feature is supported on limited HW's,


multi-line comment style

is there any guide available for kernel comment style ? checkPatch.pl doesn't fix the problem , it only shows the problem.
if no warning/error while checkPatch.pl run hard to fix problem without guide.

vengutta18 · 2025-01-28T08:19:15Z

src/driver/amdxdna/aie2_event_trace.c

+		return;
+	}
+
+	ndev->event_trace_enabled = state;


Could you assign the state once the trace is enabled successfully, It could fail at line no. 55!

vengutta18 · 2025-01-28T08:23:38Z

src/driver/amdxdna/aie2_message.c

+	req.event_trace_categories = 0xFFFFFFFF;
+	req.event_trace_timestamp = EVENT_TRACE_TIMESTAMP_FW_CHRONO;
+
+	XDNA_INFO(ndev->xdna, "send start event trace msg");


make it XDNA_DBG

vengutta18 · 2025-01-28T08:23:50Z

src/driver/amdxdna/aie2_message.c

+{
+	DECLARE_AIE2_MSG(stop_event_trace, MSG_OP_STOP_EVENT_TRACE);
+
+	XDNA_INFO(ndev->xdna, "send stop event trace msg");


vengutta18 · 2025-01-28T08:34:00Z

src/driver/amdxdna/aie2_pci.c


 	aie2_hw_stop(xdna);
 	aie2_error_async_events_free(ndev);
+	if (is_event_trace_supported)
+		aie2_event_trace_free(ndev);


Couple of things here, are trace_alloc and trace_free are in sync?
what happens during the driver unload or suspend/resume?

are trace_alloc and trace_free are in sync?---> yes both are in sync

what happens during the driver unload ---> when driver unload , aie2_fini is called and which triggers stop event trace and free even trace resource.

suspend/resume -> Currently there is FW issue when PG applied FW loosing event trace buffer information, causing crash in resume (FWDEV-103771

[Why] Log buffer support is required to enhance debugging support on NPU stack [How] 1. Allocate log buffer and share with firmware via 'start_event_trace' message Config start_event_trace request param, send via mgmt channel. Handle the send buffer resp. 2. Receive interrupt about log buffer half fullness from firmware. Handle interrupt, process further buffer data and update buffer head_offset for FW. 3. Add stop_event_trace_send api and it's handle to stop logging when aie2 shutdown. 4. Add event_trace debugfs for dynamic control of logging. Signed-off-by: vinit shukla <[email protected]>

mamin506 reviewed Dec 16, 2024

View reviewed changes

src/driver/amdxdna/aie2_msg_priv.h Outdated Show resolved Hide resolved

src/driver/amdxdna/aie2_msg_priv.h Outdated Show resolved Hide resolved

src/driver/amdxdna/aie2_msg_priv.h Outdated Show resolved Hide resolved

mamin506 requested a review from vengutta18 December 16, 2024 19:51

VinitAmd force-pushed the fw_log_buff_x branch from 8e8ff65 to 2b8b148 Compare December 17, 2024 05:59

maxzhen added the draft label Dec 19, 2024

VinitAmd force-pushed the fw_log_buff_x branch 3 times, most recently from 5ac87bf to add9c42 Compare December 23, 2024 09:35

VinitAmd force-pushed the fw_log_buff_x branch 3 times, most recently from a64f459 to bf466db Compare January 7, 2025 08:34

VinitAmd marked this pull request as ready for review January 7, 2025 08:38

vengutta18 removed the draft label Jan 7, 2025

VinitAmd force-pushed the fw_log_buff_x branch from bf466db to b1e6bfd Compare January 8, 2025 06:54

mamin506 requested changes Jan 10, 2025

View reviewed changes

VinitAmd force-pushed the fw_log_buff_x branch 5 times, most recently from 7d9850d to 2d0d0b4 Compare January 13, 2025 11:02

mamin506 reviewed Jan 13, 2025

View reviewed changes

src/driver/amdxdna/aie2_msg_priv.h Outdated Show resolved Hide resolved

VinitAmd force-pushed the fw_log_buff_x branch 2 times, most recently from e77d462 to 8d6b7a1 Compare January 15, 2025 17:45

VinitAmd force-pushed the fw_log_buff_x branch from 8d6b7a1 to 8b6da01 Compare January 20, 2025 11:11

VinitAmd force-pushed the fw_log_buff_x branch from 8b6da01 to 6be54a1 Compare January 20, 2025 17:27

VinitAmd changed the title ~~EDGEML-8777 - [Linux NPU Driver]: support firmware log buffer~~ EDGEML-8777: Support firmware log buffer Jan 20, 2025

VinitAmd force-pushed the fw_log_buff_x branch from 6be54a1 to 2234571 Compare January 22, 2025 10:13

VinitAmd force-pushed the fw_log_buff_x branch from 2234571 to 615feef Compare January 24, 2025 11:32

amd-akshatah closed this Jan 24, 2025

amd-akshatah reopened this Jan 24, 2025

VinitAmd force-pushed the fw_log_buff_x branch from 615feef to 7efb043 Compare January 27, 2025 05:53

amd-akshatah closed this Jan 27, 2025

amd-akshatah reopened this Jan 27, 2025

vengutta18 reviewed Jan 28, 2025

View reviewed changes

vengutta18 requested changes Jan 28, 2025

View reviewed changes

VinitAmd force-pushed the fw_log_buff_x branch from 7efb043 to be8f46f Compare January 28, 2025 14:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EDGEML-8777: Support firmware log buffer #328

EDGEML-8777: Support firmware log buffer #328

VinitAmd commented Dec 16, 2024 •

edited

Loading

AMDGithubSCIMAdmin commented Dec 16, 2024

mamin506 left a comment

mamin506 commented Jan 9, 2025

VinitAmd commented Jan 9, 2025

mamin506 commented Jan 10, 2025

mamin506 left a comment •

edited

Loading

VinitAmd commented Jan 10, 2025

VinitAmd commented Jan 13, 2025

mamin506 commented Jan 16, 2025

VinitAmd commented Jan 22, 2025

mamin506 commented Jan 24, 2025

mamin506 commented Jan 24, 2025

amd-akshatah commented Jan 24, 2025

amd-akshatah commented Jan 24, 2025

sreedharamd commented Jan 27, 2025

amd-akshatah commented Jan 27, 2025

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

VinitAmd Jan 28, 2025

vengutta18 Jan 28, 2025 •

edited

Loading

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

VinitAmd Jan 28, 2025

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

vengutta18 Jan 28, 2025

VinitAmd Jan 28, 2025

		@@ -263,6 +263,45 @@ static int aie2_dpm_level_get(struct seq_file m, void unused)

		AIE2_DBGFS_FOPS(dpm_level, aie2_dpm_level_get, aie2_dpm_level_set);

		//write debufs for event trace

EDGEML-8777: Support firmware log buffer #328

Are you sure you want to change the base?

EDGEML-8777: Support firmware log buffer #328

Conversation

VinitAmd commented Dec 16, 2024 • edited Loading

AMDGithubSCIMAdmin commented Dec 16, 2024

mamin506 left a comment

Choose a reason for hiding this comment

mamin506 commented Jan 9, 2025

VinitAmd commented Jan 9, 2025

mamin506 commented Jan 10, 2025

mamin506 left a comment • edited Loading

Choose a reason for hiding this comment

VinitAmd commented Jan 10, 2025

VinitAmd commented Jan 13, 2025

mamin506 commented Jan 16, 2025

VinitAmd commented Jan 22, 2025

mamin506 commented Jan 24, 2025

mamin506 commented Jan 24, 2025

amd-akshatah commented Jan 24, 2025

amd-akshatah commented Jan 24, 2025

sreedharamd commented Jan 27, 2025

amd-akshatah commented Jan 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vengutta18 Jan 28, 2025 • 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

VinitAmd commented Dec 16, 2024 •

edited

Loading

mamin506 left a comment •

edited

Loading

vengutta18 Jan 28, 2025 •

edited

Loading