[PATCH v3 0/5] drm/panthor: Collection of tiler heap related fixes

[PATCH v3 0/5] drm/panthor: Collection of tiler heap related fixes

[Boris Brezillon]

This is a collection of tiler heap fixes for bugs/oddities found while
looking at incremental rendering.

Ideally, we want to land those before 6.10 is released, so we don't need
to increment the driver version to reflect the ABI changes.

Changelog detailed in each commit.

Regards,

Boris

Antonino Maniscalco (1):
  drm/panthor: Fix tiler OOM handling to allow incremental rendering

Boris Brezillon (4):
  drm/panthor: Make sure the tiler initial/max chunks are consistent
  drm/panthor: Relax the constraints on the tiler chunk size
  drm/panthor: Fix an off-by-one in the heap context retrieval logic
  drm/panthor: Document drm_panthor_tiler_heap_destroy::handle validity
    constraints

 drivers/gpu/drm/panthor/panthor_heap.c  | 26 ++++++++++++++++---------
 drivers/gpu/drm/panthor/panthor_sched.c |  7 ++++++-
 include/uapi/drm/panthor_drm.h          | 20 +++++++++++++++----
 3 files changed, 39 insertions(+), 14 deletions(-)

-- 
2.44.0

[PATCH v3 1/5] drm/panthor: Fix tiler OOM handling to allow incremental rendering

[Boris Brezillon]

From: Antonino Maniscalco <antonino.maniscalco@collabora.com>

If the kernel couldn't allocate memory because we reached the maximum
number of chunks but no render passes are in flight
(panthor_heap_grow() returning -ENOMEM), we should defer the OOM
handling to the FW by returning a NULL chunk. The FW will then call
the tiler OOM exception handler, which is supposed to implement
incremental rendering (execute an intermediate fragment job to flush
the pending primitives, release the tiler memory that was used to
store those primitives, and start over from where it stopped).

Instead of checking for both ENOMEM and EBUSY, make panthor_heap_grow()
return ENOMEM no matter the reason of this allocation failure, the FW
doesn't care anyway.

v3:
- Add R-bs

v2:
- Make panthor_heap_grow() return -ENOMEM for all kind of allocation
  failures
- Document the panthor_heap_grow() semantics

Fixes: de8548813824 ("drm/panthor: Add the scheduler logical block")
Signed-off-by: Antonino Maniscalco <antonino.maniscalco@collabora.com>
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_heap.c  | 12 ++++++++----
 drivers/gpu/drm/panthor/panthor_sched.c |  7 ++++++-
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 143fa35f2e74..c3c0ba744937 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -410,6 +410,13 @@ int panthor_heap_return_chunk(struct panthor_heap_pool *pool,
  * @renderpasses_in_flight: Number of render passes currently in-flight.
  * @pending_frag_count: Number of fragment jobs waiting for execution/completion.
  * @new_chunk_gpu_va: Pointer used to return the chunk VA.
+ *
+ * Return:
+ * - 0 if a new heap was allocated
+ * - -ENOMEM if the tiler context reached the maximum number of chunks
+ *   or if too many render passes are in-flight
+ *   or if the allocation failed
+ * - -EINVAL if any of the arguments passed to panthor_heap_grow() is invalid
  */
 int panthor_heap_grow(struct panthor_heap_pool *pool,
 		      u64 heap_gpu_va,
@@ -439,10 +446,7 @@ int panthor_heap_grow(struct panthor_heap_pool *pool,
 	 * handler provided by the userspace driver, if any).
 	 */
 	if (renderpasses_in_flight > heap->target_in_flight ||
-	    (pending_frag_count > 0 && heap->chunk_count >= heap->max_chunks)) {
-		ret = -EBUSY;
-		goto out_unlock;
-	} else if (heap->chunk_count >= heap->max_chunks) {
+	    heap->chunk_count >= heap->max_chunks) {
 		ret = -ENOMEM;
 		goto out_unlock;
 	}
diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index b3a51a6de523..fd928362d45e 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1354,7 +1354,12 @@ static int group_process_tiler_oom(struct panthor_group *group, u32 cs_id)
 					pending_frag_count, &new_chunk_va);
 	}
 
-	if (ret && ret != -EBUSY) {
+	/* If the heap context doesn't have memory for us, we want to let the
+	 * FW try to reclaim memory by waiting for fragment jobs to land or by
+	 * executing the tiler OOM exception handler, which is supposed to
+	 * implement incremental rendering.
+	 */
+	if (ret && ret != -ENOMEM) {
 		drm_warn(&ptdev->base, "Failed to extend the tiler heap\n");
 		group->fatal_queues |= BIT(cs_id);
 		sched_queue_delayed_work(sched, tick, 0);
-- 
2.44.0

[PATCH v3 2/5] drm/panthor: Make sure the tiler initial/max chunks are consistent

[Boris Brezillon]

It doesn't make sense to have a maximum number of chunks smaller than
the initial number of chunks attached to the context.

Fix the uAPI header to reflect the new constraint, and mention the
undocumented "initial_chunk_count > 0" constraint while at it.

v3:
- Add R-b

v2:
- Fix the check

Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_heap.c | 3 +++
 include/uapi/drm/panthor_drm.h         | 8 ++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index c3c0ba744937..3be86ec383d6 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -281,6 +281,9 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 	if (initial_chunk_count == 0)
 		return -EINVAL;
 
+	if (initial_chunk_count > max_chunks)
+		return -EINVAL;
+
 	if (hweight32(chunk_size) != 1 ||
 	    chunk_size < SZ_256K || chunk_size > SZ_2M)
 		return -EINVAL;
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index dadb05ab1235..5db80a0682d5 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -895,13 +895,17 @@ struct drm_panthor_tiler_heap_create {
 	/** @vm_id: VM ID the tiler heap should be mapped to */
 	__u32 vm_id;
 
-	/** @initial_chunk_count: Initial number of chunks to allocate. */
+	/** @initial_chunk_count: Initial number of chunks to allocate. Must be at least one. */
 	__u32 initial_chunk_count;
 
 	/** @chunk_size: Chunk size. Must be a power of two at least 256KB large. */
 	__u32 chunk_size;
 
-	/** @max_chunks: Maximum number of chunks that can be allocated. */
+	/**
+	 * @max_chunks: Maximum number of chunks that can be allocated.
+	 *
+	 * Must be at least @initial_chunk_count.
+	 */
 	__u32 max_chunks;
 
 	/**
-- 
2.44.0

[PATCH v3 3/5] drm/panthor: Relax the constraints on the tiler chunk size

[Boris Brezillon]

The field used to store the chunk size if 12 bits wide, and the encoding
is chunk_size = chunk_header.chunk_size << 12, which gives us a
theoretical [4k:8M] range. This range is further limited by
implementation constraints, and all known implementations seem to
impose a [128k:8M] range, so do the same here.

We also relax the power-of-two constraint, which doesn't seem to
exist on v10. This will allow userspace to fine-tune initial/max
tiler memory on memory-constrained devices.

v3:
- Add R-bs
- Fix valid range in the kerneldoc

v2:
- Turn the power-of-two constraint into a page-aligned constraint to allow
  fine-tune of the initial/max heap memory size
- Fix the panthor_heap_create() kerneldoc

Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
Reviewed-by: Steven Price <steven.price@arm.com>
---
 drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
 include/uapi/drm/panthor_drm.h         | 6 +++++-
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 3be86ec383d6..683bb94761bc 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -253,8 +253,8 @@ int panthor_heap_destroy(struct panthor_heap_pool *pool, u32 handle)
  * @pool: Pool to instantiate the heap context from.
  * @initial_chunk_count: Number of chunk allocated at initialization time.
  * Must be at least 1.
- * @chunk_size: The size of each chunk. Must be a power of two between 256k
- * and 2M.
+ * @chunk_size: The size of each chunk. Must be page-aligned and lie in the
+ * [128k:2M] range.
  * @max_chunks: Maximum number of chunks that can be allocated.
  * @target_in_flight: Maximum number of in-flight render passes.
  * @heap_ctx_gpu_va: Pointer holding the GPU address of the allocated heap
@@ -284,8 +284,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 	if (initial_chunk_count > max_chunks)
 		return -EINVAL;
 
-	if (hweight32(chunk_size) != 1 ||
-	    chunk_size < SZ_256K || chunk_size > SZ_2M)
+	if (!IS_ALIGNED(chunk_size, PAGE_SIZE) ||
+	    chunk_size < SZ_128K || chunk_size > SZ_8M)
 		return -EINVAL;
 
 	down_read(&pool->lock);
diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index 5db80a0682d5..b8220d2e698f 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -898,7 +898,11 @@ struct drm_panthor_tiler_heap_create {
 	/** @initial_chunk_count: Initial number of chunks to allocate. Must be at least one. */
 	__u32 initial_chunk_count;
 
-	/** @chunk_size: Chunk size. Must be a power of two at least 256KB large. */
+	/**
+	 * @chunk_size: Chunk size.
+	 *
+	 * Must be page-aligned and lie in the [128k:8M] range.
+	 */
 	__u32 chunk_size;
 
 	/**
-- 
2.44.0

[PATCH v3 4/5] drm/panthor: Fix an off-by-one in the heap context retrieval logic

[Boris Brezillon]

The heap ID is used to index the heap context pool, and allocating
in the [1:MAX_HEAPS_PER_POOL] leads to an off-by-one. This was
originally to avoid returning a zero heap handle, but given the handle
is formed with (vm_id << 16) | heap_id, with vm_id > 0, we already can't
end up with a valid heap handle that's zero.

v3:
- Allocate in the [0:MAX_HEAPS_PER_POOL-1] range

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 | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 683bb94761bc..252332f5390f 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -323,7 +323,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 	if (!pool->vm) {
 		ret = -EINVAL;
 	} else {
-		ret = xa_alloc(&pool->xa, &id, heap, XA_LIMIT(1, MAX_HEAPS_PER_POOL), GFP_KERNEL);
+		ret = xa_alloc(&pool->xa, &id, heap,
+			       XA_LIMIT(0, MAX_HEAPS_PER_POOL - 1), GFP_KERNEL);
 		if (!ret) {
 			void *gpu_ctx = panthor_get_heap_ctx(pool, id);
 
-- 
2.44.0

[PATCH v3 5/5] drm/panthor: Document drm_panthor_tiler_heap_destroy::handle validity constraints

[Boris Brezillon]

Make sure the user is aware that drm_panthor_tiler_heap_destroy::handle
must be a handle previously returned by
DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE.

Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 include/uapi/drm/panthor_drm.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
index b8220d2e698f..aaed8e12ad0b 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -939,7 +939,11 @@ struct drm_panthor_tiler_heap_create {
  * struct drm_panthor_tiler_heap_destroy - Arguments passed to DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY
  */
 struct drm_panthor_tiler_heap_destroy {
-	/** @handle: Handle of the tiler heap to destroy */
+	/**
+	 * @handle: Handle of the tiler heap to destroy.
+	 *
+	 * Must be a valid heap handle returned by DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE.
+	 */
 	__u32 handle;
 
 	/** @pad: Padding field, MBZ. */
-- 
2.44.0

Re: [PATCH v3 3/5] drm/panthor: Relax the constraints on the tiler chunk size

[Steven Price]

On 02/05/2024 16:40, Boris Brezillon wrote:
> The field used to store the chunk size if 12 bits wide, and the encoding
> is chunk_size = chunk_header.chunk_size << 12, which gives us a
> theoretical [4k:8M] range. This range is further limited by
> implementation constraints, and all known implementations seem to
> impose a [128k:8M] range, so do the same here.
> 
> We also relax the power-of-two constraint, which doesn't seem to
> exist on v10. This will allow userspace to fine-tune initial/max
> tiler memory on memory-constrained devices.
> 
> v3:
> - Add R-bs
> - Fix valid range in the kerneldoc

Sadly the fixed range didn't make it to this posting... ;)

Steve

> 
> v2:
> - Turn the power-of-two constraint into a page-aligned constraint to allow
>   fine-tune of the initial/max heap memory size
> - Fix the panthor_heap_create() kerneldoc
> 
> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> Reviewed-by: Steven Price <steven.price@arm.com>
> ---
>  drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
>  include/uapi/drm/panthor_drm.h         | 6 +++++-
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 3be86ec383d6..683bb94761bc 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -253,8 +253,8 @@ int panthor_heap_destroy(struct panthor_heap_pool *pool, u32 handle)
>   * @pool: Pool to instantiate the heap context from.
>   * @initial_chunk_count: Number of chunk allocated at initialization time.
>   * Must be at least 1.
> - * @chunk_size: The size of each chunk. Must be a power of two between 256k
> - * and 2M.
> + * @chunk_size: The size of each chunk. Must be page-aligned and lie in the
> + * [128k:2M] range.
>   * @max_chunks: Maximum number of chunks that can be allocated.
>   * @target_in_flight: Maximum number of in-flight render passes.
>   * @heap_ctx_gpu_va: Pointer holding the GPU address of the allocated heap
> @@ -284,8 +284,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
>  	if (initial_chunk_count > max_chunks)
>  		return -EINVAL;
>  
> -	if (hweight32(chunk_size) != 1 ||
> -	    chunk_size < SZ_256K || chunk_size > SZ_2M)
> +	if (!IS_ALIGNED(chunk_size, PAGE_SIZE) ||
> +	    chunk_size < SZ_128K || chunk_size > SZ_8M)
>  		return -EINVAL;
>  
>  	down_read(&pool->lock);
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index 5db80a0682d5..b8220d2e698f 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -898,7 +898,11 @@ struct drm_panthor_tiler_heap_create {
>  	/** @initial_chunk_count: Initial number of chunks to allocate. Must be at least one. */
>  	__u32 initial_chunk_count;
>  
> -	/** @chunk_size: Chunk size. Must be a power of two at least 256KB large. */
> +	/**
> +	 * @chunk_size: Chunk size.
> +	 *
> +	 * Must be page-aligned and lie in the [128k:8M] range.
> +	 */
>  	__u32 chunk_size;
>  
>  	/**

Re: [PATCH v3 4/5] drm/panthor: Fix an off-by-one in the heap context retrieval logic

[Steven Price]

On 02/05/2024 16:40, Boris Brezillon wrote:
> The heap ID is used to index the heap context pool, and allocating
> in the [1:MAX_HEAPS_PER_POOL] leads to an off-by-one. This was
> originally to avoid returning a zero heap handle, but given the handle
> is formed with (vm_id << 16) | heap_id, with vm_id > 0, we already can't
> end up with a valid heap handle that's zero.
> 
> v3:
> - Allocate in the [0:MAX_HEAPS_PER_POOL-1] range
> 
> 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>

Don't we also need to change the xa_init_flags() in
panthor_heap_pool_create()?

Steve

> ---
>  drivers/gpu/drm/panthor/panthor_heap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 683bb94761bc..252332f5390f 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -323,7 +323,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
>  	if (!pool->vm) {
>  		ret = -EINVAL;
>  	} else {
> -		ret = xa_alloc(&pool->xa, &id, heap, XA_LIMIT(1, MAX_HEAPS_PER_POOL), GFP_KERNEL);
> +		ret = xa_alloc(&pool->xa, &id, heap,
> +			       XA_LIMIT(0, MAX_HEAPS_PER_POOL - 1), GFP_KERNEL);
>  		if (!ret) {
>  			void *gpu_ctx = panthor_get_heap_ctx(pool, id);
>  

Re: [PATCH v3 5/5] drm/panthor: Document drm_panthor_tiler_heap_destroy::handle validity constraints

[Steven Price]

On 02/05/2024 16:40, Boris Brezillon wrote:
> Make sure the user is aware that drm_panthor_tiler_heap_destroy::handle
> must be a handle previously returned by
> DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE.
> 
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>  include/uapi/drm/panthor_drm.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> index b8220d2e698f..aaed8e12ad0b 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -939,7 +939,11 @@ struct drm_panthor_tiler_heap_create {
>   * struct drm_panthor_tiler_heap_destroy - Arguments passed to DRM_IOCTL_PANTHOR_TILER_HEAP_DESTROY
>   */
>  struct drm_panthor_tiler_heap_destroy {
> -	/** @handle: Handle of the tiler heap to destroy */
> +	/**
> +	 * @handle: Handle of the tiler heap to destroy.
> +	 *
> +	 * Must be a valid heap handle returned by DRM_IOCTL_PANTHOR_TILER_HEAP_CREATE.
> +	 */
>  	__u32 handle;
>  
>  	/** @pad: Padding field, MBZ. */

Re: [PATCH v3 4/5] drm/panthor: Fix an off-by-one in the heap context retrieval logic

[Boris Brezillon]

On Thu, 2 May 2024 16:52:24 +0100
Steven Price <steven.price@arm.com> wrote:

> On 02/05/2024 16:40, Boris Brezillon wrote:
> > The heap ID is used to index the heap context pool, and allocating
> > in the [1:MAX_HEAPS_PER_POOL] leads to an off-by-one. This was
> > originally to avoid returning a zero heap handle, but given the handle
> > is formed with (vm_id << 16) | heap_id, with vm_id > 0, we already can't
> > end up with a valid heap handle that's zero.
> > 
> > v3:
> > - Allocate in the [0:MAX_HEAPS_PER_POOL-1] range
> > 
> > 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>  
> 
> Don't we also need to change the xa_init_flags() in
> panthor_heap_pool_create()?

Uh, we should, indeed.

> 
> Steve
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_heap.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> > index 683bb94761bc..252332f5390f 100644
> > --- a/drivers/gpu/drm/panthor/panthor_heap.c
> > +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> > @@ -323,7 +323,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
> >  	if (!pool->vm) {
> >  		ret = -EINVAL;
> >  	} else {
> > -		ret = xa_alloc(&pool->xa, &id, heap, XA_LIMIT(1, MAX_HEAPS_PER_POOL), GFP_KERNEL);
> > +		ret = xa_alloc(&pool->xa, &id, heap,
> > +			       XA_LIMIT(0, MAX_HEAPS_PER_POOL - 1), GFP_KERNEL);
> >  		if (!ret) {
> >  			void *gpu_ctx = panthor_get_heap_ctx(pool, id);
> >    
> 

Re: [PATCH v3 3/5] drm/panthor: Relax the constraints on the tiler chunk size

[Boris Brezillon]

On Thu, 2 May 2024 16:47:56 +0100
Steven Price <steven.price@arm.com> wrote:

> On 02/05/2024 16:40, Boris Brezillon wrote:
> > The field used to store the chunk size if 12 bits wide, and the encoding
> > is chunk_size = chunk_header.chunk_size << 12, which gives us a
> > theoretical [4k:8M] range. This range is further limited by
> > implementation constraints, and all known implementations seem to
> > impose a [128k:8M] range, so do the same here.
> > 
> > We also relax the power-of-two constraint, which doesn't seem to
> > exist on v10. This will allow userspace to fine-tune initial/max
> > tiler memory on memory-constrained devices.
> > 
> > v3:
> > - Add R-bs
> > - Fix valid range in the kerneldoc  
> 
> Sadly the fixed range didn't make it to this posting... ;)

My bad, I was checking the uAPI header and thought I had already fixed
it the other day. Should be good in v4.

> 
> Steve
> 
> > 
> > v2:
> > - Turn the power-of-two constraint into a page-aligned constraint to allow
> >   fine-tune of the initial/max heap memory size
> > - Fix the panthor_heap_create() kerneldoc
> > 
> > Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> > Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>
> > Reviewed-by: Steven Price <steven.price@arm.com>
> > ---
> >  drivers/gpu/drm/panthor/panthor_heap.c | 8 ++++----
> >  include/uapi/drm/panthor_drm.h         | 6 +++++-
> >  2 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> > index 3be86ec383d6..683bb94761bc 100644
> > --- a/drivers/gpu/drm/panthor/panthor_heap.c
> > +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> > @@ -253,8 +253,8 @@ int panthor_heap_destroy(struct panthor_heap_pool *pool, u32 handle)
> >   * @pool: Pool to instantiate the heap context from.
> >   * @initial_chunk_count: Number of chunk allocated at initialization time.
> >   * Must be at least 1.
> > - * @chunk_size: The size of each chunk. Must be a power of two between 256k
> > - * and 2M.
> > + * @chunk_size: The size of each chunk. Must be page-aligned and lie in the
> > + * [128k:2M] range.
> >   * @max_chunks: Maximum number of chunks that can be allocated.
> >   * @target_in_flight: Maximum number of in-flight render passes.
> >   * @heap_ctx_gpu_va: Pointer holding the GPU address of the allocated heap
> > @@ -284,8 +284,8 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
> >  	if (initial_chunk_count > max_chunks)
> >  		return -EINVAL;
> >  
> > -	if (hweight32(chunk_size) != 1 ||
> > -	    chunk_size < SZ_256K || chunk_size > SZ_2M)
> > +	if (!IS_ALIGNED(chunk_size, PAGE_SIZE) ||
> > +	    chunk_size < SZ_128K || chunk_size > SZ_8M)
> >  		return -EINVAL;
> >  
> >  	down_read(&pool->lock);
> > diff --git a/include/uapi/drm/panthor_drm.h b/include/uapi/drm/panthor_drm.h
> > index 5db80a0682d5..b8220d2e698f 100644
> > --- a/include/uapi/drm/panthor_drm.h
> > +++ b/include/uapi/drm/panthor_drm.h
> > @@ -898,7 +898,11 @@ struct drm_panthor_tiler_heap_create {
> >  	/** @initial_chunk_count: Initial number of chunks to allocate. Must be at least one. */
> >  	__u32 initial_chunk_count;
> >  
> > -	/** @chunk_size: Chunk size. Must be a power of two at least 256KB large. */
> > +	/**
> > +	 * @chunk_size: Chunk size.
> > +	 *
> > +	 * Must be page-aligned and lie in the [128k:8M] range.
> > +	 */
> >  	__u32 chunk_size;
> >  
> >  	/**  
>