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

[Boris Brezillon <boris.brezillon@collabora.com>]

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.