| Message ID | 20240425071837.529039-4-boris.brezillon@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/panthor: Collection of tiler heap related fixes | expand |
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; > > /**
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.
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; /**
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(-)