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

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 08 Jan 2024, Thomas Hellström <thomas.hellstrom@linux.intel.com> wrote:
> On Fri, 2024-01-05 at 10:54 +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)
>> 
>> v3: Move mem_type_to_name() to xe_bo.[ch] (Thomas, Matt)
>> 
>> 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         | 11 +++++++++--
>>  drivers/gpu/drm/xe/xe_bo.h         |  1 +
>>  drivers/gpu/drm/xe/xe_drm_client.c |  8 --------
>>  drivers/gpu/drm/xe/xe_trace.h      | 24 +++++++++++++++++++++---
>>  4 files changed, 31 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
>> index 8e4a3b1f6b93..1154d8f7b3b1 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.c
>> +++ b/drivers/gpu/drm/xe/xe_bo.c
>> @@ -28,6 +28,14 @@
>>  #include "xe_ttm_stolen_mgr.h"
>>  #include "xe_vm.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"
>> +};
>> +
>>  static const struct ttm_place sys_placement_flags = {
>>  	.fpfn = 0,
>>  	.lpfn = 0,
>> @@ -713,8 +721,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_bo.h b/drivers/gpu/drm/xe/xe_bo.h
>> index 97b32528c600..bf77bf17e519 100644
>> --- a/drivers/gpu/drm/xe/xe_bo.h
>> +++ b/drivers/gpu/drm/xe/xe_bo.h
>> @@ -244,6 +244,7 @@ int xe_bo_evict_pinned(struct xe_bo *bo);
>>  int xe_bo_restore_pinned(struct xe_bo *bo);
>>  
>>  extern struct ttm_device_funcs xe_ttm_funcs;
>> +extern const char *const mem_type_to_name[];
>
> NIT: We should name this xe_mem_type_to_name[] now that it's extern.
>
> With that,
> Reviewed-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

FWIW, in i915 land I would've required converting that to a proper
function, with bounds checks etc.

Data is not an interface, and all that.

BR,
Jani.

>
>
>>  
>>  int xe_gem_create_ioctl(struct drm_device *dev, void *data,
>>  			struct drm_file *file);
>> diff --git a/drivers/gpu/drm/xe/xe_drm_client.c
>> b/drivers/gpu/drm/xe/xe_drm_client.c
>> index 82d1305e831f..1ac14c910491 100644
>> --- a/drivers/gpu/drm/xe/xe_drm_client.c
>> +++ b/drivers/gpu/drm/xe/xe_drm_client.c
>> @@ -131,14 +131,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_trace.h
>> b/drivers/gpu/drm/xe/xe_trace.h
>> index 95163c303f3e..c66a91df7d8b 100644
>> --- a/drivers/gpu/drm/xe/xe_trace.h
>> +++ b/drivers/gpu/drm/xe/xe_trace.h
>> @@ -12,6 +12,7 @@
>>  #include <linux/tracepoint.h>
>>  #include <linux/types.h>
>>  
>> +#include "xe_bo.h"
>>  #include "xe_bo_types.h"
>>  #include "xe_exec_queue_types.h"
>>  #include "xe_gpu_scheduler_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,
>

-- 
Jani Nikula, Intel