Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] drm/xe: UAPI mismatch for VRAM-only BOs in gem_create_parse_numa_node() - [Low] drm/xe: missing numa_node kernel-doc in xe_bo_init_locked() --
commit 4dfdcc93fc9d778a45958834231a36c288d379fc Author: Himal Prasad Ghimiray <[email protected]> drm/xe/uapi: Wire up GEM_CREATE NUMA node placement handler This commit parses the DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE user extension prior to BO creation in xe_gem_create_ioctl() and passes the requested NUMA node ID down through the BO creation helpers to store on the xe_bo. > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c > index fc9bfb436b8e7..bbbc337372641 100644 > --- a/drivers/gpu/drm/xe/xe_bo.c > +++ b/drivers/gpu/drm/xe/xe_bo.c [ ... ] > @@ -2480,7 +2480,7 @@ struct xe_bo *xe_bo_init_locked(struct xe_device *xe, > struct xe_bo *bo, > struct ttm_lru_bulk_move *bulk, size_t size, > u16 cpu_caching, enum ttm_bo_type type, > u32 flags, struct dma_buf *dma_buf, > - struct drm_exec *exec) > + int numa_node, struct drm_exec *exec) > { [ ... ] > @@ -2966,7 +2967,8 @@ static void > xe_bo_debug_mixed_mode_cur_index_advance(struct xe_device *xe, > struct xe_bo *xe_bo_create_user(struct xe_device *xe, > struct xe_vm *vm, size_t size, > u16 cpu_caching, > - u32 flags, struct drm_exec *exec) > + u32 flags, int numa_node, > + struct drm_exec *exec) > { [Severity: Low] This isn't a bug, but both xe_bo_init_locked() and xe_bo_create_user() have kernel-doc comment blocks immediately above their definitions. Should @numa_node be added to both kernel-doc blocks to avoid W=1 kernel-doc warnings for undescribed function parameters? [ ... ] > @@ -3658,6 +3666,56 @@ static int gem_create_user_extensions(struct xe_device > *xe, struct xe_bo *bo, > return 0; > } > > +/* > + * Parse a host NUMA node request from the extension chain. The node must be > + * known before the BO is created, because the backing pages are allocated > + * during creation. Returns the requested node in @numa_node (NUMA_NO_NODE if > + * none or a negative value is given) or a negative errno on an invalid > request. > + */ > +static int gem_create_parse_numa_node(struct xe_device *xe, u64 extensions, > + u32 bo_flags, int *numa_node) > +{ > + struct drm_xe_user_extension base; > + struct drm_xe_ext_set_property ext; > + unsigned int i = 0; > + > + *numa_node = NUMA_NO_NODE; > + > + while (extensions && i++ < MAX_USER_EXTENSIONS) { > + u64 __user *address = u64_to_user_ptr(extensions); > + s64 nid; > + > + if (XE_IOCTL_DBG(xe, copy_from_user(&base, address, > sizeof(base)))) > + return -EFAULT; > + > + if (base.name != DRM_XE_GEM_CREATE_EXTENSION_SET_PROPERTY) > + goto next; > + > + if (XE_IOCTL_DBG(xe, copy_from_user(&ext, address, > sizeof(ext)))) > + return -EFAULT; > + > + if (ext.property != DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE) > + goto next; > + > + nid = (s64)ext.value; > + *numa_node = NUMA_NO_NODE; > + if (nid < 0) > + goto next; > + > + if (XE_IOCTL_DBG(xe, nid >= MAX_NUMNODES || !node_online(nid))) > + return -EINVAL; > + > + if (XE_IOCTL_DBG(xe, !(bo_flags & XE_BO_FLAG_SYSTEM))) > + return -EINVAL; > + > + *numa_node = nid; > +next: > + extensions = base.next_extension; > + } > + > + return 0; > +} [Severity: Medium] Does rejecting !(bo_flags & XE_BO_FLAG_SYSTEM) with -EINVAL in xe_gem_create_ioctl() -> gem_create_parse_numa_node() match the UAPI contract in include/uapi/drm/xe_drm.h? In struct drm_xe_gem_create, the comment for DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE states: /* * Host NUMA node (as a signed value) to allocate system-memory * backing pages from. Only honored for objects with a system-memory * placement; ignored for VRAM-only objects. Use a value < 0 to keep * the default device-local placement. */ #define DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE 1 However, gem_create_parse_numa_node() returns -EINVAL when nid >= 0 and !(bo_flags & XE_BO_FLAG_SYSTEM) instead of ignoring the property on VRAM-only objects. In addition, because nid < 0 jumps to next before reaching the !(bo_flags & XE_BO_FLAG_SYSTEM) check, a VRAM-only BO with nid < 0 is accepted while nid >= 0 fails with -EINVAL, and an extension chain on a VRAM-only BO that sets nid >= 0 followed by a later extension resetting nid = -1 will fail on the first extension. Should the UAPI comment and gem_create_parse_numa_node() be aligned on whether VRAM-only BOs ignore DRM_XE_GEM_CREATE_SET_PROPERTY_NUMA_NODE or reject it with -EINVAL? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7
