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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Jul 11, 2024 at 10:09:01PM +0200, Michal Wajdeczko wrote:
> 
> 
> 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" ;)

it is actually used in one of the cases after this print...
string_get_size(total, 1, STRING_UNITS_2, buf, sizeof(buf));

perhaps print is a bad name then... but I'm out of ideas...

> 
> > +
> > +	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() ?

hmm... likely, but this needs to calculate the whole...
but we could perhaps

s/xe_ggtt_print/xe_ggtt_get_holes_total
s/xe_ggtt_dump/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));