On Mon, 4 Mar 2024 16:04:34 +0000 Steven Price <steven.price@xxxxxxx> wrote: > On 02/03/2024 15:48, Adrián Larumbe wrote: > > Debugfs isn't always available in production builds that try to squeeze > > every single byte out of the kernel image, but we still need a way to > > toggle the timestamp and cycle counter registers so that jobs can be > > profiled for fdinfo's drm engine and cycle calculations. > > > > Drop the debugfs knob and replace it with a sysfs file that accomplishes > > the same functionality, and document its ABI in a separate file. > > > > Signed-off-by: Adrián Larumbe <adrian.larumbe@xxxxxxxxxxxxx> > > I'm happy with this. > > Reviewed-by: Steven Price <steven.price@xxxxxxx> > > Boris: are you happy with the sysfs ABI, or would you like to > investigate further the implications of leaving the counters enabled all > the time during execution before committing to the sysfs ABI? No, that's fine, but I have a few comments on the implementation. > > +static ssize_t > > +profiling_show(struct kobject *kobj, struct kobj_attribute *attr, char *buf) > > +{ > > + bool *profile_mode = > > + &container_of(kobj, struct panfrost_device, > > + profiling.base)->profiling.profile_mode; > > + > > + return sysfs_emit(buf, "%d\n", *profile_mode); > > +} > > + > > +static ssize_t > > +profiling_store(struct kobject *kobj, struct kobj_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + bool *profile_mode = > > + &container_of(kobj, struct panfrost_device, > > + profiling.base)->profiling.profile_mode; > > + int err, value; > > + > > + err = kstrtoint(buf, 0, &value); I'd suggest using kstrtobool() since you make the result a boolean anyway. > > + if (err) > > + return err; > > + > > + *profile_mode = !!value; > > + > > + return count; > > +} > > + > > +static const struct kobj_attribute profiling_status = > > +__ATTR(status, 0644, profiling_show, profiling_store); > > + > > +static const struct kobj_type kobj_profile_type = { > > + .sysfs_ops = &kobj_sysfs_ops, > > +}; DEVICE_ATTR(profiling, 0644, profiling_show, profiling_store); ? > > + > > +int panfrost_sysfs_init(struct panfrost_device *pfdev) > > +{ > > + struct device *kdev = pfdev->ddev->primary->kdev; > > + int err; > > + > > + kobject_init(&pfdev->profiling.base, &kobj_profile_type); > > + > > + err = kobject_add(&pfdev->profiling.base, &kdev->kobj, "%s", "profiling"); Can we make it a device attribute instead of adding an extra kboj? > > + if (err) > > + return err; > > + > > + err = sysfs_create_file(&pfdev->profiling.base, &profiling_status.attr); > > + if (err) > > + kobject_del(&pfdev->profiling.base); > > + > > + return err; > > +}