Re: [Mesa-dev] [PATCH 1/3] panfrost: Fix various leaks unmapping resources
On Fri, 15 Feb 2019 at 21:42, Alyssa Rosenzweig 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 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] panfrost: Fix various leaks unmapping resources
> 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. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] panfrost: Fix various leaks unmapping resources
Hi Alyssa,
On Fri, 15 Feb 2019 at 08:50, Alyssa Rosenzweig wrote:
>
> Signed-off-by: Alyssa Rosenzweig
> ---
> 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(>slabs, >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(>slabs, >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
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] panfrost: Fix various leaks unmapping resources
Signed-off-by: Alyssa Rosenzweig
---
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(>slabs, >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(>slabs, >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 {
--
2.20.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
