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

[Upadhyay, Tejas]

> -----Original Message-----
> From: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> Sent: Tuesday, June 6, 2023 6:28 PM
> To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> xe at lists.freedesktop.org
> Subject: RE: [Intel-xe] [PATCH 3/3] drm/xe: Add sysfs entry to report per tile
> memory size
> 
> 
> 
> > -----Original Message-----
> > From: Upadhyay, Tejas <tejas.upadhyay at intel.com>
> > Sent: 06 June 2023 18:06
> > To: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>; intel-
> > xe at lists.freedesktop.org
> > Subject: RE: [Intel-xe] [PATCH 3/3] drm/xe: Add sysfs entry to report
> > per tile memory size
> >
> >
> >
> > > -----Original Message-----
> > > From: Ghimiray, Himal Prasad <himal.prasad.ghimiray at intel.com>
> > > Sent: Tuesday, June 6, 2023 5:07 PM
> > > To: Upadhyay, Tejas <tejas.upadhyay at intel.com>; intel-
> > > xe at lists.freedesktop.org
> > > Subject: RE: [Intel-xe] [PATCH 3/3] drm/xe: Add sysfs entry to
> > > report per tile memory size
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf
> > > > Of Tejas Upadhyay
> > > > Sent: 06 June 2023 16:43
> > > > To: intel-xe at lists.freedesktop.org
> > > > Subject: [Intel-xe] [PATCH 3/3] drm/xe: Add sysfs entry to report
> > > > per tile memory size
> > > >
> > > > Add sysfs entry to read per tile physical memory including stolen
> > memory.
> > > >
> > > > Signed-off-by: Tejas Upadhyay <tejas.upadhyay at intel.com>
> > > > ---
> > > >  drivers/gpu/drm/xe/xe_device_types.h |  5 +++
> > > >  drivers/gpu/drm/xe/xe_mmio.c         |  1 +
> > > >  drivers/gpu/drm/xe/xe_tile_sysfs.c   | 47
> > > > ++++++++++++++++++++++++++++
> > > >  3 files changed, 53 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/xe/xe_device_types.h
> > > > b/drivers/gpu/drm/xe/xe_device_types.h
> > > > index db61da4d6212..7b5443aebd00 100644
> > > > --- a/drivers/gpu/drm/xe/xe_device_types.h
> > > > +++ b/drivers/gpu/drm/xe/xe_device_types.h
> > > > @@ -107,6 +107,11 @@ 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 475b14fe4356..86a7c65c301f
> > > 100644
> > > > --- a/drivers/gpu/drm/xe/xe_mmio.c
> > > > +++ b/drivers/gpu/drm/xe/xe_mmio.c
> > > > @@ -283,6 +283,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 821bd49de277..d48c599e8cc1 100644
> > > > --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > > > +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > > > @@ -22,6 +22,43 @@ static struct kobj_type xe_tile_sysfs_kobj_type = {
> > > >  	.sysfs_ops = &kobj_sysfs_ops,
> > > >  };
> > > >
> > > > +typedef ssize_t (*show)(struct device *dev, struct
> > > > +device_attribute *attr, char *buf);
> > > > +
> > > > +struct xe_ext_attr {
> > > > +	struct device_attribute attr;
> > > > +	show xe_show;
> > > > +};
> > > > +
> > > > +	static ssize_t
> > > > +xe_sysfs_show(struct device *dev, struct device_attribute *attr,
> > > > +char
> > > > +*buf) {
> > > > +	ssize_t value;
> > > > +	struct xe_ext_attr *ea = container_of(attr, struct xe_ext_attr,
> > > > +attr);
> > > > +
> > > > +	value = ea->xe_show(dev, attr, buf);
> > > > +
> > > > +	return value;
> > > > +}
> > > > +
> > > > +	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);
> > > > }
> > > > +
> > > > +#define XE_DEVICE_ATTR_RO(_name, _show) \
> > > > +	struct xe_ext_attr dev_attr_##_name = \ { __ATTR(_name, 0444,
> > > > +xe_sysfs_show, NULL), _show}
> > > > +
> > > > +static XE_DEVICE_ATTR_RO(addr_range, addr_range_show);
> > > > +
> > > > +static const struct attribute *addr_range_attrs[] = {
> > > > +	&dev_attr_addr_range.attr.attr,
> > > > +	NULL
> > > > +};
> > > > +
> > > >  static void tile_sysfs_fini(struct drm_device *drm, void *arg)  {
> > > >  	struct xe_tile *tile = arg;
> > > > @@ -50,9 +87,19 @@ int xe_tile_sysfs_init(struct xe_tile *tile)
> > > >
> > > >  	tile->sysfs = &kg->base;
> > > >
> > > > +	if (sysfs_create_files(tile->sysfs, addr_range_attrs)) {
> > > > +		drm_err(&tile_to_xe(tile)->drm,
> > > > +			"Setting up sysfs to read total physical memory per
> > > > tile failed\n");
> > > > +		goto err_object;
> > > Will do kobject_put and return 0. Which is wrong.
> > > Better to do kobject_put and return err here.
> >
> > I don't think I have any action to do based on error, clean up was
> > already done. Am I missing something here?
> 
> In case of sysfs creation failure, you goto err_object do the cleanup and
> return 0.
> Should you be returning 0 incase of sysfs creation failure ?

I am saying it does not make difference as we don't handle err outside of this API. So does not matter we return err or not.

Thanks,
Tejas
> >
> > Thanks,
> > Tejas
> > > > +	}
> > > > +
> > > >  	err = drmm_add_action_or_reset(&tile_to_xe(tile)->drm,
> > > > tile_sysfs_fini, tile);
> > > >  	if (err)
> > > >  		return err;
> > > >
> > > > +	if (0) {
> > > > +err_object:
> > > > +		kobject_put(&kg->base);
> > > > +	}
> > > >  	return 0;
> > > >  }
> > > > --
> > > > 2.25.1