Re: [PATCH] xe/xe_bo_move: Enhance xe_bo_move trace

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Jan 03, 2024 at 02:19:52PM +0000, Dandamudi, Priyanka wrote:
> 
> 
> > -----Original Message-----
> > From: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > Sent: Wednesday, January 3, 2024 6:52 PM
> > To: Dandamudi, Priyanka <priyanka.dandamudi@intel.com>; intel-
> > xe@lists.freedesktop.org; Kempczynski, Zbigniew
> > <zbigniew.kempczynski@intel.com>; Zeng, Oak <oak.zeng@intel.com>; Welty,
> > Brian <brian.welty@intel.com>
> > Subject: Re: [PATCH] xe/xe_bo_move: Enhance xe_bo_move trace
> > 
> > Hi, Priyanka,
> > 
> > On Wed, 2024-01-03 at 18:08 +0530, priyanka.dandamudi@intel.com wrote:
> > > From: Priyanka Dandamudi <priyanka.dandamudi@intel.com>
> > >
> > > Enhanced xe_bo_move trace to be more readable.
> > > It will help to show the migration details.
> > > Src and dst details.
> > >
> > > v2: Modify trace_xe_bo_move(), it takes the integer mem_type rather
> > > than a string.
> > > Make mem_type_to_name() extern, it will be used by trace.(Thomas)
> > >
> > > Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> > > Cc: Oak Zeng <oak.zeng@intel.com>
> > > Cc: Kempczynski Zbigniew <Zbigniew.Kempczynski@intel.com>
> > > Cc: Matthew Brost <matthew.brost@intel.com>
> > > Cc: Brian Welty <brian.welty@intel.com>
> > > Signed-off-by: Priyanka Dandamudi <priyanka.dandamudi@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_bo.c         |  3 +--
> > >  drivers/gpu/drm/xe/xe_drm_client.c | 16 ++++++++--------
> > >  drivers/gpu/drm/xe/xe_drm_client.h |  2 ++
> > >  drivers/gpu/drm/xe/xe_trace.h      | 24 +++++++++++++++++++++---
> > >  4 files changed, 32 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> > > index 8e4a3b1f6b93..f7075fd86547 100644
> > > --- a/drivers/gpu/drm/xe/xe_bo.c
> > > +++ b/drivers/gpu/drm/xe/xe_bo.c
> > > @@ -713,8 +713,7 @@ static int xe_bo_move(struct ttm_buffer_object
> > > *ttm_bo, bool evict,
> > >  		migrate = xe->tiles[0].migrate;
> > >
> > >  	xe_assert(xe, migrate);
> > > -
> > > -	trace_xe_bo_move(bo);
> > > +	trace_xe_bo_move(bo, new_mem->mem_type, old_mem_type);
> > >  	xe_device_mem_access_get(xe);
> > >
> > >  	if (xe_bo_is_pinned(bo) && !xe_bo_is_user(bo)) { diff --git
> > > a/drivers/gpu/drm/xe/xe_drm_client.c
> > > b/drivers/gpu/drm/xe/xe_drm_client.c
> > > index 82d1305e831f..e845b7f22e38 100644
> > > --- a/drivers/gpu/drm/xe/xe_drm_client.c
> > > +++ b/drivers/gpu/drm/xe/xe_drm_client.c
> > > @@ -15,6 +15,14 @@
> > >  #include "xe_drm_client.h"
> > >  #include "xe_trace.h"
> > >
> > > +const char *const mem_type_to_name[TTM_NUM_MEM_TYPES]  = {
> > > +	[XE_PL_SYSTEM] = "system",
> > > +	[XE_PL_TT] = "gtt",
> > > +	[XE_PL_VRAM0] = "vram0",
> > > +	[XE_PL_VRAM1] = "vram1",
> > > +	[XE_PL_STOLEN] = "stolen"
> > > +};
> > > +
> > 
> > Please use xe_bo.[ch] for this, since it is related to the buffer object
> > placements, and the Xe placements are defined in xe_bo.h
> > 
> > /Thomas
> > 
> The above is already declared in this program in show_meminfo(). I just moved of it to make it global.
> Is it required again to define it in xe_bo.[ch]?
> --Priyanka

The change to trace looks good to me. I think Thomas is saying move
mem_type_to_name to xe_bo.c and extern to xe_bo.h.

Matt

> > 
> > >  /**
> > >   * xe_drm_client_alloc() - Allocate drm client
> > >   * @void: No arg
> > > @@ -131,14 +139,6 @@ static void bo_meminfo(struct xe_bo *bo,
> > >
> > >  static void show_meminfo(struct drm_printer *p, struct drm_file
> > > *file)
> > >  {
> > > -	static const char *const
> > > mem_type_to_name[TTM_NUM_MEM_TYPES]  = {
> > > -		[XE_PL_SYSTEM] = "system",
> > > -		[XE_PL_TT] = "gtt",
> > > -		[XE_PL_VRAM0] = "vram0",
> > > -		[XE_PL_VRAM1] = "vram1",
> > > -		[4 ... 6] = NULL,
> > > -		[XE_PL_STOLEN] = "stolen"
> > > -	};
> > >  	struct drm_memory_stats stats[TTM_NUM_MEM_TYPES] = {};
> > >  	struct xe_file *xef = file->driver_priv;
> > >  	struct ttm_device *bdev = &xef->xe->ttm; diff --git
> > > a/drivers/gpu/drm/xe/xe_drm_client.h
> > > b/drivers/gpu/drm/xe/xe_drm_client.h
> > > index a9649aa36011..4e33d0981b6a 100644
> > > --- a/drivers/gpu/drm/xe/xe_drm_client.h
> > > +++ b/drivers/gpu/drm/xe/xe_drm_client.h
> > > @@ -13,6 +13,8 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/spinlock.h>
> > >
> > > +extern const char *const mem_type_to_name[];
> > > +
> > >  struct drm_file;
> > >  struct drm_printer;
> > >  struct xe_bo;
> > > diff --git a/drivers/gpu/drm/xe/xe_trace.h
> > > b/drivers/gpu/drm/xe/xe_trace.h index 95163c303f3e..196d277795a0
> > > 100644
> > > --- a/drivers/gpu/drm/xe/xe_trace.h
> > > +++ b/drivers/gpu/drm/xe/xe_trace.h
> > > @@ -13,6 +13,7 @@
> > >  #include <linux/types.h>
> > >
> > >  #include "xe_bo_types.h"
> > > +#include "xe_drm_client.h"
> > >  #include "xe_exec_queue_types.h"
> > >  #include "xe_gpu_scheduler_types.h"
> > >  #include "xe_gt_tlb_invalidation_types.h"
> > > @@ -100,9 +101,26 @@ DEFINE_EVENT(xe_bo, xe_bo_cpu_fault,
> > >  	     TP_ARGS(bo)
> > >  );
> > >
> > > -DEFINE_EVENT(xe_bo, xe_bo_move,
> > > -	     TP_PROTO(struct xe_bo *bo),
> > > -	     TP_ARGS(bo)
> > > +TRACE_EVENT(xe_bo_move,
> > > +	    TP_PROTO(struct xe_bo *bo, uint32_t new_placement,
> > > uint32_t old_placement),
> > > +	    TP_ARGS(bo, new_placement, old_placement),
> > > +	    TP_STRUCT__entry(
> > > +		     __field(struct xe_bo *, bo)
> > > +		     __field(size_t, size)
> > > +		     __field(u32, new_placement)
> > > +		     __field(u32, old_placement)
> > > +			),
> > > +
> > > +	    TP_fast_assign(
> > > +		   __entry->bo      = bo;
> > > +		   __entry->size = bo->size;
> > > +		   __entry->new_placement = new_placement;
> > > +		   __entry->old_placement = old_placement;
> > > +
> > > +		   ),
> > > +	    TP_printk("migrate object %p [size %zu] from %s to %s",
> > > +		      __entry->bo, __entry->size,
> > > mem_type_to_name[__entry->old_placement],
> > > +		      mem_type_to_name[__entry->new_placement])
> > >  );
> > >
> > >  DECLARE_EVENT_CLASS(xe_exec_queue,
>