* [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
@ 2025-08-20 15:23 Himal Prasad Ghimiray
2025-08-20 16:07 ` Boris Brezillon
` (3 more replies)
0 siblings, 4 replies; 14+ messages in thread
From: Himal Prasad Ghimiray @ 2025-08-20 15:23 UTC (permalink / raw)
To: intel-xe
Cc: Himal Prasad Ghimiray, Boris Brezillon, Danilo Krummrich,
Matt Coster, Rob Clark, Matthew Brost, dri-devel
Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
corresponding documentation. No functional changes introduced.
Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
Suggested-by: Danilo Krummrich <dakr@kernel.org>
Cc: Danilo Krummrich <dakr@kernel.org>
Cc: Matt Coster <matt.coster@imgtec.com>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: Rob Clark <robin.clark@oss.qualcomm.com>
Cc: Matthew Brost <matthew.brost@intel.com>
Cc: <dri-devel@lists.freedesktop.org>
Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
---
drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
drivers/gpu/drm/xe/xe_vm.c | 8 +++---
include/drm/drm_gpuvm.h | 4 +--
7 files changed, 44 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
index 39f934a91a7b..e9aaf9b287e7 100644
--- a/drivers/gpu/drm/drm_gpuvm.c
+++ b/drivers/gpu/drm/drm_gpuvm.c
@@ -552,11 +552,11 @@
* struct drm_gem_object *obj, u64 offset)
* {
* struct drm_gpuvm_map_req map_req = {
- * .map.va.addr = addr,
- * .map.va.range = range,
- * .map.gem.obj = obj,
- * .map.gem.offset = offset,
- * };
+ * .op.va.addr = addr,
+ * .op.va.range = range,
+ * .op.gem.obj = obj,
+ * .op.gem.offset = offset,
+ * };
* struct drm_gpuva_ops *ops;
* struct drm_gpuva_op *op
* struct drm_gpuvm_bo *vm_bo;
@@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
return 0;
op.op = DRM_GPUVA_OP_MAP;
- op.map.va.addr = req->map.va.addr;
- op.map.va.range = req->map.va.range;
- op.map.gem.obj = req->map.gem.obj;
- op.map.gem.offset = req->map.gem.offset;
+ op.map.va.addr = req->op.va.addr;
+ op.map.va.range = req->op.va.range;
+ op.map.gem.obj = req->op.gem.obj;
+ op.map.gem.offset = req->op.gem.offset;
return fn->sm_step_map(&op, priv);
}
@@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
const struct drm_gpuvm_map_req *req,
bool madvise)
{
- struct drm_gem_object *req_obj = req->map.gem.obj;
+ struct drm_gem_object *req_obj = req->op.gem.obj;
const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
struct drm_gpuva *va, *next;
- u64 req_offset = req->map.gem.offset;
- u64 req_range = req->map.va.range;
- u64 req_addr = req->map.va.addr;
+ u64 req_offset = req->op.gem.offset;
+ u64 req_range = req->op.va.range;
+ u64 req_addr = req->op.va.addr;
u64 req_end = req_addr + req_range;
int ret;
@@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
if (madvise) {
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = req_addr,
- .map.va.range = end - req_addr,
+ .op.va.addr = req_addr,
+ .op.va.range = end - req_addr,
};
ret = op_map_cb(ops, priv, &map_req);
@@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
if (madvise) {
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = addr,
- .map.va.range = req_end - addr,
+ .op.va.addr = addr,
+ .op.va.range = req_end - addr,
};
return op_map_cb(ops, priv, &map_req);
@@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
struct drm_exec *exec, unsigned int num_fences,
struct drm_gpuvm_map_req *req)
{
- struct drm_gem_object *req_obj = req->map.gem.obj;
+ struct drm_gem_object *req_obj = req->op.gem.obj;
if (req_obj) {
int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
index 3d97990170bf..983165eb3e6a 100644
--- a/drivers/gpu/drm/imagination/pvr_vm.c
+++ b/drivers/gpu/drm/imagination/pvr_vm.c
@@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
switch (bind_op->type) {
case PVR_VM_BIND_TYPE_MAP: {
const struct drm_gpuvm_map_req map_req = {
- .map.va.addr = bind_op->device_addr,
- .map.va.range = bind_op->size,
- .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
- .map.gem.offset = bind_op->offset,
+ .op.va.addr = bind_op->device_addr,
+ .op.va.range = bind_op->size,
+ .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
+ .op.gem.offset = bind_op->offset,
};
return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
index 210604181c05..9b5d003bc5a2 100644
--- a/drivers/gpu/drm/msm/msm_gem_vma.c
+++ b/drivers/gpu/drm/msm/msm_gem_vma.c
@@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
case MSM_VM_BIND_OP_MAP:
case MSM_VM_BIND_OP_MAP_NULL: {
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = op->iova,
- .map.va.range = op->range,
- .map.gem.obj = op->obj,
- .map.gem.offset = op->obj_offset,
+ .op.va.addr = op->iova,
+ .op.va.range = op->range,
+ .op.gem.obj = op->obj,
+ .op.gem.offset = op->obj_offset,
};
ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
@@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
fallthrough;
case MSM_VM_BIND_OP_MAP_NULL: {
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = op->iova,
- .map.va.range = op->range,
- .map.gem.obj = op->obj,
- .map.gem.offset = op->obj_offset,
+ .op.va.addr = op->iova,
+ .op.va.range = op->range,
+ .op.gem.obj = op->obj,
+ .op.gem.offset = op->obj_offset,
};
ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
index d94a85509176..314121a857e7 100644
--- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
+++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
@@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
case OP_MAP: {
struct nouveau_uvma_region *reg;
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = op->va.addr,
- .map.va.range = op->va.range,
- .map.gem.obj = op->gem.obj,
- .map.gem.offset = op->gem.offset,
+ .op.va.addr = op->va.addr,
+ .op.va.range = op->va.range,
+ .op.gem.obj = op->gem.obj,
+ .op.gem.offset = op->gem.offset,
};
reg = nouveau_uvma_region_find_first(uvmm,
diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
index 2003b91a8409..3799e2c6ea59 100644
--- a/drivers/gpu/drm/panthor/panthor_mmu.c
+++ b/drivers/gpu/drm/panthor/panthor_mmu.c
@@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
switch (op_type) {
case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
const struct drm_gpuvm_map_req map_req = {
- .map.va.addr = op->va.addr,
- .map.va.range = op->va.range,
- .map.gem.obj = op->map.vm_bo->obj,
- .map.gem.offset = op->map.bo_offset,
+ .op.va.addr = op->va.addr,
+ .op.va.range = op->va.range,
+ .op.gem.obj = op->map.vm_bo->obj,
+ .op.gem.offset = op->map.bo_offset,
};
if (vm->unusable) {
diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index f35d69c0b4c6..66b54b152446 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
case DRM_XE_VM_BIND_OP_MAP:
case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
struct drm_gpuvm_map_req map_req = {
- .map.va.addr = addr,
- .map.va.range = range,
- .map.gem.obj = obj,
- .map.gem.offset = bo_offset_or_userptr,
+ .op.va.addr = addr,
+ .op.va.range = range,
+ .op.gem.obj = obj,
+ .op.gem.offset = bo_offset_or_userptr,
};
ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
index 4a22b9d848f7..751c96a817ed 100644
--- a/include/drm/drm_gpuvm.h
+++ b/include/drm/drm_gpuvm.h
@@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
*/
struct drm_gpuvm_map_req {
/**
- * @op_map: struct drm_gpuva_op_map
+ * @op: struct drm_gpuva_op_map
*/
- struct drm_gpuva_op_map map;
+ struct drm_gpuva_op_map op;
};
struct drm_gpuva_ops *
--
2.34.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-20 15:23 [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req Himal Prasad Ghimiray
@ 2025-08-20 16:07 ` Boris Brezillon
2025-08-21 11:01 ` Boris Brezillon
2025-08-20 16:38 ` Danilo Krummrich
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2025-08-20 16:07 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: intel-xe, Boris Brezillon, Danilo Krummrich, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Wed, 20 Aug 2025 20:53:35 +0530
Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
> Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> corresponding documentation. No functional changes introduced.
>
> Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matt Coster <matt.coster@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
> drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
> drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
> drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
> drivers/gpu/drm/xe/xe_vm.c | 8 +++---
> include/drm/drm_gpuvm.h | 4 +--
> 7 files changed, 44 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 39f934a91a7b..e9aaf9b287e7 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -552,11 +552,11 @@
> * struct drm_gem_object *obj, u64 offset)
> * {
> * struct drm_gpuvm_map_req map_req = {
> - * .map.va.addr = addr,
> - * .map.va.range = range,
> - * .map.gem.obj = obj,
> - * .map.gem.offset = offset,
> - * };
> + * .op.va.addr = addr,
> + * .op.va.range = range,
> + * .op.gem.obj = obj,
> + * .op.gem.offset = offset,
> + * };
> * struct drm_gpuva_ops *ops;
> * struct drm_gpuva_op *op
> * struct drm_gpuvm_bo *vm_bo;
> @@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> return 0;
>
> op.op = DRM_GPUVA_OP_MAP;
> - op.map.va.addr = req->map.va.addr;
> - op.map.va.range = req->map.va.range;
> - op.map.gem.obj = req->map.gem.obj;
> - op.map.gem.offset = req->map.gem.offset;
> + op.map.va.addr = req->op.va.addr;
> + op.map.va.range = req->op.va.range;
> + op.map.gem.obj = req->op.gem.obj;
> + op.map.gem.offset = req->op.gem.offset;
>
> return fn->sm_step_map(&op, priv);
> }
> @@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> const struct drm_gpuvm_map_req *req,
> bool madvise)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
> const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> struct drm_gpuva *va, *next;
> - u64 req_offset = req->map.gem.offset;
> - u64 req_range = req->map.va.range;
> - u64 req_addr = req->map.va.addr;
> + u64 req_offset = req->op.gem.offset;
> + u64 req_range = req->op.va.range;
> + u64 req_addr = req->op.va.addr;
> u64 req_end = req_addr + req_range;
> int ret;
>
> @@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = req_addr,
> - .map.va.range = end - req_addr,
> + .op.va.addr = req_addr,
> + .op.va.range = end - req_addr,
> };
>
> ret = op_map_cb(ops, priv, &map_req);
> @@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = req_end - addr,
> + .op.va.addr = addr,
> + .op.va.range = req_end - addr,
> };
>
> return op_map_cb(ops, priv, &map_req);
> @@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
> struct drm_exec *exec, unsigned int num_fences,
> struct drm_gpuvm_map_req *req)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
>
> if (req_obj) {
> int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 3d97990170bf..983165eb3e6a 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
> switch (bind_op->type) {
> case PVR_VM_BIND_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = bind_op->device_addr,
> - .map.va.range = bind_op->size,
> - .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> - .map.gem.offset = bind_op->offset,
> + .op.va.addr = bind_op->device_addr,
> + .op.va.range = bind_op->size,
> + .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> + .op.gem.offset = bind_op->offset,
> };
>
> return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 210604181c05..9b5d003bc5a2 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
> case MSM_VM_BIND_OP_MAP:
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
> @@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
> fallthrough;
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index d94a85509176..314121a857e7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> case OP_MAP: {
> struct nouveau_uvma_region *reg;
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->gem.obj,
> - .map.gem.offset = op->gem.offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->gem.obj,
> + .op.gem.offset = op->gem.offset,
> };
>
> reg = nouveau_uvma_region_find_first(uvmm,
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 2003b91a8409..3799e2c6ea59 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
> switch (op_type) {
> case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->map.vm_bo->obj,
> - .map.gem.offset = op->map.bo_offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->map.vm_bo->obj,
> + .op.gem.offset = op->map.bo_offset,
> };
>
> if (vm->unusable) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f35d69c0b4c6..66b54b152446 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> case DRM_XE_VM_BIND_OP_MAP:
> case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = range,
> - .map.gem.obj = obj,
> - .map.gem.offset = bo_offset_or_userptr,
> + .op.va.addr = addr,
> + .op.va.range = range,
> + .op.gem.obj = obj,
> + .op.gem.offset = bo_offset_or_userptr,
> };
>
> ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 4a22b9d848f7..751c96a817ed 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
> */
> struct drm_gpuvm_map_req {
> /**
> - * @op_map: struct drm_gpuva_op_map
> + * @op: struct drm_gpuva_op_map
> */
> - struct drm_gpuva_op_map map;
> + struct drm_gpuva_op_map op;
> };
>
> struct drm_gpuva_ops *
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-20 15:23 [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req Himal Prasad Ghimiray
2025-08-20 16:07 ` Boris Brezillon
@ 2025-08-20 16:38 ` Danilo Krummrich
2025-08-20 16:53 ` Rob Clark
2025-08-20 16:56 ` Matt Coster
3 siblings, 0 replies; 14+ messages in thread
From: Danilo Krummrich @ 2025-08-20 16:38 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: intel-xe, Boris Brezillon, Matt Coster, Rob Clark, Matthew Brost,
dri-devel
On Wed Aug 20, 2025 at 5:23 PM CEST, Himal Prasad Ghimiray wrote:
> Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> corresponding documentation. No functional changes introduced.
>
> Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matt Coster <matt.coster@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Acked-by: Danilo Krummrich <dakr@kernel.org>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-20 15:23 [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req Himal Prasad Ghimiray
2025-08-20 16:07 ` Boris Brezillon
2025-08-20 16:38 ` Danilo Krummrich
@ 2025-08-20 16:53 ` Rob Clark
2025-08-20 16:56 ` Matt Coster
3 siblings, 0 replies; 14+ messages in thread
From: Rob Clark @ 2025-08-20 16:53 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: intel-xe, Boris Brezillon, Danilo Krummrich, Matt Coster,
Matthew Brost, dri-devel
On Wed, Aug 20, 2025 at 7:56 AM Himal Prasad Ghimiray
<himal.prasad.ghimiray@intel.com> wrote:
>
> Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> corresponding documentation. No functional changes introduced.
>
> Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matt Coster <matt.coster@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Acked-by: Rob Clark <robin.clark@oss.qualcomm.com>
> ---
> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
> drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
> drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
> drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
> drivers/gpu/drm/xe/xe_vm.c | 8 +++---
> include/drm/drm_gpuvm.h | 4 +--
> 7 files changed, 44 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 39f934a91a7b..e9aaf9b287e7 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -552,11 +552,11 @@
> * struct drm_gem_object *obj, u64 offset)
> * {
> * struct drm_gpuvm_map_req map_req = {
> - * .map.va.addr = addr,
> - * .map.va.range = range,
> - * .map.gem.obj = obj,
> - * .map.gem.offset = offset,
> - * };
> + * .op.va.addr = addr,
> + * .op.va.range = range,
> + * .op.gem.obj = obj,
> + * .op.gem.offset = offset,
> + * };
> * struct drm_gpuva_ops *ops;
> * struct drm_gpuva_op *op
> * struct drm_gpuvm_bo *vm_bo;
> @@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> return 0;
>
> op.op = DRM_GPUVA_OP_MAP;
> - op.map.va.addr = req->map.va.addr;
> - op.map.va.range = req->map.va.range;
> - op.map.gem.obj = req->map.gem.obj;
> - op.map.gem.offset = req->map.gem.offset;
> + op.map.va.addr = req->op.va.addr;
> + op.map.va.range = req->op.va.range;
> + op.map.gem.obj = req->op.gem.obj;
> + op.map.gem.offset = req->op.gem.offset;
>
> return fn->sm_step_map(&op, priv);
> }
> @@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> const struct drm_gpuvm_map_req *req,
> bool madvise)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
> const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> struct drm_gpuva *va, *next;
> - u64 req_offset = req->map.gem.offset;
> - u64 req_range = req->map.va.range;
> - u64 req_addr = req->map.va.addr;
> + u64 req_offset = req->op.gem.offset;
> + u64 req_range = req->op.va.range;
> + u64 req_addr = req->op.va.addr;
> u64 req_end = req_addr + req_range;
> int ret;
>
> @@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = req_addr,
> - .map.va.range = end - req_addr,
> + .op.va.addr = req_addr,
> + .op.va.range = end - req_addr,
> };
>
> ret = op_map_cb(ops, priv, &map_req);
> @@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = req_end - addr,
> + .op.va.addr = addr,
> + .op.va.range = req_end - addr,
> };
>
> return op_map_cb(ops, priv, &map_req);
> @@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
> struct drm_exec *exec, unsigned int num_fences,
> struct drm_gpuvm_map_req *req)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
>
> if (req_obj) {
> int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 3d97990170bf..983165eb3e6a 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
> switch (bind_op->type) {
> case PVR_VM_BIND_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = bind_op->device_addr,
> - .map.va.range = bind_op->size,
> - .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> - .map.gem.offset = bind_op->offset,
> + .op.va.addr = bind_op->device_addr,
> + .op.va.range = bind_op->size,
> + .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> + .op.gem.offset = bind_op->offset,
> };
>
> return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 210604181c05..9b5d003bc5a2 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
> case MSM_VM_BIND_OP_MAP:
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
> @@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
> fallthrough;
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index d94a85509176..314121a857e7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> case OP_MAP: {
> struct nouveau_uvma_region *reg;
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->gem.obj,
> - .map.gem.offset = op->gem.offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->gem.obj,
> + .op.gem.offset = op->gem.offset,
> };
>
> reg = nouveau_uvma_region_find_first(uvmm,
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 2003b91a8409..3799e2c6ea59 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
> switch (op_type) {
> case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->map.vm_bo->obj,
> - .map.gem.offset = op->map.bo_offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->map.vm_bo->obj,
> + .op.gem.offset = op->map.bo_offset,
> };
>
> if (vm->unusable) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f35d69c0b4c6..66b54b152446 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> case DRM_XE_VM_BIND_OP_MAP:
> case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = range,
> - .map.gem.obj = obj,
> - .map.gem.offset = bo_offset_or_userptr,
> + .op.va.addr = addr,
> + .op.va.range = range,
> + .op.gem.obj = obj,
> + .op.gem.offset = bo_offset_or_userptr,
> };
>
> ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 4a22b9d848f7..751c96a817ed 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
> */
> struct drm_gpuvm_map_req {
> /**
> - * @op_map: struct drm_gpuva_op_map
> + * @op: struct drm_gpuva_op_map
> */
> - struct drm_gpuva_op_map map;
> + struct drm_gpuva_op_map op;
> };
>
> struct drm_gpuva_ops *
> --
> 2.34.1
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-20 15:23 [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req Himal Prasad Ghimiray
` (2 preceding siblings ...)
2025-08-20 16:53 ` Rob Clark
@ 2025-08-20 16:56 ` Matt Coster
3 siblings, 0 replies; 14+ messages in thread
From: Matt Coster @ 2025-08-20 16:56 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: Boris Brezillon, Danilo Krummrich, Rob Clark, Matthew Brost,
dri-devel@lists.freedesktop.org, intel-xe@lists.freedesktop.org
[-- Attachment #1.1: Type: text/plain, Size: 9453 bytes --]
On 20/08/2025 16:23, Himal Prasad Ghimiray wrote:
> Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> corresponding documentation. No functional changes introduced.
>
> Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> Suggested-by: Danilo Krummrich <dakr@kernel.org>
> Cc: Danilo Krummrich <dakr@kernel.org>
> Cc: Matt Coster <matt.coster@imgtec.com>
> Cc: Boris Brezillon <bbrezillon@kernel.org>
> Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: <dri-devel@lists.freedesktop.org>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
Acked-by: Matt Coster <matt.coster@imgtec.com>
Cheers,
Matt
> ---
> drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
> drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
> drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
> drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
> drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
> drivers/gpu/drm/xe/xe_vm.c | 8 +++---
> include/drm/drm_gpuvm.h | 4 +--
> 7 files changed, 44 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 39f934a91a7b..e9aaf9b287e7 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -552,11 +552,11 @@
> * struct drm_gem_object *obj, u64 offset)
> * {
> * struct drm_gpuvm_map_req map_req = {
> - * .map.va.addr = addr,
> - * .map.va.range = range,
> - * .map.gem.obj = obj,
> - * .map.gem.offset = offset,
> - * };
> + * .op.va.addr = addr,
> + * .op.va.range = range,
> + * .op.gem.obj = obj,
> + * .op.gem.offset = offset,
> + * };
> * struct drm_gpuva_ops *ops;
> * struct drm_gpuva_op *op
> * struct drm_gpuvm_bo *vm_bo;
> @@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> return 0;
>
> op.op = DRM_GPUVA_OP_MAP;
> - op.map.va.addr = req->map.va.addr;
> - op.map.va.range = req->map.va.range;
> - op.map.gem.obj = req->map.gem.obj;
> - op.map.gem.offset = req->map.gem.offset;
> + op.map.va.addr = req->op.va.addr;
> + op.map.va.range = req->op.va.range;
> + op.map.gem.obj = req->op.gem.obj;
> + op.map.gem.offset = req->op.gem.offset;
>
> return fn->sm_step_map(&op, priv);
> }
> @@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> const struct drm_gpuvm_map_req *req,
> bool madvise)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
> const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> struct drm_gpuva *va, *next;
> - u64 req_offset = req->map.gem.offset;
> - u64 req_range = req->map.va.range;
> - u64 req_addr = req->map.va.addr;
> + u64 req_offset = req->op.gem.offset;
> + u64 req_range = req->op.va.range;
> + u64 req_addr = req->op.va.addr;
> u64 req_end = req_addr + req_range;
> int ret;
>
> @@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = req_addr,
> - .map.va.range = end - req_addr,
> + .op.va.addr = req_addr,
> + .op.va.range = end - req_addr,
> };
>
> ret = op_map_cb(ops, priv, &map_req);
> @@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>
> if (madvise) {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = req_end - addr,
> + .op.va.addr = addr,
> + .op.va.range = req_end - addr,
> };
>
> return op_map_cb(ops, priv, &map_req);
> @@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
> struct drm_exec *exec, unsigned int num_fences,
> struct drm_gpuvm_map_req *req)
> {
> - struct drm_gem_object *req_obj = req->map.gem.obj;
> + struct drm_gem_object *req_obj = req->op.gem.obj;
>
> if (req_obj) {
> int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> index 3d97990170bf..983165eb3e6a 100644
> --- a/drivers/gpu/drm/imagination/pvr_vm.c
> +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> @@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
> switch (bind_op->type) {
> case PVR_VM_BIND_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = bind_op->device_addr,
> - .map.va.range = bind_op->size,
> - .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> - .map.gem.offset = bind_op->offset,
> + .op.va.addr = bind_op->device_addr,
> + .op.va.range = bind_op->size,
> + .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> + .op.gem.offset = bind_op->offset,
> };
>
> return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> index 210604181c05..9b5d003bc5a2 100644
> --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> @@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
> case MSM_VM_BIND_OP_MAP:
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
> @@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
> fallthrough;
> case MSM_VM_BIND_OP_MAP_NULL: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->iova,
> - .map.va.range = op->range,
> - .map.gem.obj = op->obj,
> - .map.gem.offset = op->obj_offset,
> + .op.va.addr = op->iova,
> + .op.va.range = op->range,
> + .op.gem.obj = op->obj,
> + .op.gem.offset = op->obj_offset,
> };
>
> ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
> diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> index d94a85509176..314121a857e7 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> @@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> case OP_MAP: {
> struct nouveau_uvma_region *reg;
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->gem.obj,
> - .map.gem.offset = op->gem.offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->gem.obj,
> + .op.gem.offset = op->gem.offset,
> };
>
> reg = nouveau_uvma_region_find_first(uvmm,
> diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> index 2003b91a8409..3799e2c6ea59 100644
> --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> @@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
> switch (op_type) {
> case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> const struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = op->va.addr,
> - .map.va.range = op->va.range,
> - .map.gem.obj = op->map.vm_bo->obj,
> - .map.gem.offset = op->map.bo_offset,
> + .op.va.addr = op->va.addr,
> + .op.va.range = op->va.range,
> + .op.gem.obj = op->map.vm_bo->obj,
> + .op.gem.offset = op->map.bo_offset,
> };
>
> if (vm->unusable) {
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f35d69c0b4c6..66b54b152446 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> case DRM_XE_VM_BIND_OP_MAP:
> case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> struct drm_gpuvm_map_req map_req = {
> - .map.va.addr = addr,
> - .map.va.range = range,
> - .map.gem.obj = obj,
> - .map.gem.offset = bo_offset_or_userptr,
> + .op.va.addr = addr,
> + .op.va.range = range,
> + .op.gem.obj = obj,
> + .op.gem.offset = bo_offset_or_userptr,
> };
>
> ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
> diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> index 4a22b9d848f7..751c96a817ed 100644
> --- a/include/drm/drm_gpuvm.h
> +++ b/include/drm/drm_gpuvm.h
> @@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
> */
> struct drm_gpuvm_map_req {
> /**
> - * @op_map: struct drm_gpuva_op_map
> + * @op: struct drm_gpuva_op_map
> */
> - struct drm_gpuva_op_map map;
> + struct drm_gpuva_op_map op;
> };
>
> struct drm_gpuva_ops *
--
Matt Coster
E: matt.coster@imgtec.com
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-20 16:07 ` Boris Brezillon
@ 2025-08-21 11:01 ` Boris Brezillon
2025-08-21 11:25 ` Boris Brezillon
0 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2025-08-21 11:01 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: intel-xe, Boris Brezillon, Danilo Krummrich, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Wed, 20 Aug 2025 18:07:42 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Wed, 20 Aug 2025 20:53:35 +0530
> Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
>
> > Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> > corresponding documentation. No functional changes introduced.
> >
> > Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> > Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> > Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > Cc: Danilo Krummrich <dakr@kernel.org>
> > Cc: Matt Coster <matt.coster@imgtec.com>
> > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> > Cc: Matthew Brost <matthew.brost@intel.com>
> > Cc: <dri-devel@lists.freedesktop.org>
> > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
>
> Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
>
> > ---
> > drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
> > drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
> > drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
> > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
> > drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
> > drivers/gpu/drm/xe/xe_vm.c | 8 +++---
> > include/drm/drm_gpuvm.h | 4 +--
> > 7 files changed, 44 insertions(+), 44 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > index 39f934a91a7b..e9aaf9b287e7 100644
> > --- a/drivers/gpu/drm/drm_gpuvm.c
> > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > @@ -552,11 +552,11 @@
> > * struct drm_gem_object *obj, u64 offset)
> > * {
> > * struct drm_gpuvm_map_req map_req = {
> > - * .map.va.addr = addr,
> > - * .map.va.range = range,
> > - * .map.gem.obj = obj,
> > - * .map.gem.offset = offset,
> > - * };
> > + * .op.va.addr = addr,
> > + * .op.va.range = range,
> > + * .op.gem.obj = obj,
> > + * .op.gem.offset = offset,
> > + * };
> > * struct drm_gpuva_ops *ops;
> > * struct drm_gpuva_op *op
> > * struct drm_gpuvm_bo *vm_bo;
> > @@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> > return 0;
> >
> > op.op = DRM_GPUVA_OP_MAP;
> > - op.map.va.addr = req->map.va.addr;
> > - op.map.va.range = req->map.va.range;
> > - op.map.gem.obj = req->map.gem.obj;
> > - op.map.gem.offset = req->map.gem.offset;
> > + op.map.va.addr = req->op.va.addr;
> > + op.map.va.range = req->op.va.range;
> > + op.map.gem.obj = req->op.gem.obj;
> > + op.map.gem.offset = req->op.gem.offset;
> >
> > return fn->sm_step_map(&op, priv);
> > }
> > @@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > const struct drm_gpuvm_map_req *req,
> > bool madvise)
> > {
> > - struct drm_gem_object *req_obj = req->map.gem.obj;
> > + struct drm_gem_object *req_obj = req->op.gem.obj;
> > const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> > struct drm_gpuva *va, *next;
> > - u64 req_offset = req->map.gem.offset;
> > - u64 req_range = req->map.va.range;
> > - u64 req_addr = req->map.va.addr;
> > + u64 req_offset = req->op.gem.offset;
> > + u64 req_range = req->op.va.range;
> > + u64 req_addr = req->op.va.addr;
> > u64 req_end = req_addr + req_range;
> > int ret;
> >
> > @@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> >
> > if (madvise) {
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = req_addr,
> > - .map.va.range = end - req_addr,
> > + .op.va.addr = req_addr,
> > + .op.va.range = end - req_addr,
> > };
> >
> > ret = op_map_cb(ops, priv, &map_req);
> > @@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> >
> > if (madvise) {
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = addr,
> > - .map.va.range = req_end - addr,
> > + .op.va.addr = addr,
> > + .op.va.range = req_end - addr,
> > };
> >
> > return op_map_cb(ops, priv, &map_req);
> > @@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
> > struct drm_exec *exec, unsigned int num_fences,
> > struct drm_gpuvm_map_req *req)
> > {
> > - struct drm_gem_object *req_obj = req->map.gem.obj;
> > + struct drm_gem_object *req_obj = req->op.gem.obj;
> >
> > if (req_obj) {
> > int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> > index 3d97990170bf..983165eb3e6a 100644
> > --- a/drivers/gpu/drm/imagination/pvr_vm.c
> > +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> > @@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
> > switch (bind_op->type) {
> > case PVR_VM_BIND_TYPE_MAP: {
> > const struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = bind_op->device_addr,
> > - .map.va.range = bind_op->size,
> > - .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> > - .map.gem.offset = bind_op->offset,
> > + .op.va.addr = bind_op->device_addr,
> > + .op.va.range = bind_op->size,
> > + .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> > + .op.gem.offset = bind_op->offset,
> > };
> >
> > return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> > index 210604181c05..9b5d003bc5a2 100644
> > --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> > @@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
> > case MSM_VM_BIND_OP_MAP:
> > case MSM_VM_BIND_OP_MAP_NULL: {
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = op->iova,
> > - .map.va.range = op->range,
> > - .map.gem.obj = op->obj,
> > - .map.gem.offset = op->obj_offset,
> > + .op.va.addr = op->iova,
> > + .op.va.range = op->range,
> > + .op.gem.obj = op->obj,
> > + .op.gem.offset = op->obj_offset,
> > };
> >
> > ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
> > @@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
> > fallthrough;
> > case MSM_VM_BIND_OP_MAP_NULL: {
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = op->iova,
> > - .map.va.range = op->range,
> > - .map.gem.obj = op->obj,
> > - .map.gem.offset = op->obj_offset,
> > + .op.va.addr = op->iova,
> > + .op.va.range = op->range,
> > + .op.gem.obj = op->obj,
> > + .op.gem.offset = op->obj_offset,
> > };
> >
> > ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
> > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > index d94a85509176..314121a857e7 100644
> > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > @@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> > case OP_MAP: {
> > struct nouveau_uvma_region *reg;
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = op->va.addr,
> > - .map.va.range = op->va.range,
> > - .map.gem.obj = op->gem.obj,
> > - .map.gem.offset = op->gem.offset,
> > + .op.va.addr = op->va.addr,
> > + .op.va.range = op->va.range,
> > + .op.gem.obj = op->gem.obj,
> > + .op.gem.offset = op->gem.offset,
> > };
> >
> > reg = nouveau_uvma_region_find_first(uvmm,
> > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > index 2003b91a8409..3799e2c6ea59 100644
> > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > @@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
> > switch (op_type) {
> > case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> > const struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = op->va.addr,
> > - .map.va.range = op->va.range,
> > - .map.gem.obj = op->map.vm_bo->obj,
> > - .map.gem.offset = op->map.bo_offset,
> > + .op.va.addr = op->va.addr,
> > + .op.va.range = op->va.range,
> > + .op.gem.obj = op->map.vm_bo->obj,
> > + .op.gem.offset = op->map.bo_offset,
> > };
> >
> > if (vm->unusable) {
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index f35d69c0b4c6..66b54b152446 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> > case DRM_XE_VM_BIND_OP_MAP:
> > case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> > struct drm_gpuvm_map_req map_req = {
> > - .map.va.addr = addr,
> > - .map.va.range = range,
> > - .map.gem.obj = obj,
> > - .map.gem.offset = bo_offset_or_userptr,
> > + .op.va.addr = addr,
> > + .op.va.range = range,
> > + .op.gem.obj = obj,
> > + .op.gem.offset = bo_offset_or_userptr,
> > };
> >
> > ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
> > diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> > index 4a22b9d848f7..751c96a817ed 100644
> > --- a/include/drm/drm_gpuvm.h
> > +++ b/include/drm/drm_gpuvm.h
> > @@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
> > */
> > struct drm_gpuvm_map_req {
> > /**
> > - * @op_map: struct drm_gpuva_op_map
> > + * @op: struct drm_gpuva_op_map
> > */
> > - struct drm_gpuva_op_map map;
> > + struct drm_gpuva_op_map op;
On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
the first place. It would kinda make sense if it was containing an
bool madvise;
field, so you don't have to pass it around, but even then, I'm
wondering if we wouldn't be better off adding this field to
drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
the various map helpers (like Danilo suggested in his review of the
REPEATED mode series Caterina sent).
> > };
> >
> > struct drm_gpuva_ops *
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 11:01 ` Boris Brezillon
@ 2025-08-21 11:25 ` Boris Brezillon
2025-08-21 12:55 ` Danilo Krummrich
0 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2025-08-21 11:25 UTC (permalink / raw)
To: Himal Prasad Ghimiray
Cc: intel-xe, Boris Brezillon, Danilo Krummrich, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Thu, 21 Aug 2025 13:01:46 +0200
Boris Brezillon <boris.brezillon@collabora.com> wrote:
> On Wed, 20 Aug 2025 18:07:42 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>
> > On Wed, 20 Aug 2025 20:53:35 +0530
> > Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com> wrote:
> >
> > > Renamed 'map' to 'op' in drm_gpuvm_map_req for clarity and added
> > > corresponding documentation. No functional changes introduced.
> > >
> > > Fixes: baf1638c0956 ("drm/gpuvm: Introduce drm_gpuvm_madvise_ops_create")
> > > Fixes: 000a45dce7ad ("drm/gpuvm: Pass map arguments through a struct")
> > > Suggested-by: Boris Brezillon <bbrezillon@kernel.org>
> > > Suggested-by: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Danilo Krummrich <dakr@kernel.org>
> > > Cc: Matt Coster <matt.coster@imgtec.com>
> > > Cc: Boris Brezillon <bbrezillon@kernel.org>
> > > Cc: Rob Clark <robin.clark@oss.qualcomm.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: <dri-devel@lists.freedesktop.org>
> > > Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> >
> > Acked-by: Boris Brezillon <boris.brezillon@collabora.com>
> >
> > > ---
> > > drivers/gpu/drm/drm_gpuvm.c | 36 +++++++++++++-------------
> > > drivers/gpu/drm/imagination/pvr_vm.c | 8 +++---
> > > drivers/gpu/drm/msm/msm_gem_vma.c | 16 ++++++------
> > > drivers/gpu/drm/nouveau/nouveau_uvmm.c | 8 +++---
> > > drivers/gpu/drm/panthor/panthor_mmu.c | 8 +++---
> > > drivers/gpu/drm/xe/xe_vm.c | 8 +++---
> > > include/drm/drm_gpuvm.h | 4 +--
> > > 7 files changed, 44 insertions(+), 44 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> > > index 39f934a91a7b..e9aaf9b287e7 100644
> > > --- a/drivers/gpu/drm/drm_gpuvm.c
> > > +++ b/drivers/gpu/drm/drm_gpuvm.c
> > > @@ -552,11 +552,11 @@
> > > * struct drm_gem_object *obj, u64 offset)
> > > * {
> > > * struct drm_gpuvm_map_req map_req = {
> > > - * .map.va.addr = addr,
> > > - * .map.va.range = range,
> > > - * .map.gem.obj = obj,
> > > - * .map.gem.offset = offset,
> > > - * };
> > > + * .op.va.addr = addr,
> > > + * .op.va.range = range,
> > > + * .op.gem.obj = obj,
> > > + * .op.gem.offset = offset,
> > > + * };
> > > * struct drm_gpuva_ops *ops;
> > > * struct drm_gpuva_op *op
> > > * struct drm_gpuvm_bo *vm_bo;
> > > @@ -2132,10 +2132,10 @@ op_map_cb(const struct drm_gpuvm_ops *fn, void *priv,
> > > return 0;
> > >
> > > op.op = DRM_GPUVA_OP_MAP;
> > > - op.map.va.addr = req->map.va.addr;
> > > - op.map.va.range = req->map.va.range;
> > > - op.map.gem.obj = req->map.gem.obj;
> > > - op.map.gem.offset = req->map.gem.offset;
> > > + op.map.va.addr = req->op.va.addr;
> > > + op.map.va.range = req->op.va.range;
> > > + op.map.gem.obj = req->op.gem.obj;
> > > + op.map.gem.offset = req->op.gem.offset;
> > >
> > > return fn->sm_step_map(&op, priv);
> > > }
> > > @@ -2180,12 +2180,12 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > > const struct drm_gpuvm_map_req *req,
> > > bool madvise)
> > > {
> > > - struct drm_gem_object *req_obj = req->map.gem.obj;
> > > + struct drm_gem_object *req_obj = req->op.gem.obj;
> > > const struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;
> > > struct drm_gpuva *va, *next;
> > > - u64 req_offset = req->map.gem.offset;
> > > - u64 req_range = req->map.va.range;
> > > - u64 req_addr = req->map.va.addr;
> > > + u64 req_offset = req->op.gem.offset;
> > > + u64 req_range = req->op.va.range;
> > > + u64 req_addr = req->op.va.addr;
> > > u64 req_end = req_addr + req_range;
> > > int ret;
> > >
> > > @@ -2272,8 +2272,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > >
> > > if (madvise) {
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = req_addr,
> > > - .map.va.range = end - req_addr,
> > > + .op.va.addr = req_addr,
> > > + .op.va.range = end - req_addr,
> > > };
> > >
> > > ret = op_map_cb(ops, priv, &map_req);
> > > @@ -2340,8 +2340,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
> > >
> > > if (madvise) {
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = addr,
> > > - .map.va.range = req_end - addr,
> > > + .op.va.addr = addr,
> > > + .op.va.range = req_end - addr,
> > > };
> > >
> > > return op_map_cb(ops, priv, &map_req);
> > > @@ -2583,7 +2583,7 @@ drm_gpuvm_sm_map_exec_lock(struct drm_gpuvm *gpuvm,
> > > struct drm_exec *exec, unsigned int num_fences,
> > > struct drm_gpuvm_map_req *req)
> > > {
> > > - struct drm_gem_object *req_obj = req->map.gem.obj;
> > > + struct drm_gem_object *req_obj = req->op.gem.obj;
> > >
> > > if (req_obj) {
> > > int ret = drm_exec_prepare_obj(exec, req_obj, num_fences);
> > > diff --git a/drivers/gpu/drm/imagination/pvr_vm.c b/drivers/gpu/drm/imagination/pvr_vm.c
> > > index 3d97990170bf..983165eb3e6a 100644
> > > --- a/drivers/gpu/drm/imagination/pvr_vm.c
> > > +++ b/drivers/gpu/drm/imagination/pvr_vm.c
> > > @@ -187,10 +187,10 @@ static int pvr_vm_bind_op_exec(struct pvr_vm_bind_op *bind_op)
> > > switch (bind_op->type) {
> > > case PVR_VM_BIND_TYPE_MAP: {
> > > const struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = bind_op->device_addr,
> > > - .map.va.range = bind_op->size,
> > > - .map.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> > > - .map.gem.offset = bind_op->offset,
> > > + .op.va.addr = bind_op->device_addr,
> > > + .op.va.range = bind_op->size,
> > > + .op.gem.obj = gem_from_pvr_gem(bind_op->pvr_obj),
> > > + .op.gem.offset = bind_op->offset,
> > > };
> > >
> > > return drm_gpuvm_sm_map(&bind_op->vm_ctx->gpuvm_mgr,
> > > diff --git a/drivers/gpu/drm/msm/msm_gem_vma.c b/drivers/gpu/drm/msm/msm_gem_vma.c
> > > index 210604181c05..9b5d003bc5a2 100644
> > > --- a/drivers/gpu/drm/msm/msm_gem_vma.c
> > > +++ b/drivers/gpu/drm/msm/msm_gem_vma.c
> > > @@ -1179,10 +1179,10 @@ vm_bind_job_lock_objects(struct msm_vm_bind_job *job, struct drm_exec *exec)
> > > case MSM_VM_BIND_OP_MAP:
> > > case MSM_VM_BIND_OP_MAP_NULL: {
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = op->iova,
> > > - .map.va.range = op->range,
> > > - .map.gem.obj = op->obj,
> > > - .map.gem.offset = op->obj_offset,
> > > + .op.va.addr = op->iova,
> > > + .op.va.range = op->range,
> > > + .op.gem.obj = op->obj,
> > > + .op.gem.offset = op->obj_offset,
> > > };
> > >
> > > ret = drm_gpuvm_sm_map_exec_lock(job->vm, exec, 1, &map_req);
> > > @@ -1296,10 +1296,10 @@ vm_bind_job_prepare(struct msm_vm_bind_job *job)
> > > fallthrough;
> > > case MSM_VM_BIND_OP_MAP_NULL: {
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = op->iova,
> > > - .map.va.range = op->range,
> > > - .map.gem.obj = op->obj,
> > > - .map.gem.offset = op->obj_offset,
> > > + .op.va.addr = op->iova,
> > > + .op.va.range = op->range,
> > > + .op.gem.obj = op->obj,
> > > + .op.gem.offset = op->obj_offset,
> > > };
> > >
> > > ret = drm_gpuvm_sm_map(job->vm, &arg, &map_req);
> > > diff --git a/drivers/gpu/drm/nouveau/nouveau_uvmm.c b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > > index d94a85509176..314121a857e7 100644
> > > --- a/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > > +++ b/drivers/gpu/drm/nouveau/nouveau_uvmm.c
> > > @@ -1277,10 +1277,10 @@ nouveau_uvmm_bind_job_submit(struct nouveau_job *job,
> > > case OP_MAP: {
> > > struct nouveau_uvma_region *reg;
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = op->va.addr,
> > > - .map.va.range = op->va.range,
> > > - .map.gem.obj = op->gem.obj,
> > > - .map.gem.offset = op->gem.offset,
> > > + .op.va.addr = op->va.addr,
> > > + .op.va.range = op->va.range,
> > > + .op.gem.obj = op->gem.obj,
> > > + .op.gem.offset = op->gem.offset,
> > > };
> > >
> > > reg = nouveau_uvma_region_find_first(uvmm,
> > > diff --git a/drivers/gpu/drm/panthor/panthor_mmu.c b/drivers/gpu/drm/panthor/panthor_mmu.c
> > > index 2003b91a8409..3799e2c6ea59 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_mmu.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_mmu.c
> > > @@ -2204,10 +2204,10 @@ panthor_vm_exec_op(struct panthor_vm *vm, struct panthor_vm_op_ctx *op,
> > > switch (op_type) {
> > > case DRM_PANTHOR_VM_BIND_OP_TYPE_MAP: {
> > > const struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = op->va.addr,
> > > - .map.va.range = op->va.range,
> > > - .map.gem.obj = op->map.vm_bo->obj,
> > > - .map.gem.offset = op->map.bo_offset,
> > > + .op.va.addr = op->va.addr,
> > > + .op.va.range = op->va.range,
> > > + .op.gem.obj = op->map.vm_bo->obj,
> > > + .op.gem.offset = op->map.bo_offset,
> > > };
> > >
> > > if (vm->unusable) {
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index f35d69c0b4c6..66b54b152446 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -2339,10 +2339,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_vma_ops *vops,
> > > case DRM_XE_VM_BIND_OP_MAP:
> > > case DRM_XE_VM_BIND_OP_MAP_USERPTR: {
> > > struct drm_gpuvm_map_req map_req = {
> > > - .map.va.addr = addr,
> > > - .map.va.range = range,
> > > - .map.gem.obj = obj,
> > > - .map.gem.offset = bo_offset_or_userptr,
> > > + .op.va.addr = addr,
> > > + .op.va.range = range,
> > > + .op.gem.obj = obj,
> > > + .op.gem.offset = bo_offset_or_userptr,
> > > };
> > >
> > > ops = drm_gpuvm_sm_map_ops_create(&vm->gpuvm, &map_req);
> > > diff --git a/include/drm/drm_gpuvm.h b/include/drm/drm_gpuvm.h
> > > index 4a22b9d848f7..751c96a817ed 100644
> > > --- a/include/drm/drm_gpuvm.h
> > > +++ b/include/drm/drm_gpuvm.h
> > > @@ -1054,9 +1054,9 @@ struct drm_gpuva_ops {
> > > */
> > > struct drm_gpuvm_map_req {
> > > /**
> > > - * @op_map: struct drm_gpuva_op_map
> > > + * @op: struct drm_gpuva_op_map
> > > */
> > > - struct drm_gpuva_op_map map;
> > > + struct drm_gpuva_op_map op;
>
> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
> the first place. It would kinda make sense if it was containing an
>
> bool madvise;
>
> field, so you don't have to pass it around, but even then, I'm
> wondering if we wouldn't be better off adding this field to
> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
> the various map helpers (like Danilo suggested in his review of the
> REPEATED mode series Caterina sent).
More on that: the very reason I introduced drm_gpuvm_map_req in the
first place is so we have a clear differentiation between an overall
map request and the sub-operations that are created to fulfill it.
Looks like this was not a concern for Danilo and he was happy with us
using _op_map for this.
The other reason we might want to add drm_gpuvm_map_req is so that
information we only need while splitting a req don't pollute
drm_gpuva_op_map. Given I was going to pass the flags to the driver's
callback anyway (meaning it's needed at the op_map level), and given
you're passing madvise as a separate bool argument to various helpers
(_map_req just contains the op, not the madvise bool), I don't think
this aspect matters.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 11:25 ` Boris Brezillon
@ 2025-08-21 12:55 ` Danilo Krummrich
2025-08-21 13:01 ` Boris Brezillon
0 siblings, 1 reply; 14+ messages in thread
From: Danilo Krummrich @ 2025-08-21 12:55 UTC (permalink / raw)
To: Boris Brezillon
Cc: Himal Prasad Ghimiray, intel-xe, Boris Brezillon, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 13:01:46 +0200
> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>> the first place. It would kinda make sense if it was containing an
>>
>> bool madvise;
>>
>> field, so you don't have to pass it around, but even then, I'm
>> wondering if we wouldn't be better off adding this field to
>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>> the various map helpers (like Danilo suggested in his review of the
>> REPEATED mode series Caterina sent).
>
> More on that: the very reason I introduced drm_gpuvm_map_req in the
> first place is so we have a clear differentiation between an overall
> map request and the sub-operations that are created to fulfill it.
> Looks like this was not a concern for Danilo and he was happy with us
> using _op_map for this.
>
> The other reason we might want to add drm_gpuvm_map_req is so that
> information we only need while splitting a req don't pollute
> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
> callback anyway (meaning it's needed at the op_map level), and given
> you're passing madvise as a separate bool argument to various helpers
> (_map_req just contains the op, not the madvise bool), I don't think
> this aspect matters.
Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
there were additional flags included in the structure. Now that it is
essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
directly.
However, given that you still have patches in flight that will add a flags field
to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
away. Or did you drop this plan of adding those flags?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 12:55 ` Danilo Krummrich
@ 2025-08-21 13:01 ` Boris Brezillon
2025-08-21 13:30 ` Ghimiray, Himal Prasad
2025-08-21 13:35 ` Danilo Krummrich
0 siblings, 2 replies; 14+ messages in thread
From: Boris Brezillon @ 2025-08-21 13:01 UTC (permalink / raw)
To: Danilo Krummrich
Cc: Himal Prasad Ghimiray, intel-xe, Boris Brezillon, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Thu, 21 Aug 2025 14:55:06 +0200
"Danilo Krummrich" <dakr@kernel.org> wrote:
> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
> > On Thu, 21 Aug 2025 13:01:46 +0200
> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
> >> the first place. It would kinda make sense if it was containing an
> >>
> >> bool madvise;
> >>
> >> field, so you don't have to pass it around, but even then, I'm
> >> wondering if we wouldn't be better off adding this field to
> >> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
> >> the various map helpers (like Danilo suggested in his review of the
> >> REPEATED mode series Caterina sent).
> >
> > More on that: the very reason I introduced drm_gpuvm_map_req in the
> > first place is so we have a clear differentiation between an overall
> > map request and the sub-operations that are created to fulfill it.
> > Looks like this was not a concern for Danilo and he was happy with us
> > using _op_map for this.
> >
> > The other reason we might want to add drm_gpuvm_map_req is so that
> > information we only need while splitting a req don't pollute
> > drm_gpuva_op_map. Given I was going to pass the flags to the driver's
> > callback anyway (meaning it's needed at the op_map level), and given
> > you're passing madvise as a separate bool argument to various helpers
> > (_map_req just contains the op, not the madvise bool), I don't think
> > this aspect matters.
>
> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
> there were additional flags included in the structure. Now that it is
> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
> directly.
>
> However, given that you still have patches in flight that will add a flags field
> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
> away. Or did you drop this plan of adding those flags?
I need the flags field in the op_map too (so I can propagate it to the
drm_gpuva object), so I'd rather go with an op_map object directly and
kill drm_gpuvm_map_req now.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 13:01 ` Boris Brezillon
@ 2025-08-21 13:30 ` Ghimiray, Himal Prasad
2025-08-21 13:35 ` Danilo Krummrich
1 sibling, 0 replies; 14+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-08-21 13:30 UTC (permalink / raw)
To: Boris Brezillon, Danilo Krummrich
Cc: intel-xe, Boris Brezillon, Matt Coster, Rob Clark, Matthew Brost,
dri-devel
On 21-08-2025 18:31, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 14:55:06 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
>>> On Thu, 21 Aug 2025 13:01:46 +0200
>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>>>> the first place. It would kinda make sense if it was containing an
>>>>
>>>> bool madvise;
>>>>
>>>> field, so you don't have to pass it around, but even then, I'm
>>>> wondering if we wouldn't be better off adding this field to
>>>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>>>> the various map helpers (like Danilo suggested in his review of the
>>>> REPEATED mode series Caterina sent).
>>>
>>> More on that: the very reason I introduced drm_gpuvm_map_req in the
>>> first place is so we have a clear differentiation between an overall
>>> map request and the sub-operations that are created to fulfill it.
>>> Looks like this was not a concern for Danilo and he was happy with us
>>> using _op_map for this.
>>>
>>> The other reason we might want to add drm_gpuvm_map_req is so that
>>> information we only need while splitting a req don't pollute
>>> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
>>> callback anyway (meaning it's needed at the op_map level), and given
>>> you're passing madvise as a separate bool argument to various helpers
>>> (_map_req just contains the op, not the madvise bool), I don't think
>>> this aspect matters.
>>
>> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
>> there were additional flags included in the structure. Now that it is
>> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
>> directly.
>>
>> However, given that you still have patches in flight that will add a flags field
>> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
>> away. Or did you drop this plan of adding those flags?
>
> I need the flags field in the op_map too (so I can propagate it to the
> drm_gpuva object), so I'd rather go with an op_map object directly and
> kill drm_gpuvm_map_req now.
Thanks, Boris, for your comments, and Danilo for joining the discussion.
The patch I built upon is this version, where I dropped
drm_gpuvm_map_req and opted to use drm_gpuva_op_map directly.[1]
As I understand it, the initial recommendation was to introduce
drm_gpuvm_map_req with a flag to control the split/merge logic in
gpuvm_layer. However, with the introduction of madvise, we eventually
decided to proceed with a separate API, so the flag wasn’t added.
If needed, drm_gpuva_op_map can still introduce a flag for
driver-specific use cases.
If we’re confident that the flags in drm_gpuvm_map_req and
drm_gpuva_op_map will always align, I’m okay with dropping map_req.
Until we reach a final decision, I’ll hold off on submitting this patch.
[1] https://patchwork.freedesktop.org/patch/666205/?series=149550&rev=5
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 13:01 ` Boris Brezillon
2025-08-21 13:30 ` Ghimiray, Himal Prasad
@ 2025-08-21 13:35 ` Danilo Krummrich
2025-08-21 16:55 ` Ghimiray, Himal Prasad
1 sibling, 1 reply; 14+ messages in thread
From: Danilo Krummrich @ 2025-08-21 13:35 UTC (permalink / raw)
To: Boris Brezillon
Cc: Himal Prasad Ghimiray, intel-xe, Boris Brezillon, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Thu Aug 21, 2025 at 3:01 PM CEST, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 14:55:06 +0200
> "Danilo Krummrich" <dakr@kernel.org> wrote:
>
>> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
>> > On Thu, 21 Aug 2025 13:01:46 +0200
>> > Boris Brezillon <boris.brezillon@collabora.com> wrote:
>> >> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>> >> the first place. It would kinda make sense if it was containing an
>> >>
>> >> bool madvise;
>> >>
>> >> field, so you don't have to pass it around, but even then, I'm
>> >> wondering if we wouldn't be better off adding this field to
>> >> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>> >> the various map helpers (like Danilo suggested in his review of the
>> >> REPEATED mode series Caterina sent).
>> >
>> > More on that: the very reason I introduced drm_gpuvm_map_req in the
>> > first place is so we have a clear differentiation between an overall
>> > map request and the sub-operations that are created to fulfill it.
>> > Looks like this was not a concern for Danilo and he was happy with us
>> > using _op_map for this.
>> >
>> > The other reason we might want to add drm_gpuvm_map_req is so that
>> > information we only need while splitting a req don't pollute
>> > drm_gpuva_op_map. Given I was going to pass the flags to the driver's
>> > callback anyway (meaning it's needed at the op_map level), and given
>> > you're passing madvise as a separate bool argument to various helpers
>> > (_map_req just contains the op, not the madvise bool), I don't think
>> > this aspect matters.
>>
>> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
>> there were additional flags included in the structure. Now that it is
>> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
>> directly.
>>
>> However, given that you still have patches in flight that will add a flags field
>> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
>> away. Or did you drop this plan of adding those flags?
>
> I need the flags field in the op_map too (so I can propagate it to the
> drm_gpuva object), so I'd rather go with an op_map object directly and
> kill drm_gpuvm_map_req now.
In this case I agree, let's use struct drm_gpuva_op_map directly.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 13:35 ` Danilo Krummrich
@ 2025-08-21 16:55 ` Ghimiray, Himal Prasad
2025-08-22 7:35 ` Boris Brezillon
0 siblings, 1 reply; 14+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-08-21 16:55 UTC (permalink / raw)
To: Danilo Krummrich, Boris Brezillon
Cc: intel-xe, Boris Brezillon, Matt Coster, Rob Clark, Matthew Brost,
dri-devel
On 21-08-2025 19:05, Danilo Krummrich wrote:
> On Thu Aug 21, 2025 at 3:01 PM CEST, Boris Brezillon wrote:
>> On Thu, 21 Aug 2025 14:55:06 +0200
>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>
>>> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
>>>> On Thu, 21 Aug 2025 13:01:46 +0200
>>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>>>>> the first place. It would kinda make sense if it was containing an
>>>>>
>>>>> bool madvise;
>>>>>
>>>>> field, so you don't have to pass it around, but even then, I'm
>>>>> wondering if we wouldn't be better off adding this field to
>>>>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>>>>> the various map helpers (like Danilo suggested in his review of the
>>>>> REPEATED mode series Caterina sent).
>>>>
>>>> More on that: the very reason I introduced drm_gpuvm_map_req in the
>>>> first place is so we have a clear differentiation between an overall
>>>> map request and the sub-operations that are created to fulfill it.
>>>> Looks like this was not a concern for Danilo and he was happy with us
>>>> using _op_map for this.
>>>>
>>>> The other reason we might want to add drm_gpuvm_map_req is so that
>>>> information we only need while splitting a req don't pollute
>>>> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
>>>> callback anyway (meaning it's needed at the op_map level), and given
>>>> you're passing madvise as a separate bool argument to various helpers
>>>> (_map_req just contains the op, not the madvise bool), I don't think
>>>> this aspect matters.
>>>
>>> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
>>> there were additional flags included in the structure. Now that it is
>>> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
>>> directly.
>>>
>>> However, given that you still have patches in flight that will add a flags field
>>> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
>>> away. Or did you drop this plan of adding those flags?
>>
>> I need the flags field in the op_map too (so I can propagate it to the
>> drm_gpuva object), so I'd rather go with an op_map object directly and
>> kill drm_gpuvm_map_req now.
>
> In this case I agree, let's use struct drm_gpuva_op_map directly.
According to the kernel documentation for the drm_gpuva_op_map
structure, it is intended to represent a single map operation generated
as the output of ops_create or the GPU VA manager. Using it as a direct
input to ops_create contradicts this definition.
For drm_gpuvm_sm_map_ops_create, the values align with those in
drm_gpuvm_map_req. However, this is not the case for
drm_gpuvm_madvise_ops_create.
If we plan to proceed with deprecating drm_gpuvm_map_req, we need to
clarify the fundamental definition of drm_gpuva_op_map:
Should it represent a user-requested map, or an operation generated by
the GPU VA manager?
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-21 16:55 ` Ghimiray, Himal Prasad
@ 2025-08-22 7:35 ` Boris Brezillon
2025-08-22 7:52 ` Ghimiray, Himal Prasad
0 siblings, 1 reply; 14+ messages in thread
From: Boris Brezillon @ 2025-08-22 7:35 UTC (permalink / raw)
To: Ghimiray, Himal Prasad
Cc: Danilo Krummrich, intel-xe, Boris Brezillon, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On Thu, 21 Aug 2025 22:25:06 +0530
"Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com> wrote:
> On 21-08-2025 19:05, Danilo Krummrich wrote:
> > On Thu Aug 21, 2025 at 3:01 PM CEST, Boris Brezillon wrote:
> >> On Thu, 21 Aug 2025 14:55:06 +0200
> >> "Danilo Krummrich" <dakr@kernel.org> wrote:
> >>
> >>> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
> >>>> On Thu, 21 Aug 2025 13:01:46 +0200
> >>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
> >>>>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
> >>>>> the first place. It would kinda make sense if it was containing an
> >>>>>
> >>>>> bool madvise;
> >>>>>
> >>>>> field, so you don't have to pass it around, but even then, I'm
> >>>>> wondering if we wouldn't be better off adding this field to
> >>>>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
> >>>>> the various map helpers (like Danilo suggested in his review of the
> >>>>> REPEATED mode series Caterina sent).
> >>>>
> >>>> More on that: the very reason I introduced drm_gpuvm_map_req in the
> >>>> first place is so we have a clear differentiation between an overall
> >>>> map request and the sub-operations that are created to fulfill it.
> >>>> Looks like this was not a concern for Danilo and he was happy with us
> >>>> using _op_map for this.
> >>>>
> >>>> The other reason we might want to add drm_gpuvm_map_req is so that
> >>>> information we only need while splitting a req don't pollute
> >>>> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
> >>>> callback anyway (meaning it's needed at the op_map level), and given
> >>>> you're passing madvise as a separate bool argument to various helpers
> >>>> (_map_req just contains the op, not the madvise bool), I don't think
> >>>> this aspect matters.
> >>>
> >>> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
> >>> there were additional flags included in the structure. Now that it is
> >>> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
> >>> directly.
> >>>
> >>> However, given that you still have patches in flight that will add a flags field
> >>> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
> >>> away. Or did you drop this plan of adding those flags?
> >>
> >> I need the flags field in the op_map too (so I can propagate it to the
> >> drm_gpuva object), so I'd rather go with an op_map object directly and
> >> kill drm_gpuvm_map_req now.
> >
> > In this case I agree, let's use struct drm_gpuva_op_map directly.
>
> According to the kernel documentation for the drm_gpuva_op_map
> structure, it is intended to represent a single map operation generated
> as the output of ops_create or the GPU VA manager. Using it as a direct
> input to ops_create contradicts this definition.
>
> For drm_gpuvm_sm_map_ops_create, the values align with those in
> drm_gpuvm_map_req. However, this is not the case for
> drm_gpuvm_madvise_ops_create.
>
> If we plan to proceed with deprecating drm_gpuvm_map_req, we need to
> clarify the fundamental definition of drm_gpuva_op_map:
> Should it represent a user-requested map, or an operation generated by
> the GPU VA manager?
I would say, update the doc to reflect it can be used to pass a user
map request too, but I'll let Danilo make the final call. BTW,
embedding an op in _map_req is equivalent to saying the _op_map object
can describe a user map request to me :P.
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req
2025-08-22 7:35 ` Boris Brezillon
@ 2025-08-22 7:52 ` Ghimiray, Himal Prasad
0 siblings, 0 replies; 14+ messages in thread
From: Ghimiray, Himal Prasad @ 2025-08-22 7:52 UTC (permalink / raw)
To: Boris Brezillon
Cc: Danilo Krummrich, intel-xe, Boris Brezillon, Matt Coster,
Rob Clark, Matthew Brost, dri-devel
On 22-08-2025 13:05, Boris Brezillon wrote:
> On Thu, 21 Aug 2025 22:25:06 +0530
> "Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com> wrote:
>
>> On 21-08-2025 19:05, Danilo Krummrich wrote:
>>> On Thu Aug 21, 2025 at 3:01 PM CEST, Boris Brezillon wrote:
>>>> On Thu, 21 Aug 2025 14:55:06 +0200
>>>> "Danilo Krummrich" <dakr@kernel.org> wrote:
>>>>
>>>>> On Thu Aug 21, 2025 at 1:25 PM CEST, Boris Brezillon wrote:
>>>>>> On Thu, 21 Aug 2025 13:01:46 +0200
>>>>>> Boris Brezillon <boris.brezillon@collabora.com> wrote:
>>>>>>> On a second thought, I'm now wondering why we need drm_gpuvm_map_req in
>>>>>>> the first place. It would kinda make sense if it was containing an
>>>>>>>
>>>>>>> bool madvise;
>>>>>>>
>>>>>>> field, so you don't have to pass it around, but even then, I'm
>>>>>>> wondering if we wouldn't be better off adding this field to
>>>>>>> drm_gpuva_op_map instead and passing an drm_gpuva_op_map object to
>>>>>>> the various map helpers (like Danilo suggested in his review of the
>>>>>>> REPEATED mode series Caterina sent).
>>>>>>
>>>>>> More on that: the very reason I introduced drm_gpuvm_map_req in the
>>>>>> first place is so we have a clear differentiation between an overall
>>>>>> map request and the sub-operations that are created to fulfill it.
>>>>>> Looks like this was not a concern for Danilo and he was happy with us
>>>>>> using _op_map for this.
>>>>>>
>>>>>> The other reason we might want to add drm_gpuvm_map_req is so that
>>>>>> information we only need while splitting a req don't pollute
>>>>>> drm_gpuva_op_map. Given I was going to pass the flags to the driver's
>>>>>> callback anyway (meaning it's needed at the op_map level), and given
>>>>>> you're passing madvise as a separate bool argument to various helpers
>>>>>> (_map_req just contains the op, not the madvise bool), I don't think
>>>>>> this aspect matters.
>>>>>
>>>>> Good catch! Indeed, when Himal picked up your struct drm_gpuvm_map_req patch,
>>>>> there were additional flags included in the structure. Now that it is
>>>>> essentially a transparent wrapper, I prefer to use struct drm_gpuva_op_map
>>>>> directly.
>>>>>
>>>>> However, given that you still have patches in flight that will add a flags field
>>>>> to struct drm_gpuvm_map_req I think it's probably fine to introduce it right
>>>>> away. Or did you drop this plan of adding those flags?
>>>>
>>>> I need the flags field in the op_map too (so I can propagate it to the
>>>> drm_gpuva object), so I'd rather go with an op_map object directly and
>>>> kill drm_gpuvm_map_req now.
>>>
>>> In this case I agree, let's use struct drm_gpuva_op_map directly.
>>
>> According to the kernel documentation for the drm_gpuva_op_map
>> structure, it is intended to represent a single map operation generated
>> as the output of ops_create or the GPU VA manager. Using it as a direct
>> input to ops_create contradicts this definition.
>>
>> For drm_gpuvm_sm_map_ops_create, the values align with those in
>> drm_gpuvm_map_req. However, this is not the case for
>> drm_gpuvm_madvise_ops_create.
>>
>> If we plan to proceed with deprecating drm_gpuvm_map_req, we need to
>> clarify the fundamental definition of drm_gpuva_op_map:
>> Should it represent a user-requested map, or an operation generated by
>> the GPU VA manager?
>
> I would say, update the doc to reflect it can be used to pass a user
> map request too, but I'll let Danilo make the final call. BTW,
> embedding an op in _map_req is equivalent to saying the _op_map object
> can describe a user map request to me :P.
True :P
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-08-22 7:52 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-08-20 15:23 [PATCH] drm/gpuvm: Rename 'map' to 'op' in drm_gpuvm_map_req Himal Prasad Ghimiray
2025-08-20 16:07 ` Boris Brezillon
2025-08-21 11:01 ` Boris Brezillon
2025-08-21 11:25 ` Boris Brezillon
2025-08-21 12:55 ` Danilo Krummrich
2025-08-21 13:01 ` Boris Brezillon
2025-08-21 13:30 ` Ghimiray, Himal Prasad
2025-08-21 13:35 ` Danilo Krummrich
2025-08-21 16:55 ` Ghimiray, Himal Prasad
2025-08-22 7:35 ` Boris Brezillon
2025-08-22 7:52 ` Ghimiray, Himal Prasad
2025-08-20 16:38 ` Danilo Krummrich
2025-08-20 16:53 ` Rob Clark
2025-08-20 16:56 ` Matt Coster
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).