[PATCH RFC 0/4] drm/panfrost: Expose memory usage stats through fdinfo

[PATCH RFC 0/4] drm/panfrost: Expose memory usage stats through fdinfo

[Boris Brezillon]

Hello,

Here's an attempt at exposing some memory usage stats through fdinfo,
which recently proved useful in debugging a memory leak. Not entirely
sure the name I chose are accurate, so feel free to propose
alternatives, and let me know if you see any other mem-related stuff
that would be interesting to expose.

Regards,

Boris

Boris Brezillon (4):
  drm/panfrost: Provide a dummy show_fdinfo() implementation
  drm/panfrost: Track BO resident size
  drm/panfrost: Add a helper to retrieve MMU context stats
  drm/panfrost: Expose some memory related stats through fdinfo

 drivers/gpu/drm/panfrost/panfrost_drv.c       | 24 ++++++++++++++++-
 drivers/gpu/drm/panfrost/panfrost_gem.h       |  7 +++++
 .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  1 +
 drivers/gpu/drm/panfrost/panfrost_mmu.c       | 27 +++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_mmu.h       | 10 +++++++
 5 files changed, 68 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH RFC 1/4] drm/panfrost: Provide a dummy show_fdinfo() implementation

[Boris Brezillon]

Provide a dummy show_fdinfo() implementation exposing drm-driver and
drm-client-id. More stats will be added soon.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 2fa5afe21288..6ee43559fc14 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -515,7 +515,22 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
 	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
 };
 
-DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
+static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
+{
+	struct drm_file *file = f->private_data;
+	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
+
+	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
+	seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);
+}
+
+static const struct file_operations panfrost_drm_driver_fops = {
+	.owner = THIS_MODULE,
+	DRM_GEM_FOPS,
+#ifdef CONFIG_PROC_FS
+	.show_fdinfo = panfrost_show_fdinfo,
+#endif
+};
 
 /*
  * Panfrost driver version:
-- 
2.38.1

Re: [PATCH RFC 1/4] drm/panfrost: Provide a dummy show_fdinfo() implementation

[Daniel Vetter]

On Wed, Jan 04, 2023 at 02:03:05PM +0100, Boris Brezillon wrote:
> Provide a dummy show_fdinfo() implementation exposing drm-driver and
> drm-client-id. More stats will be added soon.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 2fa5afe21288..6ee43559fc14 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -515,7 +515,22 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
>  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
>  };
>  
> -DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
> +static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
> +{
> +	struct drm_file *file = f->private_data;
> +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> +
> +	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
> +	seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);

I think at this point we really need to not just have a document that says
what this should look like, but drm infrastructure with shared code.
Drivers all inventing their fdinfo really doesn't seem like a great idea
to me.
-Daniel

> +}
> +
> +static const struct file_operations panfrost_drm_driver_fops = {
> +	.owner = THIS_MODULE,
> +	DRM_GEM_FOPS,
> +#ifdef CONFIG_PROC_FS
> +	.show_fdinfo = panfrost_show_fdinfo,
> +#endif
> +};
>  
>  /*
>   * Panfrost driver version:
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RFC 1/4] drm/panfrost: Provide a dummy show_fdinfo() implementation

[Boris Brezillon]

Hi Daniel,

On Thu, 5 Jan 2023 16:31:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jan 04, 2023 at 02:03:05PM +0100, Boris Brezillon wrote:
> > Provide a dummy show_fdinfo() implementation exposing drm-driver and
> > drm-client-id. More stats will be added soon.
> > 
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > ---
> >  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 2fa5afe21288..6ee43559fc14 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -515,7 +515,22 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
> >  	PANFROST_IOCTL(MADVISE,		madvise,	DRM_RENDER_ALLOW),
> >  };
> >  
> > -DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
> > +static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
> > +{
> > +	struct drm_file *file = f->private_data;
> > +	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> > +
> > +	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
> > +	seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);  
> 
> I think at this point we really need to not just have a document that says
> what this should look like, but drm infrastructure with shared code.
> Drivers all inventing their fdinfo really doesn't seem like a great idea
> to me.

Okay. I'm just curious how far you want to go with this common
infrastructure? Are we talking about having a generic helper printing
the pretty generic drm-{driver,client-id} props and letting the driver
prints its driver specific properties, or do you also want to
standardize/automate printing of some drm-memory/drm-engine props too?

Regards,

Boris

Re: [PATCH RFC 1/4] drm/panfrost: Provide a dummy show_fdinfo() implementation

[Daniel Vetter]

On Mon, 9 Jan 2023 at 09:34, Boris Brezillon
<boris.brezillon@collabora.com> wrote:
>
> Hi Daniel,
>
> On Thu, 5 Jan 2023 16:31:49 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > On Wed, Jan 04, 2023 at 02:03:05PM +0100, Boris Brezillon wrote:
> > > Provide a dummy show_fdinfo() implementation exposing drm-driver and
> > > drm-client-id. More stats will be added soon.
> > >
> > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > ---
> > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > index 2fa5afe21288..6ee43559fc14 100644
> > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > @@ -515,7 +515,22 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
> > >     PANFROST_IOCTL(MADVISE,         madvise,        DRM_RENDER_ALLOW),
> > >  };
> > >
> > > -DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
> > > +static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
> > > +{
> > > +   struct drm_file *file = f->private_data;
> > > +   struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> > > +
> > > +   seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
> > > +   seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);
> >
> > I think at this point we really need to not just have a document that says
> > what this should look like, but drm infrastructure with shared code.
> > Drivers all inventing their fdinfo really doesn't seem like a great idea
> > to me.
>
> Okay. I'm just curious how far you want to go with this common
> infrastructure? Are we talking about having a generic helper printing
> the pretty generic drm-{driver,client-id} props and letting the driver
> prints its driver specific properties, or do you also want to
> standardize/automate printing of some drm-memory/drm-engine props too?

I think we should standardized what's used by multiple drivers at
least. It might be a bit tough for the memory/engine props, because
there's really not much standard stuff there yet (e.g. for memory I'm
still hoping for cgroups work, for engines we should probably base
this on drm_sched_entity and maybe untie that somewhat from sched
itself for i915-sched and fw sched and whatever there is).

But as usual, try to be reasonable with standard code :-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH RFC 1/4] drm/panfrost: Provide a dummy show_fdinfo() implementation

[Boris Brezillon]

On Mon, 9 Jan 2023 11:17:49 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, 9 Jan 2023 at 09:34, Boris Brezillon
> <boris.brezillon@collabora.com> wrote:
> >
> > Hi Daniel,
> >
> > On Thu, 5 Jan 2023 16:31:49 +0100
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >  
> > > On Wed, Jan 04, 2023 at 02:03:05PM +0100, Boris Brezillon wrote:  
> > > > Provide a dummy show_fdinfo() implementation exposing drm-driver and
> > > > drm-client-id. More stats will be added soon.
> > > >
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > > > ---
> > > >  drivers/gpu/drm/panfrost/panfrost_drv.c | 17 ++++++++++++++++-
> > > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > index 2fa5afe21288..6ee43559fc14 100644
> > > > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > > > @@ -515,7 +515,22 @@ static const struct drm_ioctl_desc panfrost_drm_driver_ioctls[] = {
> > > >     PANFROST_IOCTL(MADVISE,         madvise,        DRM_RENDER_ALLOW),
> > > >  };
> > > >
> > > > -DEFINE_DRM_GEM_FOPS(panfrost_drm_driver_fops);
> > > > +static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
> > > > +{
> > > > +   struct drm_file *file = f->private_data;
> > > > +   struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> > > > +
> > > > +   seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
> > > > +   seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);  
> > >
> > > I think at this point we really need to not just have a document that says
> > > what this should look like, but drm infrastructure with shared code.
> > > Drivers all inventing their fdinfo really doesn't seem like a great idea
> > > to me.  
> >
> > Okay. I'm just curious how far you want to go with this common
> > infrastructure? Are we talking about having a generic helper printing
> > the pretty generic drm-{driver,client-id} props and letting the driver
> > prints its driver specific properties, or do you also want to
> > standardize/automate printing of some drm-memory/drm-engine props too?  
> 
> I think we should standardized what's used by multiple drivers at
> least. It might be a bit tough for the memory/engine props, because
> there's really not much standard stuff there yet (e.g. for memory I'm
> still hoping for cgroups work, for engines we should probably base
> this on drm_sched_entity and maybe untie that somewhat from sched
> itself for i915-sched and fw sched and whatever there is).

Good, didn't want to be drawn in endless discussions about what should
be standardized and what shouldn't anyway. So I'll start with
drm-{driver,client-id}. For the client-id, we'll probably need
some sort of unique-id stored at the drm_file level (ida-based?),
unless you want to leave that to drivers too.

[PATCH RFC 2/4] drm/panfrost: Track BO resident size

[Boris Brezillon]

Heap BOs use an on-demand allocation scheme, meaning that the resident
size is different from the BO side. Track resident size so we can more
accurately per-FD expose memory usage.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_gem.h          | 7 +++++++
 drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c | 1 +
 drivers/gpu/drm/panfrost/panfrost_mmu.c          | 2 ++
 3 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gem.h b/drivers/gpu/drm/panfrost/panfrost_gem.h
index 8088d5fd8480..58f5d091c983 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem.h
+++ b/drivers/gpu/drm/panfrost/panfrost_gem.h
@@ -36,6 +36,13 @@ struct panfrost_gem_object {
 	 */
 	atomic_t gpu_usecount;
 
+	/* Actual memory used by the BO. Should be zero before pages are
+	 * pinned, then the size of the BO, unless it's a heap BO. In
+	 * this case the resident size is updated when the fault handler
+	 * allocates memory.
+	 */
+	size_t resident_size;
+
 	bool noexec		:1;
 	bool is_heap		:1;
 };
diff --git a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
index bf0170782f25..efbc8dec4a9f 100644
--- a/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
+++ b/drivers/gpu/drm/panfrost/panfrost_gem_shrinker.c
@@ -54,6 +54,7 @@ static bool panfrost_gem_purge(struct drm_gem_object *obj)
 	panfrost_gem_teardown_mappings_locked(bo);
 	drm_gem_shmem_purge_locked(&bo->base);
 	ret = true;
+	bo->resident_size = 0;
 
 	mutex_unlock(&shmem->pages_lock);
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 4e83a1891f3e..454799d5a0ef 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -340,6 +340,7 @@ int panfrost_mmu_map(struct panfrost_gem_mapping *mapping)
 	mmu_map_sg(pfdev, mapping->mmu, mapping->mmnode.start << PAGE_SHIFT,
 		   prot, sgt);
 	mapping->active = true;
+	bo->resident_size = bo->base.base.size;
 
 	return 0;
 }
@@ -508,6 +509,7 @@ static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
 		}
 	}
 
+	bo->resident_size += SZ_2M;
 	mutex_unlock(&bo->base.pages_lock);
 
 	sgt = &bo->sgts[page_offset / (SZ_2M / PAGE_SIZE)];
-- 
2.38.1

[PATCH RFC 3/4] drm/panfrost: Add a helper to retrieve MMU context stats

[Boris Brezillon]

For now we only gather a few memory usage stats that we'll expose
through fdinfo, but this can be extended if needed.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_mmu.c | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_mmu.h | 10 ++++++++++
 2 files changed, 35 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.c b/drivers/gpu/drm/panfrost/panfrost_mmu.c
index 454799d5a0ef..80c6e0e17195 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.c
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.c
@@ -435,6 +435,31 @@ addr_to_mapping(struct panfrost_device *pfdev, int as, u64 addr)
 	return mapping;
 }
 
+void panfrost_mmu_get_stats(struct panfrost_mmu *mmu,
+			    struct panfrost_mmu_stats *stats)
+{
+	struct drm_mm_node *node;
+
+	memset(stats, 0, sizeof(*stats));
+
+	spin_lock(&mmu->mm_lock);
+	drm_mm_for_each_node(node, &mmu->mm) {
+		struct panfrost_gem_mapping *mapping;
+		struct panfrost_gem_object *bo;
+
+		mapping = container_of(node, struct panfrost_gem_mapping, mmnode);
+		bo = mapping->obj;
+
+		stats->all += bo->base.base.size;
+		stats->resident += bo->resident_size;
+		if (bo->base.madv > 0)
+			stats->purgeable += bo->resident_size;
+		if (bo->base.base.dma_buf)
+			stats->shared += bo->base.base.size;
+	}
+	spin_unlock(&mmu->mm_lock);
+}
+
 #define NUM_FAULT_PAGES (SZ_2M / PAGE_SIZE)
 
 static int panfrost_mmu_map_fault_addr(struct panfrost_device *pfdev, int as,
diff --git a/drivers/gpu/drm/panfrost/panfrost_mmu.h b/drivers/gpu/drm/panfrost/panfrost_mmu.h
index cc2a0d307feb..bbffd39deaf3 100644
--- a/drivers/gpu/drm/panfrost/panfrost_mmu.h
+++ b/drivers/gpu/drm/panfrost/panfrost_mmu.h
@@ -8,6 +8,13 @@ struct panfrost_gem_mapping;
 struct panfrost_file_priv;
 struct panfrost_mmu;
 
+struct panfrost_mmu_stats {
+	u64 all;
+	u64 resident;
+	u64 purgeable;
+	u64 shared;
+};
+
 int panfrost_mmu_map(struct panfrost_gem_mapping *mapping);
 void panfrost_mmu_unmap(struct panfrost_gem_mapping *mapping);
 
@@ -22,4 +29,7 @@ struct panfrost_mmu *panfrost_mmu_ctx_get(struct panfrost_mmu *mmu);
 void panfrost_mmu_ctx_put(struct panfrost_mmu *mmu);
 struct panfrost_mmu *panfrost_mmu_ctx_create(struct panfrost_device *pfdev);
 
+void panfrost_mmu_get_stats(struct panfrost_mmu *mmu,
+			    struct panfrost_mmu_stats *stats);
+
 #endif
-- 
2.38.1

[PATCH RFC 4/4] drm/panfrost: Expose some memory related stats through fdinfo

[Boris Brezillon]

drm-memory-all: memory hold by this context. Not that all the memory is
not necessarily resident: heap BO size is counted even though only part
of the memory reserved for those BOs might be allocated.

drm-memory-resident: resident memory size. For normal BOs it's the same
as drm-memory-all, but for heap BOs, only the memory actually allocated
is counted.

drm-memory-purgeable: amount of memory that can be reclaimed by the
system (madvise(DONT_NEED)).

drm-memory-shared: amount of memory shared through dma-buf.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 6ee43559fc14..05d5d480df2a 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -519,9 +519,16 @@ static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
 {
 	struct drm_file *file = f->private_data;
 	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
+	struct panfrost_mmu_stats mmu_stats;
+
+	panfrost_mmu_get_stats(panfrost_priv->mmu, &mmu_stats);
 
 	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
 	seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);
+	seq_printf(m, "drm-memory-all:\t%llu KiB\n", mmu_stats.all >> 10);
+	seq_printf(m, "drm-memory-resident:\t%llu KiB\n", mmu_stats.resident >> 10);
+	seq_printf(m, "drm-memory-purgeable:\t%llu KiB\n", mmu_stats.purgeable >> 10);
+	seq_printf(m, "drm-memory-shared:\t%llu KiB\n", mmu_stats.shared >> 10);
 }
 
 static const struct file_operations panfrost_drm_driver_fops = {
-- 
2.38.1

Re: [PATCH RFC 4/4] drm/panfrost: Expose some memory related stats through fdinfo

[Tvrtko Ursulin]

Hi,

Chris was kind enough to bring my attention to this thread. Indeed this 
information was asked for by various people for many years so it sounds 
very useful to actually do attempt it.

On 04/01/2023 13:03, Boris Brezillon wrote:
> drm-memory-all: memory hold by this context. Not that all the memory is
> not necessarily resident: heap BO size is counted even though only part
> of the memory reserved for those BOs might be allocated.
> 
> drm-memory-resident: resident memory size. For normal BOs it's the same
> as drm-memory-all, but for heap BOs, only the memory actually allocated
> is counted.
> 
> drm-memory-purgeable: amount of memory that can be reclaimed by the
> system (madvise(DONT_NEED)).
> 
> drm-memory-shared: amount of memory shared through dma-buf.

A bunch of comments/questions..

First of all, lets please continue documenting the fdinfo content in 
Documentation/gpu/drm-usage-stats.rst as stuff is proposed to be added. 
Idea was to have as much commonality as reasonably possible, and so to 
have more usable user facing tools etc. And also please copy people 
involved with adding new stuff to that file.

(Half-digression - for some reason get_maintainers.pl does not play nice 
with this file, I was expecting it to count and sort contributors after 
maintainers but it doesn't for some reason. Perhaps it only does that 
for source code.)

For the actual key/fields name.. I suspect apart from category we will 
need a memory type part, at least with discrete GPUs (non unified/shared 
memory designs), that further breakdown will be required. We therefore 
need to discuss if we do that from the start, or start with your 
proposal and extend later.

In more practical terms I am talking about something this:

drm-memory-local-all: <ulong> <unit>
drm-memory-local-resident: <ulong> <unit>
drm-memory-system-all: <ulong> <unit>
drm-memory-system-resident: <ulong> <unit>

"All/resident/..." could then probably be standardized and the 
"local/system" could be partially standardized or left for drivers to 
use names they see fit (same as with engine names).

Also, I an not quite liking "all". Analogy from the CPU land for it 
would be "virtual", but does translate into the GPU world?

Finally, we also need to define the semantics of resident when we deal 
with shared objects. For instance process P / fd Fp creates a buffer and 
passes the handle to process Q / fd Fq. Whose fdinfo sees what, in what 
fields and when?

I suspect if Q is the first to start using the shared object it should 
show it under "resident". P should not, until it starts using it itself. 
For the "all" category both should see it.

For "shared" are you proposing to count imported or exported counts as 
well? I think it needs to be clearly documented. Is it "this fd is using 
this much shared buffers", or "this fd has exported this much shared 
buffers", or both?

I don't know your driver to quickly figure out what semantics you proposed?

Regards,

Tvrtko

> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_drv.c | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 6ee43559fc14..05d5d480df2a 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -519,9 +519,16 @@ static void panfrost_show_fdinfo(struct seq_file *m, struct file *f)
>   {
>   	struct drm_file *file = f->private_data;
>   	struct panfrost_file_priv *panfrost_priv = file->driver_priv;
> +	struct panfrost_mmu_stats mmu_stats; > +
> +	panfrost_mmu_get_stats(panfrost_priv->mmu, &mmu_stats);
>   
>   	seq_printf(m, "drm-driver:\t%s\n", file->minor->dev->driver->name);
>   	seq_printf(m, "drm-client-id:\t%llu\n", panfrost_priv->sched_entity[0].fence_context);
> +	seq_printf(m, "drm-memory-all:\t%llu KiB\n", mmu_stats.all >> 10);
> +	seq_printf(m, "drm-memory-resident:\t%llu KiB\n", mmu_stats.resident >> 10);
> +	seq_printf(m, "drm-memory-purgeable:\t%llu KiB\n", mmu_stats.purgeable >> 10);
> +	seq_printf(m, "drm-memory-shared:\t%llu KiB\n", mmu_stats.shared >> 10);
>   }
>   
>   static const struct file_operations panfrost_drm_driver_fops = {

Re: [PATCH RFC 0/4] drm/panfrost: Expose memory usage stats through fdinfo

[Steven Price]

On 04/01/2023 13:03, Boris Brezillon wrote:
> Hello,
> 
> Here's an attempt at exposing some memory usage stats through fdinfo,
> which recently proved useful in debugging a memory leak. Not entirely
> sure the name I chose are accurate, so feel free to propose
> alternatives, and let me know if you see any other mem-related stuff
> that would be interesting to expose.

Sorry it's taken me a while to look at this - I'm still working through
the holiday backlog.

The names look reasonable to me, and I gave this a quick spin and it
seemed to work (the numbers reported looks reasonable). As Daniel
suggested it would be good if some of the boiler plate fdinfo code could
be moved to generic code (although to be fair there's not much here).

Of course what we're missing is the 'engine' usage information for
gputop - it's been on my todo list of a while, but I'm more than happy
for you to do it for me ;) It's somewhat more tricky because of the
whole 'queuing' on slots mechanism that Mali has. But we obviously
shouldn't block this memory implementation on that, it can be added
afterwards.

Anyway, for the series as it is:

Reviewed-by: Steven Price <steven.price@arm.com>

Thanks,

Steve

> Regards,
> 
> Boris
> 
> Boris Brezillon (4):
>   drm/panfrost: Provide a dummy show_fdinfo() implementation
>   drm/panfrost: Track BO resident size
>   drm/panfrost: Add a helper to retrieve MMU context stats
>   drm/panfrost: Expose some memory related stats through fdinfo
> 
>  drivers/gpu/drm/panfrost/panfrost_drv.c       | 24 ++++++++++++++++-
>  drivers/gpu/drm/panfrost/panfrost_gem.h       |  7 +++++
>  .../gpu/drm/panfrost/panfrost_gem_shrinker.c  |  1 +
>  drivers/gpu/drm/panfrost/panfrost_mmu.c       | 27 +++++++++++++++++++
>  drivers/gpu/drm/panfrost/panfrost_mmu.h       | 10 +++++++
>  5 files changed, 68 insertions(+), 1 deletion(-)
> 

Re: [PATCH RFC 0/4] drm/panfrost: Expose memory usage stats through fdinfo

[Boris Brezillon]

Hi Steven,

On Mon, 16 Jan 2023 10:30:21 +0000
Steven Price <steven.price@arm.com> wrote:

> On 04/01/2023 13:03, Boris Brezillon wrote:
> > Hello,
> > 
> > Here's an attempt at exposing some memory usage stats through fdinfo,
> > which recently proved useful in debugging a memory leak. Not entirely
> > sure the name I chose are accurate, so feel free to propose
> > alternatives, and let me know if you see any other mem-related stuff
> > that would be interesting to expose.  
> 
> Sorry it's taken me a while to look at this - I'm still working through
> the holiday backlog.
> 
> The names look reasonable to me, and I gave this a quick spin and it
> seemed to work (the numbers reported looks reasonable). As Daniel
> suggested it would be good if some of the boiler plate fdinfo code could
> be moved to generic code (although to be fair there's not much here).
> 
> Of course what we're missing is the 'engine' usage information for
> gputop - it's been on my todo list of a while, but I'm more than happy
> for you to do it for me ;) It's somewhat more tricky because of the
> whole 'queuing' on slots mechanism that Mali has. But we obviously
> shouldn't block this memory implementation on that, it can be added
> afterwards.

Yeah, we've been discussing this drm-engine-xxx feature with Chris, and
I was telling him there's no easy way to get accurate numbers when
_NEXT queuing is involved. It all depends on whether we're able to
process the first job DONE interrupt before the second one kicks in, and
even then, we can't tell for sure for how long the second job has been
running when we get to process the first job interrupt. Inserting
WRITE_JOB(CYCLE_COUNT) before a job chain is doable, but inserting it
after isn't, and I'm not sure we want to add such tricks to the kernel
driver anyway. Don't know if you have any better ideas. If not, I guess
we can leave with this inaccuracy and still expose drm-engine-xxx...

Regards,

Boris

Re: [PATCH RFC 0/4] drm/panfrost: Expose memory usage stats through fdinfo

[Steven Price]

On 16/01/2023 11:05, Boris Brezillon wrote:
> Hi Steven,
> 
> On Mon, 16 Jan 2023 10:30:21 +0000
> Steven Price <steven.price@arm.com> wrote:
> 
>> On 04/01/2023 13:03, Boris Brezillon wrote:
>>> Hello,
>>>
>>> Here's an attempt at exposing some memory usage stats through fdinfo,
>>> which recently proved useful in debugging a memory leak. Not entirely
>>> sure the name I chose are accurate, so feel free to propose
>>> alternatives, and let me know if you see any other mem-related stuff
>>> that would be interesting to expose.  
>>
>> Sorry it's taken me a while to look at this - I'm still working through
>> the holiday backlog.
>>
>> The names look reasonable to me, and I gave this a quick spin and it
>> seemed to work (the numbers reported looks reasonable). As Daniel
>> suggested it would be good if some of the boiler plate fdinfo code could
>> be moved to generic code (although to be fair there's not much here).
>>
>> Of course what we're missing is the 'engine' usage information for
>> gputop - it's been on my todo list of a while, but I'm more than happy
>> for you to do it for me ;) It's somewhat more tricky because of the
>> whole 'queuing' on slots mechanism that Mali has. But we obviously
>> shouldn't block this memory implementation on that, it can be added
>> afterwards.
> 
> Yeah, we've been discussing this drm-engine-xxx feature with Chris, and
> I was telling him there's no easy way to get accurate numbers when
> _NEXT queuing is involved. It all depends on whether we're able to
> process the first job DONE interrupt before the second one kicks in, and
> even then, we can't tell for sure for how long the second job has been
> running when we get to process the first job interrupt. Inserting
> WRITE_JOB(CYCLE_COUNT) before a job chain is doable, but inserting it
> after isn't, and I'm not sure we want to add such tricks to the kernel
> driver anyway. Don't know if you have any better ideas. If not, I guess
> we can leave with this inaccuracy and still expose drm-engine-xxx...

It's fun isn't it ;) I spent many hours in the past puzzling over this!

Realistically it doesn't make sense for the kernel to get involved in
inserting write_jobs. You open up so many cans of worms regarding how to
manage the memory for the GPU to write in. The closed DDK handles this
by the user space driver adding these jobs and the tooling capturing
data from both the kernel and user space.

But for just the gputop type tooling I don't think we need that level of
accuracy[1]. If you ignore the impacts of interrupt latency then it's
possible to tell which job the GPU is currently executing and do the
accounting. Obviously interrupt latency is far from zero (that's why we
have _NEXT) but it's usually small enough that it won't skew the results
too far.

Obviously in the case you describe (second DONE interrupt before the
first one is handled) you also get the weird reporting that the second
job took no time. Which is 'wrong' but clearly the second job was
'quick' so 0 isn't likely to be too far out. And fdinfo isn't exposing
these 'per job' timings so it's unlikely to be very visible.

Thanks,

Steve

[1] As a side-note, there's a bunch of issues even with the data from
write_job(cycle_count): the frequency of the GPU may change, the
frequency of the memory system may change, a job running on the other
slot may be competing for resources on the GPU etc. When actually
profiling an application reliably it becomes necessary to lock
frequencies and ensure that it is the only thing running. I think the
DDK even includes an option to run each job by itself to avoid
cross-slot impacts (although that is unrealistic in itself).

> Regards,
> 
> Boris