ps kernel hard code handling changes #8276

karthdmg-xilinx · 2024-07-08T18:42:57Z

Problem solved by the commit

The existing code reads/assumes ps kernel slot at 0th index by default when loading the xclbin based on UUID.
Made changes such that User queries based on UUID using IOCTL and reads the appropriate AXLF buffer.

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

Program PS kernel on latest XRT.

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

eliminate hard codes changes of loading ps kernel.

Risks (if any) associated the changes in the commit

None

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

Program PS, PL/PS and AIE kernels and verify the behavior.

Documentation impact (if any)

Need to update multi slot behavior for ps kernels.

Signed-off-by: rave <karthik>

stsoe · 2024-07-08T18:52:51Z

src/runtime_src/core/common/device.cpp

+#ifdef XCLBIN_FULL_READ
  auto xclbin_full = xrt_core::device_query<xrt_core::query::xclbin_full>(this);
  if (xclbin_full.empty())
    throw error(ENODEV, "no cached xclbin data");

-  const axlf* top = reinterpret_cast<axlf *>(xclbin_full.data());
+  const axlf* skd_axlf_ptr = reinterpret_cast<axlf *>(xclbin_full.data());
+#else
+  uint64_t ps_uuid_ptr = reinterpret_cast<uint64_t>(xclbin_id.get());
+  const axlf* skd_axlf_ptr;
+  char* buf = new char[MAX_AXLF_BUF_SIZE];
+
+  if (!buf)
+    throw error(ENOMEM, "Cannot allocate axlf buffer");
+
+  skd_axlf_ptr = reinterpret_cast<axlf*> (buf);
+  xrt_core::query::aie_skd_xclbin::args arg = {reinterpret_cast<uint64_t>(ps_uuid_ptr), reinterpret_cast<uint64_t>(skd_axlf_ptr)};
+
+  try {
+      xrt_core::device_query<xrt_core::query::aie_skd_xclbin>(this, arg);
+  }
+  catch (const std::exception &e) {
+    std::cerr << boost::format("ERROR: Failed to get xclbin for uuid (%lx)\n %s\n") % uuid_loaded.get() % e.what();
+    throw xrt_core::error(std::errc::operation_canceled);
+  }
+#endif


This needs commenting and cannot use new.

Added another query to read axlf size, please review

stsoe · 2024-07-08T18:53:24Z

src/runtime_src/core/include/xclbin.h

@@ -108,6 +108,8 @@ typedef GUID xuid_t;
 #define SIZE_OF_STRUCT(s) \
   char (*__fail)[sizeof(struct s)] = 1

+#define MAX_AXLF_BUF_SIZE       (4096*1024)


Looks random. What is this and why?

i Still see this macro

stsoe · 2024-07-08T18:54:05Z

src/runtime_src/core/common/query_requests.h

@@ -3819,6 +3820,23 @@ struct read_trace_data : request
  virtual std::any
  get(const device*, const std::any&) const = 0;
 };
+
+struct aie_skd_xclbin : request


Add a comment about this request type.

chvamshi-xilinx

As discussed offline, Please simplify the logic and update existing load_xclbin to support this case also.
No new #ifdefs . We are already struggling to remove AIE* defines

Signed-off-by: rave <karthik>

saifuddin-xilinx · 2024-07-12T17:11:57Z

src/runtime_src/core/edge/drm/zocl/common/zocl_sysfs.c

@@ -600,7 +600,7 @@ static ssize_t read_xclbin_full(struct file *filp, struct kobject *kobj,
 	read_lock(&zdev->attr_rwlock);

 	// Only read slot 0's xclbin - TODO: extend to multi-slot 
-	zocl_slot = zdev->pr_slot[0];
+	zocl_slot = zdev->pr_slot[1];


This index should come as an input.
Think about, if two or more PS XCLBINs are loaded

this is a dead code, sysfs functionality is being used anymore

saifuddin-xilinx · 2024-07-12T17:14:08Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+	}
+	mutex_lock(&zdev->aie_lock);
+
+	for (i = 0;i < MAX_PR_SLOT_NUM;i++) {


Please correct the format
for (i = 0; i < MAX_PR_SLOT_NUM; i++)

saifuddin-xilinx · 2024-07-12T17:15:03Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+
+	for (i = 0;i < MAX_PR_SLOT_NUM;i++) {
+		struct drm_zocl_slot *slot = NULL;
+		slot = zdev->pr_slot[i];


Sanity Check required, before accessing the fields

if (!slot)
continue;

saifuddin-xilinx · 2024-07-12T17:15:55Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+	}
+	mutex_lock(&zdev->aie_lock);
+
+	for (i = 0;i < MAX_PR_SLOT_NUM;i++) {


Please correct the format

saifuddin-xilinx · 2024-07-12T17:16:11Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+		struct drm_zocl_slot *slot = NULL;
+		slot = zdev->pr_slot[i];
+		mutex_lock(&zdev->pr_slot[i]->slot_xclbin_lock);
+		slot_uuid = zocl_xclbin_get_uuid(zdev->pr_slot[i]);


Sanity check for unused slot

saifuddin-xilinx · 2024-07-12T17:18:34Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+	if(i == MAX_PR_SLOT_NUM)
+	{
+		DRM_ERROR(" %s: UUID not found \n",__func__);
+		return -ENODEV;


mutex_unlock(&zdev->aie_lock);

Signed-off-by: rave <karthik>

chvamshi-xilinx · 2024-07-13T13:14:02Z

src/runtime_src/core/include/xclbin.h

@@ -108,6 +108,8 @@ typedef GUID xuid_t;
 #define SIZE_OF_STRUCT(s) \
   char (*__fail)[sizeof(struct s)] = 1

+#define MAX_AXLF_BUF_SIZE       (4096*1024)


i Still see this macro

chvamshi-xilinx · 2024-07-13T13:15:16Z

src/runtime_src/core/edge/skd/xrt_skd.cpp

@@ -52,6 +52,7 @@ namespace xrt {
      m_parent_bo_handle(xrt::shim_int::get_buffer_handle(xrtDeviceToXclDevice(handle), parent_mem_bo_in)),
      m_mem_start_paddr(mem_start_paddr_in),
      m_mem_size(mem_size_in)
+


remove the new line?

chvamshi-xilinx · 2024-07-13T13:18:37Z

src/runtime_src/core/edge/user/device_linux.cpp

+    if (fd_obj->fd < 0)
+      throw xrt_core::error(-EINVAL, boost::str(boost::format("Cannot open %s") % zocl_device));
+
+    if (ioctl(fd_obj->fd, DRM_IOCTL_ZOCL_SKD_AXLF_SIZE, &aie_arg))


please dont call ioctls from here. create shim function and call it..

chvamshi-xilinx · 2024-07-13T13:19:10Z

src/runtime_src/core/edge/user/device_linux.cpp

+    struct drm_zocl_skd_axlf_size aie_arg;
+    aie_arg.ps_uuid_ptr = std::any_cast<xrt_core::query::skd_axlf_size::ps_uuid_ptr_type>(skd_uuid_ptr);
+
+    const std::string zocl_device = "/dev/dri/" + get_render_devname();


shim function should take care of this. please dont repeat the logic

chvamshi-xilinx · 2024-07-13T13:19:48Z

src/runtime_src/core/edge/user/device_linux.cpp

+    aie_arg.ps_uuid_ptr = std::any_cast<xrt_core::query::aie_skd_xclbin::ps_uuid_ptr_type>(args.ps_uuid_ptr);
+    aie_arg.skd_axlf_ptr = std::any_cast<xrt_core::query::aie_skd_xclbin::skd_axlf_ptr_type>(args.skd_axlf_ptr);
+
+    const std::string zocl_device = "/dev/dri/" + get_render_devname();


same as below. create shim function to handle this

chvamshi-xilinx · 2024-07-13T13:21:03Z

src/runtime_src/core/edge/include/zynq_ioctl.h

@@ -124,7 +124,9 @@ enum drm_zocl_ops {
 	DRM_ZOCL_AIE_FREQSCALE,
 	/* Set CU read-only range */
 	DRM_ZOCL_SET_CU_READONLY_RANGE,
-	DRM_ZOCL_NUM_IOCTLS
+	DRM_ZOCL_NUM_IOCTLS,


NUM_IOCTL should be at the end. it should be last macro.

chvamshi-xilinx · 2024-07-13T13:22:11Z

src/runtime_src/core/common/query_requests.h

+  get(const device*, const std::any& arg) const = 0;
+};
+
+struct skd_axlf_size : request


add comment for this too.

chvamshi-xilinx · 2024-07-13T13:23:37Z

src/runtime_src/core/common/device.cpp


-  const axlf* top = reinterpret_cast<axlf *>(xclbin_full.data());
+  xrt_core::query::aie_skd_xclbin::args arg = {reinterpret_cast<uint64_t>(ps_uuid_ptr), reinterpret_cast<uint64_t>(buf.data())};


ps_uuid_ptr is already uint64_t, dont call reinterpret cast.

Signed-off-by: rave <karthik>

saifuddin-xilinx · 2024-07-15T10:55:10Z

src/runtime_src/core/edge/drm/zocl/edge/zocl_aie.c

+
+	for (i = 0; i < MAX_PR_SLOT_NUM; i++) {
+		struct drm_zocl_slot *slot = NULL;
+		slot = zdev->pr_slot[i];


if (!slot)
continue;

Signed-off-by: rave <karthik>

chvamshi-xilinx · 2024-07-24T10:19:08Z

Hi @karthdmg-xilinx ,
As discussed offline, Please close this PR and make modifications to choose slot 0 for all of these cases.

karthdmg-xilinx · 2024-07-26T06:49:20Z

Not handling the multi slot changes for now, hence rejecting this PR

ps kernel hard code handling changes

1e7e50f

Signed-off-by: rave <karthik>

karthdmg-xilinx requested review from xdavidz, saifuddin-xilinx and bisingha-xilinx July 8, 2024 18:42

karthdmg-xilinx requested review from chvamshi-xilinx, stsoe, jeffli-xilinx, sonals, mamin506 and rozumx as code owners July 8, 2024 18:42

stsoe requested changes Jul 8, 2024

View reviewed changes

chvamshi-xilinx requested changes Jul 12, 2024

View reviewed changes

rave added 2 commits July 12, 2024 22:17

Addressed comments, defined new ioctl to query axlf size

25ecdb9

Signed-off-by: rave <karthik>

deleted .rej file

aace971

Signed-off-by: rave <karthik>

karthdmg-xilinx requested review from stsoe and chvamshi-xilinx July 12, 2024 16:55

saifuddin-xilinx requested changes Jul 12, 2024

View reviewed changes

format changes

1141d8c

Signed-off-by: rave <karthik>

chvamshi-xilinx requested changes Jul 13, 2024

View reviewed changes

shim changes

2eb8138

Signed-off-by: rave <karthik>

karthdmg-xilinx requested review from chvamshi-xilinx and saifuddin-xilinx July 15, 2024 09:47

saifuddin-xilinx reviewed Jul 15, 2024

View reviewed changes

sanity check

1323044

Signed-off-by: rave <karthik>

karthdmg-xilinx closed this Jul 26, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ps kernel hard code handling changes #8276

ps kernel hard code handling changes #8276

karthdmg-xilinx commented Jul 8, 2024

stsoe Jul 8, 2024

karthdmg-xilinx Jul 12, 2024

stsoe Jul 8, 2024

karthdmg-xilinx Jul 12, 2024

chvamshi-xilinx Jul 13, 2024

stsoe Jul 8, 2024

karthdmg-xilinx Jul 12, 2024

chvamshi-xilinx left a comment

saifuddin-xilinx Jul 12, 2024

karthdmg-xilinx Jul 13, 2024

saifuddin-xilinx Jul 12, 2024

saifuddin-xilinx Jul 12, 2024

saifuddin-xilinx Jul 15, 2024

saifuddin-xilinx Jul 12, 2024

saifuddin-xilinx Jul 12, 2024

saifuddin-xilinx Jul 12, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

chvamshi-xilinx Jul 13, 2024

saifuddin-xilinx Jul 15, 2024

chvamshi-xilinx commented Jul 24, 2024

karthdmg-xilinx commented Jul 26, 2024


		const axlf* top = reinterpret_cast<axlf *>(xclbin_full.data());
		xrt_core::query::aie_skd_xclbin::args arg = {reinterpret_cast<uint64_t>(ps_uuid_ptr), reinterpret_cast<uint64_t>(buf.data())};

ps kernel hard code handling changes #8276

ps kernel hard code handling changes #8276

Conversation

karthdmg-xilinx commented Jul 8, 2024

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)

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

chvamshi-xilinx left a 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

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

chvamshi-xilinx commented Jul 24, 2024

karthdmg-xilinx commented Jul 26, 2024