Re: [PATCH 4/6] drm/xe: Separate early xe_device initialization

[Gustavo Sousa <gustavo.sousa@intel.com>]

Gustavo Sousa <gustavo.sousa@intel.com> writes:

> Michal Wajdeczko <michal.wajdeczko@intel.com> writes:
>
>> We would like to initialize more of the xe_device struct also from
>> the kunit code, as it should be safe to use most of the generic drm
>> or xe components without doing any additional tweaks. Separate early
>> xe initialization code to a new function, so it can be reused.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_device.c | 39 ++++++++++++++++++++++++----------
>>  drivers/gpu/drm/xe/xe_device.h |  1 +
>>  2 files changed, 29 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
>> index b498147dcf61..7ba407f73a02 100644
>> --- a/drivers/gpu/drm/xe/xe_device.c
>> +++ b/drivers/gpu/drm/xe/xe_device.c
>> @@ -506,27 +506,45 @@ struct xe_device *xe_device_create(struct pci_dev *pdev)
>>  	if (IS_ERR(xe))
>>  		return xe;
>>  
>> +	err = xe_device_init_early(xe);
>> +	if (err)
>> +		return ERR_PTR(err);
>> +
>> +	return xe;
>> +}
>> +ALLOW_ERROR_INJECTION(xe_device_create, ERRNO); /* See xe_pci_probe() */
>> +
>> +/**
>> + * xe_device_init_early() - Initialize a new &xe_device instance
>> + * @xe: the &xe_device to initialize
>> + *
>> + * Return: 0 on success or a negative error code on failure.
>> + */
>> +int xe_device_init_early(struct xe_device *xe)
>> +{
>> +	int err;
>> +
>>  	err = ttm_device_init(&xe->ttm, &xe_ttm_funcs, xe->drm.dev,
>>  			      xe->drm.anon_inode->i_mapping,
>>  			      xe->drm.vma_offset_manager, 0);
>> -	if (WARN_ON(err))
>> -		return ERR_PTR(err);
>> +	if (err)
>> +		return err;
>>  
>>  	xe_bo_dev_init(&xe->bo_device);
>>  	err = drmm_add_action_or_reset(&xe->drm, xe_device_destroy, NULL);
>
> Curious: are the drmm_* calls going to work fine for the automated
> teardown when they are called from a kunit context?
>
> What about devm_* calls? From a quick look, I know that at least
> xe_bo_pinned_init() will make one of those.
>
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>>  	err = xe_shrinker_create(xe);
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>>  	xe->atomic_svm_timeslice_ms = 5;
>>  	xe->min_run_period_lr_ms = 5;
>>  
>>  	err = xe_irq_init(xe);
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>>  	xe_validation_device_init(&xe->val);
>>  
>> @@ -536,7 +554,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev)
>>  
>>  	err = xe_pagemap_shrinker_create(xe);
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>>  	xa_init_flags(&xe->usm.asid_to_vm, XA_FLAGS_ALLOC);
>>  
>> @@ -555,7 +573,7 @@ struct xe_device *xe_device_create(struct pci_dev *pdev)
>>  
>>  	err = xe_bo_pinned_init(xe);
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>>  	xe->preempt_fence_wq = alloc_ordered_workqueue("xe-preempt-fence-wq",
>>  						       WQ_MEM_RECLAIM);
>> @@ -569,16 +587,15 @@ struct xe_device *xe_device_create(struct pci_dev *pdev)
>>  		 * drmm_add_action_or_reset register above
>>  		 */
>>  		drm_err(&xe->drm, "Failed to allocate xe workqueues\n");
>> -		return ERR_PTR(-ENOMEM);
>> +		return -ENOMEM;
>>  	}
>>  
>>  	err = drmm_mutex_init(&xe->drm, &xe->pmt.lock);
>>  	if (err)
>> -		return ERR_PTR(err);
>> +		return err;
>>  
>> -	return xe;
>> +	return 0;
>>  }
>> -ALLOW_ERROR_INJECTION(xe_device_create, ERRNO); /* See xe_pci_probe() */
>
> So, there are things allocated/initialized in xe_device_create() that
> get deallocated/finalized in xe_device_destroy().  With this change, we
> now have things extracted out of xe_device_create() into
> xe_device_init_early(), and it appears that xe_device_destroy() is now
> really the counterpart of the latter than the former.
>
> I think we should just call the function xe_device_init() and then

I guess it could still be called xe_device_init_early() for consistenty
with the naming for the other components of the driver, but I would
still rename xe_device_destroy() to xe_device_fini() to avoid people
mistakenly pairing it with xe_device_create().

Honestly, I think I prefer much more a _init_nohw() suffix than
_init_early(); that way we can pair with _fini_nohw() (as _fini_early()
would be a bit innacurate or ambiguous IMO).  But that's a bit
off-topic...

--
Gustavo Sousa

> rename xe_device_destroy() to xe_device_fini().
>
>>  
>>  static bool xe_driver_flr_disabled(struct xe_device *xe)
>>  {
>> diff --git a/drivers/gpu/drm/xe/xe_device.h b/drivers/gpu/drm/xe/xe_device.h
>> index 27cd2329b99f..975768a6a9c8 100644
>> --- a/drivers/gpu/drm/xe/xe_device.h
>> +++ b/drivers/gpu/drm/xe/xe_device.h
>> @@ -44,6 +44,7 @@ static inline struct xe_device *ttm_to_xe_device(struct ttm_device *ttm)
>>  }
>>  
>>  struct xe_device *xe_device_create(struct pci_dev *pdev);
>> +int xe_device_init_early(struct xe_device *xe);
>
> Right now we do not have any external users for this function.  We could
> keep it static until we get to use them in kunit.
>
> That said, I think it would also make sense to separate it completely
> from xe_device_create(), i.e., have xe_pci.c call the two functions
> instead of xe_device_init_early() being implicitly called by
> xe_device_create().  I think that would be more alligned with the
> function names.
>
> --
> Gustavo Sousa
>
>>  int xe_device_probe_early(struct xe_device *xe);
>>  int xe_device_probe(struct xe_device *xe);
>>  void xe_device_remove(struct xe_device *xe);
>> -- 
>> 2.47.1