[v3,09/25] panfrost: Rework the panfrost_bo API

Submitted by Boris Brezillon on Sept. 5, 2019, 7:41 p.m.

Details

Message ID 20190905194150.17608-10-boris.brezillon@collabora.com
State New
Headers show
Series "panfrost: Rework the batch pipelining logic" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Boris Brezillon Sept. 5, 2019, 7:41 p.m.
* BO related functions/structs are now exposed in pan_bo.h instead of
  being spread in pan_screen.h/pan_resource.h
* cache related functions are no longer exposed
* panfrost_bo now has a ->screen field to avoid passing screen around
* the function names are made consistent (all BO related functions are
  prefixed with panfrost_bo_)
* release functions are no longer exposed, existing users are converted
  to use panfrost_bo_unreference() instead

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 src/gallium/drivers/panfrost/pan_allocate.c   |   5 +-
 src/gallium/drivers/panfrost/pan_allocate.h   |  20 --
 src/gallium/drivers/panfrost/pan_assemble.c   |   3 +-
 src/gallium/drivers/panfrost/pan_blend_cso.c  |   5 +-
 src/gallium/drivers/panfrost/pan_bo.c         | 236 +++++++++++++++++-
 src/gallium/drivers/panfrost/pan_bo.h         |  78 ++++++
 src/gallium/drivers/panfrost/pan_context.c    |  16 +-
 src/gallium/drivers/panfrost/pan_drm.c        | 187 +-------------
 src/gallium/drivers/panfrost/pan_instancing.c |   1 +
 src/gallium/drivers/panfrost/pan_job.c        |   7 +-
 src/gallium/drivers/panfrost/pan_mfbd.c       |   1 +
 src/gallium/drivers/panfrost/pan_resource.c   |  38 +--
 src/gallium/drivers/panfrost/pan_resource.h   |   8 +-
 src/gallium/drivers/panfrost/pan_screen.c     |   1 +
 src/gallium/drivers/panfrost/pan_screen.h     |  24 --
 src/gallium/drivers/panfrost/pan_sfbd.c       |   1 +
 src/gallium/drivers/panfrost/pan_varyings.c   |   1 +
 17 files changed, 338 insertions(+), 294 deletions(-)
 create mode 100644 src/gallium/drivers/panfrost/pan_bo.h

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/panfrost/pan_allocate.c b/src/gallium/drivers/panfrost/pan_allocate.c
index a22b1a5a88d6..beebb0bc6d7e 100644
--- a/src/gallium/drivers/panfrost/pan_allocate.c
+++ b/src/gallium/drivers/panfrost/pan_allocate.c
@@ -29,6 +29,7 @@ 
 #include <assert.h>
 #include <panfrost-misc.h>
 #include <panfrost-job.h>
+#include "pan_bo.h"
 #include "pan_context.h"
 
 /* TODO: What does this actually have to be? */
@@ -66,12 +67,12 @@  panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz)
                                TRANSIENT_SLAB_SIZE : ALIGN_POT(sz, 4096);
 
                 /* We can't reuse the current BO, but we can create a new one. */
-                bo = panfrost_drm_create_bo(screen, bo_sz, 0);
+                bo = panfrost_bo_create(screen, bo_sz, 0);
                 panfrost_batch_add_bo(batch, bo);
 
                 /* Creating a BO adds a reference, and then the job adds a
                  * second one. So we need to pop back one reference */
-                panfrost_bo_unreference(&screen->base, bo);
+                panfrost_bo_unreference(bo);
 
                 if (sz < TRANSIENT_SLAB_SIZE) {
                         batch->transient_bo = bo;
diff --git a/src/gallium/drivers/panfrost/pan_allocate.h b/src/gallium/drivers/panfrost/pan_allocate.h
index c0aff62df4a1..91c2af9c4f17 100644
--- a/src/gallium/drivers/panfrost/pan_allocate.h
+++ b/src/gallium/drivers/panfrost/pan_allocate.h
@@ -43,26 +43,6 @@  struct panfrost_transfer {
         mali_ptr gpu;
 };
 
-struct panfrost_bo {
-        /* Must be first for casting */
-        struct list_head link;
-
-        struct pipe_reference reference;
-
-        /* Mapping for the entire object (all levels) */
-        uint8_t *cpu;
-
-        /* GPU address for the object */
-        mali_ptr gpu;
-
-        /* Size of all entire trees */
-        size_t size;
-
-        int gem_handle;
-
-        uint32_t flags;
-};
-
 struct panfrost_transfer
 panfrost_allocate_transient(struct panfrost_context *ctx, size_t sz);
 
diff --git a/src/gallium/drivers/panfrost/pan_assemble.c b/src/gallium/drivers/panfrost/pan_assemble.c
index b57cd5ef6ad2..79c000367632 100644
--- a/src/gallium/drivers/panfrost/pan_assemble.c
+++ b/src/gallium/drivers/panfrost/pan_assemble.c
@@ -25,6 +25,7 @@ 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
+#include "pan_bo.h"
 #include "pan_context.h"
 
 #include "compiler/nir/nir.h"
@@ -82,7 +83,7 @@  panfrost_shader_compile(
          * I bet someone just thought that would be a cute pun. At least,
          * that's how I'd do it. */
 
-        state->bo = panfrost_drm_create_bo(screen, size, PAN_ALLOCATE_EXECUTE);
+        state->bo = panfrost_bo_create(screen, size, PAN_ALLOCATE_EXECUTE);
         memcpy(state->bo->cpu, dst, size);
         meta->shader = state->bo->gpu | program.first_tag;
 
diff --git a/src/gallium/drivers/panfrost/pan_blend_cso.c b/src/gallium/drivers/panfrost/pan_blend_cso.c
index ab49772f3ba3..69897be4f007 100644
--- a/src/gallium/drivers/panfrost/pan_blend_cso.c
+++ b/src/gallium/drivers/panfrost/pan_blend_cso.c
@@ -29,6 +29,7 @@ 
 #include "util/u_memory.h"
 #include "pan_blend_shaders.h"
 #include "pan_blending.h"
+#include "pan_bo.h"
 
 /* A given Gallium blend state can be encoded to the hardware in numerous,
  * dramatically divergent ways due to the interactions of blending with
@@ -272,12 +273,12 @@  panfrost_get_blend_for_context(struct panfrost_context *ctx, unsigned rti)
         final.shader.first_tag = shader->first_tag;
 
         /* Upload the shader */
-        final.shader.bo = panfrost_drm_create_bo(screen, shader->size, PAN_ALLOCATE_EXECUTE);
+        final.shader.bo = panfrost_bo_create(screen, shader->size, PAN_ALLOCATE_EXECUTE);
         memcpy(final.shader.bo->cpu, shader->buffer, shader->size);
 
         /* Pass BO ownership to job */
         panfrost_batch_add_bo(batch, final.shader.bo);
-        panfrost_bo_unreference(ctx->base.screen, final.shader.bo);
+        panfrost_bo_unreference(final.shader.bo);
 
         if (shader->patch_index) {
                 /* We have to specialize the blend shader to use constants, so
diff --git a/src/gallium/drivers/panfrost/pan_bo.c b/src/gallium/drivers/panfrost/pan_bo.c
index f2f49437a89f..1f87c18e9ad5 100644
--- a/src/gallium/drivers/panfrost/pan_bo.c
+++ b/src/gallium/drivers/panfrost/pan_bo.c
@@ -23,11 +23,20 @@ 
  * Authors (Collabora):
  *   Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
  */
+#include <stdio.h>
+#include <fcntl.h>
 #include <xf86drm.h>
 #include <pthread.h>
 #include "drm-uapi/panfrost_drm.h"
 
+#include "pan_bo.h"
 #include "pan_screen.h"
+#include "pan_util.h"
+#include "pandecode/decode.h"
+
+#include "os/os_mman.h"
+
+#include "util/u_inlines.h"
 #include "util/u_math.h"
 
 /* This file implements a userspace BO cache. Allocating and freeing
@@ -45,6 +54,51 @@ 
  * around the linked list.
  */
 
+static struct panfrost_bo *
+panfrost_bo_alloc(struct panfrost_screen *screen, size_t size,
+                  uint32_t flags)
+{
+        struct drm_panfrost_create_bo create_bo = { .size = size };
+        struct panfrost_bo *bo;
+        int ret;
+
+        if (screen->kernel_version->version_major > 1 ||
+            screen->kernel_version->version_minor >= 1) {
+                if (flags & PAN_ALLOCATE_GROWABLE)
+                        create_bo.flags |= PANFROST_BO_HEAP;
+                if (!(flags & PAN_ALLOCATE_EXECUTE))
+                        create_bo.flags |= PANFROST_BO_NOEXEC;
+        }
+
+        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
+        if (ret)
+                return NULL;
+
+        bo = rzalloc(screen, struct panfrost_bo);
+        assert(bo);
+        bo->size = create_bo.size;
+        bo->gpu = create_bo.offset;
+        bo->gem_handle = create_bo.handle;
+        bo->flags = flags;
+        bo->screen = screen;
+        return bo;
+}
+
+static void
+panfrost_bo_free(struct panfrost_bo *bo)
+{
+        struct drm_gem_close gem_close = { .handle = bo->gem_handle };
+        int ret;
+
+        ret = drmIoctl(bo->screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
+        if (ret) {
+                fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed: %m\n");
+                assert(0);
+        }
+
+        ralloc_free(bo);
+}
+
 /* Helper to calculate the bucket index of a BO */
 
 static unsigned
@@ -80,10 +134,9 @@  pan_bucket(struct panfrost_screen *screen, unsigned size)
  * cache. If it fails, it returns NULL signaling the caller to allocate a new
  * BO. */
 
-struct panfrost_bo *
-panfrost_bo_cache_fetch(
-                struct panfrost_screen *screen,
-                size_t size, uint32_t flags)
+static struct panfrost_bo *
+panfrost_bo_cache_fetch(struct panfrost_screen *screen,
+                        size_t size, uint32_t flags)
 {
         pthread_mutex_lock(&screen->bo_cache_lock);
         struct list_head *bucket = pan_bucket(screen, size);
@@ -105,7 +158,7 @@  panfrost_bo_cache_fetch(
 
                         ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MADVISE, &madv);
                         if (!ret && !madv.retained) {
-                                panfrost_drm_release_bo(screen, entry, false);
+                                panfrost_bo_free(entry);
                                 continue;
                         }
                         /* Let's go! */
@@ -121,11 +174,14 @@  panfrost_bo_cache_fetch(
 /* Tries to add a BO to the cache. Returns if it was
  * successful */
 
-bool
-panfrost_bo_cache_put(
-                struct panfrost_screen *screen,
-                struct panfrost_bo *bo)
+static bool
+panfrost_bo_cache_put(struct panfrost_bo *bo)
 {
+        struct panfrost_screen *screen = bo->screen;
+
+        if (bo->dont_reuse)
+                return false;
+
         pthread_mutex_lock(&screen->bo_cache_lock);
         struct list_head *bucket = pan_bucket(screen, bo->size);
         struct drm_panfrost_madvise madv;
@@ -150,8 +206,7 @@  panfrost_bo_cache_put(
  * OS) */
 
 void
-panfrost_bo_cache_evict_all(
-                struct panfrost_screen *screen)
+panfrost_bo_cache_evict_all(struct panfrost_screen *screen)
 {
         pthread_mutex_lock(&screen->bo_cache_lock);
         for (unsigned i = 0; i < ARRAY_SIZE(screen->bo_cache); ++i) {
@@ -159,9 +214,166 @@  panfrost_bo_cache_evict_all(
 
                 list_for_each_entry_safe(struct panfrost_bo, entry, bucket, link) {
                         list_del(&entry->link);
-                        panfrost_drm_release_bo(screen, entry, false);
+                        panfrost_bo_free(entry);
                 }
         }
         pthread_mutex_unlock(&screen->bo_cache_lock);
 }
 
+void
+panfrost_bo_mmap(struct panfrost_bo *bo)
+{
+        struct drm_panfrost_mmap_bo mmap_bo = { .handle = bo->gem_handle };
+        int ret;
+
+        if (bo->cpu)
+                return;
+
+        ret = drmIoctl(bo->screen->fd, DRM_IOCTL_PANFROST_MMAP_BO, &mmap_bo);
+        if (ret) {
+                fprintf(stderr, "DRM_IOCTL_PANFROST_MMAP_BO failed: %m\n");
+                assert(0);
+        }
+
+        bo->cpu = os_mmap(NULL, bo->size, PROT_READ | PROT_WRITE, MAP_SHARED,
+                          bo->screen->fd, mmap_bo.offset);
+        if (bo->cpu == MAP_FAILED) {
+                fprintf(stderr, "mmap failed: %p %m\n", bo->cpu);
+                assert(0);
+        }
+
+        /* Record the mmap if we're tracing */
+        if (pan_debug & PAN_DBG_TRACE)
+                pandecode_inject_mmap(bo->gpu, bo->cpu, bo->size, NULL);
+}
+
+void
+panfrost_bo_munmap(struct panfrost_bo *bo)
+{
+        if (!bo->cpu)
+                return;
+
+        if (os_munmap((void *) (uintptr_t)bo->cpu, bo->size)) {
+                perror("munmap");
+                abort();
+        }
+
+        bo->cpu = NULL;
+}
+
+struct panfrost_bo *
+panfrost_bo_create(struct panfrost_screen *screen, size_t size,
+                   uint32_t flags)
+{
+        struct panfrost_bo *bo;
+
+        /* Kernel will fail (confusingly) with EPERM otherwise */
+        assert(size > 0);
+
+        /* To maximize BO cache usage, don't allocate tiny BOs */
+        size = MAX2(size, 4096);
+
+        /* GROWABLE BOs cannot be mmapped */
+        if (flags & PAN_ALLOCATE_GROWABLE)
+                assert(flags & PAN_ALLOCATE_INVISIBLE);
+
+        /* Before creating a BO, we first want to check the cache, otherwise,
+         * the cache misses and we need to allocate a BO fresh from the kernel
+         */
+        bo = panfrost_bo_cache_fetch(screen, size, flags);
+        if (!bo)
+                bo = panfrost_bo_alloc(screen, size, flags);
+
+        assert(bo);
+
+        /* Only mmap now if we know we need to. For CPU-invisible buffers, we
+         * never map since we don't care about their contents; they're purely
+         * for GPU-internal use. But we do trace them anyway.
+         */
+        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
+                panfrost_bo_mmap(bo);
+	else if ((flags & PAN_ALLOCATE_INVISIBLE) && (pan_debug & PAN_DBG_TRACE))
+                pandecode_inject_mmap(bo->gpu, NULL, bo->size, NULL);
+
+        pipe_reference_init(&bo->reference, 1);
+        return bo;
+}
+
+static void
+panfrost_bo_release(struct panfrost_bo *bo)
+{
+        if (!bo)
+                return;
+
+        /* Rather than freeing the BO now, we'll cache the BO for later
+         * allocations if we're allowed to */
+
+        panfrost_bo_munmap(bo);
+
+        if (panfrost_bo_cache_put(bo))
+                return;
+
+        panfrost_bo_free(bo);
+}
+
+void
+panfrost_bo_reference(struct panfrost_bo *bo)
+{
+        if (bo)
+                pipe_reference(NULL, &bo->reference);
+}
+
+void
+panfrost_bo_unreference(struct panfrost_bo *bo)
+{
+        if (!bo)
+                return;
+
+        /* When the reference count goes to zero, we need to cleanup */
+        if (pipe_reference(&bo->reference, NULL))
+                panfrost_bo_release(bo);
+}
+
+struct panfrost_bo *
+panfrost_bo_import(struct panfrost_screen *screen, int fd)
+{
+        struct panfrost_bo *bo = rzalloc(screen, struct panfrost_bo);
+        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
+        ASSERTED int ret;
+        unsigned gem_handle;
+
+        ret = drmPrimeFDToHandle(screen->fd, fd, &gem_handle);
+        assert(!ret);
+
+        get_bo_offset.handle = gem_handle;
+        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_GET_BO_OFFSET, &get_bo_offset);
+        assert(!ret);
+
+        bo->screen = screen;
+        bo->gem_handle = gem_handle;
+        bo->gpu = (mali_ptr)get_bo_offset.offset;
+        bo->size = lseek(fd, 0, SEEK_END);
+        bo->dont_reuse = true;
+        assert(bo->size > 0);
+        pipe_reference_init(&bo->reference, 1);
+
+        // TODO map and unmap on demand?
+        panfrost_bo_mmap(bo);
+        return bo;
+}
+
+int
+panfrost_bo_export(struct panfrost_bo *bo)
+{
+        struct drm_prime_handle args = {
+                .handle = bo->gem_handle,
+                .flags = DRM_CLOEXEC,
+        };
+
+        int ret = drmIoctl(bo->screen->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args);
+        if (ret == -1)
+                return -1;
+
+        bo->dont_reuse = true;
+        return args.fd;
+}
diff --git a/src/gallium/drivers/panfrost/pan_bo.h b/src/gallium/drivers/panfrost/pan_bo.h
new file mode 100644
index 000000000000..b4bff645e257
--- /dev/null
+++ b/src/gallium/drivers/panfrost/pan_bo.h
@@ -0,0 +1,78 @@ 
+/*
+ * © Copyright 2019 Alyssa Rosenzweig
+ * © Copyright 2019 Collabora, Ltd.
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+ * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#ifndef __PAN_BO_H__
+#define __PAN_BO_H__
+
+#include <panfrost-misc.h>
+#include "pipe/p_state.h"
+#include "util/list.h"
+
+struct panfrost_bo {
+        /* Must be first for casting */
+        struct list_head link;
+
+        struct pipe_reference reference;
+
+        struct panfrost_screen *screen;
+
+        /* Mapping for the entire object (all levels) */
+        uint8_t *cpu;
+
+        /* GPU address for the object */
+        mali_ptr gpu;
+
+        /* Size of all entire trees */
+        size_t size;
+
+        int gem_handle;
+
+        /* The BO should not be added to the re-use cache when released. Should
+         * be true for imported/exported BOs.
+         */
+        bool dont_reuse;
+
+        uint32_t flags;
+};
+
+void
+panfrost_bo_mmap(struct panfrost_bo *bo);
+void
+panfrost_bo_munmap(struct panfrost_bo *bo);
+void
+panfrost_bo_reference(struct panfrost_bo *bo);
+void
+panfrost_bo_unreference(struct panfrost_bo *bo);
+struct panfrost_bo *
+panfrost_bo_create(struct panfrost_screen *screen, size_t size,
+                   uint32_t flags);
+struct panfrost_bo *
+panfrost_bo_import(struct panfrost_screen *screen, int fd);
+int
+panfrost_bo_export(struct panfrost_bo *bo);
+void
+panfrost_bo_cache_evict_all(struct panfrost_screen *screen);
+
+#endif /* __PAN_BO_H__ */
diff --git a/src/gallium/drivers/panfrost/pan_context.c b/src/gallium/drivers/panfrost/pan_context.c
index 0fb4c2584e40..f0cd8cdb12ea 100644
--- a/src/gallium/drivers/panfrost/pan_context.c
+++ b/src/gallium/drivers/panfrost/pan_context.c
@@ -27,6 +27,7 @@ 
 #include <sys/poll.h>
 #include <errno.h>
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "pan_format.h"
 
@@ -1889,7 +1890,7 @@  panfrost_delete_shader_state(
 
         for (unsigned i = 0; i < cso->variant_count; ++i) {
                 struct panfrost_shader_state *shader_state = &cso->variants[i];
-                panfrost_bo_unreference(pctx->screen, shader_state->bo);
+                panfrost_bo_unreference(shader_state->bo);
                 shader_state->bo = NULL;
         }
 
@@ -2556,7 +2557,6 @@  static void
 panfrost_destroy(struct pipe_context *pipe)
 {
         struct panfrost_context *panfrost = pan_context(pipe);
-        struct panfrost_screen *screen = pan_screen(pipe->screen);
 
         if (panfrost->blitter)
                 util_blitter_destroy(panfrost->blitter);
@@ -2564,9 +2564,9 @@  panfrost_destroy(struct pipe_context *pipe)
         if (panfrost->blitter_wallpaper)
                 util_blitter_destroy(panfrost->blitter_wallpaper);
 
-        panfrost_drm_release_bo(screen, panfrost->scratchpad, false);
-        panfrost_drm_release_bo(screen, panfrost->tiler_heap, false);
-        panfrost_drm_release_bo(screen, panfrost->tiler_dummy, false);
+        panfrost_bo_unreference(panfrost->scratchpad);
+        panfrost_bo_unreference(panfrost->tiler_heap);
+        panfrost_bo_unreference(panfrost->tiler_dummy);
 
         ralloc_free(pipe);
 }
@@ -2749,11 +2749,11 @@  panfrost_setup_hardware(struct panfrost_context *ctx)
         struct pipe_context *gallium = (struct pipe_context *) ctx;
         struct panfrost_screen *screen = pan_screen(gallium->screen);
 
-        ctx->scratchpad = panfrost_drm_create_bo(screen, 64 * 4 * 4096, 0);
-        ctx->tiler_heap = panfrost_drm_create_bo(screen, 4096 * 4096,
+        ctx->scratchpad = panfrost_bo_create(screen, 64 * 4 * 4096, 0);
+        ctx->tiler_heap = panfrost_bo_create(screen, 4096 * 4096,
                                                  PAN_ALLOCATE_INVISIBLE |
                                                  PAN_ALLOCATE_GROWABLE);
-        ctx->tiler_dummy = panfrost_drm_create_bo(screen, 4096,
+        ctx->tiler_dummy = panfrost_bo_create(screen, 4096,
                                                   PAN_ALLOCATE_INVISIBLE);
         assert(ctx->scratchpad && ctx->tiler_heap && ctx->tiler_dummy);
 }
diff --git a/src/gallium/drivers/panfrost/pan_drm.c b/src/gallium/drivers/panfrost/pan_drm.c
index e7dcd2e58751..e4b75fad4078 100644
--- a/src/gallium/drivers/panfrost/pan_drm.c
+++ b/src/gallium/drivers/panfrost/pan_drm.c
@@ -32,198 +32,13 @@ 
 #include "util/os_time.h"
 #include "os/os_mman.h"
 
+#include "pan_bo.h"
 #include "pan_screen.h"
 #include "pan_resource.h"
 #include "pan_context.h"
 #include "pan_util.h"
 #include "pandecode/decode.h"
 
-void
-panfrost_drm_mmap_bo(struct panfrost_screen *screen, struct panfrost_bo *bo)
-{
-        struct drm_panfrost_mmap_bo mmap_bo = { .handle = bo->gem_handle };
-        int ret;
-
-        if (bo->cpu)
-                return;
-
-        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MMAP_BO, &mmap_bo);
-        if (ret) {
-                fprintf(stderr, "DRM_IOCTL_PANFROST_MMAP_BO failed: %m\n");
-                assert(0);
-        }
-
-        bo->cpu = os_mmap(NULL, bo->size, PROT_READ | PROT_WRITE, MAP_SHARED,
-                          screen->fd, mmap_bo.offset);
-        if (bo->cpu == MAP_FAILED) {
-                fprintf(stderr, "mmap failed: %p %m\n", bo->cpu);
-                assert(0);
-        }
-
-        /* Record the mmap if we're tracing */
-        if (pan_debug & PAN_DBG_TRACE)
-                pandecode_inject_mmap(bo->gpu, bo->cpu, bo->size, NULL);
-}
-
-static void
-panfrost_drm_munmap_bo(struct panfrost_screen *screen, struct panfrost_bo *bo)
-{
-        if (!bo->cpu)
-                return;
-
-        if (os_munmap((void *) (uintptr_t)bo->cpu, bo->size)) {
-                perror("munmap");
-                abort();
-        }
-
-        bo->cpu = NULL;
-}
-
-struct panfrost_bo *
-panfrost_drm_create_bo(struct panfrost_screen *screen, size_t size,
-                       uint32_t flags)
-{
-        struct panfrost_bo *bo;
-
-        /* Kernel will fail (confusingly) with EPERM otherwise */
-        assert(size > 0);
-
-        /* To maximize BO cache usage, don't allocate tiny BOs */
-        size = MAX2(size, 4096);
-
-        /* GROWABLE BOs cannot be mmapped */
-        if (flags & PAN_ALLOCATE_GROWABLE)
-                assert(flags & PAN_ALLOCATE_INVISIBLE);
-
-        unsigned translated_flags = 0;
-
-        if (screen->kernel_version->version_major > 1 ||
-            screen->kernel_version->version_minor >= 1) {
-                if (flags & PAN_ALLOCATE_GROWABLE)
-                        translated_flags |= PANFROST_BO_HEAP;
-                if (!(flags & PAN_ALLOCATE_EXECUTE))
-                        translated_flags |= PANFROST_BO_NOEXEC;
-        }
-
-        struct drm_panfrost_create_bo create_bo = {
-                .size = size,
-                .flags = translated_flags,
-        };
-
-        /* Before creating a BO, we first want to check the cache */
-
-        bo = panfrost_bo_cache_fetch(screen, size, flags);
-
-        if (bo == NULL) {
-                /* Otherwise, the cache misses and we need to allocate a BO fresh from
-                 * the kernel */
-
-                int ret;
-
-                ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
-                if (ret) {
-                        fprintf(stderr, "DRM_IOCTL_PANFROST_CREATE_BO failed: %m\n");
-                        assert(0);
-                }
-
-                /* We have a BO allocated from the kernel; fill in the userspace
-                 * version */
-
-                bo = rzalloc(screen, struct panfrost_bo);
-                bo->size = create_bo.size;
-                bo->gpu = create_bo.offset;
-                bo->gem_handle = create_bo.handle;
-                bo->flags = flags;
-        }
-
-        /* Only mmap now if we know we need to. For CPU-invisible buffers, we
-         * never map since we don't care about their contents; they're purely
-         * for GPU-internal use. But we do trace them anyway. */
-
-        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
-                panfrost_drm_mmap_bo(screen, bo);
-        else if (flags & PAN_ALLOCATE_INVISIBLE) {
-                if (pan_debug & PAN_DBG_TRACE)
-                        pandecode_inject_mmap(bo->gpu, NULL, bo->size, NULL);
-        }
-
-        pipe_reference_init(&bo->reference, 1);
-        return bo;
-}
-
-void
-panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo, bool cacheable)
-{
-        if (!bo)
-                return;
-
-        struct drm_gem_close gem_close = { .handle = bo->gem_handle };
-        int ret;
-
-        /* Rather than freeing the BO now, we'll cache the BO for later
-         * allocations if we're allowed to */
-
-        panfrost_drm_munmap_bo(screen, bo);
-
-        if (cacheable) {
-                bool cached = panfrost_bo_cache_put(screen, bo);
-
-                if (cached)
-                        return;
-        }
-
-        /* Otherwise, if the BO wasn't cached, we'll legitimately free the BO */
-
-        ret = drmIoctl(screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
-        if (ret) {
-                fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed: %m\n");
-                assert(0);
-        }
-
-        ralloc_free(bo);
-}
-
-struct panfrost_bo *
-panfrost_drm_import_bo(struct panfrost_screen *screen, int fd)
-{
-        struct panfrost_bo *bo = rzalloc(screen, struct panfrost_bo);
-        struct drm_panfrost_get_bo_offset get_bo_offset = {0,};
-        ASSERTED int ret;
-        unsigned gem_handle;
-
-        ret = drmPrimeFDToHandle(screen->fd, fd, &gem_handle);
-        assert(!ret);
-
-        get_bo_offset.handle = gem_handle;
-        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_GET_BO_OFFSET, &get_bo_offset);
-        assert(!ret);
-
-        bo->gem_handle = gem_handle;
-        bo->gpu = (mali_ptr) get_bo_offset.offset;
-        bo->size = lseek(fd, 0, SEEK_END);
-        assert(bo->size > 0);
-        pipe_reference_init(&bo->reference, 1);
-
-        // TODO map and unmap on demand?
-        panfrost_drm_mmap_bo(screen, bo);
-        return bo;
-}
-
-int
-panfrost_drm_export_bo(struct panfrost_screen *screen, const struct panfrost_bo *bo)
-{
-        struct drm_prime_handle args = {
-                .handle = bo->gem_handle,
-                .flags = DRM_CLOEXEC,
-        };
-
-        int ret = drmIoctl(screen->fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args);
-        if (ret == -1)
-                return -1;
-
-        return args.fd;
-}
-
 static int
 panfrost_drm_submit_batch(struct panfrost_batch *batch, u64 first_job_desc,
                           int reqs)
diff --git a/src/gallium/drivers/panfrost/pan_instancing.c b/src/gallium/drivers/panfrost/pan_instancing.c
index 44fe0a344aab..bcdcbe429394 100644
--- a/src/gallium/drivers/panfrost/pan_instancing.c
+++ b/src/gallium/drivers/panfrost/pan_instancing.c
@@ -23,6 +23,7 @@ 
  *
  */
 
+#include "pan_bo.h"
 #include "pan_context.h"
 
 /* See mali_job for notes on how this works. But basically, for small vertex
diff --git a/src/gallium/drivers/panfrost/pan_job.c b/src/gallium/drivers/panfrost/pan_job.c
index 58b494108e2d..7c40bcee0fca 100644
--- a/src/gallium/drivers/panfrost/pan_job.c
+++ b/src/gallium/drivers/panfrost/pan_job.c
@@ -25,6 +25,7 @@ 
 
 #include <assert.h>
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "util/hash_table.h"
 #include "util/ralloc.h"
@@ -62,11 +63,11 @@  panfrost_free_batch(struct panfrost_batch *batch)
 
         set_foreach(batch->bos, entry) {
                 struct panfrost_bo *bo = (struct panfrost_bo *)entry->key;
-                panfrost_bo_unreference(ctx->base.screen, bo);
+                panfrost_bo_unreference(bo);
         }
 
         /* Unreference the polygon list */
-        panfrost_bo_unreference(ctx->base.screen, batch->polygon_list);
+        panfrost_bo_unreference(batch->polygon_list);
 
         _mesa_hash_table_remove_key(ctx->batches, &batch->key);
 
@@ -170,7 +171,7 @@  panfrost_batch_get_polygon_list(struct panfrost_batch *batch, unsigned size)
 
                 /* Create the BO as invisible, as there's no reason to map */
 
-                batch->polygon_list = panfrost_drm_create_bo(screen,
+                batch->polygon_list = panfrost_bo_create(screen,
                                 size, PAN_ALLOCATE_INVISIBLE);
         }
 
diff --git a/src/gallium/drivers/panfrost/pan_mfbd.c b/src/gallium/drivers/panfrost/pan_mfbd.c
index 3ad22f82c4ab..618ebd3c4a19 100644
--- a/src/gallium/drivers/panfrost/pan_mfbd.c
+++ b/src/gallium/drivers/panfrost/pan_mfbd.c
@@ -22,6 +22,7 @@ 
  *
  */
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "pan_util.h"
 #include "pan_format.h"
diff --git a/src/gallium/drivers/panfrost/pan_resource.c b/src/gallium/drivers/panfrost/pan_resource.c
index 0be6b2b1c330..a5869768f9de 100644
--- a/src/gallium/drivers/panfrost/pan_resource.c
+++ b/src/gallium/drivers/panfrost/pan_resource.c
@@ -41,6 +41,7 @@ 
 #include "util/u_transfer_helper.h"
 #include "util/u_gen_mipmap.h"
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "pan_screen.h"
 #include "pan_resource.h"
@@ -81,7 +82,7 @@  panfrost_resource_from_handle(struct pipe_screen *pscreen,
         pipe_reference_init(&prsc->reference, 1);
         prsc->screen = pscreen;
 
-        rsc->bo = panfrost_drm_import_bo(screen, whandle->handle);
+        rsc->bo = panfrost_bo_import(screen, whandle->handle);
         rsc->slices[0].stride = whandle->stride;
         rsc->slices[0].initialized = true;
         panfrost_resource_reset_damage(rsc);
@@ -133,7 +134,7 @@  panfrost_resource_get_handle(struct pipe_screen *pscreen,
 
                         return true;
                 } else {
-                        int fd = panfrost_drm_export_bo(screen, rsrc->bo);
+                        int fd = panfrost_bo_export(rsrc->bo);
 
                         if (fd < 0)
                                 return false;
@@ -412,7 +413,7 @@  panfrost_resource_create_bo(struct panfrost_screen *screen, struct panfrost_reso
 
         /* We create a BO immediately but don't bother mapping, since we don't
          * care to map e.g. FBOs which the CPU probably won't touch */
-        pres->bo = panfrost_drm_create_bo(screen, bo_size, PAN_ALLOCATE_DELAY_MMAP);
+        pres->bo = panfrost_bo_create(screen, bo_size, PAN_ALLOCATE_DELAY_MMAP);
 }
 
 void
@@ -521,25 +522,6 @@  panfrost_resource_create(struct pipe_screen *screen,
         return (struct pipe_resource *)so;
 }
 
-void
-panfrost_bo_reference(struct panfrost_bo *bo)
-{
-        if (bo)
-                pipe_reference(NULL, &bo->reference);
-}
-
-void
-panfrost_bo_unreference(struct pipe_screen *screen, struct panfrost_bo *bo)
-{
-        if (!bo)
-                return;
-
-        /* When the reference count goes to zero, we need to cleanup */
-
-        if (pipe_reference(&bo->reference, NULL))
-                panfrost_drm_release_bo(pan_screen(screen), bo, true);
-}
-
 static void
 panfrost_resource_destroy(struct pipe_screen *screen,
                           struct pipe_resource *pt)
@@ -551,7 +533,7 @@  panfrost_resource_destroy(struct pipe_screen *screen,
                 renderonly_scanout_destroy(rsrc->scanout, pscreen->ro);
 
         if (rsrc->bo)
-                panfrost_bo_unreference(screen, rsrc->bo);
+                panfrost_bo_unreference(rsrc->bo);
 
         util_range_destroy(&rsrc->valid_buffer_range);
         ralloc_free(rsrc);
@@ -579,11 +561,7 @@  panfrost_transfer_map(struct pipe_context *pctx,
         *out_transfer = &transfer->base;
 
         /* If we haven't already mmaped, now's the time */
-
-        if (!bo->cpu) {
-                struct panfrost_screen *screen = pan_screen(pctx->screen);
-                panfrost_drm_mmap_bo(screen, bo);
-        }
+        panfrost_bo_mmap(bo);
 
         /* Check if we're bound for rendering and this is a read pixels. If so,
          * we need to flush */
@@ -861,8 +839,8 @@  panfrost_resource_hint_layout(
 
         /* If we grew in size, reallocate the BO */
         if (new_size > rsrc->bo->size) {
-                panfrost_drm_release_bo(screen, rsrc->bo, true);
-                rsrc->bo = panfrost_drm_create_bo(screen, new_size, PAN_ALLOCATE_DELAY_MMAP);
+                panfrost_bo_unreference(rsrc->bo);
+                rsrc->bo = panfrost_bo_create(screen, new_size, PAN_ALLOCATE_DELAY_MMAP);
         }
 }
 
diff --git a/src/gallium/drivers/panfrost/pan_resource.h b/src/gallium/drivers/panfrost/pan_resource.h
index 6ed3d1fd60e8..aca3c70952b1 100644
--- a/src/gallium/drivers/panfrost/pan_resource.h
+++ b/src/gallium/drivers/panfrost/pan_resource.h
@@ -32,6 +32,8 @@ 
 #include "drm-uapi/drm.h"
 #include "util/u_range.h"
 
+struct panfrost_bo;
+
 /* Describes the memory layout of a BO */
 
 enum panfrost_memory_layout {
@@ -57,12 +59,6 @@  struct panfrost_slice {
         bool initialized;
 };
 
-void
-panfrost_bo_reference(struct panfrost_bo *bo);
-
-void
-panfrost_bo_unreference(struct pipe_screen *screen, struct panfrost_bo *bo);
-
 struct panfrost_resource {
         struct pipe_resource base;
         struct {
diff --git a/src/gallium/drivers/panfrost/pan_screen.c b/src/gallium/drivers/panfrost/pan_screen.c
index 5dcceac370d5..50a27bad1767 100644
--- a/src/gallium/drivers/panfrost/pan_screen.c
+++ b/src/gallium/drivers/panfrost/pan_screen.c
@@ -42,6 +42,7 @@ 
 
 #include "drm-uapi/drm_fourcc.h"
 
+#include "pan_bo.h"
 #include "pan_screen.h"
 #include "pan_resource.h"
 #include "pan_public.h"
diff --git a/src/gallium/drivers/panfrost/pan_screen.h b/src/gallium/drivers/panfrost/pan_screen.h
index 96044b8c8b90..aab141a563c2 100644
--- a/src/gallium/drivers/panfrost/pan_screen.h
+++ b/src/gallium/drivers/panfrost/pan_screen.h
@@ -120,17 +120,6 @@  pan_screen(struct pipe_screen *p)
         return (struct panfrost_screen *)p;
 }
 
-struct panfrost_bo *
-panfrost_drm_create_bo(struct panfrost_screen *screen, size_t size,
-                       uint32_t flags);
-void
-panfrost_drm_mmap_bo(struct panfrost_screen *screen, struct panfrost_bo *bo);
-void
-panfrost_drm_release_bo(struct panfrost_screen *screen, struct panfrost_bo *bo, bool cacheable);
-struct panfrost_bo *
-panfrost_drm_import_bo(struct panfrost_screen *screen, int fd);
-int
-panfrost_drm_export_bo(struct panfrost_screen *screen, const struct panfrost_bo *bo);
 int
 panfrost_drm_submit_vs_fs_batch(struct panfrost_batch *batch, bool has_draws);
 void
@@ -149,18 +138,5 @@  panfrost_drm_fence_finish(struct pipe_screen *pscreen,
                           struct pipe_context *ctx,
                           struct pipe_fence_handle *fence,
                           uint64_t timeout);
-struct panfrost_bo *
-panfrost_bo_cache_fetch(
-                struct panfrost_screen *screen,
-                size_t size, uint32_t flags);
-
-bool
-panfrost_bo_cache_put(
-                struct panfrost_screen *screen,
-                struct panfrost_bo *bo);
-
-void
-panfrost_bo_cache_evict_all(
-                struct panfrost_screen *screen);
 
 #endif /* PAN_SCREEN_H */
diff --git a/src/gallium/drivers/panfrost/pan_sfbd.c b/src/gallium/drivers/panfrost/pan_sfbd.c
index f58c054c8f24..2d12904e9393 100644
--- a/src/gallium/drivers/panfrost/pan_sfbd.c
+++ b/src/gallium/drivers/panfrost/pan_sfbd.c
@@ -22,6 +22,7 @@ 
  *
  */
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "pan_util.h"
 #include "pan_format.h"
diff --git a/src/gallium/drivers/panfrost/pan_varyings.c b/src/gallium/drivers/panfrost/pan_varyings.c
index 12760109b7cb..02f2e670eaf6 100644
--- a/src/gallium/drivers/panfrost/pan_varyings.c
+++ b/src/gallium/drivers/panfrost/pan_varyings.c
@@ -23,6 +23,7 @@ 
  *
  */
 
+#include "pan_bo.h"
 #include "pan_context.h"
 #include "util/u_prim.h"
 

Comments

> +static struct panfrost_bo *
> +panfrost_bo_alloc(struct panfrost_screen *screen, size_t size,
> +                  uint32_t flags)
> +{
...
> +        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
> +        if (ret)
> +                return NULL;

I notice this had a print to stderr before with an assertion out, but
now it fails silently. Is this change of behaviour intentional? BO
creation would previously return a valid BO gauranteed. This is no
longer so obviously true -- although I see we later assert that the
return is non-NULL in the caller.

Could you help me understand the new logic a bit? Thank you!

> +        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
> +                panfrost_bo_mmap(bo);
> +	else if ((flags & PAN_ALLOCATE_INVISIBLE) && (pan_debug & PAN_DBG_TRACE))

I think the spacing got wacky here (on the beginning of the last line)

> +static void
> +panfrost_bo_release(struct panfrost_bo *bo)
> +{
> +
> +        /* Rather than freeing the BO now, we'll cache the BO for later
> +         * allocations if we're allowed to */
> +
> +        panfrost_bo_munmap(bo);
> +
> +        if (panfrost_bo_cache_put(bo))
> +                return;
> +
> +        panfrost_bo_free(bo);
> +}

I see we now have the distinction between panfrost_bo_release (cached)
and panfrost_bo_free (uncached). I'm worried the distinction might not
be obvious to future Panfrost hackers.

Could you add a comment above each function clarifying the cache
behaviour?

---------------------------------------------

Other than these, the cleanup in general seems like a good idea. But in
general, please try to split up patches like this to aid reviewin. Thank
you!
On Thu, 5 Sep 2019 16:31:04 -0400
Alyssa Rosenzweig <alyssa@rosenzweig.io> wrote:

> > +static struct panfrost_bo *
> > +panfrost_bo_alloc(struct panfrost_screen *screen, size_t size,
> > +                  uint32_t flags)
> > +{  
> ...
> > +        ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
> > +        if (ret)
> > +                return NULL;  
> 
> I notice this had a print to stderr before with an assertion out, but
> now it fails silently. Is this change of behaviour intentional? 

It is.

> BO
> creation would previously return a valid BO gauranteed. This is no
> longer so obviously true -- although I see we later assert that the
> return is non-NULL in the caller.
> 
> Could you help me understand the new logic a bit? Thank you!
> 

The rationale behind this change being that panfrost_bo_alloc() will
not be our last option (see patch 9). I can add the fprintf() back in
this patch, and move it to the caller in patch 9 if you prefer.

> > +        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
> > +                panfrost_bo_mmap(bo);
> > +	else if ((flags & PAN_ALLOCATE_INVISIBLE) && (pan_debug & PAN_DBG_TRACE))  
> 
> I think the spacing got wacky here (on the beginning of the last line)
>

Will fix that.
 
> > +static void
> > +panfrost_bo_release(struct panfrost_bo *bo)
> > +{
> > +
> > +        /* Rather than freeing the BO now, we'll cache the BO for later
> > +         * allocations if we're allowed to */
> > +
> > +        panfrost_bo_munmap(bo);
> > +
> > +        if (panfrost_bo_cache_put(bo))
> > +                return;
> > +
> > +        panfrost_bo_free(bo);
> > +}  
> 
> I see we now have the distinction between panfrost_bo_release (cached)
> and panfrost_bo_free (uncached). I'm worried the distinction might not
> be obvious to future Panfrost hackers.
> 
> Could you add a comment above each function clarifying the cache
> behaviour?

Looks like the _release() function can be inlined in
panfrost_bo_unreference(). I'm still not happy with the
panfrost_bo_create() name though. Maybe we should rename this one into
panfrost_get_bo().

> 
> ---------------------------------------------
> 
> Other than these, the cleanup in general seems like a good idea. But in
> general, please try to split up patches like this to aid reviewin. Thank
> you!

Yes, I guess I got tired splitting things up and decided to group
changes that were kind of related in a single patch (also don't like
having 30+ patch series). I'll split that up in v4.

Thanks for the review!

Boris
> > I notice this had a print to stderr before with an assertion out, but
> > now it fails silently. Is this change of behaviour intentional? 
> 
> It is.

Alright! :-)

> > BO
> > creation would previously return a valid BO gauranteed. This is no
> > longer so obviously true -- although I see we later assert that the
> > return is non-NULL in the caller.
> > 
> > Could you help me understand the new logic a bit? Thank you!
> 
> The rationale behind this change being that panfrost_bo_alloc() will
> not be our last option (see patch 9). I can add the fprintf() back in
> this patch, and move it to the caller in patch 9 if you prefer.

Ah, that makes sense; thank you for clarifying!

> > > +        if (!(flags & (PAN_ALLOCATE_INVISIBLE | PAN_ALLOCATE_DELAY_MMAP)))
> > > +                panfrost_bo_mmap(bo);
> > > +	else if ((flags & PAN_ALLOCATE_INVISIBLE) && (pan_debug & PAN_DBG_TRACE))  
> > 
> > I think the spacing got wacky here (on the beginning of the last line)
> >
> 
> Will fix that.

+1

> > I see we now have the distinction between panfrost_bo_release (cached)
> > and panfrost_bo_free (uncached). I'm worried the distinction might not
> > be obvious to future Panfrost hackers.
> > 
> > Could you add a comment above each function clarifying the cache
> > behaviour?
> 
> Looks like the _release() function can be inlined in
> panfrost_bo_unreference(). I'm still not happy with the
> panfrost_bo_create() name though. Maybe we should rename this one into
> panfrost_get_bo().

I think splitting free/release to separate functions is good; I don't
know that inlining _release() is inherently needed. I'm just wondering
if we want a comment to make the distinction clear for future denizens
trying to figure out which routine to use --- although inlining one
would certainly solve that part...

> Yes, I guess I got tired splitting things up and decided to group
> changes that were kind of related in a single patch (also don't like
> having 30+ patch series). I'll split that up in v4.

No need to split it for v4; just a general note for future series :)