[AMD Official Use Only - AMD Internal Distribution Only]
> -----Original Message-----
> From: Christian König <[email protected]>
> Sent: Tuesday, December 2, 2025 8:43 PM
> To: [email protected]; SHANMUGAM, SRINIVASAN
> <[email protected]>; Liu, Leo <[email protected]>; Dong,
> Ruijing <[email protected]>; [email protected]
> Subject: [RFC PATCH 2/3] drm/amdgpu: add
> AMDGPU_GEM_OP_OPEN_GLOBAL
>
> Instead of abusing the create IOCTL to open global BO add a new
> AMDGPU_GEM_OP_OPEN_GLOBAL functionality.
>
> The new AMDGPU_GEM_OP_OPEN_GLOBAL functionality expects an enum
> which tells it which global BO to open and copies the information about the
> BO to
> userspace similar to the AMDGPU_GEM_OP_GET_GEM_CREATE_INFO
> operation.
>
> The advantage is that we don't start overloading the create IOCTL with tons of
> special cases and opening the global BOs doesn't requires knowing the exact
> size
> and parameters of it in userspace any more.
>
> Heavily WIP and only compile tested.
>
> Signed-off-by: Christian König <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 26 ++++++++++++++++++++-----
> include/uapi/drm/amdgpu_drm.h | 5 ++++-
> 2 files changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 9b81a6677f90..9e9b94dcb699 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -968,22 +968,34 @@ int amdgpu_gem_va_ioctl(struct drm_device *dev, void
> *data, int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
> struct drm_file *filp)
> {
> + struct amdgpu_fpriv *fpriv = filp->driver_priv;
> struct drm_amdgpu_gem_op *args = data;
> struct drm_gem_object *gobj;
> struct amdgpu_vm_bo_base *base;
> struct amdgpu_bo *robj;
> struct drm_exec exec;
> - struct amdgpu_fpriv *fpriv = filp->driver_priv;
> int r;
>
> if (args->padding)
> return -EINVAL;
>
> - gobj = drm_gem_object_lookup(filp, args->handle);
> - if (!gobj)
> - return -ENOENT;
> + if (args->op == AMDGPU_GEM_OP_OPEN_GLOBAL) {
> + switch (args->handle) {
> + case AMDGPU_GEM_GLOBAL_MMIO_REMAP:
> + robj = drm_to_adev(dev)->rmmio_remap.bo;
> + break;
> + default:
> + return -EINVAL;
> + }
> + gobj = &robj->tbo.base;
> + drm_gem_object_get(gobj);
> + } else {
> + gobj = drm_gem_object_lookup(filp, args->handle);
> + if (!gobj)
> + return -ENOENT;
>
> - robj = gem_to_amdgpu_bo(gobj);
> + robj = gem_to_amdgpu_bo(gobj);
> + }
>
For AMDGPU_GEM_OP_OPEN_GLOBAL, I think it would be nice to centralise the ID→BO
mapping into a small helper, e.g. amdgpu_get_global_bo(adev, id), and have it
return NULL if the global BO is not available on this device. Then the
OPEN_GLOBAL path can simply do:
robj = amdgpu_get_global_bo(drm_to_adev(dev), args->handle);
if (!robj)
return -EOPNOTSUPP;
That avoids the potential NULL deref on adev->rmmio_remap.bo when the remap
page isn’t present and also gives us a central place to extend with additional
AMDGPU_GEM_GLOBAL_* IDs in future.
Today, OPEN_GLOBAL switches on args->handle inside amdgpu_gem_op_ioctl():
switch (args->handle) {
case AMDGPU_GEM_GLOBAL_MMIO_REMAP:
robj = drm_to_adev(dev)->rmmio_remap.bo;
break;
default:
return -EINVAL;
}
Like adding a small helper
Add a small helper:
static struct amdgpu_bo *
amdgpu_get_global_bo(struct amdgpu_device *adev, u32 id)
{
switch (id) {
case AMDGPU_GEM_GLOBAL_MMIO_REMAP:
return adev->rmmio_remap.bo;
// future global BOs can be added here
default:
return NULL;
}
}
So, that only one function knows which global ID maps to which BO.
Easy to add new globals in future (DOORBELL, KFD, etc.).
UAPI and internal mapping stay in sync.
> drm_exec_init(&exec, DRM_EXEC_INTERRUPTIBLE_WAIT |
> DRM_EXEC_IGNORE_DUPLICATES, 0);
> @@ -1002,6 +1014,7 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void
> *data,
> }
>
> switch (args->op) {
> + case AMDGPU_GEM_OP_OPEN_GLOBAL:
for AMDGPU_GEM_OP_OPEN_GLOBAL we could treat args->value == 0 as “caller only
wants a handle, not the create_info”. In that case we can skip the
copy_to_user() entirely and just create the handle. Right now the code assumes
a non-NULL pointer for both GET_GEM_CREATE_INFO and OPEN_GLOBAL; relaxing that
for the latter would make the API a bit easier to use. (Optional not blocker)
case AMDGPU_GEM_OP_GET_GEM_CREATE_INFO: {
struct drm_amdgpu_gem_create_in info;
void __user *out = NULL;
r = amdgpu_gem_get_create_info(robj, &info);
if (r)
break;
/* For OPEN_GLOBAL, allow value == 0 when caller only wants the handle */
if (args->op == AMDGPU_GEM_OP_OPEN_GLOBAL && !args->value)
break;
out = u64_to_user_ptr(args->value);
if (copy_to_user(out, &info, sizeof(info)))
r = -EFAULT;
break;
}
> case AMDGPU_GEM_OP_GET_GEM_CREATE_INFO: {
> struct drm_amdgpu_gem_create_in info;
> void __user *out = u64_to_user_ptr(args->value); @@ -1096,6
> +1109,9 @@ int amdgpu_gem_op_ioctl(struct drm_device *dev, void *data,
> r = -EINVAL;
> }
>
> + if (!r && args->op == AMDGPU_GEM_OP_OPEN_GLOBAL)
> + r = drm_gem_handle_create(filp, gobj, &args->handle);
> +
> drm_gem_object_put(gobj);
> return r;
> out_exec:
> diff --git a/include/uapi/drm/amdgpu_drm.h b/include/uapi/drm/amdgpu_drm.h
> index
> c1336ed4ff75..6927c864a6d1 100644
> --- a/include/uapi/drm/amdgpu_drm.h
> +++ b/include/uapi/drm/amdgpu_drm.h
> @@ -807,6 +807,9 @@ union drm_amdgpu_wait_fences {
> #define AMDGPU_GEM_OP_GET_GEM_CREATE_INFO 0
> #define AMDGPU_GEM_OP_SET_PLACEMENT 1
> #define AMDGPU_GEM_OP_GET_MAPPING_INFO 2
> +#define AMDGPU_GEM_OP_OPEN_GLOBAL 3
> +
> +#define AMDGPU_GEM_GLOBAL_MMIO_REMAP 0
>
> struct drm_amdgpu_gem_vm_entry {
> /* Start of mapping (in bytes) */
> @@ -824,7 +827,7 @@ struct drm_amdgpu_gem_vm_entry {
>
> /* Sets or returns a value associated with a buffer. */ struct
> drm_amdgpu_gem_op
> {
> - /** GEM object handle */
> + /** GEM object handle or AMDGPU_GEM_GLOBAL_* */
For the UAPI docs, maybe can we add a short note that for
AMDGPU_GEM_OP_OPEN_GLOBAL the handle field is interpreted as an
AMDGPU_GEM_GLOBAL_* ID on input and overwritten with a per-file GEM handle on
success. That makes the dual use of handle a bit clearer for userspace.
> __u32 handle;
> /** AMDGPU_GEM_OP_* */
> __u32 op;
> --
> 2.43.0
