On Tue, 2 Jul 2019 at 15:24, Boris Brezillon
<boris.brezil...@collabora.com> wrote:
>
> There's no point duplicating the code, and it will help us simplify
> the bo_handles[] filling logic in panfrost_drm_submit_job().
Looks good but, could we drop panfrost_memory completely? Other
drivers seem to do fine wthout such a thing.
Thanks,
Tomeu
> Signed-off-by: Boris Brezillon <boris.brezil...@collabora.com>
> ---
> src/gallium/drivers/panfrost/pan_allocate.c | 21 +++---
> src/gallium/drivers/panfrost/pan_allocate.h | 22 ++++--
> src/gallium/drivers/panfrost/pan_context.c | 28 +++----
> src/gallium/drivers/panfrost/pan_drm.c | 74 +++----------------
> src/gallium/drivers/panfrost/pan_resource.h | 15 ----
> src/gallium/drivers/panfrost/pan_scoreboard.c | 2 +-
> src/gallium/drivers/panfrost/pan_sfbd.c | 4 +-
> 7 files changed, 56 insertions(+), 110 deletions(-)
>
> diff --git a/src/gallium/drivers/panfrost/pan_allocate.c
> b/src/gallium/drivers/panfrost/pan_allocate.c
> index 91ace74d0e43..37a6785e7dff 100644
> --- a/src/gallium/drivers/panfrost/pan_allocate.c
> +++ b/src/gallium/drivers/panfrost/pan_allocate.c
> @@ -48,8 +48,8 @@ panfrost_allocate_chunk(struct panfrost_context *ctx,
> size_t size, unsigned heap
> struct panfrost_memory *backing = (struct panfrost_memory *)
> entry->slab;
>
> struct panfrost_transfer transfer = {
> - .cpu = backing->cpu + p_entry->offset,
> - .gpu = backing->gpu + p_entry->offset
> + .cpu = backing->bo->cpu + p_entry->offset,
> + .gpu = backing->bo->gpu + p_entry->offset
> };
>
> return transfer;
> @@ -97,8 +97,8 @@ panfrost_allocate_transient(struct panfrost_context *ctx,
> size_t sz)
> struct panfrost_memory *backing = (struct panfrost_memory *)
> p_entry->base.slab;
>
> struct panfrost_transfer ret = {
> - .cpu = backing->cpu + p_entry->offset + pool->entry_offset,
> - .gpu = backing->gpu + p_entry->offset + pool->entry_offset
> + .cpu = backing->bo->cpu + p_entry->offset +
> pool->entry_offset,
> + .gpu = backing->bo->gpu + p_entry->offset +
> pool->entry_offset
> };
>
> /* Advance the pointer */
> @@ -192,18 +192,19 @@ mali_ptr
> panfrost_upload(struct panfrost_memory *mem, const void *data, size_t sz,
> bool no_pad)
> {
> /* Bounds check */
> - if ((mem->stack_bottom + sz) >= mem->size) {
> - printf("Out of memory, tried to upload %zd but only %zd
> available\n", sz, mem->size - mem->stack_bottom);
> + if ((mem->stack_bottom + sz) >= mem->bo->size) {
> + printf("Out of memory, tried to upload %zd but only %zd
> available\n",
> + sz, mem->bo->size - mem->stack_bottom);
> assert(0);
> }
>
> - return pandev_upload(-1, &mem->stack_bottom, mem->gpu, mem->cpu,
> data, sz, no_pad);
> + return pandev_upload(-1, &mem->stack_bottom, mem->bo->gpu,
> mem->bo->cpu, data, sz, no_pad);
> }
>
> mali_ptr
> panfrost_upload_sequential(struct panfrost_memory *mem, const void *data,
> size_t sz)
> {
> - return pandev_upload(last_offset, &mem->stack_bottom, mem->gpu,
> mem->cpu, data, sz, true);
> + return pandev_upload(last_offset, &mem->stack_bottom, mem->bo->gpu,
> mem->bo->cpu, data, sz, true);
> }
>
> /* Simplified interface to allocate a chunk without any upload, to allow
> @@ -215,6 +216,6 @@ panfrost_allocate_transfer(struct panfrost_memory *mem,
> size_t sz, mali_ptr *gpu
> {
> int offset = pandev_allocate_offset(&mem->stack_bottom, sz);
>
> - *gpu = mem->gpu + offset;
> - return mem->cpu + offset;
> + *gpu = mem->bo->gpu + offset;
> + return mem->bo->cpu + offset;
> }
> diff --git a/src/gallium/drivers/panfrost/pan_allocate.h
> b/src/gallium/drivers/panfrost/pan_allocate.h
> index 5bbb1e4b078d..20ba204dee86 100644
> --- a/src/gallium/drivers/panfrost/pan_allocate.h
> +++ b/src/gallium/drivers/panfrost/pan_allocate.h
> @@ -58,16 +58,28 @@ struct panfrost_transfer {
> mali_ptr gpu;
> };
>
> +struct panfrost_bo {
> + 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;
> +};
> +
> struct panfrost_memory {
> /* Subclassing slab object */
> struct pb_slab slab;
>
> /* Backing for the slab in memory */
> - uint8_t *cpu;
> - mali_ptr gpu;
> + struct panfrost_bo *bo;
> int stack_bottom;
> - size_t size;
> - int gem_handle;
> };
>
> /* Slab entry sizes range from 2^min to 2^max. In this case, we range from 1k
> @@ -109,7 +121,7 @@ static inline mali_ptr
> panfrost_reserve(struct panfrost_memory *mem, size_t sz)
> {
> mem->stack_bottom += sz;
> - return mem->gpu + (mem->stack_bottom - sz);
> + return mem->bo->gpu + (mem->stack_bottom - sz);
> }
>
> struct panfrost_transfer
> diff --git a/src/gallium/drivers/panfrost/pan_context.c
> b/src/gallium/drivers/panfrost/pan_context.c
> index 139e0a1140cc..b9fb187be446 100644
> --- a/src/gallium/drivers/panfrost/pan_context.c
> +++ b/src/gallium/drivers/panfrost/pan_context.c
> @@ -99,13 +99,13 @@ panfrost_emit_sfbd(struct panfrost_context *ctx, unsigned
> vertex_count)
> .unknown2 = 0x1f,
> .format = 0x30000000,
> .clear_flags = 0x1000,
> - .unknown_address_0 = ctx->scratchpad.gpu,
> - .tiler_polygon_list = ctx->tiler_polygon_list.gpu,
> - .tiler_polygon_list_body = ctx->tiler_polygon_list.gpu +
> 40960,
> + .unknown_address_0 = ctx->scratchpad.bo->gpu,
> + .tiler_polygon_list = ctx->tiler_polygon_list.bo->gpu,
> + .tiler_polygon_list_body = ctx->tiler_polygon_list.bo->gpu +
> 40960,
> .tiler_hierarchy_mask = 0xF0,
> .tiler_flags = 0x0,
> - .tiler_heap_free = ctx->tiler_heap.gpu,
> - .tiler_heap_end = ctx->tiler_heap.gpu + ctx->tiler_heap.size,
> + .tiler_heap_free = ctx->tiler_heap.bo->gpu,
> + .tiler_heap_end = ctx->tiler_heap.bo->gpu +
> ctx->tiler_heap.bo->size,
> };
>
> panfrost_set_framebuffer_resolution(&framebuffer,
> ctx->pipe_framebuffer.width, ctx->pipe_framebuffer.height);
> @@ -133,7 +133,7 @@ panfrost_emit_mfbd(struct panfrost_context *ctx, unsigned
> vertex_count)
>
> .unknown2 = 0x1f,
>
> - .scratchpad = ctx->scratchpad.gpu,
> + .scratchpad = ctx->scratchpad.bo->gpu,
> };
>
> framebuffer.tiler_hierarchy_mask =
> @@ -152,22 +152,22 @@ panfrost_emit_mfbd(struct panfrost_context *ctx,
> unsigned vertex_count)
> unsigned total_size = header_size + body_size;
>
> if (framebuffer.tiler_hierarchy_mask) {
> - assert(ctx->tiler_polygon_list.size >= total_size);
> + assert(ctx->tiler_polygon_list.bo->size >= total_size);
>
> /* Specify allocated tiler structures */
> - framebuffer.tiler_polygon_list = ctx->tiler_polygon_list.gpu;
> + framebuffer.tiler_polygon_list =
> ctx->tiler_polygon_list.bo->gpu;
>
> /* Allow the entire tiler heap */
> - framebuffer.tiler_heap_start = ctx->tiler_heap.gpu;
> + framebuffer.tiler_heap_start = ctx->tiler_heap.bo->gpu;
> framebuffer.tiler_heap_end =
> - ctx->tiler_heap.gpu + ctx->tiler_heap.size;
> + ctx->tiler_heap.bo->gpu + ctx->tiler_heap.bo->size;
> } else {
> /* The tiler is disabled, so don't allow the tiler heap */
> - framebuffer.tiler_heap_start = ctx->tiler_heap.gpu;
> + framebuffer.tiler_heap_start = ctx->tiler_heap.bo->gpu;
> framebuffer.tiler_heap_end = framebuffer.tiler_heap_start;
>
> /* Use a dummy polygon list */
> - framebuffer.tiler_polygon_list = ctx->tiler_dummy.gpu;
> + framebuffer.tiler_polygon_list = ctx->tiler_dummy.bo->gpu;
>
> /* Also, set a "tiler disabled?" flag? */
> framebuffer.tiler_hierarchy_mask |= 0x1000;
> @@ -529,7 +529,7 @@ panfrost_emit_varyings(
> unsigned stride,
> unsigned count)
> {
> - mali_ptr varying_address = ctx->varying_mem.gpu +
> ctx->varying_height;
> + mali_ptr varying_address = ctx->varying_mem.bo->gpu +
> ctx->varying_height;
>
> /* Fill out the descriptor */
> slot->elements = varying_address | MALI_ATTR_LINEAR;
> @@ -537,7 +537,7 @@ panfrost_emit_varyings(
> slot->size = stride * count;
>
> ctx->varying_height += ALIGN(slot->size, 64);
> - assert(ctx->varying_height < ctx->varying_mem.size);
> + assert(ctx->varying_height < ctx->varying_mem.bo->size);
>
> return varying_address;
> }
> diff --git a/src/gallium/drivers/panfrost/pan_drm.c
> b/src/gallium/drivers/panfrost/pan_drm.c
> index d49c999e0773..ac82ec583021 100644
> --- a/src/gallium/drivers/panfrost/pan_drm.c
> +++ b/src/gallium/drivers/panfrost/pan_drm.c
> @@ -136,70 +136,18 @@ panfrost_drm_allocate_slab(struct panfrost_screen
> *screen,
> int commit_count,
> int extent)
> {
> - struct drm_panfrost_create_bo create_bo = {
> - .size = pages * 4096,
> - .flags = 0, // TODO figure out proper flags..
> - };
> - struct drm_panfrost_mmap_bo mmap_bo = {0,};
> - int ret;
> -
> - // TODO cache allocations
> - // TODO properly handle errors
> - // TODO take into account extra_flags
> -
> - ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_CREATE_BO, &create_bo);
> - if (ret) {
> - fprintf(stderr, "DRM_IOCTL_PANFROST_CREATE_BO failed: %d\n",
> ret);
> - assert(0);
> - }
> -
> - mem->gpu = create_bo.offset;
> - mem->gem_handle = create_bo.handle;
> + // TODO cache allocations
> + // TODO properly handle errors
> + // TODO take into account extra_flags
> + mem->bo = panfrost_drm_create_bo(screen, pages * 4096, 0);
> mem->stack_bottom = 0;
> - mem->size = create_bo.size;
> -
> - // TODO map and unmap on demand?
> - mmap_bo.handle = create_bo.handle;
> - ret = drmIoctl(screen->fd, DRM_IOCTL_PANFROST_MMAP_BO, &mmap_bo);
> - if (ret) {
> - fprintf(stderr, "DRM_IOCTL_PANFROST_MMAP_BO failed: %d\n",
> ret);
> - assert(0);
> - }
> -
> - mem->cpu = os_mmap(NULL, mem->size, PROT_READ | PROT_WRITE,
> MAP_SHARED,
> - screen->fd, mmap_bo.offset);
> - if (mem->cpu == MAP_FAILED) {
> - fprintf(stderr, "mmap failed: %p\n", mem->cpu);
> - assert(0);
> - }
> -
> - /* Record the mmap if we're tracing */
> - if (pan_debug & PAN_DBG_TRACE)
> - pandecode_inject_mmap(mem->gpu, mem->cpu, mem->size, NULL);
> }
>
> void
> panfrost_drm_free_slab(struct panfrost_screen *screen, struct
> panfrost_memory *mem)
> {
> - struct drm_gem_close gem_close = {
> - .handle = mem->gem_handle,
> - };
> - int ret;
> -
> - if (os_munmap((void *) (uintptr_t) mem->cpu, mem->size)) {
> - perror("munmap");
> - abort();
> - }
> -
> - mem->cpu = NULL;
> -
> - ret = drmIoctl(screen->fd, DRM_IOCTL_GEM_CLOSE, &gem_close);
> - if (ret) {
> - fprintf(stderr, "DRM_IOCTL_GEM_CLOSE failed: %d\n", ret);
> - assert(0);
> - }
> -
> - mem->gem_handle = -1;
> + panfrost_bo_unreference(&screen->base, mem->bo);
> + mem->bo = NULL;
> }
>
> struct panfrost_bo *
> @@ -267,11 +215,11 @@ panfrost_drm_submit_job(struct panfrost_context *ctx,
> u64 job_desc, int reqs, st
>
> /* TODO: Add here the transient pools */
> /* TODO: Add here the BOs listed in the panfrost_job */
> - bo_handles[submit.bo_handle_count++] = ctx->shaders.gem_handle;
> - bo_handles[submit.bo_handle_count++] = ctx->scratchpad.gem_handle;
> - bo_handles[submit.bo_handle_count++] = ctx->tiler_heap.gem_handle;
> - bo_handles[submit.bo_handle_count++] = ctx->varying_mem.gem_handle;
> - bo_handles[submit.bo_handle_count++] =
> ctx->tiler_polygon_list.gem_handle;
> + bo_handles[submit.bo_handle_count++] = ctx->shaders.bo->gem_handle;
> + bo_handles[submit.bo_handle_count++] =
> ctx->scratchpad.bo->gem_handle;
> + bo_handles[submit.bo_handle_count++] =
> ctx->tiler_heap.bo->gem_handle;
> + bo_handles[submit.bo_handle_count++] =
> ctx->varying_mem.bo->gem_handle;
> + bo_handles[submit.bo_handle_count++] =
> ctx->tiler_polygon_list.bo->gem_handle;
> submit.bo_handles = (u64) (uintptr_t) bo_handles;
>
> if (drmIoctl(screen->fd, DRM_IOCTL_PANFROST_SUBMIT, &submit)) {
> diff --git a/src/gallium/drivers/panfrost/pan_resource.h
> b/src/gallium/drivers/panfrost/pan_resource.h
> index d1f0ffc84e15..9ca5f77521a4 100644
> --- a/src/gallium/drivers/panfrost/pan_resource.h
> +++ b/src/gallium/drivers/panfrost/pan_resource.h
> @@ -57,21 +57,6 @@ struct panfrost_slice {
> bool initialized;
> };
>
> -struct panfrost_bo {
> - 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;
> -};
> -
> void
> panfrost_bo_reference(struct panfrost_bo *bo);
>
> diff --git a/src/gallium/drivers/panfrost/pan_scoreboard.c
> b/src/gallium/drivers/panfrost/pan_scoreboard.c
> index 0c4cbfe5d9b4..66fb689c1f47 100644
> --- a/src/gallium/drivers/panfrost/pan_scoreboard.c
> +++ b/src/gallium/drivers/panfrost/pan_scoreboard.c
> @@ -306,7 +306,7 @@ panfrost_scoreboard_set_value(struct panfrost_job *batch)
> /* Okay, we do. Let's generate it */
>
> struct panfrost_context *ctx = batch->ctx;
> - mali_ptr polygon_list = ctx->tiler_polygon_list.gpu;
> + mali_ptr polygon_list = ctx->tiler_polygon_list.bo->gpu;
>
> struct panfrost_transfer job =
> panfrost_set_value_job(ctx, polygon_list);
> diff --git a/src/gallium/drivers/panfrost/pan_sfbd.c
> b/src/gallium/drivers/panfrost/pan_sfbd.c
> index eccae888e826..76267b746ac0 100644
> --- a/src/gallium/drivers/panfrost/pan_sfbd.c
> +++ b/src/gallium/drivers/panfrost/pan_sfbd.c
> @@ -55,14 +55,14 @@ panfrost_sfbd_clear(
> sfbd->clear_depth_3 = job->clear_depth;
> sfbd->clear_depth_4 = job->clear_depth;
>
> - sfbd->depth_buffer = ctx->depth_stencil_buffer.gpu;
> + sfbd->depth_buffer = ctx->depth_stencil_buffer.bo->gpu;
> sfbd->depth_buffer_enable = MALI_DEPTH_STENCIL_ENABLE;
> }
>
> if (job->clear & PIPE_CLEAR_STENCIL) {
> sfbd->clear_stencil = job->clear_stencil;
>
> - sfbd->stencil_buffer = ctx->depth_stencil_buffer.gpu;
> + sfbd->stencil_buffer = ctx->depth_stencil_buffer.bo->gpu;
> sfbd->stencil_buffer_enable = MALI_DEPTH_STENCIL_ENABLE;
> }
>
> --
> 2.21.0
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
