[Intel-xe] [PATCH V2 3/3] drm/xe: Add sysfs entry to report per tile memory size

[Iddamsetty, Aravind]

On 07-06-2023 18:16, Upadhyay, Tejas wrote:
> 
> 
>> -----Original Message-----
>> From: Iddamsetty, Aravind <aravind.iddamsetty at intel.com>
>> Sent: Wednesday, June 7, 2023 5:31 PM
>> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
>> xe at lists.freedesktop.org
>> Subject: Re: [Intel-xe] [PATCH V2 3/3] drm/xe: Add sysfs entry to report per
>> tile memory size
>>
>>
>>
>> On 07-06-2023 15:41, Tejas Upadhyay wrote:
>>> Add sysfs entry to read per tile physical memory including stolen
>>> memory.
>>>
>>> V2:
>>>   - Use DEVICE_ATTR_RO - Aravind
>>>   - Dont put kobj on sysfs_file_create fail - Himal
>>>   - Skip addr_range sysfs create for non dgfx - Himal
>>>
>>> Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
>>> ---
>>>  drivers/gpu/drm/xe/xe_device_types.h |  6 ++++++
>>>  drivers/gpu/drm/xe/xe_mmio.c         |  1 +
>>>  drivers/gpu/drm/xe/xe_tile_sysfs.c   | 29 ++++++++++++++++++++++------
>>>  3 files changed, 30 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h
>>> b/drivers/gpu/drm/xe/xe_device_types.h
>>> index db61da4d6212..b8a07826c65f 100644
>>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>>> @@ -107,6 +107,12 @@ struct xe_tile {
>>>
>>>  	/** @mem: memory management info for tile */
>>>  	struct {
>>> +		/**
>>> +		 * @actual_physical_mem: Track actual LMEM size
>>> +		 * including stolen mem for tile
>>> +		 */
>>> +		resource_size_t actual_physical_mem;
>>> +
>>>  		/**
>>>  		 * @vram: VRAM info for tile.
>>>  		 *
>>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c
>>> b/drivers/gpu/drm/xe/xe_mmio.c index f7a7f996b37f..6febd11d7dd9
>> 100644
>>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>>> @@ -277,6 +277,7 @@ int xe_mmio_probe_vram(struct xe_device *xe)
>>>  		if (err)
>>>  			return err;
>>>
>>> +		tile->mem.actual_physical_mem = tile_size;
>>>  		tile->mem.vram.io_start = xe->mem.vram.io_start +
>> tile_offset;
>>>  		tile->mem.vram.io_size = min_t(u64, vram_size, io_size);
>>>
>>> diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> index 09a4a8342aec..b5768c735cba 100644
>>> --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
>>> @@ -22,6 +22,21 @@ static struct kobj_type xe_tile_sysfs_kobj_type = {
>>>  	.sysfs_ops = &kobj_sysfs_ops,
>>>  };
>>>
>>> +static ssize_t
>>> +addr_range_show(struct device *kdev, struct device_attribute *attr,
>>> +char *buf) {
>>> +	struct xe_tile *tile = kobj_to_tile(&kdev->kobj);
>>> +
>>> +	return sysfs_emit(buf, "%pa\n", &tile->mem.actual_physical_mem); }
>>> +
>>> +static DEVICE_ATTR_RO(addr_range);
>>> +
>>> +static const struct attribute *addr_range_attrs[] = {
>>> +	&dev_attr_addr_range.attr,
>>> +	NULL
>>> +};
>>> +
>>>  static void tile_sysfs_fini(struct drm_device *drm, void *arg)  {
>>>  	struct xe_tile *tile = arg;
>>> @@ -37,10 +52,8 @@ int xe_tile_sysfs_init(struct xe_tile *tile)
>>>  	int err;
>>>
>>>  	kg = kzalloc(sizeof(*kg), GFP_KERNEL);
>>> -	if (!kg) {
>>> -		drm_warn(&xe->drm, "%s failed, err: %d\n", __func__, -
>> ENOMEM);
>>> +	if (!kg)
>>>  		return -ENOMEM;
>>
>> your first patch introduced it and you are fixing here should it be fixed in the
>> first patch itself and why this change as the return from tile_sysfs_init is not
>> being checked.
> 
> yes i am changing it back.....and return was already there in first patch, its just I was trying to add drm log which is not required because underlying memory API already dumps log. I will change it back in first patch. Also I am changing this API to return void and remove all return values. Aligned?
> 
>>
>>> -	}
>>>
>>>  	kobject_init(&kg->base, &xe_tile_sysfs_kobj_type);
>>>  	kg->tile = tile;
>>> @@ -48,16 +61,20 @@ int xe_tile_sysfs_init(struct xe_tile *tile)
>>>  	err = kobject_add(&kg->base, &dev->kobj, "tile%d", tile->id);
>>>  	if (err) {
>>>  		kobject_put(&kg->base);
>>> -		drm_warn(&xe->drm, "failed to register TILE sysfs directory,
>> err: %d\n", err);
>>> +		drm_err(&xe->drm, "failed to register TILE sysfs directory,
>> err:
>>> +%d\n", err);
>>
>> same here, but warn is more appropriate than err.
> 
> Ok, since we are not checking for errors now, warn is fine.
> 
>>
>>>  		return err;
>>>  	}
>>>
>>>  	tile->sysfs = &kg->base;
>>>
>>> +	if (sysfs_create_files(tile->sysfs, addr_range_attrs) &&
>>> +IS_DGFX(xe))
>>
>> this check is not correct if you want to restrict to DGFX, the check should be
>> preceeding the sysfs_create_files, if ( IS_DGFX(xe) && sysfs_create_files())
> 
> Correct, check goes from left to right for logical AND. And I can user sysfs_create_file.

also this is not applicable for DG1, as it doesn't have
XEHP_TILE_ADDR_RANGE register

Thanks,
Aravind.
> 
> Thanks,
> Tejas
>>
>> also you could use sysfs_create_file as you have just one entry.
>>
>> thanks,
>> Aravind.
>>> +		drm_warn(&xe->drm,
>>> +			 "Sysfs creation to read addr_range per tile failed\n");
>>> +
>>>  	err = drmm_add_action_or_reset(&xe->drm, tile_sysfs_fini, tile);
>>>  	if (err) {
>>> -		drm_warn(&xe->drm, "%s: drmm_add_action_or_reset
>> failed, err: %d\n",
>>> -			 __func__, err);
>>> +		drm_err(&xe->drm, "%s: drmm_add_action_or_reset failed,
>> err: %d\n",
>>> +			__func__, err);
>>>  		return err;
>>>  	}
>>>