Re: [PATCH v2 06/32] drm/xe/vm: Update xe_vma_ops_incr_pt_update_ops to take an increment value

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

On Mon, Apr 07, 2025 at 03:46:53PM +0530, Himal Prasad Ghimiray wrote:
> Prefetch for SVM ranges can have more than one operation to increment,
> hence modify the function to accept an increment value as input.
> 
> v2:
>   - Call xe_vma_ops_incr_pt_update_ops only once for REMAP (Matthew Brost)
>   - Add check for 0 ops
> 
> Suggested-by: Matthew Brost <matthew.brost@intel.com>
> Signed-off-by: Himal Prasad Ghimiray <himal.prasad.ghimiray@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c | 28 +++++++++++++++++-----------
>  1 file changed, 17 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 0c69ef6b5ec5..4d215c55a778 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -806,13 +806,16 @@ static void xe_vma_ops_fini(struct xe_vma_ops *vops)
>  		kfree(vops->pt_update_ops[i].ops);
>  }
>  
> -static void xe_vma_ops_incr_pt_update_ops(struct xe_vma_ops *vops, u8 tile_mask)
> +static void xe_vma_ops_incr_pt_update_ops(struct xe_vma_ops *vops, u8 tile_mask, u8 inc_val)

s/u8 inc_val/int inc_val

or maybe u32?

Just debugged a problem which the compute UMD + prefetch where the
inc_val was 256, thus 0, so the binding step was skipped for prefetch.

 

>  {
>  	int i;
>  
> +	if(!inc_val)
> +		return;
> +
>  	for (i = 0; i < XE_MAX_TILES_PER_DEVICE; ++i)
>  		if (BIT(i) & tile_mask)
> -			++vops->pt_update_ops[i].num_ops;
> +			vops->pt_update_ops[i].num_ops += inc_val;
>  }
>  
>  static void xe_vm_populate_rebind(struct xe_vma_op *op, struct xe_vma *vma,
> @@ -842,7 +845,7 @@ static int xe_vm_ops_add_rebind(struct xe_vma_ops *vops, struct xe_vma *vma,
>  
>  	xe_vm_populate_rebind(op, vma, tile_mask);
>  	list_add_tail(&op->link, &vops->list);
> -	xe_vma_ops_incr_pt_update_ops(vops, tile_mask);
> +	xe_vma_ops_incr_pt_update_ops(vops, tile_mask, 1);
>  
>  	return 0;
>  }
> @@ -977,7 +980,7 @@ xe_vm_ops_add_range_rebind(struct xe_vma_ops *vops,
>  
>  	xe_vm_populate_range_rebind(op, vma, range, tile_mask);
>  	list_add_tail(&op->link, &vops->list);
> -	xe_vma_ops_incr_pt_update_ops(vops, tile_mask);
> +	xe_vma_ops_incr_pt_update_ops(vops, tile_mask, 1);
>  
>  	return 0;
>  }
> @@ -1062,7 +1065,7 @@ xe_vm_ops_add_range_unbind(struct xe_vma_ops *vops,
>  
>  	xe_vm_populate_range_unbind(op, range);
>  	list_add_tail(&op->link, &vops->list);
> -	xe_vma_ops_incr_pt_update_ops(vops, range->tile_present);
> +	xe_vma_ops_incr_pt_update_ops(vops, range->tile_present, 1);
>  
>  	return 0;
>  }
> @@ -2493,7 +2496,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  			     !op->map.is_cpu_addr_mirror) ||
>  			    op->map.invalidate_on_bind)
>  				xe_vma_ops_incr_pt_update_ops(vops,
> -							      op->tile_mask);
> +							      op->tile_mask, 1);
>  			break;
>  		}
>  		case DRM_GPUVA_OP_REMAP:
> @@ -2502,6 +2505,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  				gpuva_to_vma(op->base.remap.unmap->va);
>  			bool skip = xe_vma_is_cpu_addr_mirror(old);
>  			u64 start = xe_vma_start(old), end = xe_vma_end(old);
> +			u8 num_remap_ops = 0;

u8 actually works here as the max value is 3 but I'd change this to a
u32 or int.

Otherwise LGTM.

Matt

>  
>  			if (op->base.remap.prev)
>  				start = op->base.remap.prev->va.addr +
> @@ -2554,7 +2558,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  					       (ULL)op->remap.start,
>  					       (ULL)op->remap.range);
>  				} else {
> -					xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> +					num_remap_ops++;
>  				}
>  			}
>  
> @@ -2583,11 +2587,13 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  					       (ULL)op->remap.start,
>  					       (ULL)op->remap.range);
>  				} else {
> -					xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> +					num_remap_ops++;
>  				}
>  			}
>  			if (!skip)
> -				xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> +				num_remap_ops++;
> +
> +			xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask, num_remap_ops);
>  			break;
>  		}
>  		case DRM_GPUVA_OP_UNMAP:
> @@ -2599,7 +2605,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  				return -EBUSY;
>  
>  			if (!xe_vma_is_cpu_addr_mirror(vma))
> -				xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> +				xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask, 1);
>  			break;
>  		case DRM_GPUVA_OP_PREFETCH:
>  			vma = gpuva_to_vma(op->base.prefetch.va);
> @@ -2611,7 +2617,7 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct drm_gpuva_ops *ops,
>  			}
>  
>  			if (!xe_vma_is_cpu_addr_mirror(vma))
> -				xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> +				xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask, 1);
>  			break;
>  		default:
>  			drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> -- 
> 2.34.1
>