Re: [PATCH v6 04/26] drm/gpuvm: Introduce DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE flag

[Ghimiray, Himal Prasad]

On 12-08-2025 22:28, Danilo Krummrich wrote:

On Thu Aug 7, 2025 at 6:43 PM CEST, Himal Prasad Ghimiray wrote:

@@ -2110,6 +2110,8 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
  {
        struct drm_gpuva *va, *next;
        u64 req_end = req->op_map.va.addr + req->op_map.va.range;
+       bool is_madvise_ops = (req->flags & 
DRM_GPUVM_SM_MAP_OPS_FLAG_SPLIT_MADVISE);

Let's just call this 'madvise'.

Sure.

+       bool needs_map = !is_madvise_ops;
        int ret;

if (unlikely(!drm_gpuvm_range_valid(gpuvm, req->op_map.va.addr, req->op_map.va.range)))

@@ -2122,26 +2124,35 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
                u64 range = va->va.range;
                u64 end = addr + range;
                bool merge = !!va->gem.obj;
+               bool skip_madvise_ops = is_madvise_ops && merge;

IIUC, you're either going for continue or break in this case. I think continue
would always be correct and break is an optimization if end <= req_end?

If that's correct, please just do either

        if (madvise && va->gem.obj)
                continue;

Will use this.>

or

        if (madvise && va->gem.obj) {
                if (end > req_end)
                        break;
                else
                        continue;
        }

instead of sprinkling the skip_madvise_ops checks everywhere.

True, recommended checks make it cleaner.

+ needs_map = !is_madvise_ops;

                if (addr == req->op_map.va.addr) {
                        merge &= obj == req->op_map.gem.obj &&
                                 offset == req->op_map.gem.offset;

if (end == req_end) {

-                               ret = op_unmap_cb(ops, priv, va, merge);
-                               if (ret)
-                                       return ret;
+                               if (!is_madvise_ops) {
+                                       ret = op_unmap_cb(ops, priv, va, merge);
+                                       if (ret)
+                                               return ret;
+                               }
                                break;
                        }

if (end < req_end) {

-                               ret = op_unmap_cb(ops, priv, va, merge);
-                               if (ret)
-                                       return ret;
+                               if (!is_madvise_ops) {
+                                       ret = op_unmap_cb(ops, priv, va, merge);

I think we should pass madvise as argument to op_unmap_cb() and make it a noop
internally rather than having all the conditionals.

Makes sense. Will modify in next version.

+                                       if (ret)
+                                               return ret;
+                               }
                                continue;
                        }

if (end > req_end) {

+                               if (skip_madvise_ops)
+                                       break;
+
                                struct drm_gpuva_op_map n = {
                                        .va.addr = req_end,
                                        .va.range = range - 
req->op_map.va.range,
@@ -2156,6 +2167,9 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
                                ret = op_remap_cb(ops, priv, NULL, &n, &u);
                                if (ret)
                                        return ret;
+
+                               if (is_madvise_ops)
+                                       needs_map = true;

I don't like this needs_map state...

Maybe we could have

        struct drm_gpuvm_map_req *op_map = madvise ? NULL : req;

at the beginning of the function and then change this line to

        if (madvise)
                op_map = req;

and op_map_cb() can just handle a NULL pointer.

Yeah, I feel like that's better.

Agreed.

Thanks for the review.