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

[Upadhyay, Tejas]

> -----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.

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;
> >  	}
> >