From 1d8d7ac788edcb02dce187fef516a2bdf0af5e76 Mon Sep 17 00:00:00 2001 From: Govindaraj Rajagopal Date: Thu, 24 Jun 2021 18:19:14 +0530 Subject: [PATCH] video: driver: optimize buffer pool design Added change to avoid unnecessary list iteration during msm_memory_free also added logic to catch double free request. Change-Id: Ie3fb8019fd039e95ea75ba8f3fbd266af515e5cd Signed-off-by: Govindaraj Rajagopal --- driver/vidc/inc/msm_vidc_memory.h | 6 ++-- driver/vidc/src/msm_vdec.c | 2 +- driver/vidc/src/msm_vidc_driver.c | 30 ++++++++++---------- driver/vidc/src/msm_vidc_memory.c | 42 +++++++++++++++++----------- driver/vidc/src/venus_hfi_response.c | 2 +- 5 files changed, 46 insertions(+), 36 deletions(-) diff --git a/driver/vidc/inc/msm_vidc_memory.h b/driver/vidc/inc/msm_vidc_memory.h index 3bfc4e934b..733f73b158 100644 --- a/driver/vidc/inc/msm_vidc_memory.h +++ b/driver/vidc/inc/msm_vidc_memory.h @@ -28,11 +28,12 @@ enum msm_memory_pool_type { struct msm_memory_alloc_header { struct list_head list; + u32 type; + bool busy; void *buf; }; struct msm_memory_pool { - u32 type; u32 size; char *name; struct list_head free_pool; /* list of struct msm_memory_alloc_header */ @@ -57,7 +58,6 @@ int msm_memory_pools_init(struct msm_vidc_inst *inst); void msm_memory_pools_deinit(struct msm_vidc_inst *inst); void *msm_memory_alloc(struct msm_vidc_inst *inst, enum msm_memory_pool_type type); -void msm_memory_free(struct msm_vidc_inst *inst, - enum msm_memory_pool_type type, void *vidc_buf); +void msm_memory_free(struct msm_vidc_inst *inst, void *vidc_buf); #endif // _MSM_VIDC_MEMORY_H_ \ No newline at end of file diff --git a/driver/vidc/src/msm_vdec.c b/driver/vidc/src/msm_vdec.c index e453b2e4be..65d36540a5 100644 --- a/driver/vidc/src/msm_vdec.c +++ b/driver/vidc/src/msm_vdec.c @@ -1848,7 +1848,7 @@ int msm_vdec_handle_release_buffer(struct msm_vidc_inst *inst, print_vidc_buffer(VIDC_LOW, "low ", "release done", inst, buf); /* delete the buffer from release list */ list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); return rc; } diff --git a/driver/vidc/src/msm_vidc_driver.c b/driver/vidc/src/msm_vidc_driver.c index 98dd909f3b..738b551ced 100644 --- a/driver/vidc/src/msm_vidc_driver.c +++ b/driver/vidc/src/msm_vidc_driver.c @@ -1983,7 +1983,7 @@ int msm_vidc_process_readonly_buffers(struct msm_vidc_inst *inst, buf->attr |= MSM_VIDC_ATTR_READ_ONLY; print_vidc_buffer(VIDC_LOW, "low ", "ro buf removed", inst, ro_buf); list_del(&ro_buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, ro_buf); + msm_memory_free(inst, ro_buf); break; } } @@ -2010,7 +2010,7 @@ int msm_vidc_memory_unmap_completely(struct msm_vidc_inst *inst, if (!map->refcount) { msm_vidc_memory_put_dmabuf(inst, map->dmabuf); list_del(&map->list); - msm_memory_free(inst, MSM_MEM_POOL_MAP, map); + msm_memory_free(inst, map); break; } } @@ -2175,7 +2175,7 @@ int msm_vidc_flush_ts(struct msm_vidc_inst *inst) i_vpr_l(inst, "%s: flushing ts: val %lld, rank %%lld\n", __func__, ts->sort.val, ts->rank); list_del(&ts->sort.list); - msm_memory_free(inst, MSM_MEM_POOL_TIMESTAMP, ts); + msm_memory_free(inst, ts); } inst->timestamps.count = 0; inst->timestamps.rank = 0; @@ -2222,7 +2222,7 @@ int msm_vidc_update_timestamp(struct msm_vidc_inst *inst, u64 timestamp) } inst->timestamps.count--; list_del(&ts->sort.list); - msm_memory_free(inst, MSM_MEM_POOL_TIMESTAMP, ts); + msm_memory_free(inst, ts); } return 0; @@ -2268,7 +2268,7 @@ int msm_vidc_put_delayed_unmap(struct msm_vidc_inst *inst, struct msm_vidc_map * if (!map->refcount) { msm_vidc_memory_put_dmabuf(inst, map->dmabuf); list_del(&map->list); - msm_memory_free(inst, MSM_MEM_POOL_MAP, map); + msm_memory_free(inst, map); } return rc; @@ -2336,7 +2336,7 @@ int msm_vidc_unmap_driver_buf(struct msm_vidc_inst *inst, if (!map->refcount) { msm_vidc_memory_put_dmabuf(inst, map->dmabuf); list_del(&map->list); - msm_memory_free(inst, MSM_MEM_POOL_MAP, map); + msm_memory_free(inst, map); } return rc; @@ -2387,7 +2387,7 @@ int msm_vidc_map_driver_buf(struct msm_vidc_inst *inst, rc = msm_vidc_get_delayed_unmap(inst, map); if (rc) { msm_vidc_memory_put_dmabuf(inst, map->dmabuf); - msm_memory_free(inst, MSM_MEM_POOL_MAP, map); + msm_memory_free(inst, map); return rc; } } @@ -2418,7 +2418,7 @@ int msm_vidc_put_driver_buf(struct msm_vidc_inst *inst, /* delete the buffer from buffers->list */ list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); return rc; } @@ -2472,7 +2472,7 @@ struct msm_vidc_buffer *msm_vidc_get_driver_buf(struct msm_vidc_inst *inst, error: msm_vidc_memory_put_dmabuf(inst, buf->dmabuf); list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); return NULL; } @@ -2915,7 +2915,7 @@ int msm_vidc_destroy_internal_buffer(struct msm_vidc_inst *inst, if (map->dmabuf == buffer->dmabuf) { msm_vidc_memory_unmap(inst->core, map); list_del(&map->list); - msm_memory_free(inst, MSM_MEM_POOL_MAP, map); + msm_memory_free(inst, map); break; } } @@ -2924,7 +2924,7 @@ int msm_vidc_destroy_internal_buffer(struct msm_vidc_inst *inst, if (alloc->dmabuf == buffer->dmabuf) { msm_vidc_memory_free(inst->core, alloc); list_del(&alloc->list); - msm_memory_free(inst, MSM_MEM_POOL_ALLOC, alloc); + msm_memory_free(inst, alloc); break; } } @@ -2932,7 +2932,7 @@ int msm_vidc_destroy_internal_buffer(struct msm_vidc_inst *inst, list_for_each_entry_safe(buf, dummy, &buffers->list, list) { if (buf->dmabuf == buffer->dmabuf) { list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); break; } } @@ -4599,20 +4599,20 @@ void msm_vidc_destroy_buffers(struct msm_vidc_inst *inst) list_for_each_entry_safe(buf, dummy, &inst->buffers.read_only.list, list) { print_vidc_buffer(VIDC_ERR, "err ", "destroying ro buffer", inst, buf); list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); } list_for_each_entry_safe(buf, dummy, &inst->buffers.release.list, list) { print_vidc_buffer(VIDC_ERR, "err ", "destroying release buffer", inst, buf); list_del(&buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, buf); + msm_memory_free(inst, buf); } list_for_each_entry_safe(ts, dummy_ts, &inst->timestamps.list, sort.list) { i_vpr_e(inst, "%s: removing ts: val %lld, rank %lld\n", __func__, ts->sort.val, ts->rank); list_del(&ts->sort.list); - msm_memory_free(inst, MSM_MEM_POOL_TIMESTAMP, ts); + msm_memory_free(inst, ts); } list_for_each_entry_safe(dbuf, dummy_dbuf, &inst->dmabuf_tracker, list) { diff --git a/driver/vidc/src/msm_vidc_memory.c b/driver/vidc/src/msm_vidc_memory.c index 17f3a558c3..ff2d1fc431 100644 --- a/driver/vidc/src/msm_vidc_memory.c +++ b/driver/vidc/src/msm_vidc_memory.c @@ -145,7 +145,7 @@ void msm_vidc_memory_put_dmabuf(struct msm_vidc_inst *inst, struct dma_buf *dmab dma_buf_put(buf->dmabuf); /* put tracker instance back to pool */ - msm_memory_free(inst, MSM_MEM_POOL_DMABUF, buf); + msm_memory_free(inst, buf); } void msm_vidc_memory_put_dmabuf_completely(struct msm_vidc_inst *inst, @@ -166,7 +166,7 @@ void msm_vidc_memory_put_dmabuf_completely(struct msm_vidc_inst *inst, dma_buf_put(buf->dmabuf); /* put tracker instance back to pool */ - msm_memory_free(inst, MSM_MEM_POOL_DMABUF, buf); + msm_memory_free(inst, buf); break; } } @@ -426,6 +426,9 @@ void *msm_memory_alloc(struct msm_vidc_inst *inst, enum msm_memory_pool_type typ /* add to busy pool */ list_add_tail(&hdr->list, &pool->busy_pool); + /* set busy flag to true. This is to catch double free request */ + hdr->busy = true; + return hdr->buf; } @@ -435,37 +438,46 @@ void *msm_memory_alloc(struct msm_vidc_inst *inst, enum msm_memory_pool_type typ return NULL; } INIT_LIST_HEAD(&hdr->list); + hdr->type = type; + hdr->busy = true; hdr->buf = (void *)(hdr + 1); list_add_tail(&hdr->list, &pool->busy_pool); return hdr->buf; } -void msm_memory_free(struct msm_vidc_inst *inst, enum msm_memory_pool_type type, - void *vidc_buf) +void msm_memory_free(struct msm_vidc_inst *inst, void *vidc_buf) { struct msm_memory_alloc_header *hdr; struct msm_memory_pool *pool; - bool found = false; - if (!inst || !vidc_buf || type < 0 || type >= MSM_MEM_POOL_MAX) { + if (!inst || !vidc_buf) { d_vpr_e("%s: Invalid params\n", __func__); return; } - pool = &inst->pool[type]; + hdr = (struct msm_memory_alloc_header *)vidc_buf - 1; /* sanitize buffer addr */ - list_for_each_entry(hdr, &pool->busy_pool, list) { - if (hdr->buf == vidc_buf) { - found = true; - break; - } - } - if (!found) { + if (hdr->buf != vidc_buf) { i_vpr_e(inst, "%s: invalid buf addr %#x\n", __func__, vidc_buf); return; } + /* sanitize pool type */ + if (hdr->type < 0 || hdr->type >= MSM_MEM_POOL_MAX) { + i_vpr_e(inst, "%s: invalid pool type %#x\n", __func__, hdr->type); + return; + } + pool = &inst->pool[hdr->type]; + + /* catch double-free request */ + if (!hdr->busy) { + i_vpr_e(inst, "%s: double free request. type %s, addr %#x\n", __func__, + pool->name, vidc_buf); + return; + } + hdr->busy = false; + /* remove from busy pool */ list_del_init(&hdr->list); @@ -558,8 +570,6 @@ int msm_memory_pools_init(struct msm_vidc_inst *inst) i, buftype_size_name_arr[i].type); return -EINVAL; } - - inst->pool[i].type = buftype_size_name_arr[i].type; inst->pool[i].size = buftype_size_name_arr[i].size; inst->pool[i].name = buftype_size_name_arr[i].name; INIT_LIST_HEAD(&inst->pool[i].free_pool); diff --git a/driver/vidc/src/venus_hfi_response.c b/driver/vidc/src/venus_hfi_response.c index 1af365d46b..45e45d396f 100644 --- a/driver/vidc/src/venus_hfi_response.c +++ b/driver/vidc/src/venus_hfi_response.c @@ -657,7 +657,7 @@ static int handle_non_read_only_buffer(struct msm_vidc_inst *inst, if (found) { print_vidc_buffer(VIDC_LOW, "low ", "ro buf deleted", inst, ro_buf); list_del(&ro_buf->list); - msm_memory_free(inst, MSM_MEM_POOL_BUFFER, ro_buf); + msm_memory_free(inst, ro_buf); } return 0;