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

["Ghimiray, Himal Prasad" <himal.prasad.ghimiray@intel.com>]

On 17-04-2025 05:40, Matthew Brost wrote:
> 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.

Thanks for pointing this. I tested with prefetch up to 256 MiB  hence 
missed it. >
>   
> 
>>   {
>>   	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.
> 

sure will use 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
>>