ve2: add support for create/destroy hw context by rajkumar-xilinx · Pull Request #560 · amd/xdna-driver

rajkumar-xilinx · 2025-05-27T19:39:22Z

-Support hw context create/destroy ioctls
-Add new ve2 resourse solver drivers to manage aie columns
resources among hw contexts.
-Add new ve2 management driver for cert firmware

-Support hw context create/destroy ioctls -Add new ve2 resourse solver drivers to manage aie columns resources among hw contexts. -Add new ve2 management driver for cert firmware Signed-off-by: Raj Kumar Rampelli <[email protected]>

EugeneHiew · 2025-05-27T19:41:08Z

Can one of the admins verify this patch?

rajkumar-xilinx · 2025-05-27T19:43:37Z

Internal review in progress, so marked as draft.

saifuddin-xilinx · 2025-05-29T09:17:22Z

src/driver/amdxdna/ve2_host_queue.h

@@ -0,0 +1,219 @@
+/* SPDX-License-Identifier: GPL-2.0 */


The Handshake region is got updated in CERT. Could you please check once and update accordingly

saifuddin-xilinx · 2025-06-03T07:10:09Z

src/driver/amdxdna/amdxdna_of_drv.c

+	xrs_cfg.ddev = &xdna->ddev;
+	xrs_cfg.total_col = XRS_MAX_COL;
+
+	if (xdna->dev_handle)


xdna->dev_handle
This one checks twice.

Suggested change

if (xdna->dev_handle)

if (xdna->dev_handle) {

xdna->dev_handle->xrs_hdl = xrsm_init(&xrs_cfg);

if (!xdna->dev_handle->xrs_hdl) {

XDNA_ERR(xdna, "Initialize resolver failed");

drm_dev_put(&xdna->ddev);

return -EINVAL;

}

}

saifuddin-xilinx · 2025-06-03T07:19:07Z

src/driver/amdxdna/ve2_mgmt.c

+
+	total_col = xrs_get_total_cols(xdna->dev_handle->xrs_hdl);
+	if (total_col < 0)
+		return -EINVAL;


Before return we should free xrs_req memory also unlock the lock

saifuddin-xilinx · 2025-06-03T08:27:03Z

src/driver/amdxdna/ve2_mgmt.c

+		ret = cert_setup_partition(aie_dev, col, start_col, num_col, hsa_addr);
+		if (ret < 0) {
+			XDNA_ERR(xdna, "cert_setup_partition() err %d for col %d", ret, start_col);
+			return ret;


As we are returning from here please teardown and release the aie partition

saifuddin-xilinx · 2025-06-03T08:29:00Z

src/driver/amdxdna/ve2_mgmt.c

+	for (u32 col = start_col; col < start_col + num_col; col++) {
+		rel_col = col - start_col;
+		ret = cert_clear_partition(xdna, priv->aie_dev, rel_col);
+		if (ret < 0)
+			XDNA_ERR(xdna, "cert_clear_partition() err %d for col %d", ret, rel_col);
+	}


Please update the loop.

Suggested change

for (u32 col = start_col; col < start_col + num_col; col++) {

rel_col = col - start_col;

ret = cert_clear_partition(xdna, priv->aie_dev, rel_col);

if (ret < 0)

XDNA_ERR(xdna, "cert_clear_partition() err %d for col %d", ret, rel_col);

}

for (u32 col = 0; col < num_col; col++) {

ret = cert_clear_partition(xdna, priv->aie_dev, col);

if (ret < 0)

XDNA_ERR(xdna, "cert_clear_partition() err %d for col %d", ret, col);

}

saifuddin-xilinx · 2025-06-03T08:31:35Z

src/driver/amdxdna/ve2_mgmt.c

+	aie_dev = aie_partition_request(&request);
+	if (IS_ERR(aie_dev)) {
+		XDNA_ERR(xdna, "aie parition request failed, error %ld", PTR_ERR(aie_dev));
+		return -ENODEV;


Please release the resource if failed case xrs_release_resource
Same subsequently

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 · 2025-06-04T17:16:02Z

ok to test

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 · 2025-06-09T03:04:12Z

src/driver/amdxdna/amdxdna_ctx.c

+	if (!xdna->dev_info->ops->ctx_init)
+		return -EOPNOTSUPP;
+


Why adding this check? I don't think we need this check. I don't see any device that doesn't support this ctx_init() callback.
Please let me know what the justification for this is. Otherwise, please remove.

mamin506 · 2025-06-09T03:05:48Z

src/driver/amdxdna/amdxdna_of_drv.c

 #include <drm/drm_managed.h>

 #include "amdxdna_of_drv.h"
+#include "ve2_res_solver.h"


NEVER include aie2_, aie4_ and ve2_ header in amdxdna_ files. Please remove this line and make necessary changes.

src/driver/amdxdna/ve2_hwctx.c

mamin506 · 2025-06-09T03:14:34Z

src/driver/amdxdna/ve2_host_queue.h

+		u16 minor;
+	}
+	version;
+	//Queue capacity, must be a power of two.


Use WARN_ON() instead of comment. Delete this comment pls.

mamin506 · 2025-06-09T03:16:20Z

src/driver/amdxdna/ve2_host_queue.h

+	union {
+		struct {
+			u16 type: 8;
+			u16 barrier: 1;
+			u16 acquire_fence_scope: 2;
+			u16 release_fence_scope: 2;
+		};
+		u16	header;
+	};


Not suggesting coding style. Ref aie2_msg_priv.h

If this triggers many codes change, we can merge this for open source. But this needs to be refine when upstreaming.

mamin506 · 2025-06-09T03:45:04Z

src/driver/amdxdna/ve2_host_queue.h

+		u32	size;			//5c
+	}
+	dbg_buf;
+	volatile struct {


Can you put a comment to explain why 'volatile' is needed for this but not for above fields?

mamin506 · 2025-06-09T03:45:55Z

src/driver/amdxdna/ve2_host_queue.h

+		// number of checks whether there are jobs ready
+		u32	c_job_readiness_checked;
+		// number of opcode run
+		u32	c_opcode;
+		u32	c_job_launched;
+		u32	c_job_finished;
+		// number of hsa pkt handled
+		u32	c_hsa_pkt;
+		// number of pages loaded
+		u32	c_page;
+		// number of hsa doorbell ring
+		u32	c_doorbell;
+		// number of uc memory(PM) scrub
+		u32	c_uc_scrub;
+		// number of tct requested
+		u32	c_tct_requested;
+		// number of tct received
+		u32	c_tct_received;
+		// run out of wait handle UC_DMA_WRITE_DES opcode
+		u16	c_preemption_ucdma;
+		// run out of wait handle UC_DMA_WRITE_DES_SYNC opcode
+		u16	c_preemption_ucdma_sync;
+		// POLL_32 opcode retry times
+		u16	c_preemption_poll;
+		// MASK_POLL_32 opcode retry times
+		u16	c_preemption_mask_poll;
+		// run out of physical barrier REMOTE_BARRIER opcode
+		u16	c_preemption_remote_barrier;
+		// actor entry overflow or run out of wait handle WAIT_TCTS opcode
+		u16	c_preemption_wait_tct;
+		// block UC_DMA_WRITE_DES opcode
+		u16	c_block_ucdma;
+		// block UC_DMA_WRITE_DES_SYNC opcode
+		u16	c_block_ucdma_sync;
+		// block local_barrier opcode
+		u16	c_block_local_barrier;
+		// block REMOTE_BARRIER opcode
+		u16	c_block_remote_barrier;
+		// block WAIT_TCTS opcode
+		u16	c_block_wait_tct;
+		// number of slow actor entry lookup
+		u16	c_actor_hash_conflict;


Instead of comment, use readable field name is prefer.

mamin506 · 2025-06-09T03:49:07Z

src/driver/amdxdna/ve2_hwctx.c

+free_host_queue:
+	ve2_free_hsa_queue(xdna, &hwctx->priv->hwctx_hsa_queue);


Are host_queue and hsa_queu the same? Even they are the same, please use consistent terminology to avoid confusion.

mamin506 · 2025-06-09T03:54:54Z

src/driver/amdxdna/ve2_mgmt.c

+static void ve2_irq_handler(u32 partition_id, void *cb_arg)
+{
+	struct amdxdna_ctx *hwctx = (struct amdxdna_ctx *)cb_arg;
+	struct amdxdna_dev *xdna = hwctx->client->xdna;
+
+	XDNA_DBG(xdna, "Created partition for start_col %d, num_col %d with partition_id %d\n",
+		 hwctx->start_col, hwctx->num_col, partition_id);
+}


This does nothing but only print a debug message. Is this expected?

This function doesn't look like a Linux IRQ handler. If my understanding is correct, please rename this to something else. This is more like a callback function.

src/driver/amdxdna/ve2_mgmt.c

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 · 2025-06-10T18:18:37Z

The checkpatch step failed in the CI. Please fix that.

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506

Overall, this looks good to me. Just some minor changes and suggestion. Please take a look. The upper_32_bits() macro is a must have change.

mamin506 · 2025-06-11T22:15:50Z

src/driver/amdxdna/ve2_of.h

+#define ADDR64_HIGH(x)		(((x) >> 32) & 0xFFFFFFFF)
+#define ADDR64_LOW(x)		((x) & 0xFFFFFFFF)
+


Linux has predefined macro for the same purpose. Please use those macros,
See https://elixir.bootlin.com/linux/v6.15/source/include/linux/wordpart.h#L14

mamin506 · 2025-06-11T22:24:27Z

src/driver/amdxdna/ve2_mgmt.c

+	/* save aie_dev into priv */
+	priv->aie_dev = aie_dev;


Unnecessary comment. :D

mamin506 · 2025-06-11T22:30:21Z

src/driver/amdxdna/ve2_mgmt.c

+	if (!xrs)
+		return -EINVAL;
+


The xrt_hdl is assigned when probe the device. It is OK to remove this check.
Unless xrs can be dynamically load/unload, and maybe the caller of this function doesn't aware of it. You can check such a fundamental struct. :)

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 · 2025-06-12T18:48:23Z

retest this please

ve2: add support for create/destroy hw context

6698300

-Support hw context create/destroy ioctls -Add new ve2 resourse solver drivers to manage aie columns resources among hw contexts. -Add new ve2 management driver for cert firmware Signed-off-by: Raj Kumar Rampelli <[email protected]>

rajkumar-xilinx marked this pull request as draft May 27, 2025 19:39

mamin506 requested a review from chvamshi-xilinx May 28, 2025 16:46

saifuddin-xilinx reviewed May 29, 2025

View reviewed changes

saifuddin-xilinx reviewed Jun 3, 2025

View reviewed changes

Raj Kumar Rampelli added 2 commits June 4, 2025 22:39

Merge remote-tracking branch 'origin/main' into 28may_hwctx

353f1f5

Fix review comments

3dd4071

Signed-off-by: Raj Kumar Rampelli <[email protected]>

rajkumar-xilinx requested a review from saifuddin-xilinx June 4, 2025 17:11

mamin506 marked this pull request as ready for review June 4, 2025 17:17

Raj Kumar Rampelli added 2 commits June 5, 2025 11:47

ve2: update cert handshake packet structure to latest

573187d

Signed-off-by: Raj Kumar Rampelli <[email protected]>

Merge remote-tracking branch 'origin/main' into 28may_hwctx

2dcbb73

saifuddin-xilinx approved these changes Jun 6, 2025

View reviewed changes

chvamshi-xilinx approved these changes Jun 6, 2025

View reviewed changes

ve2: fix compile time errors

b285ef0

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 suggested changes Jun 9, 2025

View reviewed changes

Raj Kumar Rampelli added 3 commits June 10, 2025 10:34

Merge remote-tracking branch 'origin/main' into 28may_hwctx

8ccb506

Fix review comments

a6ab945

Signed-off-by: Raj Kumar Rampelli <[email protected]>

Fix codingsty_check errors

a91806a

Signed-off-by: Raj Kumar Rampelli <[email protected]>

rajkumar-xilinx requested a review from mamin506 June 10, 2025 06:53

ve2: Remove typedef usage

133b3f5

Signed-off-by: Raj Kumar Rampelli <[email protected]>

Raj Kumar Rampelli added 2 commits June 11, 2025 10:04

Merge remote-tracking branch 'origin/main' into 28may_hwctx

3dd0a02

Fix checkpatch errors

48688d6

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 approved these changes Jun 11, 2025

View reviewed changes

mamin506 self-requested a review June 11, 2025 22:35

Raj Kumar Rampelli added 2 commits June 12, 2025 11:05

Merge remote-tracking branch 'origin/main' into 28may_hwctx

6ff125e

ve2: fix review comments

f3cad85

Signed-off-by: Raj Kumar Rampelli <[email protected]>

mamin506 approved these changes Jun 12, 2025

View reviewed changes

mamin506 merged commit 84455db into amd:main Jun 12, 2025
1 check passed

rajkumar-xilinx deleted the 28may_hwctx branch June 13, 2025 03:49

-	if (xdna->dev_handle)
+	        if (xdna->dev_handle) {
+                xdna->dev_handle->xrs_hdl = xrsm_init(&xrs_cfg);
+                if (!xdna->dev_handle->xrs_hdl) {
+                        XDNA_ERR(xdna, "Initialize resolver failed");
+                        drm_dev_put(&xdna->ddev);
+                        return -EINVAL;
+                }
+        }

		free_host_queue:
		ve2_free_hsa_queue(xdna, &hwctx->priv->hwctx_hsa_queue);

		#define ADDR64_HIGH(x) (((x) >> 32) & 0xFFFFFFFF)
		#define ADDR64_LOW(x) ((x) & 0xFFFFFFFF)

Conversation

rajkumar-xilinx commented May 27, 2025

Uh oh!

EugeneHiew commented May 27, 2025

Uh oh!

rajkumar-xilinx commented May 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saifuddin-xilinx Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saifuddin-xilinx Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamin506 commented Jun 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mamin506 commented Jun 10, 2025

Uh oh!

mamin506 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mamin506 commented Jun 12, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saifuddin-xilinx Jun 3, 2025 •

edited

Loading

saifuddin-xilinx Jun 3, 2025 •

edited

Loading