Re: [PATCH] drm/xe: Create LRC BO without VM

[Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>]

On Wed, May 28, 2025 at 09:18:21PM -0700, Matthew Brost wrote:
>On Wed, May 28, 2025 at 09:04:49PM -0700, Niranjana Vishwanathapura wrote:
>> Specifying VM during lrc->bo creation requires VM's reference
>> to be held for the lifetime of lrc->bo as it will use VM's dma
>> reservation object. Using VM's dma reservation object for
>> lrc->bo doesn't provide any advantage. Hence do not pass VM
>> while creating lrc->bo.
>>
>> Fixes: 264eecdba211 ("drm/xe: Decouple xe_exec_queue and xe_lrc")
>> Signed-off-by: Niranjana Vishwanathapura <niranjana.vishwanathapura@intel.com>
>
>Good cleanup. I thought this through and can't reason why this would
>work or any advantages of the LRC using VM's dma-resv.
>
>One nit, xe_lrc_finish can use xe_bo_unpin_map_no_vm on lrc->bo now.
>
>Asssuming CI passes and the above nit fixed:
>Reviewed-by: Matthew Brost <matthew.brost@intel.com>
>

Thanks. I have uploaded v2 to use xe_bo_unpin_map_no_vm.

Niranjana

>> ---
>>  drivers/gpu/drm/xe/xe_exec_queue.c |  9 ---------
>>  drivers/gpu/drm/xe/xe_lrc.c        | 18 +++---------------
>>  2 files changed, 3 insertions(+), 24 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_exec_queue.c b/drivers/gpu/drm/xe/xe_exec_queue.c
>> index ce78cee5dec6..32c6923df7c3 100644
>> --- a/drivers/gpu/drm/xe/xe_exec_queue.c
>> +++ b/drivers/gpu/drm/xe/xe_exec_queue.c
>> @@ -132,12 +132,6 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q)
>>  			flags |= XE_LRC_CREATE_RUNALONE;
>>  	}
>>
>> -	if (vm) {
>> -		err = xe_vm_lock(vm, true);
>> -		if (err)
>> -			return err;
>> -	}
>> -
>>  	for (i = 0; i < q->width; ++i) {
>>  		q->lrc[i] = xe_lrc_create(q->hwe, q->vm, SZ_16K, q->msix_vec, flags);
>>  		if (IS_ERR(q->lrc[i])) {
>> @@ -146,9 +140,6 @@ static int __xe_exec_queue_init(struct xe_exec_queue *q)
>>  		}
>>  	}
>>
>> -	if (vm)
>> -		xe_vm_unlock(vm);
>> -
>>  	err = q->ops->init(q);
>>  	if (err)
>>  		goto err_lrc;
>> diff --git a/drivers/gpu/drm/xe/xe_lrc.c b/drivers/gpu/drm/xe/xe_lrc.c
>> index 61a2e87990a9..974fe509df0e 100644
>> --- a/drivers/gpu/drm/xe/xe_lrc.c
>> +++ b/drivers/gpu/drm/xe/xe_lrc.c
>> @@ -1007,7 +1007,7 @@ static int xe_lrc_init(struct xe_lrc *lrc, struct xe_hw_engine *hwe,
>>  	 * FIXME: Perma-pinning LRC as we don't yet support moving GGTT address
>>  	 * via VM bind calls.
>>  	 */
>> -	lrc->bo = xe_bo_create_pin_map(xe, tile, vm, lrc_size,
>> +	lrc->bo = xe_bo_create_pin_map(xe, tile, NULL, lrc_size,
>>  				       ttm_bo_type_kernel,
>>  				       bo_flags);
>>  	if (IS_ERR(lrc->bo))
>> @@ -1795,9 +1795,6 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
>>  	if (!snapshot)
>>  		return NULL;
>>
>> -	if (lrc->bo->vm)
>> -		xe_vm_get(lrc->bo->vm);
>> -
>>  	snapshot->context_desc = xe_lrc_ggtt_addr(lrc);
>>  	snapshot->ring_addr = __xe_lrc_ring_ggtt_addr(lrc);
>>  	snapshot->indirect_context_desc = xe_lrc_indirect_ring_ggtt_addr(lrc);
>> @@ -1819,14 +1816,12 @@ struct xe_lrc_snapshot *xe_lrc_snapshot_capture(struct xe_lrc *lrc)
>>  void xe_lrc_snapshot_capture_delayed(struct xe_lrc_snapshot *snapshot)
>>  {
>>  	struct xe_bo *bo;
>> -	struct xe_vm *vm;
>>  	struct iosys_map src;
>>
>>  	if (!snapshot)
>>  		return;
>>
>>  	bo = snapshot->lrc_bo;
>> -	vm = bo->vm;
>>  	snapshot->lrc_bo = NULL;
>>
>>  	snapshot->lrc_snapshot = kvmalloc(snapshot->lrc_size, GFP_KERNEL);
>> @@ -1846,8 +1841,6 @@ void xe_lrc_snapshot_capture_delayed(struct xe_lrc_snapshot *snapshot)
>>  	xe_bo_unlock(bo);
>>  put_bo:
>>  	xe_bo_put(bo);
>> -	if (vm)
>> -		xe_vm_put(vm);
>>  }
>>
>>  void xe_lrc_snapshot_print(struct xe_lrc_snapshot *snapshot, struct drm_printer *p)
>> @@ -1900,14 +1893,9 @@ void xe_lrc_snapshot_free(struct xe_lrc_snapshot *snapshot)
>>  		return;
>>
>>  	kvfree(snapshot->lrc_snapshot);
>> -	if (snapshot->lrc_bo) {
>> -		struct xe_vm *vm;
>> -
>> -		vm = snapshot->lrc_bo->vm;
>> +	if (snapshot->lrc_bo)
>>  		xe_bo_put(snapshot->lrc_bo);
>> -		if (vm)
>> -			xe_vm_put(vm);
>> -	}
>> +
>>  	kfree(snapshot);
>>  }
>>
>> --
>> 2.43.0
>>