On 10/03/2022 14:47, Arunpravin wrote:
On 08/03/22 10:31 pm, Matthew Auld wrote:
On 08/03/2022 13:59, Arunpravin wrote:
On 07/03/22 10:11 pm, Matthew Auld wrote:
On 07/03/2022 14:37, Arunpravin wrote:
place BUG_ON(order < min_order) outside do..while
loop as it fails Unigine Heaven benchmark.
Unigine Heaven has buffer allocation requests for
example required pages are 161 and alignment request
is 128. To allocate the remaining 33 pages, continues
the iteration to find the order value which is 5 and
when it compares with min_order = 7, enables the
BUG_ON(). To avoid this problem, placed the BUG_ON
check outside of do..while loop.
Signed-off-by: Arunpravin <Arunpravin.PaneerSelvam@xxxxxxx>
---
drivers/gpu/drm/drm_buddy.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c
index 72f52f293249..ed94c56b720f 100644
--- a/drivers/gpu/drm/drm_buddy.c
+++ b/drivers/gpu/drm/drm_buddy.c
@@ -669,10 +669,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm,
order = fls(pages) - 1;
min_order = ilog2(min_page_size) - ilog2(mm->chunk_size);
+ BUG_ON(order < min_order);
Isn't the issue that we are allowing a size that is not aligned to the
requested min_page_size? Should we not fix the caller(and throw a normal
error here), or perhaps add the round_up() here instead?
CASE 1:
when size is not aligned to the requested min_page_size, for instance,
required size = 161 pages, min_page_size = 128 pages, here we have 3
possible options,
a. AFAIK,This kind of situation is common in any workload,the first
allocation (i.e) 128 pages is aligned to min_page_size, Should we just
allocate the left over 33 pages (2 pow 5, 2 pow 0) since the caller does
know the left over pages are not in min_page_size alignment?
So IIUC looking at amdgpu_gem_create_ioctl(), userspace can specify some
arbitrary physical alignment for an object? Is that not meant to apply
to every page/chunk? The above example would only have the correct
physical alignment guaranteed for the first chunk, or so, is this the
expected ABI behaviour?
I gone through the function amdgpu_gem_create_ioctl(), it reads the
physical alignment in bytes from userspace, does i915 round up the size
value to the alignment or does i915 fails the allocation request if size
is not aligned with min_page_size? If not, I think running unigine
heaven or similar benchmark triggers BUG_ON() on current version of drm
buddy
i915 will always round_up the obj->base.size as per the
default_page_size. But in our case the default_page_size is selected by
the kernel, which is always either PAGE_SIZE, or 64K on some platforms,
due to the HW having some minimum GPU page-size for mapping VRAM pages.
We don't currently have anything similar to
amdgpu_gem_create_in.alignment, where userspace can request some
arbitrary physical alignment.
Also looking at this some more, the other related bug here is the
order-- == min_order check, since it now won't bail when order == 0,
leading to order = -1, if we are unlucky...
will add a fix
Originally, if asking for min_page_size > chunk_size, then the
allocation was meant to fail if it can't fill the resource request with
pages of at least that size(and also alignment). Or at least that was
the original meaning in i915 IIRC.
we can follow the same here too, failing the allocation request if size
is not aligned with min_page_size?
Yeah, seems reasonable to me.
I added a debug print for requested num_pages from userspace and its
alignment request and executed unigine heaven, I see many such instances
where min_page_size is not aligned to the size, how i915 handles such
requests?
b. There are many such instances in unigine heaven workload (there would
be many such workloads), throwing a normal error would lower the FPS? is
it possible to fix at caller application?
c. adding the round_up() is possible, but in every such instances we end
up allocating extra unused memory. For example, if required pages = 1028
and min_page_size = 1024 pages, we end up round up of left over 4 pages
to the min_page_size, so the total size would be 2048 pages.
i.e if someone does:
alloc_blocks(mm, 0, end, 4096, 1<<16, &blocks, flags);
CASE 2:
I think this case should be detected (i.e) when min_page_size > size,
should we return -EINVAL?
This will still trigger the BUG_ON() even if we move it out of the loop,
AFAICT.
Should we just allow the CASE 1 proceed for the allocation and return
-EINVAL for the CASE 2?
+
do {
order = min(order, (unsigned int)fls(pages) - 1);
BUG_ON(order > mm->max_order);
- BUG_ON(order < min_order);
do {
if (flags & DRM_BUDDY_RANGE_ALLOCATION)
base-commit: 8025c79350b90e5a8029234d433578f12abbae2b
