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

Add support for allocating DMA buffers #2170

Merged
merged 4 commits into from
Feb 26, 2025
Merged

Conversation

ahesham-arm
Copy link
Contributor

@ahesham-arm ahesham-arm commented Dec 5, 2024

This adds support for allocating DMA buffers on systems that support it, i.e. Linux and Android.

On mainline Linux, starting version 5.6 (equivalent to Android 12), there is a new kernel module framework available called DMA-BUF Heaps. The goal of this framework is to provide a standardised way for user applications to allocate and share memory buffers between different devices, subsystems, etc. The main feature of interest is that the framework provides device-agnostic allocation; it abstracts away the underlying hardware, and provides a single IOCTL, DMA_HEAP_IOCTL_ALLOC. Mainline implementation provides two heaps that act as character devices that can allocate DMA buffers; system, which uses the buddy allocator, and cma, which uses the CMA (Contiguous Memory Allocator). Both of these are kernel configuration options that need to be enabled when building the Linux kernel. Generally, any kernel module implementing this framework is made available under /dev/dma_heaps/<heap_name>, e.g. /dev/dma_heaps/system.

The implementation currently only supports one type of DMA heaps; system, the default device path for which is /dev/dma_heap/system. The path can be overridden at runtime using an environment variable, OCL_CTS_DMA_HEAP_PATH_SYSTEM, if needed. Extending this in the future should be trivial (subject to platform support), by adding an entry to the enum dma_buf_heap_type, and an appropriate default path and overriding environment variable name.

The proposed implementation will conditionally compile if the conditions are met (i.e. building for Linux or Android, using kernel headers >= 5.6.0), and will provide a compile-time warning otherwise, and return -1 as the DMA handle in runtime if not.

To demonstrate the functionality, a new test is added for the cl_khr_external_memory_dma_buf extension. If the extension is supported by the device, a DMA buffer will be allocated and used to create a CL buffer, that is then used by a simple kernel.

This should provide a way forward for adding more tests that depend on DMA buffers.

@ahesham-arm ahesham-arm force-pushed the dma_buf branch 3 times, most recently from c5cdd6d to 841cf41 Compare December 5, 2024 20:22
The test demonstrates how an imported buffer can be created from a DMA
buffer, allocated using the DMA heap system, which is now supported by
the CTS.

Signed-off-by: Gorazd Sumkovski <[email protected]>
@bashbaug bashbaug self-requested a review December 10, 2024 18:05
@ahesham-arm ahesham-arm force-pushed the dma_buf branch 2 times, most recently from 01b977f to 0022809 Compare December 18, 2024 14:45
Add the ability to specify the type of DMA heap to use when allocating
a DMA buffer, the path of which can be overridden using an environment
variable, `OCL_CTS_DMA_HEAP_PATH_SYSTEM`.

Add Doxygen-style documentation for the new function.

Signed-off-by: Ahmed Hesham <[email protected]>
@kpet
Copy link
Contributor

kpet commented Jan 21, 2025

Discussed in the 2025/01/21 memory TSG: we should update the PR to skip the tests when hitting permission issues instead of failing them.

@CLAassistant
Copy link

CLAassistant commented Feb 10, 2025

CLA assistant check
All committers have signed the CLA.

@ahesham-arm
Copy link
Contributor Author

Updated to address the review comments.

@joshqti
Copy link
Contributor

joshqti commented Feb 20, 2025

We are ok with this change

bashbaug
bashbaug previously approved these changes Feb 26, 2025
Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

Mostly LGTM. I left comments for a few nice-to-have fixes, but nothing is must-fix. Thanks!

Comment on lines +71 to +74
int dma_buf_fd = allocate_dma_buf(buffer_size_bytes);
if (dma_buf_fd < 0)
{
if (dma_buf_fd == TEST_SKIPPED_ITSELF)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine for now, but it's overloading the int return type and making an assumption that TEST_SKIPPED_ITSELF is a negative value, and it's possible it won't be true in the future - see #2276.

Copy link
Contributor Author

@ahesham-arm ahesham-arm Feb 26, 2025

Choose a reason for hiding this comment

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

I agree. I think we should aim for using something like std::optional if we are sticking with standard C++ options, or an equivalent to Rust's std::Result.

Review feedback: failure to open the DMA device will typically be
caused by EACCES; i.e. insufficient filesystem permisisons. If that
happens, the test should be skipped, instead of failing.

This is done by returning the special value `TEST_SKIPPED_ITSELF` from
the `allocate_dma_buf` function.

Similarly, if the platform does not support DMA heaps, return
`TEST_SKIPPED_ITSELF` instead of failing.

Update the documentation and demo test case to reflect this change.
@bashbaug
Copy link
Contributor

Merging as discussed in the February 25th memory subgroup.

@bashbaug bashbaug merged commit 9ba6f06 into KhronosGroup:main Feb 26, 2025
9 checks passed
@ahesham-arm ahesham-arm deleted the dma_buf branch February 27, 2025 09:54
@hvdijk
Copy link
Contributor

hvdijk commented Feb 27, 2025

Hi, on systems where LINUX_VERSION_CODE < KERNEL_VERSION(5, 6, 0) , this breaks the build due to CMakeLists.txt adding -Werror and -Wunused-function where supported:

/builds/ComputeAorta/ci-github/build/OpenCL-CTS/test_common/harness/alloc.cpp:109:2: warning: #warning "Kernel version doesn't support DMA buffer heaps (at least v5.6.0 is required)." [-Wcpp]
  109 | #warning                                                                       \
      |  ^~~~~~~
/builds/ComputeAorta/ci-github/build/OpenCL-CTS/test_common/harness/alloc.cpp:53:30: error: 'dma_buf_heap_helper_t lookup_dma_heap(dma_buf_heap_type)' defined but not used [-Werror=unused-function]
   53 | static dma_buf_heap_helper_t lookup_dma_heap(dma_buf_heap_type heap_type)
      |                              ^~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

The warning seems accurate: lookup_dma_heap is unconditionally defined, but only used inside #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0). That function should probably only be defined when it will be used.

Could you take a look at this? Would you like me to?

@ahesham-arm
Copy link
Contributor Author

Hi, on systems where LINUX_VERSION_CODE < KERNEL_VERSION(5, 6, 0) , this breaks the build due to CMakeLists.txt adding -Werror and -Wunused-function where supported:

/builds/ComputeAorta/ci-github/build/OpenCL-CTS/test_common/harness/alloc.cpp:109:2: warning: #warning "Kernel version doesn't support DMA buffer heaps (at least v5.6.0 is required)." [-Wcpp]
  109 | #warning                                                                       \
      |  ^~~~~~~
/builds/ComputeAorta/ci-github/build/OpenCL-CTS/test_common/harness/alloc.cpp:53:30: error: 'dma_buf_heap_helper_t lookup_dma_heap(dma_buf_heap_type)' defined but not used [-Werror=unused-function]
   53 | static dma_buf_heap_helper_t lookup_dma_heap(dma_buf_heap_type heap_type)
      |                              ^~~~~~~~~~~~~~~
cc1plus: all warnings being treated as errors

The warning seems accurate: lookup_dma_heap is unconditionally defined, but only used inside #if LINUX_VERSION_CODE >= KERNEL_VERSION(5, 6, 0). That function should probably only be defined when it will be used.

Could you take a look at this? Would you like me to?

Thank you for reporting this, sorry for the break. I have a fix for this but unfortnatuely I cannot test this locall. Can you please try it and comment on the PR if it works for you? Thanks!

#2292

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants