Rework of gl memory / buffer handling #2822

kyren · 2019-06-12T21:42:33Z

Allocates native buffers to back memory as soon as allocate_memory is called,
hal buffers become logical sub-ranges of native buffers.

This is still WIP, there is a lot of room for improvement here. Separate INDEX
memory could be reserved only for webgl, there could be separate upload /
download memory, and the end of buffer sub-ranges could be checked with asserts
in debug mode.

Fixes #2812
PR checklist:

make succeeds (on *nix)
make reftests succeeds
tested examples with the following backends: gl
rustfmt run on changed code

kyren · 2019-06-12T21:44:32Z

This is still highly WIP, I just went ahead and created the PR to get feedback.

Notably, this doesn't fix wgpu-rs with rendy yet, rendy errors (when trying the wgpu-rs cube example) with NoSuitableMemory(0, CPU_VISIBLE). I need to look into exactly why that is, and it might either need further gfx changes or potentially rendy changes as well. (Edit: actually, never mind, it might be wgpu asking for an INDEX + non-INDEX buffer, which would make sense!) (Edit 2: never mind, I just don't know how to properly use bitflags, working on it...)

kyren · 2019-06-12T23:45:43Z

Okay, so the cube example in wgpu-rs is still broken, and it has something to do with the fact that you can't call glTexSubImage2D with a buffer bound to GL_PIXEL_UNPACK_BUFFER that is also currently memory mapped.

In the cube example, this is coming from uploading texture data to a new buffer and calling copy_buffer_to_texture on that buffer. I believe this ends up being allocated on a rendy-memory "dynamic" heap, which actually does not unmap its buffers when DynamicBlock::unmap is called. HOWEVER, even if you patch rendy's DynamicBlock::unmap to call device.unmap_memory, the cube example will still not work.

I don't know how opengl mapping of a buffer range really works, I guess you can't unmap a buffer range, you can only unmap all of the mapped buffer ranges at once? Does opengl support mapping multiple different ranges of a single buffer simultaneously?

Curiously, if you patch rendy memory to call gfx unmap_memory on DynamicBlock::unmap as described above, the cube example errors with the exact same error! AND, even more curiously, if you unmap the buffer AGAIN as a hack in gl/src/queue.rs handling of Command::CopyBufferToTexture, the call to glTexSubImage2D will not error. I don't think I'm missing a call to glMapBufferRange anywhere, so my current theory is that glUnmapBuffer unmaps the most recent call to glMapBufferRange, but not all of them.

In any case, I think the best way forward might be not to use the rendy dynamic allocator with the opengl backend and instead substitute the dedicated one? I'm not really super sure!

kvark

This looks very well written! I got a few suggestions here, but most importantly we need the persistent mapping for the cpu-visible buffers to become useful.

kvark · 2019-06-13T02:18:32Z

src/backend/gl/src/command.rs

-    // Active index type, set by the current index buffer.
-    index_type: Option<hal::IndexType>,
+    // Active index type and buffer range, set by the current index buffer.
+    index_type_range: Option<(hal::IndexType, Range<u64>)>,


we could use hal::buffer::Offset typedef here

good catch, done!

kvark · 2019-06-13T02:22:10Z

src/backend/gl/src/command.rs

@@ -1088,7 +1087,9 @@ impl command::RawCommandBuffer<Backend> for RawCommandBuffer {
    }

    unsafe fn dispatch_indirect(&mut self, buffer: &n::Buffer, offset: buffer::Offset) {
-        self.push_cmd(Command::DispatchIndirect(buffer.raw, offset));
+        let (raw_buffer, range) = buffer.borrow().as_bound();
+        assert_eq!(range.start, 0, "buffer offset unsupported in indirect draw");


we actually support indirect buffer offset, it's a part of DispatchIndirect variant.
What we do not support is draw_indexed_indirect when index buffer offset is non-zero

Right, okay that makes sense, I was confused between DispatchIndirect and draw_indirect and draw_indexed_indirect. OpenGL doesn't support draw_indexed_indirect at all right now so that's not an issue.

kvark · 2019-06-13T02:29:32Z

src/backend/gl/src/device.rs

-            gl.buffer_storage(target, buffer.requirements.size as _, None, flags);
-            gl.bind_buffer(target, None);
-        } else {
-            let flags = if cpu_can_read && cpu_can_write {


Removing these flags appears to be a regression. It's fine to catch up to this after the fact, since your change is much more important, but need to keep it on the radar.

The way the gl backend was before, allocated memory had n::Memory properties hard-coded to memory::Properties::CPU_VISIBLE | memory::Properties::CPU_CACHED, which means that it was always really allocated with glow::MAP_READ_BIT | glow::MAP_WRITE_BIT or glow::DYNAMIC_DRAW, the rest of it seemed to be dead code?

Now, it's still hard-coded, but since the allocation takes place in the same place that the n::Memory flags are set, there's nothing to check to to see what flags to use when allocating.

I agree it would probably be better if you could control the buffer flags more than this, but it should have the same behavior as it did before.

kvark · 2019-06-13T02:32:55Z

src/backend/gl/src/lib.rs

-        };
+            },
+            // Memory type for uses other than images and INDEX
+            hal::MemoryType {


would be good to also expose a device-local memory type for buffers

That sounded right to me too, but I didn't know enough to know how to do it correctly. If we have a device-local buffer type, what should the gl backend do when copying from a cpu-visible buffer (backed by a real native buffer) to a device-local buffer? I imagine the correct answer is nothing, but what happens then when the user tries to copy from a fake device-local buffer to e.g. an image buffer? This might be another case where if I knew more about how HAL works it would make more sense to me, but as of right now I need it explained a bit slower :D

Well, now that I think about it I suppose just having an actual, real buffer without any memory mapping would be the right thing to do, even if it needlessly duplicates memory on platforms with emulated memory mapping?

Okay I think I pushed something that works, it at least fixes the wgpu-rs shadow example

Copying from one buffer to another would be going through glCopyBufferSubData.

what happens then when the user tries to copy from a fake device-local buffer to e.g. an image buffer

Device-local buffer is just a GL buffer (or a sub-range of it, if we have a buffer associated with Memory) without MAP_XXX flags. Copying from it, to it, using as index/vertex, all those things are done the same way as usual.

Yeah, that makes perfect sense, and that's where I ended up.

kvark · 2019-06-13T02:33:35Z

src/backend/gl/src/lib.rs

+                properties: Properties::CPU_VISIBLE
+                    | Properties::COHERENT
+                    | Properties::CPU_CACHED,
+                heap_index: 2,


this doesn't need to be a separate heap index. We should have 0 for device local types and 1 for cpu visible types.

Yeah that makes perfect sense, done.

kvark · 2019-06-13T02:36:22Z

src/backend/gl/src/queue.rs

@@ -612,7 +612,7 @@ impl CommandQueue {
                    r.image_extent.height as _,
                    glow::RGBA,
                    glow::UNSIGNED_BYTE,
-                    0,
+                    r.buffer_offset as i32,


we should use the offset in CopyTextureToBuffer as well

Whoops! I did in fact forget about CopyTextureToBuffer, and it's now waiting on grovesNL/glow#13

Done, but it requires depending on a git rev of glow again.

kyren · 2019-06-13T22:05:06Z

I think getting this to work on webgl will require mapping without COHERENT and relying on explicit flushing. Currently working on it.

13: Add support for pixel buffer offset in get_tex_image r=grovesNL a=kyren Necessary for this gfx PR: gfx-rs/gfx#2822 Co-authored-by: kyren <kerriganw@gmail.com>

kyren · 2019-06-13T23:14:35Z

I'm very unsure about whether the non-coherent gl buffer mapping is correct...

kyren · 2019-06-14T03:39:10Z

Okay, I have tested this PR via wgpu-rs and can confirm the cube example works again!

I have also tested this PR with several other required patches and can confirm this allows wgpu-rs to work on webgl again as it did before! Only simple translucent quad is tested, and this requires several additional in-flight patches to wgpu-rs, wgpu-native, and gfx-* (start here if you're reading along and are interested).

kyren · 2019-06-14T04:54:56Z

Now the wgpu-rs shadow example works again (to the same extent it used to work, with some graphical errors).

kvark · 2019-06-14T18:53:10Z

src/backend/gl/Cargo.toml

@@ -23,7 +23,7 @@ bitflags = "1"
 log = { version = "0.4" }
 gfx-hal = { path = "../../hal", version = "0.2" }
 smallvec = "0.6"
-glow = { version = "0.2.0" }
+glow = { git = "https://github.com/grovesNL/glow", rev = "054aabc1bfbad472e4a08bc55912552d8211daee" }


@grovesNL what's the plan here? Are we waiting for more fixes to glow before a patch can be released?

Nope, 0.2.0 was published a week ago but there have been a few PRs since then. We can publish whenever this branch is ready to replace the git dependency

kvark · 2019-06-14T18:55:38Z

src/backend/gl/src/device.rs

@@ -515,7 +514,6 @@ impl d::Device<B> for Device {

            Ok(n::Memory {
                properties: memory::Properties::CPU_VISIBLE
-                    | memory::Properties::COHERENT


I'm sorry to bring it up, but it looks like we should expose both coherent and non-coherent memory types for CPU visible buffers (where persistent mapping is available), given that GL has the corresponding MAP_FLUSH_EXPLICIT_BIT bit that we can control.

It is totally fine to delay for later. My only concern would be that some of gfx-hal users may expect at least one coherent memory type exposed, since this is a guarantee of Vulkan.

I can add that as well.

done (in personal branch), waiting on glow. (edit: merged into this PR now)

kvark · 2019-06-14T18:59:47Z

src/backend/gl/src/lib.rs

-        };
+            },
+            // Memory type for uses other than images and INDEX
+            hal::MemoryType {


Copying from one buffer to another would be going through glCopyBufferSubData.

what happens then when the user tries to copy from a fake device-local buffer to e.g. an image buffer

Device-local buffer is just a GL buffer (or a sub-range of it, if we have a buffer associated with Memory) without MAP_XXX flags. Copying from it, to it, using as index/vertex, all those things are done the same way as usual.

kvark · 2019-06-14T19:01:52Z

src/backend/gl/src/device.rs

+            if self.share.private_caps.emulate_map {
+                let ptr = mem.emulate_map_allocation.get().unwrap();
+                let slice = slice::from_raw_parts_mut(ptr.offset(offset as isize), size as usize);;
+                gl.buffer_data_u8_slice(mem.target, slice, glow::DYNAMIC_DRAW);


should probably use buffer_sub_data instead of rewriting the whole buffer

Wow, that's an awful thing to miss, I'm really sorry!

Turns out, there is no buffer_sub_data in glow, I will add that.

done in personal branch, waiting on glow (edit: merged into this PR now)

kvark · 2019-06-14T19:04:41Z

src/backend/gl/src/device.rs


        if let Err(err) = self.share.check() {
            panic!("Error unmapping memory: {:?} for memory {:?}", err, memory);
        }
    }

-    unsafe fn flush_mapped_memory_ranges<'a, I, R>(&self, _: I) -> Result<(), d::OutOfMemory>
+    unsafe fn flush_mapped_memory_ranges<'a, I, R>(&self, ranges: I) -> Result<(), d::OutOfMemory>


we are also missing invalidate_mapped_memory_ranges implementation, which is required to be sensible for non-coherent memory

Added glInvalidateBufferSubData to glow and used that (in personal branch), when using emulated memory maps I'm currently doing... nothing? I guess that's the right thing to do? (edit: merged now)

Well, flush() updates the data from mapping to GPU, while invalidate() should update the data from GPU to the mapping. It sounds like this still needs to be done with the emulated maps?

Okay yeah that makes sense. This will also require a new glow method, I'll merge into this PR once that's merged into glow. (edit: done now)

kyren · 2019-06-14T23:33:25Z

I've added an implementation of invalidate_mapped_memory_ranges, added both incoherent and coherent memory types, and changed to use glBufferSubData for flushing buffer in my personal branch of gfx. Waiting on glow to merge my PRs and I will update this PR with those changes.

kyren · 2019-06-14T23:46:42Z

I have probably added a lot of portability issues with this PR, I'm open to suggestions about what needs to be fixed right now, but I don't have a great way of testing everything I might do.

I know that with the changes I'm going to add as soon as the glow PRs are merged, the gl backend will be using glInvalidateBufferSubData which I guess is core only since 4.3. A possible improvement here is to either ONLY provide incoherent memory if that function is available, or potentially provide incoherent memory for platforms with emulated buffer mapping and coherent memory only for platforms with native buffer mapping.

I'm open to suggestions here. I don't mind doing more work on this, but I do feel like the PR has gotten a bit more complex than I was really intending from the outset (and I feel a smidge unqualified to be doing some of the things I'm doing).

kvark

Thank you for driving this forward! I think we've narrowed down most of rough spots, there is just a few left (noted below)

kvark · 2019-06-17T02:37:08Z

src/backend/gl/src/device.rs

-            size,
-            emulate_map_allocation: RefCell::new(std::ptr::null_mut()),
-        })
+        if (1 << mem_type.0) & IMAGE_MEM_TYPE_MASK != 0 {


should this just be a comparison 1 << mem_type.0 == IMAGE_MEM_TYPE_MASK?

For this case, yes, but that assumes that there is only one type of image memory. For the other roles there are multiple types of memory that could work, so the intention was to see whether the given type was in the bit mask at all.

kvark · 2019-06-17T02:37:57Z

src/backend/gl/src/device.rs

+                map_flags: 0,
+            })
+        } else {
+            assert!(self.share.private_caps.buffer_role_change);


wait, so this code can't allocate buffer memory at all if buffer_role_change is false?

Yeah, I know it's bad, I figured you'd ask about this. We could either offer a unique type of memory for each role and work differently whether or not buffer_role_change was allowed, OR we could force mmap emulation if buffer_role_change is not there. I'm fine with doing either one of those now, this change was just getting to be larger than I expected it to be so I wasn't sure what the best way forward right now was.

OR we could force mmap emulation if buffer_role_change is not there.

Is this really an option though? If the user wants a buffer to support both being a vertex buffer and a uniform buffer, no amount of mmap emulation would give you that.

So it looks to me that we just need to expose this "no role change" policy in the exposed memory types and live with that, for now. Later, if we find that applications are too restricted with this, we could think of some workarounds involving copying data around to temporary buffers, but I don't want us to go there just now.

So, just to clarify, you're saying that the gl backend should have memory types based on no role changes at all, or that this should be dynamic depending on buffer_role_change? Doing the first one is... relatively easy, but the second one is a more involved change. It's easier for me to do the first one and has a higher probability of me getting it right, but if you think the that the gl backend should definitely support multi purpose buffers if available, I can do that. (If it's dynamic, I might have to move a lot of the buffer type logic around, because most of those mask constants will no longer be constant. As a plus, this would also enable mixing index buffers when not on webgl).

Edit: And yeah, you're right that mmap emulation doesn't help. Native gl buffers are still be attached to memory rather than hal buffers, and hal buffers are attached to sub-ranges of native buffers, so no matter what you will hit the buffer role change limitation.

However, I remember now why I thought this: it DOES solve one of the buffer role change problems I ran into on webgl prior to this PR (but not the new ones related to buffer sub-ranges). Currently, gfx-hal binds native buffers to the PIXEL_PACK_BUFFER target during Device::map_memory, which runs afoul of the index role change limitations on webgl. Does this mean that the gl platform doesn't work currently on !buffer_role_change platforms, or is the PIXEL_PACK_BUFFER target special somehow on non-webgl and allowed?

should be dynamic depending on buffer_role_change

We definitely need the exposed memory types being different, based on the buffer_role_change capability.

is the PIXEL_PACK_BUFFER target special somehow on non-webgl and allowed

We only have to use this binding when doing texture-buffer copies. Technically, this is subset of TRANSFER_DST capability (while PIXEL_UNPACK_BUFFER is a subset with TRANSFER_SRC). So with this in mind there is pretty much 1:1 correspondence between a buffer binding point ("role") and buffer::Usage.

When the implementation just needs to bind a buffer, regardless of the particular bind point, it should pick the bind point that the buffer supports. For example, if the user created a buffer with only VERTEX usage, we'll use the GL_ARRAY_BUFFER binding point for it, etc.

kvark · 2019-06-17T02:38:18Z

src/backend/gl/src/device.rs

+        } else {
+            assert!(self.share.private_caps.buffer_role_change);
+
+            let target = if (1 << mem_type.0) & INDEX_MEM_TYPE_MASK != 0 {


similarly, could we compare directly instead?

There's more than one type of index memory, so the intention here is to check whether the given memory type is any of the index memory types. Is there a clearer way to write this?

kvark · 2019-06-17T02:38:48Z

src/backend/gl/src/device.rs

+                glow::ARRAY_BUFFER
+            };
+
+            let is_device_local = (1 << mem_type.0) & DEVICE_LOCAL_MASK != 0;


nit: since this flag is needed in both branches, let's move this assignment up to the beginning of the function?

Done, and I went ahead and gave all of the properties clearer names at the top of the function because I felt that was easier to follow.

kvark · 2019-06-17T02:40:19Z

src/backend/gl/src/device.rs

+
+            Ok(n::Memory {
+                properties: memory::Properties::CPU_VISIBLE
+                    | memory::Properties::CPU_CACHED,


in the future, we'd want to expose "read" memory type independent of the "write", but it doesn't need to happen here

Yeah, agreed!

kvark · 2019-06-17T02:41:34Z

src/backend/gl/src/device.rs

+            // Buffers are only allowed to be INDEX usage XOR another type of usage, they are not
+            // allowed to have both INDEX and non-INDEX usage.
+            if !(buffer::Usage::INDEX | buffer::Usage::TRANSFER_SRC | buffer::Usage::TRANSFER_DST).contains(usage) {
+                (0, 1)


let's error! log here for the users to find out what happened earlier :)

kvark · 2019-06-17T02:45:09Z

src/backend/gl/src/device.rs

            // TODO: Access
-            gl.buffer_data_u8_slice(target, mapped, glow::DYNAMIC_DRAW);
-            let _ = *Box::from_raw(raw);
+            gl.buffer_sub_data_u8_slice(memory.target, 0, &allocation);


we shouldn't be doing this on unmap. In Vulkan and gfx-hal, mapping as an operation doesn't actually involve any synchronization. I.e. the only thing that synchronizes is:

submit() for coherent memory (implicitly synchronizes all active coherent mappings)

explicit flush() and invalidate() for non-coherent memory

That makes sense, thanks for being patient with me and helping me learn all of the different memory semantics for modern gfx apis!

kyren · 2019-06-17T22:50:50Z

Thank you for driving this forward! I think we've narrowed down most of rough spots, here is just a few left (noted below)

You're very very welcome, thanks for being patient with me and teaching me a lot about modern gfx APIs in the process 😛

kvark

Alright, I suppose there is only getting glow published and maybe reordering the memory types that's left to do here? Or do you think we should land this as is now and follow-up?

kvark · 2019-06-19T06:30:10Z

src/backend/gl/src/lib.rs

+        // omitting the GL_MAP_FLUSH_EXPLICIT_BIT flag.
+        if !self.0.private_caps.emulate_map {
+            memory_types.push(
+                // Coherent CPU_VISIBLE memory for INDEX buffers


let's move these to the beginning of memory_types. In Vulkan there is an interesting requirement that a memory type with more flags should be exposed first (i.e one with flags A | B should come before A).

That makes the memory type masks dynamic based on capabilities, darn.

Allocates native buffers to back memory as soon as allocate_memory is called, hal buffers become logical sub-ranges of native buffers. This is still WIP, there is a lot of room for improvement here. Separate INDEX memory could be reserved only for webgl, there could be separate upload / download memory, and the end of buffer sub-ranges could be checked with asserts in debug mode.

Currently requires master branch of `glow` from git

I believe this is necessary for emulated mapping to work properly, as emulated mapping cannot be coherent. Currently unimplemented for emulated mapping, will implement soon.

…e gl backend

… memory maps

kyren · 2019-06-19T08:46:28Z

Alright, I suppose there is only getting glow published and maybe reordering the memory types that's left to do here? Or do you think we should land this as is now and follow-up?

I guess it would be better to fix the memory types thing and the brokenness of !buffer_role_change before merging...

* Handles the case where `buffer_role_change` capability is false by generating memory types for every buffer role. * Does not add separate INDEX memory on non-webgl platforms.

kyren · 2019-06-19T23:53:46Z

Okay, I have a new system for handling memory types that is dynamic based on capabilities rather than the old simplistic system. I've also fixed what I think are several bugs that I caught in review, so there's kind of a lot of new changes.

This should fix handling the !buffer_role_change case, improve the backend on non-webgl, and order the memory types according to vulkan requirements.

I haven't tested this with the webgl backend just yet because I need to rebase onto my personal branch to try it out and stash what I'm currently working on in the project that uses it. I'll report back once I've done that.

Edit: Everything seems to work fine under webgl as well!

Move type mask logic into gfx_backend_gl::Share so that it can be shared

kvark

A few last questions/concerns :)

kvark · 2019-06-20T05:45:16Z

src/backend/gl/src/device.rs

+                if is_readable_memory {
+                    map_flags |= glow::MAP_READ_BIT;
+                }
+                if !is_coherent_memory {


the condition appears wrong

Oh! It definitely is wrong, it used to control the explicit flush bit before I figured out that it should control the coherent flag instead, and I forgot to reverse it.

And it was hiding another bug, which is now fixed 😥

kvark · 2019-06-20T05:46:10Z

src/backend/gl/src/device.rs

+                    properties: memory::Properties::DEVICE_LOCAL,
+                    buffer: None,
+                    size,
+                    target: 0,


let's move this field behind the buffer option?

Yeah, I almost did that before actually...

kvark · 2019-06-20T05:47:03Z

src/backend/gl/src/device.rs

-                // Alignment of 4 covers indexes of type u16 and u32
-                (INDEX_MEM_TYPE_MASK, 4)
-            }
+        let alignment = if usage.contains(buffer::Usage::INDEX) {


why don't we just return 4 unconditionally here? It's a reasonable alignment to adhere to.

Sure thing, that seems reasonable.

kvark · 2019-06-20T05:49:07Z

src/backend/gl/src/lib.rs

+                properties: memory::Properties::CPU_VISIBLE | memory::Properties::CPU_CACHED | memory::Properties::COHERENT,
+                heap_index: CPU_VISIBLE_HEAP,
+            });
+            buffer_role_memory_types.push(hal::MemoryType {


are we missing the CPU-visible non-cached one?

You know what, I actually did all this still with the assumption that there would only be R/W memory types this patch and not write-only ones (and leave that for a later patch). I guess though, with the way everything else is set up, the ONLY THING left to do is just add an entry here and it will work? I guess that's really all that's left to do so it's silly not to do it!

kvark · 2019-06-20T05:49:32Z

src/backend/gl/src/lib.rs

+                    memory_types.push((
+                        buffer_role_memory_type,
+                        MemoryRole::Buffer {
+                            allowed_usage: buffer::Usage::TRANSFER_SRC


could we use buffer::Usage::all() instead?

Oh sure thing I just forgot it existed :P

* Fix inverted condition around coherent memory, and fix the bug that it was hiding (also need MAP_COHERENT_BIT on storage flags!) * Move buffer target with raw buffer inside the Option in native::Memory * Hard-code buffer alignment of 4 * Actually go ahead and add cpu-visible non-cached coherent memory type, since it will now work by just adding it to the list! * Simplify using buffer::Usage flags a bit

kyren · 2019-06-20T06:27:44Z

Okay all of your new comments are addressed. Based on the discussion in gitter (I think I have this right?) I added a new memory type (set) that is cpu-visible, non-cached, and coherent, so there are now also non-cached memory types available (non-cached implies coherent, right?).

kvark · 2019-06-20T15:06:09Z

Awesome! Bors r+

…

On Jun 19, 2019, at 23:27, kyren ***@***.***> wrote: Okay all of your new comments are addressed. Based on the discussion in gitter (I think I have this right?) I added a new memory type that is cpu-visible, non-cached, and coherent, so there is now also a non-cached gl memory type available (non-cached implies coherent, right?). — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

2822: Rework of gl memory / buffer handling r=kvark a=kyren Allocates native buffers to back memory as soon as allocate_memory is called, hal buffers become logical sub-ranges of native buffers. This is still WIP, there is a lot of room for improvement here. Separate INDEX memory could be reserved only for webgl, there could be separate upload / download memory, and the end of buffer sub-ranges could be checked with asserts in debug mode. Fixes #2812 PR checklist: - [x] `make` succeeds (on *nix) - [x] `make reftests` succeeds - [x] tested examples with the following backends: gl - [ ] `rustfmt` run on changed code Co-authored-by: kyren <kerriganw@gmail.com>

bors · 2019-06-20T15:32:13Z

Build succeeded

continuous-integration/travis-ci/push

kyren mentioned this pull request Jun 12, 2019

GL panics with "No buffer has been bound yet, can't map memory!" #2812

Closed

kyren changed the title ~~[DRAFT] WIP rework of gl memory / buffer handling~~ [DRAFT] rework of gl memory / buffer handling Jun 13, 2019

kvark approved these changes Jun 13, 2019

View reviewed changes

kyren mentioned this pull request Jun 13, 2019

Add support for pixel buffer offset in get_tex_image grovesNL/glow#13

Merged

bors bot added a commit to grovesNL/glow that referenced this pull request Jun 13, 2019

Merge #13

33e3d63

13: Add support for pixel buffer offset in get_tex_image r=grovesNL a=kyren Necessary for this gfx PR: gfx-rs/gfx#2822 Co-authored-by: kyren <kerriganw@gmail.com>

kvark reviewed Jun 14, 2019

View reviewed changes

kvark requested changes Jun 14, 2019

View reviewed changes

kyren changed the title ~~[DRAFT] rework of gl memory / buffer handling~~ Rework of gl memory / buffer handling Jun 15, 2019

kvark reviewed Jun 17, 2019

View reviewed changes

kvark approved these changes Jun 19, 2019

View reviewed changes

kyren added 11 commits June 19, 2019 04:26

Use the correct buffer::Offset type alias instead of u64

b685c02

Only need two (fake) heaps, one for DEVICE_LOCAL and one for CPU_VISIBLE

44d75aa

DispatchIndirect with a buffer sub-range starting at nonzero is fine

95733c5

Use persistent and (for now!) coherent opengl mapping

ed90200

Support buffer sub-ranges in CopyTextureToBuffer

3b9d827

Currently requires master branch of `glow` from git

Do not map gl buffers with COHERENT bit

e81dec3

I believe this is necessary for emulated mapping to work properly, as emulated mapping cannot be coherent. Currently unimplemented for emulated mapping, will implement soon.

Correctly implement emulated memory mapping, flushing

e888241

Support device-local memory types for buffers

9a27069

Use glBufferSubData to upload sub-range of mapped memory

ee00716

Implement invalidate_mapped_memory_ranges for gl backend

2be9162

kyren added 5 commits June 19, 2019 04:26

Add both coherent and non-coherent memory types to gl backend

59a1385

Clarify the behavior for different memory types in allocate_memory

1d60d3c

Log an error when creating a buffer that has no suitable memory in th…

e6fe38c

…e gl backend

emulated non-coherent memory should not be synchronized on unmap

b9d212b

Update memory from the gpu to the mapping in invalidate with emulated…

e8ff6b1

… memory maps

New system for gl backend memory types, computed based on capabilities.

7c1678c

* Handles the case where `buffer_role_change` capability is false by generating memory types for every buffer role. * Does not add separate INDEX memory on non-webgl platforms.

Remove stray hard-coded image type masks of 0x7

7c84bfb

Move type mask logic into gfx_backend_gl::Share so that it can be shared

kvark reviewed Jun 20, 2019

View reviewed changes

bors bot merged commit 01807ec into gfx-rs:master Jun 20, 2019

kvark mentioned this pull request Jul 24, 2019

cube example with gl backend panics gfx-rs/wgpu#259

Closed

Rework of gl memory / buffer handling #2822

Rework of gl memory / buffer handling #2822

Conversation

kyren commented Jun 12, 2019 • edited Loading

kyren commented Jun 12, 2019 • edited Loading

kyren commented Jun 12, 2019

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren commented Jun 13, 2019

kyren commented Jun 13, 2019

kyren commented Jun 14, 2019 • edited Loading

kyren commented Jun 14, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 14, 2019 • edited Loading

Choose a reason for hiding this comment

kyren Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

kyren commented Jun 14, 2019

kyren commented Jun 14, 2019

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 17, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren commented Jun 17, 2019

kvark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyren Jun 19, 2019 • edited Loading

Choose a reason for hiding this comment

kyren commented Jun 19, 2019

kyren commented Jun 19, 2019 • edited Loading

kyren commented Jun 12, 2019 •

edited

Loading

kyren commented Jun 12, 2019 •

edited

Loading

kyren Jun 13, 2019 •

edited

Loading

kyren Jun 14, 2019 •

edited

Loading

kyren commented Jun 14, 2019 •

edited

Loading

kyren Jun 15, 2019 •

edited

Loading

kyren Jun 14, 2019 •

edited

Loading

kyren Jun 15, 2019 •

edited

Loading

kyren Jun 15, 2019 •

edited

Loading

kyren Jun 17, 2019 •

edited

Loading

kyren Jun 17, 2019 •

edited

Loading

kyren Jun 19, 2019 •

edited

Loading

kyren Jun 19, 2019 •

edited

Loading

kyren commented Jun 19, 2019 •

edited

Loading

kyren commented Jun 20, 2019 •

edited

Loading