Re: [Intel-xe] [PATCH 2/2] drm/xe: Add vram frequency sysfs attributes

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 27 Nov 2023 02:00:10 -0800, Riana Tauro wrote:
>
> Hi Suja
>
> There is an error on module unload.
>
> [ 1042.525614] ------------[ cut here ]------------
> [ 1042.525622] kernfs: can not remove 'physical_vram_size_bytes', no
> directory
> [ 1042.525641] WARNING: CPU: 3 PID: 2234 at fs/kernfs/dir.c:1662
> kernfs_remove_by_name_ns+0xb3/0xc0
> [ 1042.525965] Call Trace:
> [ 1042.525970]  <TASK>
> [ 1042.525976]  ? __warn+0xa5/0x200
> [ 1042.525986]  ? kernfs_remove_by_name_ns+0xb3/0xc0
> [ 1042.525996]  ? report_bug+0x216/0x220
> [ 1042.526011]  ? handle_bug+0x3c/0x70
> [ 1042.526018]  ? exc_invalid_op+0x18/0x50
> [ 1042.526025]  ? asm_exc_invalid_op+0x1a/0x20
> [ 1042.526033]  ? __pfx_tile_sysfs_fini+0x10/0x10 [xe]
> [ 1042.526275]  ? irq_work_claim+0x1e/0x40
> [ 1042.526288]  ? kernfs_remove_by_name_ns+0xb3/0xc0
> [ 1042.526298]  ? kernfs_remove_by_name_ns+0xb3/0xc0
> [ 1042.526310]  tile_sysfs_fini+0x1e/0x40 [xe]
> [ 1042.526515]  drm_managed_release+0x117/0x250 [drm]
> [ 1042.526626]  drm_dev_release+0x49/0x60 [drm]
> [ 1042.526723]  release_nodes+0x59/0x190
> [ 1042.526731]  ? lockdep_hardirqs_on_prepare+0x136/0x210
> [ 1042.526739]  ? _raw_spin_unlock_irqrestore+0x51/0x70
> [ 1042.526752]  devres_release_all+0xf8/0x140
> [ 1042.526761]  ? __pfx_devres_release_all+0x10/0x10
> [ 1042.526779]  device_unbind_cleanup+0x16/0xc0
> [ 1042.526788]  device_release_driver_internal+0x10d/0x160
> [ 1042.526799]  unbind_store+0x98/0xa0
> [ 1042.526809]  ? __pfx_sysfs_kf_write+0x10/0x10
> [ 1042.526815]  kernfs_fop_write_iter+0x1bc/0x260
> [ 1042.526828]  vfs_write+0x553/0x770
> [ 1042.526839]  ? __pfx_vfs_write+0x10/0x10
> [ 1042.526850]  ? do_sys_openat2+0x266/0x350
> [ 1042.526867]  ? __fget_light+0x9e/0x100
> [ 1042.526882]  ksys_write+0xc7/0x170
> [ 1042.526889]  ? __pfx_ksys_write+0x10/0x10
> [ 1042.526895]  ? mark_held_locks+0x24/0x90
> [ 1042.526906]  ? lockdep_hardirqs_on_prepare+0x136/0x210
> [ 1042.526920]  do_syscall_64+0x3c/0x90
> [ 1042.526929]  entry_SYSCALL_64_after_hwframe+0x6e/0xd8

Thanks Riana.

Hi Suja,

Let's follow the steps below to test these sysfs patches before posting
them:
* Make sure sysfs entries appear in the correct directory
* Make sure read/write works as expected (using cat/echo)
* After doing these things please unload the driver and make sure the
  driver can be unloaded without crashing. Then reload the driver and test
  again.

Check dmesg after each of these steps (leave 'dmesg -w' running in a
separate console window) to see if there's any sign of crash or kernel
oops.

Thanks,
Ashutosh



>
>
> On 11/24/2023 5:57 PM, Sujaritha Sundaresan wrote:
> > Add vram frequency sysfs attributes under the below hierarchy;
> >
> > /device/tile<n>/memory/freq
> >			|-vram_rp0_freq
> >			|-vram_rpn_freq
> >
> > Signed-off-by: Sujaritha Sundaresan <sujaritha.sundaresan@intel.com>
> > ---
> >   drivers/gpu/drm/xe/xe_pcode_api.h  |  8 +++
> >   drivers/gpu/drm/xe/xe_tile_sysfs.c | 78 +++++++++++++++++++++++++++++-
> >   2 files changed, 84 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_pcode_api.h b/drivers/gpu/drm/xe/xe_pcode_api.h
> > index 5935cfe30204..edde5335bdb1 100644
> > --- a/drivers/gpu/drm/xe/xe_pcode_api.h
> > +++ b/drivers/gpu/drm/xe/xe_pcode_api.h
> > @@ -42,6 +42,14 @@
> >   #define	    POWER_SETUP_I1_SHIFT		6	/* 10.6 fixed point format */
> >   #define	    POWER_SETUP_I1_DATA_MASK		REG_GENMASK(15, 0)
> >   +#define   XEHP_PCODE_FREQUENCY_CONFIG                  0x6e    /*
> > xehp, pvc */
> > +/* XEHP_PCODE_FREQUENCY_CONFIG sub-commands (param1) */
> > +#define     PCODE_MBOX_FC_SC_READ_FUSED_P0             0x0
> > +#define     PCODE_MBOX_FC_SC_READ_FUSED_PN             0x1
> > +/* PCODE_MBOX_DOMAIN_* - mailbox domain IDs */
> > +/* XEHP_PCODE_FREQUENCY_CONFIG param2 */
> > +#define     PCODE_MBOX_DOMAIN_HBM                      0x2
> > +
> >   struct pcode_err_decode {
> >	int errno;
> >	const char *str;
> > diff --git a/drivers/gpu/drm/xe/xe_tile_sysfs.c b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > index f354c8b2bfc6..ddf5072c40eb 100644
> > --- a/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > +++ b/drivers/gpu/drm/xe/xe_tile_sysfs.c
> > @@ -7,9 +7,14 @@
> >   #include <linux/sysfs.h>
> >   #include <drm/drm_managed.h>
> >   +#include "xe_gt_types.h"
> > +#include "xe_pcode.h"
> > +#include "xe_pcode_api.h"
> >   #include "xe_tile.h"
> >   #include "xe_tile_sysfs.h"
> >   +#define GT_FREQUENCY_MULTIPLIER        50
> > +
> >   static void xe_tile_sysfs_kobj_release(struct kobject *kobj)
> >   {
> >	kfree(kobj);
> > @@ -35,11 +40,72 @@ static DEVICE_ATTR_RO(physical_vram_size_bytes);
> >   static const struct attribute *physical_memsize_attr =
> >	&dev_attr_physical_vram_size_bytes.attr;
> >   +static ssize_t vram_rp0_freq_show(struct device *kdev, struct
> > device_attribute *attr,
> > +				  char *buf)
> > +{
> > +	struct kobject *kobj = &kdev->kobj;
> > +	struct xe_tile *tile = kobj_to_tile(kobj->parent);
> > +	struct xe_gt *gt = tile->primary_gt;
> > +	u32 val, mbox;
> > +	int err;
> > +
> > +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, XEHP_PCODE_FREQUENCY_CONFIG)
> > +		| REG_FIELD_PREP(PCODE_MB_PARAM1, PCODE_MBOX_FC_SC_READ_FUSED_P0)
> > +		| REG_FIELD_PREP(PCODE_MB_PARAM2, PCODE_MBOX_DOMAIN_HBM);
> > +
> > +	err = xe_pcode_read(gt, mbox, &val, NULL);
> > +	if (err)
> > +		return err;
> > +
> > +	/* data_out - Fused P0 for domain ID in units of 50 MHz */
> > +	val *= GT_FREQUENCY_MULTIPLIER;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);
> > +}
> > +static DEVICE_ATTR_RO(vram_rp0_freq);
> > +
> > +static ssize_t vram_rpn_freq_show(struct device *kdev, struct device_attribute *attr,
> > +				  char *buf)
> > +{
> > +	struct kobject *kobj = &kdev->kobj;
> > +	struct xe_tile *tile = kobj_to_tile(kobj->parent);
> > +	struct xe_gt *gt = tile->primary_gt;
> > +	u32 val, mbox;
> > +	int err;
> > +
> > +	mbox = REG_FIELD_PREP(PCODE_MB_COMMAND, XEHP_PCODE_FREQUENCY_CONFIG)
> > +		| REG_FIELD_PREP(PCODE_MB_PARAM1, PCODE_MBOX_FC_SC_READ_FUSED_PN)
> > +		| REG_FIELD_PREP(PCODE_MB_PARAM2, PCODE_MBOX_DOMAIN_HBM);
> > +
> > +	err = xe_pcode_read(gt, mbox, &val, NULL);
> > +	if (err)
> > +		return err;
> > +
> > +	/* data_out - Fused Pn for domain ID in units of 50 MHz */
> > +	val *= GT_FREQUENCY_MULTIPLIER;
> > +
> > +	return sysfs_emit(buf, "%u\n", val);
> > +}
> > +static DEVICE_ATTR_RO(vram_rpn_freq);
> > +
> > +static struct attribute *vram_freq_attrs[] = {
> > +	&dev_attr_vram_rp0_freq.attr,
> > +	&dev_attr_vram_rpn_freq.attr,
> > +	NULL
> > +};
> > +
> > +static const struct attribute_group freq_group_attrs = {
> > +	.name = "freq",
> > +	.attrs = vram_freq_attrs,
> > +};
> > +
> >   static void tile_sysfs_fini(struct drm_device *drm, void *arg)
> >   {
> > -	struct xe_tile *tile = arg;
> > +	struct kobject *kobj = arg;
> >   -	kobject_put(tile->sysfs);
> > +	sysfs_remove_file(kobj, physical_memsize_attr);
> > +	sysfs_remove_group(kobj, &freq_group_attrs);
> > +	kobject_put(kobj);
> >   }
> >     void xe_tile_sysfs_init(struct xe_tile *tile)
> > @@ -77,6 +143,14 @@ void xe_tile_sysfs_init(struct xe_tile *tile)
> >		drm_warn(&xe->drm,
> >			 "Sysfs creation to read addr_range per tile failed\n");
> >   +	if (xe->info.platform == XE_PVC) {
> > +	err = sysfs_create_group(kobj, &freq_group_attrs);
> indentation
> Also we are using two different methods of creating subdir in a single
> file. Should be uniform
>
> Thanks
> Riana
> > +		if (err) {
> > +			drm_warn(&xe->drm, "failed to register vram freq sysfs, err: %d\n", err);
> > +			return;
> > +		}
> > +	}
> > +
> >	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",