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

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

[Boris Brezillon]

Hello,

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

We need to make sure those land before 6.10 is released (we still have
plenty of time), so we don't need to increment the driver version to
reflect the ABI changes.

Regards,

Boris

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

Boris Brezillon (2):
  drm/panthor: Make sure the tiler initial/max chunks are consistent
  drm/panthor: Relax the check on the tiler chunk size

 drivers/gpu/drm/panthor/panthor_heap.c  | 5 ++++-
 drivers/gpu/drm/panthor/panthor_sched.c | 8 +++++++-
 2 files changed, 11 insertions(+), 2 deletions(-)

-- 
2.44.0

[PATCH 1/3] 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).

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>
---
 drivers/gpu/drm/panthor/panthor_sched.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
index b3a51a6de523..6de8c0c702cb 100644
--- a/drivers/gpu/drm/panthor/panthor_sched.c
+++ b/drivers/gpu/drm/panthor/panthor_sched.c
@@ -1354,7 +1354,13 @@ 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 kernel couldn't allocate memory because we reached the maximum
+	 * number of chunks (EBUSY if we have render passes in flight, ENOMEM
+	 * otherwise), 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 != -EBUSY && 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

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

[Steven Price]

On 25/04/2024 08:18, Boris Brezillon wrote:
> 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).
> 
> 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: Steven Price <steven.price@arm.com>

Although I think the real issue here is that we haven't clearly defined
the return values from panthor_heap_grow - it's a bit weird to have two
different error codes for the same "try again later after incremental
rendering" result. But as a fix this seems most clear.

Steve

> ---
>  drivers/gpu/drm/panthor/panthor_sched.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> index b3a51a6de523..6de8c0c702cb 100644
> --- a/drivers/gpu/drm/panthor/panthor_sched.c
> +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> @@ -1354,7 +1354,13 @@ 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 kernel couldn't allocate memory because we reached the maximum
> +	 * number of chunks (EBUSY if we have render passes in flight, ENOMEM
> +	 * otherwise), 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 != -EBUSY && 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);

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

[Boris Brezillon]

On Thu, 25 Apr 2024 10:28:49 +0100
Steven Price <steven.price@arm.com> wrote:

> On 25/04/2024 08:18, Boris Brezillon wrote:
> > 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).
> > 
> > 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: Steven Price <steven.price@arm.com>
> 
> Although I think the real issue here is that we haven't clearly defined
> the return values from panthor_heap_grow - it's a bit weird to have two
> different error codes for the same "try again later after incremental
> rendering" result. But as a fix this seems most clear.

Yeah, I actually considered returning -EBUSY for the 'max_chunks
reached' situation, but then realized we would also want to trigger
incremental rendering for actual mem allocation failures (when
chunk_count < max_chunks) once the fail-able/non-blocking allocation
logic is implemented, and for this kind of failure it makes more sense
to return -ENOMEM, even though this implies checking against two values
instead of one.

I guess returning -ENOMEM instead of -EBUSY for the case where we have
render passes in-flight wouldn't be too awkward, as this can be seen as
the kernel refusing to allocate more memory.

> 
> Steve
> 
> > ---
> >  drivers/gpu/drm/panthor/panthor_sched.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/panthor/panthor_sched.c b/drivers/gpu/drm/panthor/panthor_sched.c
> > index b3a51a6de523..6de8c0c702cb 100644
> > --- a/drivers/gpu/drm/panthor/panthor_sched.c
> > +++ b/drivers/gpu/drm/panthor/panthor_sched.c
> > @@ -1354,7 +1354,13 @@ 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 kernel couldn't allocate memory because we reached the maximum
> > +	 * number of chunks (EBUSY if we have render passes in flight, ENOMEM
> > +	 * otherwise), 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 != -EBUSY && 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);  
> 

[PATCH 2/3] 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.

Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.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 143fa35f2e74..8728c9bb76e4 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

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

[Steven Price]

On 25/04/2024 08:18, Boris Brezillon wrote:
> 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.
> 
> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.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 143fa35f2e74..8728c9bb76e4 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;
>  
>  	/**

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

[Steven Price]

On 25/04/2024 10:28, Steven Price wrote:
> On 25/04/2024 08:18, Boris Brezillon wrote:
>> 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.
>>
>> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
>> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
> 
> Reviewed-by: Steven Price <steven.price@arm.com>

Ok, I'll take that back... I've rebased (and fixed up all the out of
tree patches) and this doesn't work when I actually test it!

> 
>> ---
>>  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 143fa35f2e74..8728c9bb76e4 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)

This should be initial_chunk_count > max_chunks. Otherwise you're
requiring the initial chunk count to be equal *or greater* than the max
chunks which makes no sense!

Steve

>> +		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;
>>  
>>  	/**
> 

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

[Boris Brezillon]

On Thu, 25 Apr 2024 11:43:39 +0100
Steven Price <steven.price@arm.com> wrote:

> On 25/04/2024 10:28, Steven Price wrote:
> > On 25/04/2024 08:18, Boris Brezillon wrote:  
> >> 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.
> >>
> >> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> >> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> > 
> > Reviewed-by: Steven Price <steven.price@arm.com>  
> 
> Ok, I'll take that back... I've rebased (and fixed up all the out of
> tree patches) and this doesn't work when I actually test it!
> 
> >   
> >> ---
> >>  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 143fa35f2e74..8728c9bb76e4 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)  
> 
> This should be initial_chunk_count > max_chunks. Otherwise you're
> requiring the initial chunk count to be equal *or greater* than the max
> chunks which makes no sense!

Damn it, here's what happens when you think your changes are too
trivial to be wrong...

But I swear I would have tested the whole thing before pushing to
drm-misc. :P

[PATCH 3/3] drm/panthor: Relax the check 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, but those shouldn't be enforced kernel side.

Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>
---
 drivers/gpu/drm/panthor/panthor_heap.c | 2 +-
 include/uapi/drm/panthor_drm.h         | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
index 8728c9bb76e4..146ea2f57764 100644
--- a/drivers/gpu/drm/panthor/panthor_heap.c
+++ b/drivers/gpu/drm/panthor/panthor_heap.c
@@ -285,7 +285,7 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
 		return -EINVAL;
 
 	if (hweight32(chunk_size) != 1 ||
-	    chunk_size < SZ_256K || chunk_size > SZ_2M)
+	    chunk_size < SZ_4K || 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..80c0716361b9 100644
--- a/include/uapi/drm/panthor_drm.h
+++ b/include/uapi/drm/panthor_drm.h
@@ -898,7 +898,7 @@ 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 a power of two and lie in the [4k:8M] range. */
 	__u32 chunk_size;
 
 	/**
-- 
2.44.0

Re: [PATCH 3/3] drm/panthor: Relax the check on the tiler chunk size

[Steven Price]

On 25/04/2024 08:18, Boris Brezillon wrote:
> The field used to store the chunk size if 12 bits wide, and the encoding
NIT:                                     ^^ is

> is chunk_size = chunk_header.chunk_size << 12, which gives us a
> theoretical [4k:8M] range. This range is further limited by
> implementation constraints, but those shouldn't be enforced kernel side.
> 
> Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>

There's also a kerneldoc comment above panthor_heap_create that needs
updating too:

> /**
>  * panthor_heap_create() - Create a heap context
>  * @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.

I'm also a little unsure on whether this is the right change. The
"implementation defined" min/max in the hardware docs say 128KiB to
8MiB. I'm not convinced it makes sense to increase the range beyond that.

As far as I'm aware the "must be a power of 2" isn't actually a
requirement (it's certainly not a requirement of the storage format) so
the kernel is already being more restrictive than necessary.

It seems like a good idea to keep the uAPI requirements stricter than
necessary and relax them in the future if we have a good reason (e.g.
new hardware supports a wider range). But matching the existing hardware
range of 128KB-8MB would obviously make sense now.

Steve

> ---
>  drivers/gpu/drm/panthor/panthor_heap.c | 2 +-
>  include/uapi/drm/panthor_drm.h         | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panthor/panthor_heap.c b/drivers/gpu/drm/panthor/panthor_heap.c
> index 8728c9bb76e4..146ea2f57764 100644
> --- a/drivers/gpu/drm/panthor/panthor_heap.c
> +++ b/drivers/gpu/drm/panthor/panthor_heap.c
> @@ -285,7 +285,7 @@ int panthor_heap_create(struct panthor_heap_pool *pool,
>  		return -EINVAL;
>  
>  	if (hweight32(chunk_size) != 1 ||
> -	    chunk_size < SZ_256K || chunk_size > SZ_2M)
> +	    chunk_size < SZ_4K || 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..80c0716361b9 100644
> --- a/include/uapi/drm/panthor_drm.h
> +++ b/include/uapi/drm/panthor_drm.h
> @@ -898,7 +898,7 @@ 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 a power of two and lie in the [4k:8M] range. */
>  	__u32 chunk_size;
>  
>  	/**

Re: [PATCH 3/3] drm/panthor: Relax the check on the tiler chunk size

[Boris Brezillon]

On Thu, 25 Apr 2024 10:28:56 +0100
Steven Price <steven.price@arm.com> wrote:

> On 25/04/2024 08:18, Boris Brezillon wrote:
> > The field used to store the chunk size if 12 bits wide, and the encoding  
> NIT:                                     ^^ is
> 
> > is chunk_size = chunk_header.chunk_size << 12, which gives us a
> > theoretical [4k:8M] range. This range is further limited by
> > implementation constraints, but those shouldn't be enforced kernel side.
> > 
> > Fixes: 9cca48fa4f89 ("drm/panthor: Add the heap logical block")
> > Signed-off-by: Boris Brezillon <boris.brezillon@collabora.com>  
> 
> There's also a kerneldoc comment above panthor_heap_create that needs
> updating too:
> 
> > /**
> >  * panthor_heap_create() - Create a heap context
> >  * @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.  
> 
> I'm also a little unsure on whether this is the right change. The
> "implementation defined" min/max in the hardware docs say 128KiB to
> 8MiB. I'm not convinced it makes sense to increase the range beyond that.
> 
> As far as I'm aware the "must be a power of 2" isn't actually a
> requirement (it's certainly not a requirement of the storage format) so
> the kernel is already being more restrictive than necessary.

Ah, I got that wrong because v9 has this must-be-a-power-of-two
constraint (which is also where the erroneous 256k:2M range came from
BTW).

Chris, I guess you'd prefer to have the power-of-two constraint relaxed
too, so we can fine-tune the chunk size.

> 
> It seems like a good idea to keep the uAPI requirements stricter than
> necessary and relax them in the future if we have a good reason (e.g.
> new hardware supports a wider range). But matching the existing hardware
> range of 128KB-8MB would obviously make sense now.

Sure, I'll restrict the range to 128k:8M as you suggest.