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

VITIS-11024 enable hw_context support for xrt::graph objects 3rd commit #8292

Merged

Conversation

SravanKumarAllu-xilinx
Copy link
Collaborator

Problem solved by the commit

https://jira.xilinx.com/browse/VITIS-11024
currently we can load only one xclbin in graph supported devices , we cannot load multiple xclbins in multiple partitions in device..

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

N/A

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

These third set of changes are enabling interface to support multiple xclbins in multiple partitions in device by creating "hardware context" associated with device id and xclbin. Interfaces are created for application developers and driver component owners to load mulitple xclbins in device by creating hardware context with particular device and particualr xclbin. New shim changes (driver changes) are required to fully utilize hardware context interfaces and fullfill actual requirements. currently used old shim by extracting device and xclbin from hardware context (loaded single device id with single xclbin)

Risks (if any) associated the changes in the commit

Documentation for this change might be required, should not break old flow where it still supports to load xclbin with device id. And as mentioned in above field new shim changes (driver changes) are required.

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

Used Graph test use case where input array and input factor are provided, ouput array would be generated by muliplying input array with input factor.

Initializing ADF API...
[ 611.693563] zocl-drm axi:zyxclmm_drm: zocl_create_client: created KDS client for pid(598), ret: 0
[ 611.699710] zocl-drm axi:zyxclmm_drm: zocl_destroy_client: client exits pid(598)
[ 611.774761] zocl-drm axi:zyxclmm_drm: zocl_create_client: created KDS client for pid(598), ret: 0
[ 620.614085] [drm] Loading xclbin 1482d52e-6e09-f433-e081-3e5e84b5c99f to slot 0
[ 620.614352] [drm] found kind 29(AIE_RESOURCES)
[ 620.616651] [drm] skip kind 8(IP_LAYOUT) return code: -22
[ 620.617634] [drm] found kind 9(DEBUG_IP_LAYOUT)
[ 620.618947] [drm] found kind 25(AIE_METADATA)
[ 620.619933] [drm] skip kind 7(CONNECTIVITY) return code: -22
[ 620.621097] [drm] found kind 6(MEM_TOPOLOGY)
[ 620.623921] [drm] Memory 0 is not reserved in device tree. Will allocate memory from CMA
[ 620.884871] [drm] AIE create successfully finished.
XAIEFAL: INFO: Resource group Avail is created.
XAIEFAL: INFO: Resource group Static is created.
XAIEFAL: INFO: Resource group Generic is created.
[ 620.886708] [drm] zocl_xclbin_read_axlf 1482d52e-6e09-f433-e081-3e5e84b5c99f ret: 0
[ 621.133162] [drm] bitstream 1482d52e-6e09-f433-e081-3e5e84b5c99f locked, ref=1
[ 621.137693] zocl-drm axi:zyxclmm_drm: ffff000001f45410 kds_add_context: Client pid(598) add context Domain(65535) CU(0xffff) shared(true)
[ 621.149018] zocl-drm axi:zyxclmm_drm: ffff000001f45410 kds_del_context: Client pid(598) del context Domain(65535) CU(0xffff)
graph run completed .....
trying alias
[ 621.153148] [drm] bitstream 1482d52e-6e09-f433-e081-3e5e84b5c99f unlocked, ref=0
[ 622.304419] hrtimer: interrupt took 90673930 ns
graph rtp read completed .....
graph end completed .....
output Vector 1 value 3
output Vector 1 value 6
output Vector 1 value 9
output Vector 1 value 12
output Vector 1 value 15
output Vector 1 value 18
output Vector 1 value 21
output Vector 1 value 24
TEST PASSED

Documentation impact (if any)

Graph doumentation may needs to be updated with "hwctx" things. And currently both kernel and graph support only c++ API's with "hardware context"

@gbuildx
Copy link
Collaborator

gbuildx commented Jul 15, 2024

Can one of the admins verify this patch?

Copy link
Collaborator

@chvamshi-xilinx chvamshi-xilinx left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup. Please fix the review comments.

@@ -588,63 +533,6 @@ struct noshim : public DeviceType
{
throw ishim::not_supported_error(__func__);
}

#ifdef XRT_ENABLE_AIE
Copy link
Collaborator

Choose a reason for hiding this comment

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

we would still require these functions for noshim. Please keep them as it is.

// SPDX-License-Identifier: Apache-2.0
// Copyright (C) 2024 Advanced Micro Devices, Inc. All rights reserved.

#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

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

better to use #ifndef for the consistency.

@@ -15,16 +15,15 @@
* under the License.
*/

#ifndef AIE_D_H
#define AIE_D_H
#pragma once
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here. use #ifndef

@@ -21,6 +21,7 @@
#include <sstream>
#include <map>

#ifdef XRT_ENABLE_AIE
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file is under XRT_ENABLE_AIE flag. Can you please update CMAKe to reflect this instead of whole file under ifdef.

@@ -23,6 +23,7 @@
#include <queue>
#include <vector>

#ifdef XRT_ENABLE_AIE
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file is under XRT_ENABLE_AIE flag. Can you please update CMAKe to reflect this instead of whole file under ifdef.

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

#ifdef XRT_ENABLE_AIE
Copy link
Collaborator

Choose a reason for hiding this comment

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

This whole file is under XRT_ENABLE_AIE flag. Can you please update CMAKe to reflect this instead of whole file under ifdef.


#pragma once

#ifdef XRT_ENABLE_AIE
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ditto

@SravanKumarAllu-xilinx SravanKumarAllu-xilinx force-pushed the graph_instance_removal branch 3 times, most recently from 7ebc241 to b97c989 Compare July 16, 2024 11:51
@SravanKumarAllu-xilinx
Copy link
Collaborator Author

This PR contains

  1. Fix for "RPM build fail for zcu104_base_platform" -- CR-1205718
  2. shifting of "aie" related functions from "struct shim : public DeviceType" to "struct ishim" (ishim.h) and then to respective device_linux.cpp" file.
  3. Removed intermittant "graph_instance" class and handle graph related functions directly with "graph_object" class.
  4. Removed "graph.h" file , which is not used anymore.

Copy link
Collaborator

@chvamshi-xilinx chvamshi-xilinx left a comment

Choose a reason for hiding this comment

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

You need to implement aie* functions in sw_emu also.

Copy link
Collaborator

@stsoe stsoe left a comment

Choose a reason for hiding this comment

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

Minor nits.

src/runtime_src/core/edge/user/aie/graph_object.cpp Outdated Show resolved Hide resolved
src/runtime_src/core/edge/user/aie/graph_object.cpp Outdated Show resolved Hide resolved
src/runtime_src/core/edge/user/aie/graph_object.cpp Outdated Show resolved Hide resolved
src/runtime_src/core/edge/user/aie/graph_object.h Outdated Show resolved Hide resolved
@chvamshi-xilinx chvamshi-xilinx merged commit d765f2f into Xilinx:master Jul 17, 2024
17 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.

4 participants