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