[1/3] panfrost: Fix various leaks unmapping resources

Submitted by Alyssa Rosenzweig on Feb. 15, 2019, 1:03 a.m.

Details

Message ID 20190215010331.10647-1-alyssa@rosenzweig.io
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alyssa Rosenzweig Feb. 15, 2019, 1:03 a.m.
Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
---
 src/gallium/drivers/panfrost/pan_resource.c | 22 ++++++++++++---------
 src/gallium/drivers/panfrost/pan_screen.h   |  4 +++-
 2 files changed, 16 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
index 7fa00117a28..fb9b8e63c83 100644
--- a/src/gallium/drivers/panfrost/pan_resource.c
+++ b/src/gallium/drivers/panfrost/pan_resource.c
@@ -287,17 +287,20 @@  panfrost_destroy_bo(struct panfrost_screen *screen, struct panfrost_bo *pbo)
 {
 	struct panfrost_bo *bo = (struct panfrost_bo *)pbo;
 
-        if (bo->entry[0] != NULL) {
-                /* Most allocations have an entry to free */
-                bo->entry[0]->freed = true;
-                pb_slab_free(&screen->slabs, &bo->entry[0]->base);
+        for (int l = 0; l < MAX_MIP_LEVELS; ++l) {
+                if (bo->entry[l] != NULL) {
+                        /* Most allocations have an entry to free */
+                        bo->entry[l]->freed = true;
+                        pb_slab_free(&screen->slabs, &bo->entry[l]->base);
+                }
         }
 
         if (bo->tiled) {
                 /* Tiled has a malloc'd CPU, so just plain ol' free needed */
 
-                for (int l = 0; bo->cpu[l]; l++) {
-                        free(bo->cpu[l]);
+                for (int l = 0; l < MAX_MIP_LEVELS; ++l) {
+                        if (bo->cpu[l])
+                                free(bo->cpu[l]);
                 }
         }
 
@@ -509,9 +512,10 @@  panfrost_slab_can_reclaim(void *priv, struct pb_slab_entry *entry)
 static void
 panfrost_slab_free(void *priv, struct pb_slab *slab)
 {
-        /* STUB */
-        //struct panfrost_memory *mem = (struct panfrost_memory *) slab;
-        printf("stub: Tried to free slab\n");
+        struct panfrost_memory *mem = (struct panfrost_memory *) slab;
+        struct panfrost_screen *screen = (struct panfrost_screen *) priv;
+
+        screen->driver->free_slab(screen, mem);
 }
 
 static void
diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
index b89d921c71f..afb3d34b5b1 100644
--- a/src/gallium/drivers/panfrost/pan_screen.h
+++ b/src/gallium/drivers/panfrost/pan_screen.h
@@ -58,7 +58,9 @@  struct panfrost_driver {
 		               int extra_flags,
 		               int commit_count,
 		               int extent);
-	void (*enable_counters) (struct panfrost_screen *screen);
+        void (*free_slab) (struct panfrost_screen *screen,
+                           struct panfrost_memory *mem);
+        void (*enable_counters) (struct panfrost_screen *screen);
 };
 
 struct panfrost_screen {

Comments

Hi Alyssa,

On Fri, 15 Feb 2019 at 08:50, Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> Signed-off-by: Alyssa Rosenzweig <alyssa@rosenzweig.io>
> ---
>  src/gallium/drivers/panfrost/pan_resource.c | 22 ++++++++++++---------
>  src/gallium/drivers/panfrost/pan_screen.h   |  4 +++-
>  2 files changed, 16 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
> index 7fa00117a28..fb9b8e63c83 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.c
> +++ b/src/gallium/drivers/panfrost/pan_resource.c
> @@ -287,17 +287,20 @@ panfrost_destroy_bo(struct panfrost_screen *screen, struct panfrost_bo *pbo)
>  {
>         struct panfrost_bo *bo = (struct panfrost_bo *)pbo;
>
> -        if (bo->entry[0] != NULL) {
> -                /* Most allocations have an entry to free */
> -                bo->entry[0]->freed = true;
> -                pb_slab_free(&screen->slabs, &bo->entry[0]->base);
> +        for (int l = 0; l < MAX_MIP_LEVELS; ++l) {
> +                if (bo->entry[l] != NULL) {
Nit: staying consistent with "foo != NULL" vs "foo" checks helps a lot.

> +                        /* Most allocations have an entry to free */
> +                        bo->entry[l]->freed = true;
> +                        pb_slab_free(&screen->slabs, &bo->entry[l]->base);
> +                }
>          }
>
>          if (bo->tiled) {
>                  /* Tiled has a malloc'd CPU, so just plain ol' free needed */
>
> -                for (int l = 0; bo->cpu[l]; l++) {
> -                        free(bo->cpu[l]);
> +                for (int l = 0; l < MAX_MIP_LEVELS; ++l) {
> +                        if (bo->cpu[l])
free(NULL); is perfectly valid.

> +                                free(bo->cpu[l]);
>                  }
>          }
>
> @@ -509,9 +512,10 @@ panfrost_slab_can_reclaim(void *priv, struct pb_slab_entry *entry)
>  static void
>  panfrost_slab_free(void *priv, struct pb_slab *slab)
>  {
> -        /* STUB */
> -        //struct panfrost_memory *mem = (struct panfrost_memory *) slab;
> -        printf("stub: Tried to free slab\n");
> +        struct panfrost_memory *mem = (struct panfrost_memory *) slab;
> +        struct panfrost_screen *screen = (struct panfrost_screen *) priv;
> +
> +        screen->driver->free_slab(screen, mem);
The function pointer seems to be NULL. Did you forget to git add the
file which sets it?

HTH
-Emil
> Nit: staying consistent with "foo != NULL" vs "foo" checks helps a
> lot.

Which form is preferred?

> free(NULL); is perfectly valid.

Huh, TIL, thank you.

> The function pointer seems to be NULL. Did you forget to git add the
> file which sets it?

See my comment in the other mail about the overlay. Generally, functions
of the form "screen->driver->..." are specific to the kernel module in
question, and we're not able to upstream the code specific to the vendor
kernel for various reasons. It may be a good idea to stub out the
corresponding routines in pan_drm.c, but that's up to Rob and Tomeu.
On Fri, 15 Feb 2019 at 21:42, Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:
>
> > Nit: staying consistent with "foo != NULL" vs "foo" checks helps a
> > lot.
>
> Which form is preferred?
>
I think that varies across parts of mesa. I'd pick one and use it
through panfrost.

> > free(NULL); is perfectly valid.
>
> Huh, TIL, thank you.
>
> > The function pointer seems to be NULL. Did you forget to git add the
> > file which sets it?
>
> See my comment in the other mail about the overlay. Generally, functions
> of the form "screen->driver->..." are specific to the kernel module in
> question, and we're not able to upstream the code specific to the vendor
> kernel for various reasons. It may be a good idea to stub out the
> corresponding routines in pan_drm.c, but that's up to Rob and Tomeu.

Are there any legal reasons? Alternatively, I think it's reasonable to
have the code merged.
Sure it may not be perfect yet it's something people can grab and
start hacking with.

Plus, as in this example the fix cannot be easily verified :-(

-Emil