| Message ID | 20220330090410.135332-1-Arunpravin.PaneerSelvam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: round_up the size to the alignment value | expand |
Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam: > Round up the size value to the min_page_size and trim the last block to > the required size. > > This solves a bug detected when size is not aligned with the min_page_size. > Unigine Heaven has allocation requests for example required pages are 257 > and alignment request is 256. To allocate the left over 1 page, continues > the iteration to find the order value which is 0 and when it compares with > min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue > we round_up the size value to the min_page_size and trim the last block to > the computed required size value. Well, Matthew and you convinced me to *not* do it like this. Has that conclusion changed somehow? Regards, Christian. > > Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> > --- > drivers/gpu/drm/drm_buddy.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c > index 72f52f293249..98d7ec359b08 100644 > --- a/drivers/gpu/drm/drm_buddy.c > +++ b/drivers/gpu/drm/drm_buddy.c > @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > unsigned int min_order, order; > unsigned long pages; > LIST_HEAD(allocated); > + u64 cur_size; > int err; > > if (size < mm->chunk_size) > @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > if (start + size == end) > return __drm_buddy_alloc_range(mm, start, size, blocks); > > + cur_size = size; > + > + if (!IS_ALIGNED(size, min_page_size)) > + size = round_up(size, min_page_size); > + > pages = size >> ilog2(mm->chunk_size); > order = fls(pages) - 1; > min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); > @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > break; > } while (1); > > + > + /* > + * If size value rounded up to min_page_size, trim the last block > + * to the required size > + */ > + if (cur_size != size) { > + struct drm_buddy_block *trim_block; > + LIST_HEAD(trim_list); > + u64 required_size; > + > + trim_block = list_last_entry(&allocated, typeof(*trim_block), link); > + list_move_tail(&trim_block->link, &trim_list); > + /* > + * Compute the required_size value by subtracting the last block size > + * with (aligned size - original size) > + */ > + required_size = drm_buddy_block_size(mm, trim_block) - (size - cur_size); > + > + drm_buddy_block_trim(mm, > + required_size, > + &trim_list); > + > + list_splice_tail(&trim_list, &allocated); > + } > + > list_splice_tail(&allocated, blocks); > return 0; > > > base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
Am 30.03.22 um 11:20 schrieb Arunpravin Paneer Selvam: > > On 30/03/22 2:37 pm, Christian König wrote: >> Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam: >>> Round up the size value to the min_page_size and trim the last block to >>> the required size. >>> >>> This solves a bug detected when size is not aligned with the min_page_size. >>> Unigine Heaven has allocation requests for example required pages are 257 >>> and alignment request is 256. To allocate the left over 1 page, continues >>> the iteration to find the order value which is 0 and when it compares with >>> min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue >>> we round_up the size value to the min_page_size and trim the last block to >>> the computed required size value. >> Well, Matthew and you convinced me to *not* do it like this. >> >> Has that conclusion changed somehow? >> > Yes, now he is ok to handle rounding + trimming in drm buddy Yeah, but I'm no longer :) How do we then handle the detection of contiguous allocation? As I said we can do that like: 1. alloc 2. check if we only have a single node 3. trim But if we include the trim here we can't do it any more. Only alternative would then be to inspect each node and see if it follows directly behind the predecessor. Regards, Christian. > >> Regards, >> Christian. >> >>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >>> --- >>> drivers/gpu/drm/drm_buddy.c | 31 +++++++++++++++++++++++++++++++ >>> 1 file changed, 31 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>> index 72f52f293249..98d7ec359b08 100644 >>> --- a/drivers/gpu/drm/drm_buddy.c >>> +++ b/drivers/gpu/drm/drm_buddy.c >>> @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>> unsigned int min_order, order; >>> unsigned long pages; >>> LIST_HEAD(allocated); >>> + u64 cur_size; >>> int err; >>> >>> if (size < mm->chunk_size) >>> @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>> if (start + size == end) >>> return __drm_buddy_alloc_range(mm, start, size, blocks); >>> >>> + cur_size = size; >>> + >>> + if (!IS_ALIGNED(size, min_page_size)) >>> + size = round_up(size, min_page_size); >>> + >>> pages = size >> ilog2(mm->chunk_size); >>> order = fls(pages) - 1; >>> min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); >>> @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>> break; >>> } while (1); >>> >>> + >>> + /* >>> + * If size value rounded up to min_page_size, trim the last block >>> + * to the required size >>> + */ >>> + if (cur_size != size) { >>> + struct drm_buddy_block *trim_block; >>> + LIST_HEAD(trim_list); >>> + u64 required_size; >>> + >>> + trim_block = list_last_entry(&allocated, typeof(*trim_block), link); >>> + list_move_tail(&trim_block->link, &trim_list); >>> + /* >>> + * Compute the required_size value by subtracting the last block size >>> + * with (aligned size - original size) >>> + */ >>> + required_size = drm_buddy_block_size(mm, trim_block) - (size - cur_size); >>> + >>> + drm_buddy_block_trim(mm, >>> + required_size, >>> + &trim_list); >>> + >>> + list_splice_tail(&trim_list, &allocated); >>> + } >>> + >>> list_splice_tail(&allocated, blocks); >>> return 0; >>> >>> >>> base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b
On 30/03/22 2:37 pm, Christian König wrote: > Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam: >> Round up the size value to the min_page_size and trim the last block to >> the required size. >> >> This solves a bug detected when size is not aligned with the min_page_size. >> Unigine Heaven has allocation requests for example required pages are 257 >> and alignment request is 256. To allocate the left over 1 page, continues >> the iteration to find the order value which is 0 and when it compares with >> min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue >> we round_up the size value to the min_page_size and trim the last block to >> the computed required size value. > > Well, Matthew and you convinced me to *not* do it like this. > > Has that conclusion changed somehow? > Yes, now he is ok to handle rounding + trimming in drm buddy > Regards, > Christian. > >> >> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >> --- >> drivers/gpu/drm/drm_buddy.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >> index 72f52f293249..98d7ec359b08 100644 >> --- a/drivers/gpu/drm/drm_buddy.c >> +++ b/drivers/gpu/drm/drm_buddy.c >> @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >> unsigned int min_order, order; >> unsigned long pages; >> LIST_HEAD(allocated); >> + u64 cur_size; >> int err; >> >> if (size < mm->chunk_size) >> @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >> if (start + size == end) >> return __drm_buddy_alloc_range(mm, start, size, blocks); >> >> + cur_size = size; >> + >> + if (!IS_ALIGNED(size, min_page_size)) >> + size = round_up(size, min_page_size); >> + >> pages = size >> ilog2(mm->chunk_size); >> order = fls(pages) - 1; >> min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); >> @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >> break; >> } while (1); >> >> + >> + /* >> + * If size value rounded up to min_page_size, trim the last block >> + * to the required size >> + */ >> + if (cur_size != size) { >> + struct drm_buddy_block *trim_block; >> + LIST_HEAD(trim_list); >> + u64 required_size; >> + >> + trim_block = list_last_entry(&allocated, typeof(*trim_block), link); >> + list_move_tail(&trim_block->link, &trim_list); >> + /* >> + * Compute the required_size value by subtracting the last block size >> + * with (aligned size - original size) >> + */ >> + required_size = drm_buddy_block_size(mm, trim_block) - (size - cur_size); >> + >> + drm_buddy_block_trim(mm, >> + required_size, >> + &trim_list); >> + >> + list_splice_tail(&trim_list, &allocated); >> + } >> + >> list_splice_tail(&allocated, blocks); >> return 0; >> >> >> base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b >
On 30/03/22 2:42 pm, Christian König wrote: > Am 30.03.22 um 11:20 schrieb Arunpravin Paneer Selvam: >> >> On 30/03/22 2:37 pm, Christian König wrote: >>> Am 30.03.22 um 11:04 schrieb Arunpravin Paneer Selvam: >>>> Round up the size value to the min_page_size and trim the last block to >>>> the required size. >>>> >>>> This solves a bug detected when size is not aligned with the min_page_size. >>>> Unigine Heaven has allocation requests for example required pages are 257 >>>> and alignment request is 256. To allocate the left over 1 page, continues >>>> the iteration to find the order value which is 0 and when it compares with >>>> min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue >>>> we round_up the size value to the min_page_size and trim the last block to >>>> the computed required size value. >>> Well, Matthew and you convinced me to *not* do it like this. >>> >>> Has that conclusion changed somehow? >>> >> Yes, now he is ok to handle rounding + trimming in drm buddy > > Yeah, but I'm no longer :) > > How do we then handle the detection of contiguous allocation? > > As I said we can do that like: > 1. alloc > 2. check if we only have a single node I think verifying the list is a single node would allow all the power of 2 requests(1 page, 2 pages, 4 pages etc..) single node and CONTIGUOUS flag not enabled cases entering into the trim function and simply return since the original size == roundup_pow_of_2 size. can we handle all the situation (alignment rounding trimming + contiguous trimming) in a single if condition like below, if (cur_size != (pages << PAGE_SHIFT)) where cur_size = stores the size value before round_up(alignment rounding up) or round_pow_of_2 (contiguous rounding up) pages = stores the size value after round_up(alignment rounding up) or round_pow_of_2 (contiguous rounding up) if there is a difference b/w these 2 numbers, we enter the trim block - - For a single node, we pass the original size (contiguous trimming) - For multiple node, we fetch the last block and trim the computed size (alignment rounding trimming) > 3. trim > > But if we include the trim here we can't do it any more. > > Only alternative would then be to inspect each node and see if it > follows directly behind the predecessor. > ok. Therefore, we handle both contiguous allocation trimming and alignment rounding up trimming (only last block) in amdgpu and i915 driver. And, in drm buddy we just have a check to return -EINVAL if size is not aligned to min_page_size. If yes to above statements, I included alignment rounding up trimming (only last block) in the same place where currently we trim for the contiguous allocation. I will send the patch for review. > Regards, > Christian. > >> >>> Regards, >>> Christian. >>> >>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >>>> --- >>>> drivers/gpu/drm/drm_buddy.c | 31 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>> index 72f52f293249..98d7ec359b08 100644 >>>> --- a/drivers/gpu/drm/drm_buddy.c >>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>> @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>> unsigned int min_order, order; >>>> unsigned long pages; >>>> LIST_HEAD(allocated); >>>> + u64 cur_size; >>>> int err; >>>> >>>> if (size < mm->chunk_size) >>>> @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>> if (start + size == end) >>>> return __drm_buddy_alloc_range(mm, start, size, blocks); >>>> >>>> + cur_size = size; >>>> + >>>> + if (!IS_ALIGNED(size, min_page_size)) >>>> + size = round_up(size, min_page_size); >>>> + >>>> pages = size >> ilog2(mm->chunk_size); >>>> order = fls(pages) - 1; >>>> min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); >>>> @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>> break; >>>> } while (1); >>>> >>>> + >>>> + /* >>>> + * If size value rounded up to min_page_size, trim the last block >>>> + * to the required size >>>> + */ >>>> + if (cur_size != size) { >>>> + struct drm_buddy_block *trim_block; >>>> + LIST_HEAD(trim_list); >>>> + u64 required_size; >>>> + >>>> + trim_block = list_last_entry(&allocated, typeof(*trim_block), link); >>>> + list_move_tail(&trim_block->link, &trim_list); >>>> + /* >>>> + * Compute the required_size value by subtracting the last block size >>>> + * with (aligned size - original size) >>>> + */ >>>> + required_size = drm_buddy_block_size(mm, trim_block) - (size - cur_size); >>>> + >>>> + drm_buddy_block_trim(mm, >>>> + required_size, >>>> + &trim_list); >>>> + >>>> + list_splice_tail(&trim_list, &allocated); >>>> + } >>>> + >>>> list_splice_tail(&allocated, blocks); >>>> return 0; >>>> >>>> >>>> base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b >
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..98d7ec359b08 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -641,6 +641,7 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, unsigned int min_order, order; unsigned long pages; LIST_HEAD(allocated); + u64 cur_size; int err; if (size < mm->chunk_size) @@ -665,6 +666,11 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, if (start + size == end) return __drm_buddy_alloc_range(mm, start, size, blocks); + cur_size = size; + + if (!IS_ALIGNED(size, min_page_size)) + size = round_up(size, min_page_size); + pages = size >> ilog2(mm->chunk_size); order = fls(pages) - 1; min_order = ilog2(min_page_size) - ilog2(mm->chunk_size); @@ -702,6 +708,31 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, break; } while (1); + + /* + * If size value rounded up to min_page_size, trim the last block + * to the required size + */ + if (cur_size != size) { + struct drm_buddy_block *trim_block; + LIST_HEAD(trim_list); + u64 required_size; + + trim_block = list_last_entry(&allocated, typeof(*trim_block), link); + list_move_tail(&trim_block->link, &trim_list); + /* + * Compute the required_size value by subtracting the last block size + * with (aligned size - original size) + */ + required_size = drm_buddy_block_size(mm, trim_block) - (size - cur_size); + + drm_buddy_block_trim(mm, + required_size, + &trim_list); + + list_splice_tail(&trim_list, &allocated); + } + list_splice_tail(&allocated, blocks); return 0;
Round up the size value to the min_page_size and trim the last block to the required size. This solves a bug detected when size is not aligned with the min_page_size. Unigine Heaven has allocation requests for example required pages are 257 and alignment request is 256. To allocate the left over 1 page, continues the iteration to find the order value which is 0 and when it compares with min_order = 8, triggers the BUG_ON(order < min_order). To avoid this issue we round_up the size value to the min_page_size and trim the last block to the computed required size value. Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> --- drivers/gpu/drm/drm_buddy.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+) base-commit: ec57376fba5abc0e571617ff88e2ade7970c2e4b