| Message ID | 20220323073426.228866-1-Arunpravin.PaneerSelvam@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] drm: add a check to verify the size alignment | expand |
Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: > Add a simple check to reject any size not aligned to the > min_page_size. > > handle instances 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 problem, we added a simple check to > return -EINVAL if size is not aligned to the min_page_size. > > v2: Added more details to the commit description > > Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> > Suggested-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/drm_buddy.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c > index 72f52f293249..b503c88786b0 100644 > --- a/drivers/gpu/drm/drm_buddy.c > +++ b/drivers/gpu/drm/drm_buddy.c > @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > if (range_overflows(start, size, mm->size)) > return -EINVAL; > > + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) > + return -EINVAL; > + I'm not that happy with the handling here. See a minimum page size larger than the requested size is perfectly valid, it just means that the remaining pages needs to be trimmed. In other words when the request is to allocate 1 page with an alignment of 256 we just need to give the remaining 255 pages back to the allocator. Regards, Christian. > /* Actual range allocation */ > if (start + size == end) > return __drm_buddy_alloc_range(mm, start, size, blocks); > > base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b
On Tue, 29 Mar 2022 at 12:17, Arunpravin Paneer Selvam <arunpravin.paneerselvam@amd.com> wrote: > > > > On 23/03/22 1:15 pm, Christian König wrote: > > Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: > >> Add a simple check to reject any size not aligned to the > >> min_page_size. > >> > >> handle instances 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 problem, we added a simple check to > >> return -EINVAL if size is not aligned to the min_page_size. > >> > >> v2: Added more details to the commit description > >> > >> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> > >> Suggested-by: Matthew Auld <matthew.auld@intel.com> > >> --- > >> drivers/gpu/drm/drm_buddy.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c > >> index 72f52f293249..b503c88786b0 100644 > >> --- a/drivers/gpu/drm/drm_buddy.c > >> +++ b/drivers/gpu/drm/drm_buddy.c > >> @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, > >> if (range_overflows(start, size, mm->size)) > >> return -EINVAL; > >> > >> + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) > >> + return -EINVAL; > >> + > > > > I'm not that happy with the handling here. > > > > See a minimum page size larger than the requested size is perfectly > > valid, it just means that the remaining pages needs to be trimmed. > > > > In other words when the request is to allocate 1 page with an alignment > > of 256 we just need to give the remaining 255 pages back to the allocator. > > In one of the previous mail Matthew explained that i915 expects to > return -EINVAL error code if size is not aligned to min_page_size. We could also move the WARN_ON() into i915 as a separate patch, and just change the default buddy behaviour to transparently handle the rounding + trim, if you prefer. I don't have a strong opinion. > > can we just modify in amdgpu code where we round_up the size to the > min_page_size value and keep this error handling in drm_buddy.c? > > > > Regards, > > Christian. > > > >> /* Actual range allocation */ > >> if (start + size == end) > >> return __drm_buddy_alloc_range(mm, start, size, blocks); > >> > >> base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b > >
Am 29.03.22 um 13:28 schrieb Arunpravin Paneer Selvam: > On 23/03/22 1:15 pm, Christian König wrote: >> Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: >>> Add a simple check to reject any size not aligned to the >>> min_page_size. >>> >>> handle instances 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 problem, we added a simple check to >>> return -EINVAL if size is not aligned to the min_page_size. >>> >>> v2: Added more details to the commit description >>> >>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >>> Suggested-by: Matthew Auld <matthew.auld@intel.com> >>> --- >>> drivers/gpu/drm/drm_buddy.c | 3 +++ >>> 1 file changed, 3 insertions(+) >>> >>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>> index 72f52f293249..b503c88786b0 100644 >>> --- a/drivers/gpu/drm/drm_buddy.c >>> +++ b/drivers/gpu/drm/drm_buddy.c >>> @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>> if (range_overflows(start, size, mm->size)) >>> return -EINVAL; >>> >>> + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) >>> + return -EINVAL; >>> + >> I'm not that happy with the handling here. >> >> See a minimum page size larger than the requested size is perfectly >> valid, it just means that the remaining pages needs to be trimmed. >> >> In other words when the request is to allocate 1 page with an alignment >> of 256 we just need to give the remaining 255 pages back to the allocator. > In one of the previous mail Matthew explained that i915 expects to > return -EINVAL error code if size is not aligned to min_page_size. > > can we just modify in amdgpu code where we round_up the size to the > min_page_size value and keep this error handling in drm_buddy.c? Yeah, I'm fine with that as well now. I realized that this is probably the easiest option to check if an allocation is contiguous or not between the alloc and trim. So having two functions for that sounds like a good idea to me. Thanks, Christian. >> Regards, >> Christian. >> >>> /* Actual range allocation */ >>> if (start + size == end) >>> return __drm_buddy_alloc_range(mm, start, size, blocks); >>> >>> base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b
On 23/03/22 1:15 pm, Christian König wrote: > Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: >> Add a simple check to reject any size not aligned to the >> min_page_size. >> >> handle instances 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 problem, we added a simple check to >> return -EINVAL if size is not aligned to the min_page_size. >> >> v2: Added more details to the commit description >> >> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >> Suggested-by: Matthew Auld <matthew.auld@intel.com> >> --- >> drivers/gpu/drm/drm_buddy.c | 3 +++ >> 1 file changed, 3 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >> index 72f52f293249..b503c88786b0 100644 >> --- a/drivers/gpu/drm/drm_buddy.c >> +++ b/drivers/gpu/drm/drm_buddy.c >> @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >> if (range_overflows(start, size, mm->size)) >> return -EINVAL; >> >> + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) >> + return -EINVAL; >> + > > I'm not that happy with the handling here. > > See a minimum page size larger than the requested size is perfectly > valid, it just means that the remaining pages needs to be trimmed. > > In other words when the request is to allocate 1 page with an alignment > of 256 we just need to give the remaining 255 pages back to the allocator. In one of the previous mail Matthew explained that i915 expects to return -EINVAL error code if size is not aligned to min_page_size. can we just modify in amdgpu code where we round_up the size to the min_page_size value and keep this error handling in drm_buddy.c? > > Regards, > Christian. > >> /* Actual range allocation */ >> if (start + size == end) >> return __drm_buddy_alloc_range(mm, start, size, blocks); >> >> base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b >
On 29/03/22 4:54 pm, Matthew Auld wrote: > On Tue, 29 Mar 2022 at 12:17, Arunpravin Paneer Selvam > <arunpravin.paneerselvam@amd.com> wrote: >> >> >> >> On 23/03/22 1:15 pm, Christian König wrote: >>> Am 23.03.22 um 08:34 schrieb Arunpravin Paneer Selvam: >>>> Add a simple check to reject any size not aligned to the >>>> min_page_size. >>>> >>>> handle instances 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 problem, we added a simple check to >>>> return -EINVAL if size is not aligned to the min_page_size. >>>> >>>> v2: Added more details to the commit description >>>> >>>> Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> >>>> Suggested-by: Matthew Auld <matthew.auld@intel.com> >>>> --- >>>> drivers/gpu/drm/drm_buddy.c | 3 +++ >>>> 1 file changed, 3 insertions(+) >>>> >>>> diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c >>>> index 72f52f293249..b503c88786b0 100644 >>>> --- a/drivers/gpu/drm/drm_buddy.c >>>> +++ b/drivers/gpu/drm/drm_buddy.c >>>> @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, >>>> if (range_overflows(start, size, mm->size)) >>>> return -EINVAL; >>>> >>>> + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) >>>> + return -EINVAL; >>>> + >>> >>> I'm not that happy with the handling here. >>> >>> See a minimum page size larger than the requested size is perfectly >>> valid, it just means that the remaining pages needs to be trimmed. >>> >>> In other words when the request is to allocate 1 page with an alignment >>> of 256 we just need to give the remaining 255 pages back to the allocator. >> >> In one of the previous mail Matthew explained that i915 expects to >> return -EINVAL error code if size is not aligned to min_page_size. > > We could also move the WARN_ON() into i915 as a separate patch, and > just change the default buddy behaviour to transparently handle the > rounding + trim, if you prefer. I don't have a strong opinion. ok, I sent a patch handling rounding + trim in drm_buddy_alloc_blocks() function. Please review the patch. > >> >> can we just modify in amdgpu code where we round_up the size to the >> min_page_size value and keep this error handling in drm_buddy.c? >>> >>> Regards, >>> Christian. >>> >>>> /* Actual range allocation */ >>>> if (start + size == end) >>>> return __drm_buddy_alloc_range(mm, start, size, blocks); >>>> >>>> base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b >>>
diff --git a/drivers/gpu/drm/drm_buddy.c b/drivers/gpu/drm/drm_buddy.c index 72f52f293249..b503c88786b0 100644 --- a/drivers/gpu/drm/drm_buddy.c +++ b/drivers/gpu/drm/drm_buddy.c @@ -661,6 +661,9 @@ int drm_buddy_alloc_blocks(struct drm_buddy *mm, if (range_overflows(start, size, mm->size)) return -EINVAL; + if (WARN_ON(!IS_ALIGNED(size, min_page_size))) + return -EINVAL; + /* Actual range allocation */ if (start + size == end) return __drm_buddy_alloc_range(mm, start, size, blocks);
Add a simple check to reject any size not aligned to the min_page_size. handle instances 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 problem, we added a simple check to return -EINVAL if size is not aligned to the min_page_size. v2: Added more details to the commit description Signed-off-by: Arunpravin Paneer Selvam <Arunpravin.PaneerSelvam@amd.com> Suggested-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/drm_buddy.c | 3 +++ 1 file changed, 3 insertions(+) base-commit: 056d47eaf6ea753fa2e21da31f9cbd8b721bbb7b