Re: [PATCH 09/12] drm/xe: Introduce xe_ggtt_print_holes

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 11.07.2024 19:11, Rodrigo Vivi wrote:
> Introduce a new xe_ggtt_print_holes helper that attends the SRIOV
> demand and finishes the goal of limiting drm_mm access to xe_ggtt.
> 
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_ggtt.c               | 40 ++++++++++++++++++++++
>  drivers/gpu/drm/xe/xe_ggtt.h               |  1 +
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 25 +-------------
>  3 files changed, 42 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.c b/drivers/gpu/drm/xe/xe_ggtt.c
> index dbaf1ce87fb4..5b51753fe1ba 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.c
> +++ b/drivers/gpu/drm/xe/xe_ggtt.c
> @@ -682,3 +682,43 @@ int xe_ggtt_dump(struct xe_ggtt *ggtt, struct drm_printer *p)
>  	mutex_unlock(&ggtt->lock);
>  	return err;
>  }
> +
> +/**
> + * xe_ggtt_print_holes - Print holes
> + * @ggtt: the &xe_ggtt to be inspected
> + * @alignment: min alignment
> + * @p: the &drm_printer
> + *
> + * Print GGTT ranges that are available and return total size available.
> + *
> + * Return: Total available size.

I would rather prefer to lost that info than make the 'print' function
return some real value (which may be immediately stale as it will be out
of mutex)

> + */
> +u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer *p)
> +{
> +	const struct drm_mm *mm = &ggtt->mm;
> +	const struct drm_mm_node *entry;
> +	u64 hole_min_start = xe_wopcm_size(tile_to_xe(ggtt->tile));
> +	u64 hole_start, hole_end, hole_size;
> +	u64 total;
> +	char buf[10];
> +
> +	mutex_lock(&ggtt->lock);
> +
> +	drm_mm_for_each_hole(entry, mm, hole_start, hole_end) {
> +		hole_start = max(hole_start, hole_min_start);
> +		hole_start = ALIGN(hole_start, alignment);
> +		hole_end = ALIGN_DOWN(hole_end, alignment);
> +		if (hole_start >= hole_end)
> +			continue;
> +		hole_size = hole_end - hole_start;
> +		total += hole_size;
> +
> +		string_get_size(hole_size, 1, STRING_UNITS_2, buf, sizeof(buf));
> +		drm_printf(p, "range:\t%#llx-%#llx\t(%s)\n",
> +			   hole_start, hole_end - 1, buf);
> +	}

or we can print that 'total' here, as it is still kind of related since
it is "sum of all holes" ;)

> +
> +	mutex_unlock(&ggtt->lock);
> +
> +	return total;
> +}
> diff --git a/drivers/gpu/drm/xe/xe_ggtt.h b/drivers/gpu/drm/xe/xe_ggtt.h
> index 31060fe7644b..67ae5f1602a3 100644
> --- a/drivers/gpu/drm/xe/xe_ggtt.h
> +++ b/drivers/gpu/drm/xe/xe_ggtt.h
> @@ -32,6 +32,7 @@ void xe_ggtt_remove_bo(struct xe_ggtt *ggtt, struct xe_bo *bo);
>  u64 xe_ggtt_largest_hole(struct xe_ggtt *ggtt, u64 alignment, u64 *spare);
>  
>  int xe_ggtt_dump(struct xe_ggtt *ggtt, struct drm_printer *p);

hmm, maybe xe_ggtt_dump() from prev patch should be xe_ggtt_print() ?

> +u64 xe_ggtt_print_holes(struct xe_ggtt *ggtt, u64 alignment, struct drm_printer *p);
>  
>  #ifdef CONFIG_PCI_IOV
>  void xe_ggtt_assign(struct xe_ggtt *ggtt, const struct xe_ggtt_node *node, u16 vfid);
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index 1d17c34fe5a4..d0995680f53f 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -6,9 +6,6 @@
>  #include <linux/string_choices.h>
>  #include <linux/wordpart.h>
>  
> -/* FIXME: remove this after encapsulating all drm_mm_node access into xe_ggtt */
> -#include <drm/drm_mm.h>
> -
>  #include "abi/guc_actions_sriov_abi.h"
>  #include "abi/guc_klvs_abi.h"
>  
> @@ -2096,11 +2093,7 @@ int xe_gt_sriov_pf_config_print_dbs(struct xe_gt *gt, struct drm_printer *p)
>  int xe_gt_sriov_pf_config_print_available_ggtt(struct xe_gt *gt, struct drm_printer *p)
>  {
>  	struct xe_ggtt *ggtt = gt_to_tile(gt)->mem.ggtt;
> -	const struct drm_mm *mm = &ggtt->mm;
> -	const struct drm_mm_node *entry;
>  	u64 alignment = pf_get_ggtt_alignment(gt);
> -	u64 hole_min_start = xe_wopcm_size(gt_to_xe(gt));
> -	u64 hole_start, hole_end, hole_size;
>  	u64 spare, avail, total = 0;
>  	char buf[10];
>  
> @@ -2109,24 +2102,8 @@ int xe_gt_sriov_pf_config_print_available_ggtt(struct xe_gt *gt, struct drm_prin
>  	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
>  
>  	spare = pf_get_spare_ggtt(gt);
> +	total = xe_ggtt_print_holes(ggtt, alignment, p);
>  
> -	mutex_lock(&ggtt->lock);
> -
> -	drm_mm_for_each_hole(entry, mm, hole_start, hole_end) {
> -		hole_start = max(hole_start, hole_min_start);
> -		hole_start = ALIGN(hole_start, alignment);
> -		hole_end = ALIGN_DOWN(hole_end, alignment);
> -		if (hole_start >= hole_end)
> -			continue;
> -		hole_size = hole_end - hole_start;
> -		total += hole_size;
> -
> -		string_get_size(hole_size, 1, STRING_UNITS_2, buf, sizeof(buf));
> -		drm_printf(p, "range:\t%#llx-%#llx\t(%s)\n",
> -			   hole_start, hole_end - 1, buf);
> -	}
> -
> -	mutex_unlock(&ggtt->lock);
>  	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
>  
>  	string_get_size(total, 1, STRING_UNITS_2, buf, sizeof(buf));