Re: [PATCH] drm/gpuvm: merge adjacent gpuva range during a map operation

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Sep 18, 2024 at 12:47:40PM -0400, Oak Zeng wrote:

Please sent patches which touch common code to dri-devel.

> Considder this example. Before a map operation, the gpuva ranges
> in a vm looks like below:
> 
>  VAs | start              | range              | end                | object             | object offset
> -------------------------------------------------------------------------------------------------------------
>      | 0x0000000000000000 | 0x00007ffff5cd0000 | 0x00007ffff5cd0000 | 0x0000000000000000 | 0x0000000000000000
>      | 0x00007ffff5cf0000 | 0x00000000000c7000 | 0x00007ffff5db7000 | 0x0000000000000000 | 0x0000000000000000
> 
> Now user want to map range [0x00007ffff5cd0000 - 0x00007ffff5cf0000).
> With existing codes, the range walking in __drm_gpuvm_sm_map won't
> find any range, so we end up a single map operation for range
> [0x00007ffff5cd0000 - 0x00007ffff5cf0000). This result in:
> 
>  VAs | start              | range              | end                | object             | object offset
> -------------------------------------------------------------------------------------------------------------
>      | 0x0000000000000000 | 0x00007ffff5cd0000 | 0x00007ffff5cd0000 | 0x0000000000000000 | 0x0000000000000000
>      | 0x00007ffff5cd0000 | 0x0000000000020000 | 0x00007ffff5cf0000 | 0x0000000000000000 | 0x0000000000000000
>      | 0x00007ffff5cf0000 | 0x00000000000c7000 | 0x00007ffff5db7000 | 0x0000000000000000 | 0x0000000000000000
> 
> The correct behavior is to merge those 3 ranges. So __drm_gpuvm_sm_map

Danilo - correct me if I'm wrong, but I believe early in gpuvm you had
similar code to this which could optionally be used. I was of the
thinking Xe didn't want this behavior and eventually this behavior was
ripped out prior to merging.

> is slightly modified to handle this corner case. The walker is changed
> to find the range just before or after the mapping request, and merge
> adjacent ranges using unmap and map operations. with this change, the

This would problematic in Xe for several reasons.

1. This would create a window in which previously valid mappings are
unmapped by our bind code implementation which could result in a fault.
Remap operations can create a similar window but it is handled by either
only unmapping the required range or using dma-resv slots to close this
window ensuring nothing is running on the GPU while valid mappings are
unmapped. A series of UNMAP, UNMAP, and MAP ops currently doesn't detect
the problematic window. If we wanted to do something like this, we'd
probably need to a new op like MERGE or something to help detect this
window.

2. Consider this case.

0x0000000000000000-0x00007ffff5cd0000 VMA[A]
0x00007ffff5cf0000-0x00000000000c7000 VMA[B]
0x00007ffff5cd0000-0x0000000000020000 VMA[C] 

What is VMA[A], VMA[B], and VMA[C] are all setup with different driver
specific implmentation properties (e.g. pat_index). These VMAs cannot be
merged. GPUVM has no visablity to this. If we wanted to do this I think
we'd need a gpuvm vfunc that calls into the driver to determine if we
can merge VMAs.

3. What is the ROI of this? Slightly reducing the VMA count? Perhaps
allowing larger GPU is very specific corner cases? Give 1), 2) I'd say
just leave GPUVM as is rather than add this complexity and then make all
driver use GPUVM absorb this behavior change.

Matt

> end result of above example is as below:
> 
>  VAs | start              | range              | end                | object             | object offset
> -------------------------------------------------------------------------------------------------------------
>      | 0x0000000000000000 | 0x00007ffff5db7000 | 0x00007ffff5db7000 | 0x0000000000000000 | 0x0000000000000000
> 
> Even though this fixes a real problem, the codes looks a little ugly.
> So I welcome any better fix or suggestion.
> 
> Signed-off-by: Oak Zeng <oak.zeng@intel.com>
> ---
>  drivers/gpu/drm/drm_gpuvm.c | 62 +++++++++++++++++++++++++------------
>  1 file changed, 43 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gpuvm.c b/drivers/gpu/drm/drm_gpuvm.c
> index 4b6fcaea635e..51825c794bdc 100644
> --- a/drivers/gpu/drm/drm_gpuvm.c
> +++ b/drivers/gpu/drm/drm_gpuvm.c
> @@ -2104,28 +2104,30 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  {
>  	struct drm_gpuva *va, *next;
>  	u64 req_end = req_addr + req_range;
> +	u64 merged_req_addr = req_addr;
> +	u64 merged_req_end = req_end;
>  	int ret;
>  
>  	if (unlikely(!drm_gpuvm_range_valid(gpuvm, req_addr, req_range)))
>  		return -EINVAL;
>  
> -	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req_addr, req_end) {
> +	drm_gpuvm_for_each_va_range_safe(va, next, gpuvm, req_addr - 1, req_end + 1) {
>  		struct drm_gem_object *obj = va->gem.obj;
>  		u64 offset = va->gem.offset;
>  		u64 addr = va->va.addr;
>  		u64 range = va->va.range;
>  		u64 end = addr + range;
> -		bool merge = !!va->gem.obj;
> +		bool merge;
>  
>  		if (addr == req_addr) {
> -			merge &= obj == req_obj &&
> +			merge = obj == req_obj &&
>  				 offset == req_offset;
>  
>  			if (end == req_end) {
>  				ret = op_unmap_cb(ops, priv, va, merge);
>  				if (ret)
>  					return ret;
> -				break;
> +				continue;
>  			}
>  
>  			if (end < req_end) {
> @@ -2162,22 +2164,33 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  			};
>  			struct drm_gpuva_op_unmap u = { .va = va };
>  
> -			merge &= obj == req_obj &&
> -				 offset + ls_range == req_offset;
> +			merge = (obj && obj == req_obj &&
> +				 offset + ls_range == req_offset) ||
> +				 (!obj && !req_obj);
>  			u.keep = merge;
>  
>  			if (end == req_end) {
>  				ret = op_remap_cb(ops, priv, &p, NULL, &u);
>  				if (ret)
>  					return ret;
> -				break;
> +				continue;
>  			}
>  
>  			if (end < req_end) {
> -				ret = op_remap_cb(ops, priv, &p, NULL, &u);
> -				if (ret)
> -					return ret;
> -				continue;
> +				if (end == req_addr) {
> +					if (merge) {
> +						ret = op_unmap_cb(ops, priv, va, merge);
> +						if (ret)
> +							return ret;
> +						merged_req_addr = addr;
> +						continue;
> +					}
> +				} else {
> +					ret = op_remap_cb(ops, priv, &p, NULL, &u);
> +					if (ret)
> +						return ret;
> +					continue;
> +				}
>  			}
>  
>  			if (end > req_end) {
> @@ -2195,15 +2208,16 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  				break;
>  			}
>  		} else if (addr > req_addr) {
> -			merge &= obj == req_obj &&
> +			merge = (obj && obj == req_obj &&
>  				 offset == req_offset +
> -					   (addr - req_addr);
> +					   (addr - req_addr)) ||
> +				 (!obj && !req_obj);
>  
>  			if (end == req_end) {
>  				ret = op_unmap_cb(ops, priv, va, merge);
>  				if (ret)
>  					return ret;
> -				break;
> +				continue;
>  			}
>  
>  			if (end < req_end) {
> @@ -2225,16 +2239,26 @@ __drm_gpuvm_sm_map(struct drm_gpuvm *gpuvm,
>  					.keep = merge,
>  				};
>  
> -				ret = op_remap_cb(ops, priv, NULL, &n, &u);
> -				if (ret)
> -					return ret;
> -				break;
> +				if (addr == req_end) {
> +					if (merge) {
> +						ret = op_unmap_cb(ops, priv, va, merge);
> +						if (ret)
> +							return ret;
> +						merged_req_end = end;
> +						break;
> +					}
> +				} else {
> +					ret = op_remap_cb(ops, priv, NULL, &n, &u);
> +					if (ret)
> +						return ret;
> +					break;
> +				}
>  			}
>  		}
>  	}
>  
>  	return op_map_cb(ops, priv,
> -			 req_addr, req_range,
> +			 merged_req_addr, merged_req_end - merged_req_addr,
>  			 req_obj, req_offset);
>  }
>  
> -- 
> 2.26.3
>