Received-SPF: pass (google.com: domain of linux-kernel+bounces-90976-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Mon, 4 Mar 2024 18:03:50 +0100
From: Boris Brezillon <boris.brezillon@collabora.com>
To: Steven Price <steven.price@arm.com>
Cc: =?UTF-8?B?QWRyacOhbg==?= Larumbe <adrian.larumbe@collabora.com>, Rob
 Herring <robh@kernel.org>, David Airlie <airlied@gmail.com>, Daniel Vetter
 <daniel@ffwll.ch>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann
 <tzimmermann@suse.de>, Jonathan Corbet <corbet@lwn.net>,
 linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
 linux-doc@vger.kernel.org
Subject: Re: [PATCH v2 1/1] drm/panfrost: Replace fdinfo's profiling debugfs
 knob with sysfs
Message-ID: <20240304180350.74e7e385@collabora.com>
In-Reply-To: <51167b19-5a2c-4749-8b8c-b2a0e6050a33@arm.com>
References: <20240302154845.3223223-2-adrian.larumbe@collabora.com>
	<20240302154845.3223223-3-adrian.larumbe@collabora.com>
	<51167b19-5a2c-4749-8b8c-b2a0e6050a33@arm.com>
Organization: Collabora
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Mon, 4 Mar 2024 16:04:34 +0000
Steven Price <steven.price@arm.com> wrote:

> On 02/03/2024 15:48, Adri=C3=A1n 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.
> >=20
> > Drop the debugfs knob and replace it with a sysfs file that accomplishes
> > the same functionality, and document its ABI in a separate file.
> >=20
> > Signed-off-by: Adri=C3=A1n Larumbe <adrian.larumbe@collabora.com> =20
>=20
> I'm happy with this.
>=20
> Reviewed-by: Steven Price <steven.price@arm.com>
>=20
> 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 =3D
> > +		&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 =3D
> > +		&container_of(kobj, struct panfrost_device,
> > +			      profiling.base)->profiling.profile_mode;
> > +	int err, value;
> > +
> > +	err =3D kstrtoint(buf, 0, &value);

I'd suggest using kstrtobool() since you make the result a boolean
anyway.

> > +	if (err)
> > +		return err;
> > +
> > +	*profile_mode =3D !!value;
> > +
> > +	return count;
> > +}
> > +
> > +static const struct kobj_attribute profiling_status =3D
> > +__ATTR(status, 0644, profiling_show, profiling_store);
> > +
> > +static const struct kobj_type kobj_profile_type =3D {
> > +	.sysfs_ops =3D &kobj_sysfs_ops,
> > +};

DEVICE_ATTR(profiling, 0644, profiling_show, profiling_store);

?

> > +
> > +int panfrost_sysfs_init(struct panfrost_device *pfdev)
> > +{
> > +	struct device *kdev =3D pfdev->ddev->primary->kdev;
> > +	int err;
> > +
> > +	kobject_init(&pfdev->profiling.base, &kobj_profile_type);
> > +
> > +	err =3D kobject_add(&pfdev->profiling.base, &kdev->kobj, "%s", "profi=
ling");

Can we make it a device attribute instead of adding an extra kboj?

> > +	if (err)
> > +		return err;
> > +
> > +	err =3D sysfs_create_file(&pfdev->profiling.base, &profiling_status.a=
ttr);
> > +	if (err)
> > +		kobject_del(&pfdev->profiling.base);
> > +
> > +	return err;
> > +}