On Thu, 25 Apr 2024 10:28:56 +0100
Steven Price <steven.price@xxxxxxx> 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@xxxxxxxxxxxxx>
>
> 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.
