rradjabi / xrt Goto Github PK

This is a memory leak:

	/* Copy MEM_TOPOLOGY from new_toplogy if not preserving memory. */
	if (!preserve_mem) {
		xdev->topology = new_topology;
	}

Apply fix suggested below:
Xilinx#466 (comment)

Add logging in shim.cpp to aid debugging.

XRT on Pegasus cannot find device when using rradjabi/XRT:aws-2018_2-data-retention

See changes in xbutil query and dump as of 2018.3.

maxzhen requested changes a day ago
src/runtime_src/driver/xclng/drm/xocl/userpf/xocl_ioctl.c
size = xocl_read_sect(MEM_TOPOLOGY, &new_topology, axlf, buf);
if (size <= 0) {
if (size != 0)
goto done;
@maxzhen
maxzhen a day ago Collaborator
We will fall through when size == 0. Please double check the code to see if the rest of the code still makes sense in 0 size case, especially line 551 if (!memcmp(new_topology, xdev->topology, size)).

@rradjabi
rradjabi 21 hours ago Collaborator
This chunk isn't an addition from my change, it already existed. See Lines 575-577 of the original.

I'd prefer to keep the changes and open a new issue to patch this.

@maxzhen
maxzhen 20 hours ago Collaborator
But you added memcmp(new_topology, xdev->topology, size). The old code makes sense, but does it work for your memcmp()? If size == 0, will memcmp() thinks the new topology is the same or different? Does the result match your expectation? Also, you've moved the code around, what makes sense before may not make sense any more. I don't understand why you think it's OK to leave it as is only because it was written that way at a different place?

These will produce warnings when an AFI/xclbin is loaded and they're confusing. This is because of the default shell load.

Data retention is in the common xocl driver, but should only apply to AWS either on F1 or Pegasus.

Super-issue with sub-issues:

• #4
• #5
• #6

For the TA we need to build an RPM for AWS. This requires awsmgmt dkms build. It would be ideal to generate an RPM with *-aws.rpm for this purpose.

We should do the same as the xbsak->xbutil transition.

When DRAM Data Retention is not supported, we should show the warning, but make the message friendlier.