feature: Complete madvise for System Allocator #845
Conversation
Provides implmentation of getAtomicAccessAttribute using system allocator. Completes the implementation of the madvise feature using the new IOCTLs in xe_drm header. Signed-off-by: Chandio, Bibrak Qamar <bibrak.qamar.chandio@intel.com>
Provides implmentation of getAtomicAccessAttribute using system allocator. Completes the implementation of the madvise feature using the new IOCTLs in xe_drm header. Signed-off-by: Chandio, Bibrak Qamar <bibrak.qamar.chandio@intel.com>
Provides implmentation of getAtomicAccessAttribute using system allocator. Completes the implementation of the madvise feature using the new IOCTLs in xe_drm header. Signed-off-by: Chandio, Bibrak Qamar <bibrak.qamar.chandio@intel.com>
Provides implmentation of getAtomicAccessAttribute using system allocator. Completes the implementation of the madvise feature using the new IOCTLs in xe_drm header. Signed-off-by: Chandio, Bibrak Qamar <bibrak.qamar.chandio@intel.com>
bibrak
changed the title
| default: | ||
| return false; | ||
| } | ||
| uint64_t param = (static_cast<uint64_t>(atomicParam) << 32) | 0; |
There was a problem hiding this comment.
why we need to do those shifts after returning the value from iocthHelper?
IoctlHelper should rather do this translation internally if needed.
Also what is the point of | 0 ?
There was a problem hiding this comment.
ioctlHelper->getAtomicAccess(mode); returns 32bit int. Therefore, it is needed to shift it to the left since param needs to be 64bit containing two 32bit values. I will remove the | 0 its not needed.
There was a problem hiding this comment.
you can change it to 64 bit
There was a problem hiding this comment.
Its being used in many other places especially for legacy i915. That will require major refactoring.
There was a problem hiding this comment.
we need it anyway, as right now you need to do those shifts, which requires helper function
| auto ioctlHelper = drm.getIoctlHelper(); | ||
|
|
||
| // Apply the shared system USM IOCTL to all the VMs of the device | ||
| std::vector<uint32_t> vmIds; |
| return true; | ||
| } | ||
|
|
||
| AtomicAccessMode IoctlHelperXe::getVmSharedSystemAtomicAttribute(uint64_t handle, const size_t size, const std::vector<uint32_t> &vmIds) { | ||
| std::string vmIdsStr = "["; |
There was a problem hiding this comment.
do not create string just for purpose of logging
this would cause overhead even if logging is disabled
| query.vm_id, query.start, query.range, query.num_mem_ranges, query.sizeof_mem_range_attr, ret); | ||
|
|
||
| struct drm_xe_mem_range_attr *attr = (struct drm_xe_mem_range_attr *)ptr; | ||
| uint32_t val = attr->atomic.val; |
There was a problem hiding this comment.
what's the point of passing / allocating multiple ranges if atomic attribute is extracted only from the first one ?
There was a problem hiding this comment.
Currently, the L0 Spec is such that the user provides a range and expects a single atomic attribute.
There is no notion of user providing a range to zeMemSetAtomicAccessAttributeExp (in the form of const void *ptr, size_t size pair) that contains multiple ranges and therefore expects the return of multiple attributes.
So, this PR assumes that when the user calls zeMemSetAtomicAccessAttributeExp the call is made on a single allocation with the entire range having a single atomic attribute.
There was a problem hiding this comment.
yes, can we have similar simple IOCTL call with just one range?
Provides implmentation of getAtomicAccessAttribute using system allocator.
Completes the implementation of the madvise feature using the new IOCTLs in xe_drm header.