New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gl_rasterizer: Use buffer_storage for uniform data. #3711

Merged
merged 1 commit into from May 5, 2018

Conversation

Projects
None yet
3 participants
@degasus
Contributor

degasus commented May 2, 2018

This replaces the glBufferData logic with the shared stream buffer code.
The new code doesn't need a temporary staging buffer any more, so the
performance should imrpove quite a bit.


This change is Reviewable

@degasus

This comment has been minimized.

Contributor

degasus commented May 2, 2018

OGLFramebuffer framebuffer;
GLint uniform_buffer_alignment;
size_t uniform_size_aligned_ps;

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

I understand that "PS" stands for "pixel shader" which is a more understandable alias for fragment shader, but we have already used "FragmentShader/FS" as the name convention in the code, so you might want to keep the consistency.

This comment has been minimized.

@degasus

degasus May 5, 2018

Contributor

done

@@ -240,9 +243,12 @@ class RasterizerOpenGL : public VideoCore::RasterizerInterface {
std::array<SamplerInfo, 3> texture_samplers;
OGLVertexArray vertex_array;
static constexpr size_t VERTEX_BUFFER_SIZE = 32 * 1024 * 1024;
static constexpr size_t UNIFORM_BUFFER_SIZE = 2 * 1024 * 1024;

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

Could you list the game you tested to get this optimal size? Or did you get it by other means?

This comment has been minimized.

@degasus

degasus May 5, 2018

Contributor

This was just my first guess. I tested link between worlds, and it was a bit too big. The buffer was reallocated a few times per second. But this value may need to be touched with 3499, as we upload lots of new uniforms there.

@@ -70,6 +67,10 @@ RasterizerOpenGL::RasterizerOpenGL()
uniform_block_data.proctex_lut_dirty = true;
uniform_block_data.proctex_diff_lut_dirty = true;
glGetIntegerv(GL_UNIFORM_BUFFER_OFFSET_ALIGNMENT, &uniform_buffer_alignment);
uniform_size_aligned_ps =

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

Is this aligned size needed? The stream buffer would map the buffer according to uniform_buffer_alignment anyway, and you are not modifying the padding data after the real struct data. In UploadUniforms you can just do uniform_buffer.Map(sizeof(UniformData) uniform_buffer_alignment);

This comment has been minimized.

@degasus

degasus May 5, 2018

Contributor

Indeed, it is not needed here. Shall I move it to the 3499 rebase? degasus@d5d6a50#diff-d1b91c65951bd9733c0e7d924161e57dR1942

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

I see. You can keep it here in that case.

return;
size_t uniform_size = uniform_size_aligned_ps;
u8* vbo;

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

"vbo"? Please give a proper name instead of copy-pasting.

This comment has been minimized.

@degasus

degasus May 5, 2018

Contributor

sorry

u8* vbo;
GLintptr offset;
std::tie(vbo, offset, std::ignore) = uniform_buffer.Map(uniform_size, uniform_buffer_alignment);
memcpy(vbo, &uniform_block_data.data, sizeof(UniformData));

This comment has been minimized.

@wwylele

wwylele May 5, 2018

Member

std::memcpy

This comment has been minimized.

@degasus

degasus May 5, 2018

Contributor

done

gl_rasterizer: Use buffer_storage for uniform data.
This replaces the glBufferData logic with the shared stream buffer code.
The new code doesn't need a temporary staging buffer any more, so the
performance should imrpove quite a bit.

@degasus degasus force-pushed the degasus:vertex_streaming branch from 4c28aa3 to 5960282 May 5, 2018

@wwylele

wwylele approved these changes May 5, 2018

@wwylele wwylele merged commit 9c55a1f into citra-emu:master May 5, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@degasus degasus referenced this pull request May 6, 2018

Merged

Vertex streaming #3504

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment