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'.
> + 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;
or
if (madvise && va->gem.obj) {
if (end > req_end)
break;
else
continue;
}
instead of sprinkling the skip_madvise_ops checks everywhere.
>
> + 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.
> + 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.
