Re: [PATCH v2 1/1] drm/panfrost: Replace fdinfo's profiling debugfs knob with sysfs

[Boris Brezillon <boris.brezillon@collabora.com>]

On Mon, 4 Mar 2024 16:04:34 +0000
Steven Price <steven.price@arm.com> 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@collabora.com>  
> 
> I'm happy with this.
> 
> Reviewed-by: Steven Price <steven.price@arm.com>
> 
> 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;
> > +}