ve2: added support to allocate cma bo from multiple cma regions #887
Conversation
Signed-off-by: bisingha <[email protected]>
Signed-off-by: Bikash Singha <[email protected]>
Signed-off-by: Bikash Singha <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR adds support for allocating buffer objects from multiple CMA (Contiguous Memory Allocator) regions in the ve2 driver, improving memory management flexibility on embedded platforms. The implementation enables user-specified CMA region selection with automatic fallback to other regions and backward compatibility with existing code.
Key changes:
- Implements CMA region selection through the flags parameter where bits [7:0] encode the region index (0-15)
- Adds automatic fallback logic: requested region → other available regions → system default CMA
- Initializes CMA regions from device tree memory-region properties with proper device lifecycle management
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shim_ve2/xdna_bo.cpp | Passes flags parameter to buffer object creation ioctl |
| src/include/uapi/drm_local/amdxdna_accel.h | Updates documentation to define flags field bit layout for CMA region selection |
| src/driver/amdxdna/ve2_of.c | Implements CMA region initialization and cleanup from device tree |
| src/driver/amdxdna/amdxdna_gem.c | Adds allocation logic with region selection and fallback mechanism |
| src/driver/amdxdna/amdxdna_drm.h | Defines structures and constants for CMA region management |
| src/driver/amdxdna/amdxdna_cma_buf.h | Updates function signature to accept device pointer and adds region index extraction |
| src/driver/amdxdna/amdxdna_cma_buf.c | Changes allocation functions to use device pointer instead of drm_device and implements index extraction |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/driver/amdxdna/amdxdna_cma_buf.c
Outdated
| * Valid indexes: 0-15 (validated at call site against MAX_MEM_REGIONS) | ||
| * Invalid indexes (>=16): handled as fallback to default CMA | ||
| */ | ||
| return ((flags) & 0xFF); |
There was a problem hiding this comment.
[nitpick] Unnecessary parentheses around flags make the code less readable. Simplify to return flags & 0xFF;
| return ((flags) & 0xFF); | |
| return flags & 0xFF; |
| XDNA_DBG(xdna, | ||
| "Failed to set name for cma region %d\n", i); | ||
| put_device(child_dev); | ||
| continue; |
There was a problem hiding this comment.
On dev_set_name() failure, the device is freed but ve2_cma_mem_region_remove() is not called to clean up any previously initialized regions before returning. This leaves partially initialized state. Consider calling ve2_cma_mem_region_remove(xdna) before continuing or ensure proper cleanup on any failure path.
| continue; | |
| ve2_cma_mem_region_remove(xdna); | |
| return ret; |
There was a problem hiding this comment.
The continue statement implements intentional fallback behavior. If one CMA region
fails to initialize, the driver tries remaining regions. The put_device() call
properly cleans up the failed region. Only if all regions fail the function
returns -EINVAL (line 170).
| ret = of_reserved_mem_device_init_by_idx(child_dev, | ||
| pdev->dev.of_node, i); | ||
| if (ret) { | ||
| XDNA_DBG(xdna, | ||
| "Failed to init reserved cma region %d\n", i); | ||
| put_device(child_dev); | ||
| continue; |
There was a problem hiding this comment.
Similar to the dev_set_name() failure case, when of_reserved_mem_device_init_by_idx() fails, the code continues without cleaning up previously initialized regions. If all regions fail to initialize after some succeed, those successful regions remain allocated until the final check at line 170. Consider more explicit cleanup or ensure the final check properly handles partial initialization.
There was a problem hiding this comment.
Partial initialization is the intended behavior. Successfully initialized regions are
tracked in xdna->cma_mem_regions[] and used for buffer allocation with automatic
fallback. Only if zero regions initialized the function returns -EINVAL. This
design provides resilience - the system works even if some CMA regions are unavailable.
All regions are properly cleaned up during device removal via ve2_cma_mem_region_remove().
Signed-off-by: Bikash Singha <[email protected]>
|
retest this please |
3 participants
Added Multiple CMA Region Support for Buffer Allocation
Enables buffer object allocation from multiple CMA memory regions with automatic fallback, improving memory management on embedded platforms. It ads better memory utilization across multiple reserved cma regions, robust allocation with automatic fallback and its backward compatible
Changes
Tested