xrt-smi validate test for spatial-sharing overhead (VITIS-13000) #8395

aktondak · 2024-09-05T15:26:51Z

Problem solved by the commit

VITIS-13000 : Multi-application spatial scheduling overhead
New micro-benchmark test for xrt-smi validate :

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

New Enhancement.

How problem was solved, alternative solutions (if any) and why they were rejected

The enhancement was made via a 2-phase test comparison between single threaded single hardware context run and a multi-threaded spatially shared hardware contexts to calculate time overhead. More details : https://confluence.amd.com/display/~aktondak/spatial+sharing+overhead+via+xrt-smi+validate

Z:\Repos\XRT-MCDM-FORK\XRT-MCDM\build\WRelease\xilinx\xrt>xrt-smi validate --run spatial-sharing-overhead
Validate Device           : [00c5:00:01.1]
    Platform              : NPU
    Power Mode            : Default
-------------------------------------------------------------------------------
Verbose: Enabling Verbosity
Test 1 [00c5:00:01.1]     : spatial-sharing-overhead
    Description           : Run Spatial Sharing Overhead Test
    Xclbin                : C:\WINDOWS\System32\DriverStore\FileRepository\ipukmddrv.inf_amd64_eba19bf0f9f62474\validate_17f0_20.xclbin
    Details               : Kernel name is 'DPU_PDI_0'
                            Single context latency: '198.3' ms
                            Spatially shared multiple context latency: '225.1' ms
                            Overhead: '26.9' ms
    Test Status           : [PASSED]

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Needs further testing from DSV.

Documentation impact (if any)

https://confluence.amd.com/display/~aktondak/spatial+sharing+overhead+via+xrt-smi+validate

Signed-off-by: Akshay Tondak <[email protected]>

gbuildx · 2024-09-05T15:28:02Z

Can one of the admins verify this patch?

AShivangi

Implementation looks good, just a few restructuring and minor comments!

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.h

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.cpp

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.h

Signed-off-by: Akshay Tondak <[email protected]>

sonals

Please document the overall design and add comments to the key functions to explain the control flow logic.

Signed-off-by: Akshay Tondak <[email protected]>

aktondak · 2024-09-16T18:51:42Z

Please document the overall design and add comments to the key functions to explain the control flow logic.

Updated the PR with API level comments.
The Documentation for overall design is at https://confluence.amd.com/display/~aktondak/spatial+sharing+overhead+via+xrt-smi+validate

…benchmark Test Signed-off-by: Akshay Tondak <[email protected]>

Signed-off-by: Akshay Tondak <[email protected]>

AShivangi

Looks good! Minor comments.
Thanks for the verbose documentation :)

src/runtime_src/core/tools/common/tests/TestHelper.cpp

@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.


AShivangi · 2024-09-17T17:40:15Z

src/runtime_src/core/tools/common/tests/TestHelper.h

@@ -0,0 +1,55 @@
+// SPDX-License-Identifier: Apache-2.0
+// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.


Suggested change

// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.

// Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.

src/runtime_src/core/tools/common/tests/TestHelper.h

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.cpp

Signed-off-by: Akshay Tondak <[email protected]>

…mbda for thread signalling Signed-off-by: Akshay Tondak <[email protected]>

aktondak · 2024-09-19T22:18:52Z

Please document the overall design and add comments to the key functions to explain the control flow logic.

@sonals Based on our offline sync, the updated patch moves thread synchronization variables to the TestSpatialSharing class and avoids passing them around with help from thread_ready_to_run lambda. Cleaner this way.
I plan to re-use the TestCase class for Temporal sharing test as well and I'll explore if the thread synchronization can be avoided completely with that code change.

Signed-off-by: Akshay Tondak <[email protected]>

sonals · 2024-09-20T19:38:57Z

src/runtime_src/core/tools/common/tests/TestHelper.cpp

+  bo_param = xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(2));
+  bo_ofm = xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(3));
+  bo_inter = xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(4));
+  bo_mc = xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(7));


Please use member initializer list to construct the bo objects

That's a good idea !
I have updated the latest patch with this change.
Thanks

Signed-off-by: Akshay Tondak <[email protected]>

sonals · 2024-09-20T23:09:41Z

src/runtime_src/core/tools/common/tests/TestHelper.cpp

+    bo_param (xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(2))),
+    bo_ofm   (xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(3))),
+    bo_inter (xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(4))),
+    bo_mc    (xrt::bo(device, buffer_size, XRT_BO_FLAGS_HOST_ONLY, kernel.group_id(7)))


Here we are first creating a temporary xrt::bo() and using to initialize the member class xrt::bo()
Please directly invoke the xrt::bo() ctor.

sonals · 2024-09-20T23:28:06Z

src/runtime_src/core/tools/common/tests/TestHelper.cpp

+      run_list[cnt].wait2();
+    }
+  }
+}


I think you should move the for loop at line 80 into a thread function that you kick off as part of thread constructor in TestSpatialSharingOvd::run(). The timer can be started just before the threads are constructed.

You shouldn't need thread_ready_to_run();

sonals · 2024-09-20T23:28:34Z

src/runtime_src/core/tools/common/tests/TestHelper.h

+  TestCase(xrt::xclbin& xclbin, std::string& kernel, xrt::device& device)
+      : device(device), xclbin(xclbin), kernel_name(kernel), buffer_size(1024), itr_count(1000) 
+  {
+    hw_ctx = xrt::hw_context(device, xclbin.get_uuid());


member initializer list please.

sonals · 2024-09-20T23:29:13Z

src/runtime_src/core/tools/common/tests/TestHelper.h

+
+public:
+  // Constructor to initialize the test case with xclbin and kernel name with hardware context creation
+  TestCase(xrt::xclbin& xclbin, std::string& kernel, xrt::device& device)


Should some of the args be const ref?

sonals · 2024-09-20T23:31:43Z

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.cpp

+
+  // Wait for both threads to be ready to begin clocking
+  wait_for_threads_ready((int)threads.size());
+


Not needed.

sonals · 2024-09-20T23:32:58Z

src/runtime_src/core/tools/common/tests/TestSpatialSharingOvd.cpp

+
+  /* Run 2 */
+  // Create a single test case and run it in a single thread
+  TestCase t(xclbin, kernelName, working_dev);


For run 2 use the same approach but call the loop with only one entry.

Spatial-sharing overhead implementation V1

f02d9a5

Signed-off-by: Akshay Tondak <[email protected]>

AShivangi reviewed Sep 5, 2024

View reviewed changes

Akshay Tondak added 2 commits September 11, 2024 15:36

Spatial sharing overhead V2 : Code cleanup and review comments handled

bb022ac

Signed-off-by: Akshay Tondak <[email protected]>

Spatial sharing overhead V3 : potential rhel build failure fix

e2e88ca

Signed-off-by: Akshay Tondak <[email protected]>

aktondak marked this pull request as ready for review September 11, 2024 23:09

aktondak requested review from rozumx and chvamshi-xilinx as code owners September 11, 2024 23:09

Spatial sharing overhead V3 : potential rhel build failure fix

2fb62a2

Signed-off-by: Akshay Tondak <[email protected]>

aktondak changed the title ~~Spatial-sharing overhead implementation~~ xrt-smi validate test for spatial-sharing overhead (VITIS-13000) Sep 12, 2024

aktondak requested review from sonals and pranjalv-xilinx September 12, 2024 16:54

sonals requested changes Sep 16, 2024

View reviewed changes

Comments addition with API explanations

7be236d

Signed-off-by: Akshay Tondak <[email protected]>

Akshay Tondak added 2 commits September 16, 2024 15:22

Moving common classes to a new file to be re-used by TemporalSharing …

c126e54

…benchmark Test Signed-off-by: Akshay Tondak <[email protected]>

Added Comment for control flow explanation

2574b5b

Signed-off-by: Akshay Tondak <[email protected]>

AShivangi approved these changes Sep 17, 2024

View reviewed changes

Akshay Tondak added 2 commits September 17, 2024 12:15

Review comments handling

cd11ea3

Signed-off-by: Akshay Tondak <[email protected]>

Review Comments handling

90a4c6d

Signed-off-by: Akshay Tondak <[email protected]>

aktondak requested a review from sonals September 17, 2024 19:26

Adding thread syncronization variable to class members and passing la…

af505f2

…mbda for thread signalling Signed-off-by: Akshay Tondak <[email protected]>

Akshay Tondak and others added 2 commits September 19, 2024 16:13

Build fix for xrt

26c91f0

Signed-off-by: Akshay Tondak <[email protected]>

Merge branch 'Xilinx:master' into VITIS-13000

ef37f4e

sonals reviewed Sep 20, 2024

View reviewed changes

Adding buffer objects creation to class initialization list

0b9b9ba

Signed-off-by: Akshay Tondak <[email protected]>

sonals reviewed Sep 20, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xrt-smi validate test for spatial-sharing overhead (VITIS-13000) #8395

xrt-smi validate test for spatial-sharing overhead (VITIS-13000) #8395

aktondak commented Sep 5, 2024 •

edited

Loading

gbuildx commented Sep 5, 2024

AShivangi left a comment

sonals left a comment

aktondak commented Sep 16, 2024

AShivangi left a comment

This comment was marked as resolved.

AShivangi Sep 17, 2024

aktondak commented Sep 19, 2024

sonals Sep 20, 2024

aktondak Sep 20, 2024

sonals Sep 20, 2024

sonals Sep 20, 2024

sonals Sep 20, 2024

sonals Sep 20, 2024

sonals Sep 20, 2024

sonals Sep 20, 2024

		@@ -0,0 +1,100 @@
		// SPDX-License-Identifier: Apache-2.0
		// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.

		@@ -0,0 +1,55 @@
		// SPDX-License-Identifier: Apache-2.0
		// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.

	// Copyright (C) 2023-2024 Advanced Micro Devices, Inc. All rights reserved.
	// Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.


		// Wait for both threads to be ready to begin clocking
		wait_for_threads_ready((int)threads.size());

xrt-smi validate test for spatial-sharing overhead (VITIS-13000) #8395

Are you sure you want to change the base?

xrt-smi validate test for spatial-sharing overhead (VITIS-13000) #8395

Conversation

aktondak commented Sep 5, 2024 • edited Loading

Problem solved by the commit

Bug / issue (if any) fixed, which PR introduced the bug, how it was discovered

How problem was solved, alternative solutions (if any) and why they were rejected

Risks (if any) associated the changes in the commit

What has been tested and how, request additional testing if necessary

Documentation impact (if any)

gbuildx commented Sep 5, 2024

AShivangi left a comment

Choose a reason for hiding this comment

sonals left a comment

Choose a reason for hiding this comment

aktondak commented Sep 16, 2024

AShivangi left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

aktondak commented Sep 19, 2024

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

aktondak commented Sep 5, 2024 •

edited

Loading