Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Quoting Sergii Romantsov (2018-07-25 10:37:29)
> Hello, Chris.
> Your variant also works.
> But i wonder about comment:
> /* If we don't have caching at this size, don't actually round the
> * allocation up.
> */
> if (bucket == NULL) {
>
> Has it any sense now? If 'no' - will delete it in next patch update.
It's actually talking about rounding up to bucket size which started off
as next power-of-two, since reduced to some fractions and even now the
rounding is debatable. The page size allocation is a property of the
uABI -- objects are allocated in pages.
Now there is no reason the bufmgr can't do sub-page allocations, it
would take a bit of work to factor out the offset_in_page() later but
doable.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Hello, Chris.
Your variant also works.
But i wonder about comment:
/* If we don't have caching at this size, don't actually round the
* allocation up.
*/
if (bucket == NULL) {
Has it any sense now? If 'no' - will delete it in next patch update.
Was trying to get commit where and why it was added and it brings me to:
commit 514db96c117adc84940bb08ebd0e8f84879bd4ad
Author: Kenneth Graunke
Date: Mon Mar 20 16:40:01 2017 -0700
i965: Import libdrm_intel.
This imports commit 19c4cfc54918d361f2535aec16650e9f0be667cd of
libdrm/intel/*.[ch], minus a few files that we're never going to use
(and would immediately delete), plus a few necessary dependencies.
We rename intel_bufmgr.h to brw_bufmgr.h to avoid #include conflicts.
We also fix UTF-8 symbol problems in intel_bufmgr_gem.c comments
because vim keeps trying to fix that every time I edit the file,
and we may as well fix it right away.
Repository of libdrm (master and tags about 2.4.7 and 2.4.8) doesn't
contain such commit. Observed only that it fixes some crash of VB (
https://www.mail-archive.com/ubuntu-bugs@lists.ubuntu.com/msg5146035.html)
On Wed, Jul 25, 2018 at 10:48 AM, Chris Wilson
wrote:
> Quoting Sergii Romantsov (2018-07-25 08:42:55)
> > Hello,
> > here is a backtrace:
> ...
>
> Please try:
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 09d45e30ecc..8274c2e0b2f 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -496,7 +496,6 @@ bo_alloc_internal(struct brw_bufmgr *bufmgr,
>uint32_t stride)
> {
> struct brw_bo *bo;
> - unsigned int page_size = getpagesize();
> int ret;
> struct bo_cache_bucket *bucket;
> bool alloc_from_cache;
> @@ -522,12 +521,12 @@ bo_alloc_internal(struct brw_bufmgr *bufmgr,
> * allocation up.
> */
> if (bucket == NULL) {
> - bo_size = size;
> - if (bo_size < page_size)
> - bo_size = page_size;
> + unsigned int page_size = getpagesize();
> + bo_size = ALIGN(size, page_size);
> } else {
>bo_size = bucket->size;
> }
> + assert(bo_size);
>
> mtx_lock(>lock);
> /* Get a buffer out of the cache if available */
>
--
Sergii Romantsov
GlobalLogic Inc.
www.globallogic.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Quoting Sergii Romantsov (2018-07-25 08:42:55)
> Hello,
> here is a backtrace:
...
Please try:
diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index 09d45e30ecc..8274c2e0b2f 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -496,7 +496,6 @@ bo_alloc_internal(struct brw_bufmgr *bufmgr,
uint32_t stride)
{
struct brw_bo *bo;
- unsigned int page_size = getpagesize();
int ret;
struct bo_cache_bucket *bucket;
bool alloc_from_cache;
@@ -522,12 +521,12 @@ bo_alloc_internal(struct brw_bufmgr *bufmgr,
* allocation up.
*/
if (bucket == NULL) {
- bo_size = size;
- if (bo_size < page_size)
- bo_size = page_size;
+ unsigned int page_size = getpagesize();
+ bo_size = ALIGN(size, page_size);
} else {
bo_size = bucket->size;
}
+ assert(bo_size);
mtx_lock(>lock);
/* Get a buffer out of the cache if available */
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Hello, here is a backtrace: Core was generated by `glretrace ./DyingLightGame.trace'. Program terminated with signal SIGABRT, Aborted. #0 0x7f2852895428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 54 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory. [Current thread is 1 (Thread 0x7f2854cb48c0 (LWP 15940))] (gdb) bt #0 0x7f2852895428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54 #1 0x7f285289702a in __GI_abort () at abort.c:89 #2 0x7f285288dbd7 in __assert_fail_base (fmt=, assertion=assertion@entry=0x7f284f4fb62f "size % 4096 == 0", file=file@entry=0x7f284f4fb58f "brw_bufmgr.c", line=line@entry=405, function=function@entry=0x7f284f4fbc30 <__PRETTY_FUNCTION__.43658> "vma_alloc") at assert.c:92 #3 0x7f285288dc82 in __GI___assert_fail (assertion=0x7f284f4fb62f "size % 4096 == 0", file=0x7f284f4fb58f "brw_bufmgr.c", line=405, function=0x7f284f4fbc30 <__PRETTY_FUNCTION__.43658> "vma_alloc") at assert.c:101 #4 0x7f284ef43777 in vma_alloc (bufmgr=0x1414840, memzone=BRW_MEMZONE_OTHER, size=255726400, alignment=4096) at brw_bufmgr.c:405 #5 0x7f284ef43ef7 in bo_alloc_internal (bufmgr=0x1414840, name=0x7f284f506370 "bufferobj", size=255726400, memzone=BRW_MEMZONE_OTHER, flags=0, tiling_mode=0, stride=0) at brw_bufmgr.c:652 #6 0x7f284ef43ff7 in brw_bo_alloc (bufmgr=0x1414840, name=0x7f284f506370 "bufferobj", size=255726400, memzone=BRW_MEMZONE_OTHER) at brw_bufmgr.c:677 #7 0x7f284ef88465 in alloc_buffer_object (brw=0x1598d80, intel_obj=0x1f9c68d0) at intel_buffer_objects.c:100 #8 0x7f284ef88759 in brw_buffer_data (ctx=0x1598d80, target=34962, size=255726400, data=0x7f282f1b1010, usage=35044, storageFlags=259, obj=0x1f9c68d0) at intel_buffer_objects.c:210 #9 0x7f284e9f33d7 in buffer_data (no_error=false, func=0x7f284f41a17c "glBufferData", usage=35044, data=0x7f282f1b1010, size=255726400, target=34962, bufObj=0x1f9c68d0, ctx=0x1598d80) at main/bufferobj.c:2065 #10 buffer_data_error (ctx=0x1598d80, bufObj=0x1f9c68d0, target=34962, size=255726400, data=0x7f282f1b1010, usage=35044, func=0x7f284f41a17c "glBufferData") at main/bufferobj.c:2091 #11 0x7f284e9f37e3 in _mesa_buffer_data (ctx=0x1598d80, bufObj=0x1f9c68d0, target=34962, size=255726400, data=0x7f282f1b1010, usage=35044, func=0x7f284f41a17c "glBufferData") at main/bufferobj.c:2107 #12 0x7f284e9f38cf in _mesa_BufferData (target=34962, size=255726400, data=0x7f282f1b1010, usage=35044) at main/bufferobj.c:2132 #13 0x004c910b in ?? () #14 0x0040bff4 in ?? () #15 0x0040c61c in ?? () #16 0x00407e95 in ?? () #17 0x7f2852880830 in __libc_start_main (main=0x407810, argc=2, argv=0x7ffc8d9394b8, init=, fini=, rtld_fini=, stack_end=0x7ffc8d9394a8) at ../csu/libc-start.c:291 #18 0x0040 in _start () On Tue, Jul 24, 2018 at 8:43 PM, Lionel Landwerlin < lionel.g.landwer...@intel.com> wrote: > On 24/07/18 18:34, Kenneth Graunke wrote: > >> On Tuesday, July 24, 2018 5:34:57 AM PDT Lionel Landwerlin wrote: >> >>> That looks correct to me (and we do the same in Anv). >>> Also a bit baffled that we haven't run into issues earlier :( >>> >>> But would be good to have Ken's Rb too. >>> >>> Thanks a lot! >>> >>> Reviewed-by: Lionel Landwerlin >>> >> Yeah, this is probably for the best...we used to just ask the kernel >> for memory and it would do this for us. Now that we're doing it >> ourselves, we ought to be defensive here. >> >> Reviewed-by: Kenneth Graunke >> >> But I agree with Chris, I'm surprised that this would actually fix >> anything...all of our allocations ought to multiples of PAGE_SIZE, >> so unless we're starting at a funny address, they ought to remain >> that way... >> >> I wonder if this isn't papering over another bug. >> > > Sergii, > > If you can reproduce this bug locally, would you mind adding > > assert(size % 4096 == 0); > > at the top of vma_alloc() and see if it hits the asserts. > Having a backtrace would be great :) > > Thanks a lot, > > - > Lionel > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev > -- Sergii Romantsov GlobalLogic Inc. www.globallogic.com ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
On 24/07/18 18:34, Kenneth Graunke wrote: On Tuesday, July 24, 2018 5:34:57 AM PDT Lionel Landwerlin wrote: That looks correct to me (and we do the same in Anv). Also a bit baffled that we haven't run into issues earlier :( But would be good to have Ken's Rb too. Thanks a lot! Reviewed-by: Lionel Landwerlin Yeah, this is probably for the best...we used to just ask the kernel for memory and it would do this for us. Now that we're doing it ourselves, we ought to be defensive here. Reviewed-by: Kenneth Graunke But I agree with Chris, I'm surprised that this would actually fix anything...all of our allocations ought to multiples of PAGE_SIZE, so unless we're starting at a funny address, they ought to remain that way... I wonder if this isn't papering over another bug. Sergii, If you can reproduce this bug locally, would you mind adding assert(size % 4096 == 0); at the top of vma_alloc() and see if it hits the asserts. Having a backtrace would be great :) Thanks a lot, - Lionel ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
On Tuesday, July 24, 2018 5:34:57 AM PDT Lionel Landwerlin wrote: > That looks correct to me (and we do the same in Anv). > Also a bit baffled that we haven't run into issues earlier :( > > But would be good to have Ken's Rb too. > > Thanks a lot! > > Reviewed-by: Lionel Landwerlin Yeah, this is probably for the best...we used to just ask the kernel for memory and it would do this for us. Now that we're doing it ourselves, we ought to be defensive here. Reviewed-by: Kenneth Graunke But I agree with Chris, I'm surprised that this would actually fix anything...all of our allocations ought to multiples of PAGE_SIZE, so unless we're starting at a funny address, they ought to remain that way... I wonder if this isn't papering over another bug. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Quoting Lionel Landwerlin (2018-07-24 13:45:18) > On 24/07/18 13:42, Chris Wilson wrote: > > Quoting Lionel Landwerlin (2018-07-24 13:34:57) > >> That looks correct to me (and we do the same in Anv). > >> Also a bit baffled that we haven't run into issues earlier :( > > All the allocations should be in multiples of page size, alignment less > > than a page size should be a no-op. Tracking down who doesn't think > > IS_ALIGNED(bo->size, PAGE_SIZE) would be interesting. > > -Chris > > > Buckets? The bucket size is still a multiple of PAGE_SIZE, and gtt_offsets within a bucket are computed as an offset from the base of the bucket (i.e. vma_alloc() is only called once for the entire bucket, and it acts as a cache to reduce the number of vma operations.) -Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
On 24/07/18 13:42, Chris Wilson wrote: Quoting Lionel Landwerlin (2018-07-24 13:34:57) That looks correct to me (and we do the same in Anv). Also a bit baffled that we haven't run into issues earlier :( All the allocations should be in multiples of page size, alignment less than a page size should be a no-op. Tracking down who doesn't think IS_ALIGNED(bo->size, PAGE_SIZE) would be interesting. -Chris Buckets? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Quoting Lionel Landwerlin (2018-07-24 13:34:57) > That looks correct to me (and we do the same in Anv). > Also a bit baffled that we haven't run into issues earlier :( All the allocations should be in multiples of page size, alignment less than a page size should be a no-op. Tracking down who doesn't think IS_ALIGNED(bo->size, PAGE_SIZE) would be interesting. -Chris ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
That looks correct to me (and we do the same in Anv).
Also a bit baffled that we haven't run into issues earlier :(
But would be good to have Ken's Rb too.
Thanks a lot!
Reviewed-by: Lionel Landwerlin
On 24/07/18 12:50, Sergii Romantsov wrote:
Kernel (for ppgtt) requires memory address to be
aligned to page size (4096).
Added such alignment for buffers marked with EXEC_OBJECT_PINNED.
-v2: added marking that also fixes initial commit 01058a552294
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106997
Fixes: a363bb2cd0e2 (i965: Allocate VMA in userspace for full-PPGTT systems.)
Fixes: 01058a552294 (i965: Add virtual memory allocator infrastructure to
brw_bufmgr.)
Signed-off-by: Sergii Romantsov
---
src/mesa/drivers/dri/i965/brw_bufmgr.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index 09d45e3..8383735 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -643,7 +643,7 @@ retry:
bo->kflags = bufmgr->initial_kflags;
if ((bo->kflags & EXEC_OBJECT_PINNED) && bo->gtt_offset == 0ull) {
- bo->gtt_offset = vma_alloc(bufmgr, memzone, bo->size, 1);
+ bo->gtt_offset = vma_alloc(bufmgr, memzone, bo->size, 4096);
if (bo->gtt_offset == 0ull)
goto err_free;
@@ -784,7 +784,7 @@ brw_bo_gem_create_from_name(struct brw_bufmgr *bufmgr,
bo->kflags = bufmgr->initial_kflags;
if (bo->kflags & EXEC_OBJECT_PINNED)
- bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 1);
+ bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 4096);
_mesa_hash_table_insert(bufmgr->handle_table, >gem_handle, bo);
_mesa_hash_table_insert(bufmgr->name_table, >global_name, bo);
@@ -1424,7 +1424,7 @@ brw_bo_gem_create_from_prime_internal(struct brw_bufmgr
*bufmgr, int prime_fd,
if (bo->kflags & EXEC_OBJECT_PINNED) {
assert(bo->size > 0);
- bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 1);
+ bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 4096);
}
if (tiling_mode < 0) {
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] intel/ppgtt: memory address alignment
Hello, Kenneth.
Looks like you are expert in memory management for i965.
Could you, please, point me if that idea of patch is correct and what could
i improve?
Seems its better somehow to bind it to I915_GTT_PAGE_SIZE... right?
On Tue, Jul 24, 2018 at 2:50 PM, Sergii Romantsov <
sergii.romant...@gmail.com> wrote:
> Kernel (for ppgtt) requires memory address to be
> aligned to page size (4096).
> Added such alignment for buffers marked with EXEC_OBJECT_PINNED.
>
> -v2: added marking that also fixes initial commit 01058a552294
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=106997
> Fixes: a363bb2cd0e2 (i965: Allocate VMA in userspace for full-PPGTT
> systems.)
> Fixes: 01058a552294 (i965: Add virtual memory allocator infrastructure to
> brw_bufmgr.)
> Signed-off-by: Sergii Romantsov
> ---
> src/mesa/drivers/dri/i965/brw_bufmgr.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> index 09d45e3..8383735 100644
> --- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
> +++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
> @@ -643,7 +643,7 @@ retry:
> bo->kflags = bufmgr->initial_kflags;
>
> if ((bo->kflags & EXEC_OBJECT_PINNED) && bo->gtt_offset == 0ull) {
> - bo->gtt_offset = vma_alloc(bufmgr, memzone, bo->size, 1);
> + bo->gtt_offset = vma_alloc(bufmgr, memzone, bo->size, 4096);
>
>if (bo->gtt_offset == 0ull)
> goto err_free;
> @@ -784,7 +784,7 @@ brw_bo_gem_create_from_name(struct brw_bufmgr *bufmgr,
> bo->kflags = bufmgr->initial_kflags;
>
> if (bo->kflags & EXEC_OBJECT_PINNED)
> - bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 1);
> + bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size,
> 4096);
>
> _mesa_hash_table_insert(bufmgr->handle_table, >gem_handle, bo);
> _mesa_hash_table_insert(bufmgr->name_table, >global_name, bo);
> @@ -1424,7 +1424,7 @@ brw_bo_gem_create_from_prime_internal(struct
> brw_bufmgr *bufmgr, int prime_fd,
>
> if (bo->kflags & EXEC_OBJECT_PINNED) {
>assert(bo->size > 0);
> - bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size, 1);
> + bo->gtt_offset = vma_alloc(bufmgr, BRW_MEMZONE_OTHER, bo->size,
> 4096);
> }
>
> if (tiling_mode < 0) {
> --
> 2.7.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
--
Sergii Romantsov
GlobalLogic Inc.
www.globallogic.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
