| Message ID | 20240430112852.486424-5-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Collection of tiler heap related fixes | expand |
On Tue, Apr 30, 2024 at 01:28:52PM +0200, 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. > > 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) Can we make id and the return type here u32? I keep thinking about returning large negative values here and how they can end up being exploited. > { > - 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); I would keep heap_id here u32. Why do we need to change it? Also, I don't see how panthor_get_heap_ctx_id() can ever return negative values unless we expect MAX_HEAPS_PER_POOL to be S32_MAX at some moment. > 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; This can then be removed if heap_id is u32. > > 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); Again, keep u32 unless you have good reasons ... > 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; ... and we will not need this. Best regards, Liviu > > down_read(&pool->lock); > heap = xa_load(&pool->xa, heap_id); > -- > 2.44.0 >
On Tue, 30 Apr 2024 17:40:54 +0100 Liviu Dudau <liviu.dudau@arm.com> wrote: > On Tue, Apr 30, 2024 at 01:28:52PM +0200, 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. > > > > 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) > > Can we make id and the return type here u32? I keep thinking about returning large negative > values here and how they can end up being exploited. Actually, I had the prototype changed to take/return an u32 locally, but decided to drop it to both keep the amount of changes minimal and keep prototype consistent with the new helper. I'm fine switching to u32s though. > > > { > > - 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); > > I would keep heap_id here u32. Why do we need to change it? Also, I don't see how > panthor_get_heap_ctx_id() can ever return negative values unless we expect MAX_HEAPS_PER_POOL > to be S32_MAX at some moment. The reason I made it a signed value is so we could return an error code too - > 0 => valid - < 0 error, with the value encoding the error though we're likely to always return EINVAL anyway. > > > 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; > > This can then be removed if heap_id is u32. We need to replace that by an extra check on the VA, and given this is done in two different places, I thought having an helper that does both the VA to offset and the offset consistency check was simpler. I mean, I could make this helper return an u32, and consider 0 as the 'no-context-found' special-value, but I can't drop this check.
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);
On Thu, 2 May 2024 15:03:51 +0100 Steven Price <steven.price@arm.com> wrote: > 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? Hm, I thought we were passing the heap handle to the group creation ioctl, but heap queue/heap association is actually done through a CS instruction, so I guess you have a point. The only thing that makes a bit hesitant is that handle=0 is reserved for all other kind of handles we return, and I think I'd prefer to keep it the same for heap handles. This being said, we could do the `+- 1` in panthor_ioctl_tiler_heap_{create,destroy}() to keep things simple in panthor_heap.c.
On 02/05/2024 15:15, Boris Brezillon wrote: > On Thu, 2 May 2024 15:03:51 +0100 > Steven Price <steven.price@arm.com> wrote: > >> 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? > > Hm, I thought we were passing the heap handle to the group creation > ioctl, but heap queue/heap association is actually done through a CS > instruction, so I guess you have a point. The only thing that makes a > bit hesitant is that handle=0 is reserved for all other kind of handles > we return, and I think I'd prefer to keep it the same for heap handles. > > This being said, we could do the `+- 1` in > panthor_ioctl_tiler_heap_{create,destroy}() to keep things simple in > panthor_heap.c. The heap handles returned to user space have the upper 16 bits encoding the VM ID - so hopefully no one is doing anything crazy and splitting it up to treat the lower part specially. And (unless I'm mistaken) the VM IDs start from 1 so we'd still not have IDs of 0. So I don't think we need the +- 1 part anywhere for tiler heaps. I'd certainly consider it a user space bug to treat the handles as anything other than opaque. Really user space shouldn't be treating 0 as special either: the uAPI doesn't say it's not valid. But I'd be open to updating the uAPI to say 0 is invalid if there's some desire for that. Steve
On Thu, 2 May 2024 15:26:55 +0100 Steven Price <steven.price@arm.com> wrote: > On 02/05/2024 15:15, Boris Brezillon wrote: > > On Thu, 2 May 2024 15:03:51 +0100 > > Steven Price <steven.price@arm.com> wrote: > > > >> 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? > > > > Hm, I thought we were passing the heap handle to the group creation > > ioctl, but heap queue/heap association is actually done through a CS > > instruction, so I guess you have a point. The only thing that makes a > > bit hesitant is that handle=0 is reserved for all other kind of handles > > we return, and I think I'd prefer to keep it the same for heap handles. > > > > This being said, we could do the `+- 1` in > > panthor_ioctl_tiler_heap_{create,destroy}() to keep things simple in > > panthor_heap.c. > > The heap handles returned to user space have the upper 16 bits encoding > the VM ID - so hopefully no one is doing anything crazy and splitting it > up to treat the lower part specially. And (unless I'm mistaken) the VM > IDs start from 1 so we'd still not have IDs of 0. So I don't think we > need the +- 1 part anywhere for tiler heaps. Ah, I forgot about that too. Guess we're all good with a [0,MAX_HEAPS_PER_POOL-1] range then. > > I'd certainly consider it a user space bug to treat the handles as > anything other than opaque. Really user space shouldn't be treating 0 as > special either: the uAPI doesn't say it's not valid. But I'd be open to > updating the uAPI to say 0 is invalid if there's some desire for that. Will do that in v3 then. Thanks! Boris
On Thu, 2 May 2024 16:36:02 +0200 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Thu, 2 May 2024 15:26:55 +0100 > Steven Price <steven.price@arm.com> wrote: > > > On 02/05/2024 15:15, Boris Brezillon wrote: > > > On Thu, 2 May 2024 15:03:51 +0100 > > > Steven Price <steven.price@arm.com> wrote: > > > > > >> 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? > > > > > > Hm, I thought we were passing the heap handle to the group creation > > > ioctl, but heap queue/heap association is actually done through a CS > > > instruction, so I guess you have a point. The only thing that makes a > > > bit hesitant is that handle=0 is reserved for all other kind of handles > > > we return, and I think I'd prefer to keep it the same for heap handles. > > > > > > This being said, we could do the `+- 1` in > > > panthor_ioctl_tiler_heap_{create,destroy}() to keep things simple in > > > panthor_heap.c. > > > > The heap handles returned to user space have the upper 16 bits encoding > > the VM ID - so hopefully no one is doing anything crazy and splitting it > > up to treat the lower part specially. And (unless I'm mistaken) the VM > > IDs start from 1 so we'd still not have IDs of 0. So I don't think we > > need the +- 1 part anywhere for tiler heaps. > > Ah, I forgot about that too. Guess we're all good with a > [0,MAX_HEAPS_PER_POOL-1] range then. > > > > > I'd certainly consider it a user space bug to treat the handles as > > anything other than opaque. Really user space shouldn't be treating 0 as > > special either: the uAPI doesn't say it's not valid. But I'd be open to > > updating the uAPI to say 0 is invalid if there's some desire for that. > > Will do that in v3 then. Taking that back. I don't think it needs to be enforced in the uAPI. As you said, it's supposed to be opaque, so I'm tempted to update the drm_panthor_tiler_heap_destroy::handle kerneldoc saying it must be a valid handle returned by DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE instead. It's just that making the handle non-zero is kinda nice for debugging purposes, and as I said, this way it's consistent with other kind of handles (GEMs, VMs, syncobjs, ...).
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);
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. 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(-)