Re: [PATCH v2 04/15] drm/ttm: Export functions to initialize and finalize the ttm range manager standalone

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, May 18, 2021 at 10:26:50AM +0200, Thomas Hellström wrote:
> i915 mock selftests are run without the device set up. In order to be able
> to run the region related mock selftests, export functions in order for the
> TTM range manager to be set up without a device to attach it to.
> 
> Cc: Christian König <christian.koenig@amd.com>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---
>  drivers/gpu/drm/ttm/ttm_range_manager.c | 55 +++++++++++++++++--------
>  include/drm/ttm/ttm_bo_driver.h         | 23 +++++++++++
>  2 files changed, 61 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ttm/ttm_range_manager.c b/drivers/gpu/drm/ttm/ttm_range_manager.c
> index b9d5da6e6a81..6957dfb0cf5a 100644
> --- a/drivers/gpu/drm/ttm/ttm_range_manager.c
> +++ b/drivers/gpu/drm/ttm/ttm_range_manager.c
> @@ -125,55 +125,76 @@ static const struct ttm_resource_manager_func ttm_range_manager_func = {
>  	.debug = ttm_range_man_debug
>  };
>  
> -int ttm_range_man_init(struct ttm_device *bdev,
> -		       unsigned type, bool use_tt,
> -		       unsigned long p_size)
> +struct ttm_resource_manager *
> +ttm_range_man_init_standalone(unsigned long size, bool use_tt)
>  {
>  	struct ttm_resource_manager *man;
>  	struct ttm_range_manager *rman;
>  
>  	rman = kzalloc(sizeof(*rman), GFP_KERNEL);
>  	if (!rman)
> -		return -ENOMEM;
> +		return ERR_PTR(-ENOMEM);
>  
>  	man = &rman->manager;
>  	man->use_tt = use_tt;
>  
>  	man->func = &ttm_range_manager_func;
>  
> -	ttm_resource_manager_init(man, p_size);
> +	ttm_resource_manager_init(man, size);
>  
> -	drm_mm_init(&rman->mm, 0, p_size);
> +	drm_mm_init(&rman->mm, 0, size);
>  	spin_lock_init(&rman->lock);
>  
> -	ttm_set_driver_manager(bdev, type, &rman->manager);
> +	return man;
> +}
> +EXPORT_SYMBOL(ttm_range_man_init_standalone);
> +
> +int ttm_range_man_init(struct ttm_device *bdev,
> +		       unsigned int type, bool use_tt,
> +		       unsigned long p_size)
> +{
> +	struct ttm_resource_manager *man;
> +
> +	man = ttm_range_man_init_standalone(p_size, use_tt);
> +	if (IS_ERR(man))
> +		return PTR_ERR(man);
> +
>  	ttm_resource_manager_set_used(man, true);
> +	ttm_set_driver_manager(bdev, type, man);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(ttm_range_man_init);
>  
> +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man)
> +{
> +	struct ttm_range_manager *rman = to_range_manager(man);
> +	struct drm_mm *mm = &rman->mm;
> +
> +	spin_lock(&rman->lock);
> +	drm_mm_clean(mm);
> +	drm_mm_takedown(mm);
> +	spin_unlock(&rman->lock);
> +
> +	ttm_resource_manager_cleanup(man);
> +	kfree(rman);
> +}
> +EXPORT_SYMBOL(ttm_range_man_fini_standalone);
> +
>  int ttm_range_man_fini(struct ttm_device *bdev,
>  		       unsigned type)
>  {
>  	struct ttm_resource_manager *man = ttm_manager_type(bdev, type);
> -	struct ttm_range_manager *rman = to_range_manager(man);
> -	struct drm_mm *mm = &rman->mm;
>  	int ret;
>  
>  	ttm_resource_manager_set_used(man, false);
> -
>  	ret = ttm_resource_manager_evict_all(bdev, man);
>  	if (ret)
>  		return ret;
>  
> -	spin_lock(&rman->lock);
> -	drm_mm_clean(mm);
> -	drm_mm_takedown(mm);
> -	spin_unlock(&rman->lock);
> -
> -	ttm_resource_manager_cleanup(man);
>  	ttm_set_driver_manager(bdev, type, NULL);
> -	kfree(rman);
> +	ttm_range_man_fini_standalone(man);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(ttm_range_man_fini);
> diff --git a/include/drm/ttm/ttm_bo_driver.h b/include/drm/ttm/ttm_bo_driver.h
> index dbccac957f8f..734b1712ea72 100644
> --- a/include/drm/ttm/ttm_bo_driver.h
> +++ b/include/drm/ttm/ttm_bo_driver.h
> @@ -321,6 +321,20 @@ int ttm_range_man_init(struct ttm_device *bdev,
>  		       unsigned type, bool use_tt,
>  		       unsigned long p_size);
>  
> +/**
> + * ttm_range_man_init_standalone - Initialize a ttm range manager without
> + * device interaction.
> + * @size: Size of the area to be managed in pages.
> + * @use_tt: The memory type requires tt backing.
> + *
> + * This function is intended for selftests. It initializes a range manager
> + * without any device interaction.
> + *
> + * Return: pointer to a range manager on success. Error pointer on failure.
> + */

Kerneldoc is great and I'm happy you're updating them (Christian's not so
much good for this), but I think would be good to go one step further with
a prep patch:

- Make sure ttm_bo_driver.h is appropriately included in
  Documentation/gpu/drm-mm.rst.

- Fix up any kerneldoc fallout. Specifically I think common usage at least
  is that for non-inline functions, the kerneldoc is in the .c file, not
  in the headers.

But also this might be way too much work since ttm hasn't been properly
kerneldoc-ified, so maybe later.
-Daniel

> +struct ttm_resource_manager *
> +ttm_range_man_init_standalone(unsigned long size, bool use_tt);
> +
>  /**
>   * ttm_range_man_fini
>   *
> @@ -332,4 +346,13 @@ int ttm_range_man_init(struct ttm_device *bdev,
>  int ttm_range_man_fini(struct ttm_device *bdev,
>  		       unsigned type);
>  
> +/**
> + * ttm_range_man_fini_standalone
> + * @man: The range manager
> + *
> + * Tear down a range manager initialized with
> + * ttm_range_manager_init_standalone().
> + */
> +void ttm_range_man_fini_standalone(struct ttm_resource_manager *man);
> +
>  #endif
> -- 
> 2.31.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch