Re: [PATCH 4/4] drm/panthor: Display heap chunk entries in DebugFS GEMS file
On Wed, 19 Mar 2025 13:18:53 +
Adrián Larumbe wrote:
> On 17.03.2025 09:31, Boris Brezillon wrote:
> > On Sun, 16 Mar 2025 21:51:35 +
> > Adrián Larumbe wrote:
> >
> > > Expand the driver's DebugFS GEMS file to display entries for the heap
> > > chunks' GEM objects, both those allocated at heap creation time through an
> > > ioctl(), or in response to a tiler OOM event.
> > >
> > > Signed-off-by: Adrián Larumbe
> > > ---
> > > drivers/gpu/drm/panthor/panthor_heap.c | 15 +++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c
> > > b/drivers/gpu/drm/panthor/panthor_heap.c
> > > index db0285ce5812..520d1fcf5c36 100644
> > > --- a/drivers/gpu/drm/panthor/panthor_heap.c
> > > +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> > > @@ -139,6 +139,10 @@ static int panthor_alloc_heap_chunk(struct
> > > panthor_device *ptdev,
> > > struct panthor_heap_chunk *chunk;
> > > struct panthor_heap_chunk_header *hdr;
> > > int ret;
> > > +#ifdef CONFIG_DEBUG_FS
> > > + struct panthor_gem_object *obj;
> > > + const char *label;
> > > +#endif
> > >
> > > chunk = kmalloc(sizeof(*chunk), GFP_KERNEL);
> > > if (!chunk)
> > > @@ -180,6 +184,17 @@ static int panthor_alloc_heap_chunk(struct
> > > panthor_device *ptdev,
> > > heap->chunk_count++;
> > > mutex_unlock(&heap->lock);
> > >
> > > +#ifdef CONFIG_DEBUG_FS
> > > + obj = to_panthor_bo(chunk->bo->obj);
> > > +
> > > + mutex_lock(&ptdev->gems_lock);
> > > + list_add_tail(&obj->gems_node, &ptdev->gems);
> > > + mutex_unlock(&ptdev->gems_lock);
> > > +
> > > + label = kstrdup_const("\"Tiler heap chunk\"", GFP_KERNEL);
> >
> > Do we really need the extra quotes around 'Tiler heap chunk'?
>
> We want them quoted like this so that the BO name can be told apart from the
> the extra tagging information (like modifiers) and any suffix sent down from
> gl
Feels like you want the kernel to use a format that's used by mesa but
not really standardized. What happens if we decide to change the
formatting there? Suddenly, kernel BOs become the outliers again, and
you're back to the inconsistency you were trying to avoid. My
recommendation would be to keep kernel BO labels simple (just a regular
string, no quoting, no extra props) and let the debugfs printer
segregate kernel BOs from user BOs to make the distinction clear.
Re: [PATCH 4/4] drm/panthor: Display heap chunk entries in DebugFS GEMS file
On 17.03.2025 09:31, Boris Brezillon wrote:
> On Sun, 16 Mar 2025 21:51:35 +
> Adrián Larumbe wrote:
>
> > Expand the driver's DebugFS GEMS file to display entries for the heap
> > chunks' GEM objects, both those allocated at heap creation time through an
> > ioctl(), or in response to a tiler OOM event.
> >
> > Signed-off-by: Adrián Larumbe
> > ---
> > drivers/gpu/drm/panthor/panthor_heap.c | 15 +++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c
> > b/drivers/gpu/drm/panthor/panthor_heap.c
> > index db0285ce5812..520d1fcf5c36 100644
> > --- a/drivers/gpu/drm/panthor/panthor_heap.c
> > +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> > @@ -139,6 +139,10 @@ static int panthor_alloc_heap_chunk(struct
> > panthor_device *ptdev,
> > struct panthor_heap_chunk *chunk;
> > struct panthor_heap_chunk_header *hdr;
> > int ret;
> > +#ifdef CONFIG_DEBUG_FS
> > + struct panthor_gem_object *obj;
> > + const char *label;
> > +#endif
> >
> > chunk = kmalloc(sizeof(*chunk), GFP_KERNEL);
> > if (!chunk)
> > @@ -180,6 +184,17 @@ static int panthor_alloc_heap_chunk(struct
> > panthor_device *ptdev,
> > heap->chunk_count++;
> > mutex_unlock(&heap->lock);
> >
> > +#ifdef CONFIG_DEBUG_FS
> > + obj = to_panthor_bo(chunk->bo->obj);
> > +
> > + mutex_lock(&ptdev->gems_lock);
> > + list_add_tail(&obj->gems_node, &ptdev->gems);
> > + mutex_unlock(&ptdev->gems_lock);
> > +
> > + label = kstrdup_const("\"Tiler heap chunk\"", GFP_KERNEL);
>
> Do we really need the extra quotes around 'Tiler heap chunk'?
We want them quoted like this so that the BO name can be told apart from the
the extra tagging information (like modifiers) and any suffix sent down from gl
> > + panthor_gem_label_bo(chunk->bo->obj, label);
> > +#endif
>
> Let's define a helper to assign a label to a kernel BO instead of
> open-coding it here. BTW, I suspect we'll want to assign labels to
> other kernel BOs too (FW buffers).
>
> > +
> > return 0;
> >
> > err_destroy_bo:
Adrian Larumbe
Re: [PATCH 4/4] drm/panthor: Display heap chunk entries in DebugFS GEMS file
On Sun, 16 Mar 2025 21:51:35 +
Adrián Larumbe wrote:
> Expand the driver's DebugFS GEMS file to display entries for the heap
> chunks' GEM objects, both those allocated at heap creation time through an
> ioctl(), or in response to a tiler OOM event.
>
> Signed-off-by: Adrián Larumbe
> ---
> drivers/gpu/drm/panthor/panthor_heap.c | 15 +++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c
> b/drivers/gpu/drm/panthor/panthor_heap.c
> index db0285ce5812..520d1fcf5c36 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -139,6 +139,10 @@ static int panthor_alloc_heap_chunk(struct
> panthor_device *ptdev,
> struct panthor_heap_chunk *chunk;
> struct panthor_heap_chunk_header *hdr;
> int ret;
> +#ifdef CONFIG_DEBUG_FS
> + struct panthor_gem_object *obj;
> + const char *label;
> +#endif
>
> chunk = kmalloc(sizeof(*chunk), GFP_KERNEL);
> if (!chunk)
> @@ -180,6 +184,17 @@ static int panthor_alloc_heap_chunk(struct
> panthor_device *ptdev,
> heap->chunk_count++;
> mutex_unlock(&heap->lock);
>
> +#ifdef CONFIG_DEBUG_FS
> + obj = to_panthor_bo(chunk->bo->obj);
> +
> + mutex_lock(&ptdev->gems_lock);
> + list_add_tail(&obj->gems_node, &ptdev->gems);
> + mutex_unlock(&ptdev->gems_lock);
> +
> + label = kstrdup_const("\"Tiler heap chunk\"", GFP_KERNEL);
Do we really need the extra quotes around 'Tiler heap chunk'?
> + panthor_gem_label_bo(chunk->bo->obj, label);
> +#endif
Let's define a helper to assign a label to a kernel BO instead of
open-coding it here. BTW, I suspect we'll want to assign labels to
other kernel BOs too (FW buffers).
> +
> return 0;
>
> err_destroy_bo:
[PATCH 4/4] drm/panthor: Display heap chunk entries in DebugFS GEMS file
Expand the driver's DebugFS GEMS file to display entries for the heap
chunks' GEM objects, both those allocated at heap creation time through an
ioctl(), or in response to a tiler OOM event.
Signed-off-by: Adrián Larumbe
---
drivers/gpu/drm/panthor/panthor_heap.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/panthor/panthor_heap.c
b/drivers/gpu/drm/panthor/panthor_heap.c
index db0285ce5812..520d1fcf5c36 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -139,6 +139,10 @@ static int panthor_alloc_heap_chunk(struct panthor_device
*ptdev,
struct panthor_heap_chunk *chunk;
struct panthor_heap_chunk_header *hdr;
int ret;
+#ifdef CONFIG_DEBUG_FS
+ struct panthor_gem_object *obj;
+ const char *label;
+#endif
chunk = kmalloc(sizeof(*chunk), GFP_KERNEL);
if (!chunk)
@@ -180,6 +184,17 @@ static int panthor_alloc_heap_chunk(struct panthor_device
*ptdev,
heap->chunk_count++;
mutex_unlock(&heap->lock);
+#ifdef CONFIG_DEBUG_FS
+ obj = to_panthor_bo(chunk->bo->obj);
+
+ mutex_lock(&ptdev->gems_lock);
+ list_add_tail(&obj->gems_node, &ptdev->gems);
+ mutex_unlock(&ptdev->gems_lock);
+
+ label = kstrdup_const("\"Tiler heap chunk\"", GFP_KERNEL);
+ panthor_gem_label_bo(chunk->bo->obj, label);
+#endif
+
return 0;
err_destroy_bo:
--
2.48.1
