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

[Boris Brezillon]

On Wed, 6 Mar 2024 08:33:47 +0000
Tvrtko Ursulin <tursulin@xxxxxxxxxxx> wrote:

> On 06/03/2024 01:56, 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>
> > ---
> >   .../testing/sysfs-driver-panfrost-profiling   | 10 +++++
> >   Documentation/gpu/panfrost.rst                |  9 ++++
> >   drivers/gpu/drm/panfrost/Makefile             |  2 -
> >   drivers/gpu/drm/panfrost/panfrost_debugfs.c   | 21 ----------
> >   drivers/gpu/drm/panfrost/panfrost_debugfs.h   | 14 -------
> >   drivers/gpu/drm/panfrost/panfrost_device.h    |  2 +-
> >   drivers/gpu/drm/panfrost/panfrost_drv.c       | 41 ++++++++++++++++---
> >   drivers/gpu/drm/panfrost/panfrost_job.c       |  2 +-
> >   8 files changed, 57 insertions(+), 44 deletions(-)
> >   create mode 100644 Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> >   delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.c
> >   delete mode 100644 drivers/gpu/drm/panfrost/panfrost_debugfs.h
> > 
> > diff --git a/Documentation/ABI/testing/sysfs-driver-panfrost-profiling b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > new file mode 100644
> > index 000000000000..1d8bb0978920
> > --- /dev/null
> > +++ b/Documentation/ABI/testing/sysfs-driver-panfrost-profiling
> > @@ -0,0 +1,10 @@
> > +What:		/sys/bus/platform/drivers/panfrost/.../profiling
> > +Date:		February 2024
> > +KernelVersion:	6.8.0
> > +Contact:	Adrian Larumbe <adrian.larumbe@xxxxxxxxxxxxx>
> > +Description:
> > +		Get/set drm fdinfo's engine and cycles profiling status.
> > +		Valid values are:
> > +		0: Don't enable fdinfo job profiling sources.
> > +		1: Enable fdinfo job profiling sources, this enables both the GPU's
> > +		   timestamp and cycle counter registers.
> > \ No newline at end of file
> > diff --git a/Documentation/gpu/panfrost.rst b/Documentation/gpu/panfrost.rst
> > index b80e41f4b2c5..51ba375fd80d 100644
> > --- a/Documentation/gpu/panfrost.rst
> > +++ b/Documentation/gpu/panfrost.rst
> > @@ -38,3 +38,12 @@ the currently possible format options:
> >   
> >   Possible `drm-engine-` key names are: `fragment`, and  `vertex-tiler`.
> >   `drm-curfreq-` values convey the current operating frequency for that engine.
> > +
> > +Users must bear in mind that engine and cycle sampling are disabled by default,
> > +because of power saving concerns. `fdinfo` users and benchmark applications which
> > +query the fdinfo file must make sure to toggle the job profiling status of the
> > +driver by writing into the appropriate sysfs node::
> > +
> > +    echo <N> > /sys/bus/platform/drivers/panfrost/[a-f0-9]*.gpu/profiling  
> 
> A late thought - how it would work to not output the inactive fdinfo 
> keys when this knob is not enabled?
> 
> Generic userspace like gputop already handles that and wouldn't show the 
> stat. Which may be more user friendly than showing stats permanently at 
> zero. It may be moot once you add the auto-toggle to gputop (or so) but 
> perhaps worth considering.

I agree with Tvrtko, if the line being printed in fdinfo relies on some
sysfs knob to be valid, we'd rather not print the information in that
case, instead of printing zero.