From: Steven Price <steven.price@arm.com>
To: "Boris Brezillon" <boris.brezillon@collabora.com>,
"Liviu Dudau" <liviu.dudau@arm.com>,
"Adrián Larumbe" <adrian.larumbe@collabora.com>
Cc: dri-devel@lists.freedesktop.org, kernel@collabora.com,
Eric Smith <eric.smith@collabora.com>
Subject: Re: [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic
Date: Thu, 2 May 2024 15:03:51 +0100 [thread overview]
Message-ID: <791a81bf-323c-40dc-ab51-2d909adcb90b@arm.com> (raw)
In-Reply-To: <20240430112852.486424-5-boris.brezillon@collabora.com>
On 30/04/2024 12:28, Boris Brezillon wrote:
> ID 0 is reserved to encode 'no-tiler-heap', the heap ID range is
> [1:MAX_HEAPS_PER_POOL], which we occasionally need to turn into an index
> in the [0:MAX_HEAPS_PER_POOL-1] when we want to access the context object.
This might be a silly question, but do we need ID 0 to be
"no-tiler-heap"? Would it be easier to e.g. use a negative number for
that situation and avoid all the off-by-one problems?
I'm struggling to find the code which needs the 0 value to be special -
where is it exactly that we encode this "no-tiler-heap" value?
Steve
>
> v2:
> - New patch
>
> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> Reported-by: Eric Smith <eric.smith@collabora.com>
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Tested-by: Eric Smith <eric.smith@collabora.com>
> ---
> drivers/gpu/drm/panthor/panthor_heap.c | 35 +++++++++++++++++++-------
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 683bb94761bc..b1a7dbf25fb2 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -109,7 +109,11 @@ static int panthor_heap_ctx_stride(struct panthor_device *ptdev)
>
> static int panthor_get_heap_ctx_offset(struct panthor_heap_pool *pool, int id)
> {
> - return panthor_heap_ctx_stride(pool->ptdev) * id;
> + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range
> + * is [1:MAX_HEAPS_PER_POOL], which we need to turn into a
> + * [0:MAX_HEAPS_PER_POOL-1] context index, hence the minus one here.
> + */
> + return panthor_heap_ctx_stride(pool->ptdev) * (id - 1);
> }
>
> static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id)
> @@ -118,6 +122,21 @@ static void *panthor_get_heap_ctx(struct panthor_heap_pool *pool, int id)
> panthor_get_heap_ctx_offset(pool, id);
> }
>
> +static int panthor_get_heap_ctx_id(struct panthor_heap_pool *pool,
> + u64 heap_ctx_gpu_va)
> +{
> + u64 offset = heap_ctx_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> + u32 heap_idx = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> +
> + if (offset > U32_MAX || heap_idx >= MAX_HEAPS_PER_POOL)
> + return -EINVAL;
> +
> + /* ID 0 is reserved to encode 'no-tiler-heap', the valid range
> + * is [1:MAX_HEAPS_PER_POOL], hence the plus one here.
> + */
> + return heap_idx + 1;
> +}
> +
> static void panthor_free_heap_chunk(struct panthor_vm *vm,
> struct panthor_heap *heap,
> struct panthor_heap_chunk *chunk)
> @@ -364,14 +383,13 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
> u64 heap_gpu_va,
> u64 chunk_gpu_va)
> {
> - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va);
> struct panthor_heap_chunk *chunk, *tmp, *removed = NULL;
> struct panthor_heap *heap;
> int ret;
>
> - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
> - return -EINVAL;
> + if (heap_id < 0)
> + return heap_id;
>
> down_read(&pool->lock);
> heap = xa_load(&pool->xa, heap_id);
> @@ -427,14 +445,13 @@ int panthor_heap_grow(struct panthor_heap_pool *pool,
> u32 pending_frag_count,
> u64 *new_chunk_gpu_va)
> {
> - u64 offset = heap_gpu_va - panthor_kernel_bo_gpuva(pool->gpu_contexts);
> - u32 heap_id = (u32)offset / panthor_heap_ctx_stride(pool->ptdev);
> + int heap_id = panthor_get_heap_ctx_id(pool, heap_gpu_va);
> struct panthor_heap_chunk *chunk;
> struct panthor_heap *heap;
> int ret;
>
> - if (offset > U32_MAX || heap_id >= MAX_HEAPS_PER_POOL)
> - return -EINVAL;
> + if (heap_id < 0)
> + return heap_id;
>
> down_read(&pool->lock);
> heap = xa_load(&pool->xa, heap_id);
next prev parent reply other threads:[~2024-05-02 14:03 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-30 11:28 [PATCH v2 0/4] drm/panthor: Collection of tiler heap related fixes Boris Brezillon
2024-04-30 11:28 ` [PATCH v2 1/4] drm/panthor: Fix tiler OOM handling to allow incremental rendering Boris Brezillon
2024-04-30 15:27 ` Liviu Dudau
2024-05-02 14:03 ` Steven Price
2024-04-30 11:28 ` [PATCH v2 2/4] drm/panthor: Make sure the tiler initial/max chunks are consistent Boris Brezillon
2024-04-30 15:31 ` Liviu Dudau
2024-05-02 14:03 ` Steven Price
2024-04-30 11:28 ` [PATCH v2 3/4] drm/panthor: Relax the constraints on the tiler chunk size Boris Brezillon
2024-04-30 13:08 ` Adrián Larumbe
2024-05-02 14:03 ` Steven Price
2024-04-30 16:10 ` Liviu Dudau
2024-04-30 11:28 ` [PATCH v2 4/4] drm/panthor: Fix an off-by-one in the heap context retrieval logic Boris Brezillon
2024-04-30 16:40 ` Liviu Dudau
2024-04-30 17:07 ` Boris Brezillon
2024-05-02 14:03 ` Steven Price [this message]
2024-05-02 14:15 ` Boris Brezillon
2024-05-02 14:26 ` Steven Price
2024-05-02 14:36 ` Boris Brezillon
2024-05-02 14:47 ` Boris Brezillon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=791a81bf-323c-40dc-ab51-2d909adcb90b@arm.com \
--to=steven.price@arm.com \
--cc=adrian.larumbe@collabora.com \
--cc=boris.brezillon@collabora.com \
--cc=dri-devel@lists.freedesktop.org \
--cc=eric.smith@collabora.com \
--cc=kernel@collabora.com \
--cc=liviu.dudau@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.