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

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

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
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