<div dir="ltr">Looks ok for me. Couple questions:<div> </div><div>1. Is there any reason to have "glEnable(GL_FRAMEBUFFER_SRGB);" inside loop?</div><div>2. Would it be better to restore old GL_FRAMEBUFFER_SRGB state instead of disabling it?</div></div> <div class="gmail_quote"><div dir="ltr">On Thu, Jul 19, 2018 at 5:03 AM Gert Wollny <<a href="mailto:gert.wollny@collabora.com">gert.wollny@collabora.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On an GL host set the sRGB framebuffer state explicitly to make virgl behave like on a GLES host. This does not correct the handing of sRGB completely, because the state GL_FRAMEBUFFER_SRGB is not properly transmitted to the host. As a result the piglits "blit texture linear_to_srgb * * *" flip. Tests thatset "enable" failed before and pass now, and tests that set "disable" now fail. Fixes on GL host: dEQP-GLES3.functional.fbo.blit.conversion.rgb8_to_srgb8_alpha8 dEQP-GLES3.functional.fbo.blit.default_framebuffer.srgb8_alpha8 Signed-off-by: Gert Wollny <<a href="mailto:gert.wollny@collabora.com" target="_blank">gert.wollny@collabora.com</a>> --- Since this doesn't correct the piglits, only flips some of them, I thought I might add a comment about the problems fixing this, I'm actually not sure whether handling GL_FRAMEBUFFER_SRGB can really be fixed in a reliable way considering the approach how framebuffers and blits are implemented: On one hand, the state GL_FRAMEBUFFER_SRGB is currently not transmitted to the host, but even if it would be, one still would have to guess to which framebuffer it refers, because the guest never sends information about which framebuffers are currently bound: This is handled within the mesa code, and Gallium isn't notified about changes in binding. On the other hand, if GL_FRAMEBUFFER_SRGB is disabled, but an sRGB surface is bound to the framebuffer then the framebuffer's state is updated by attaching a non-sRGB surface that is also created on the host as a texture view. Then the changed state is then send to the host via VIRGL_CCMD_SET_FRAMEBUFFER_STATE, but the command doesn't have any information about *which* framebuffer's state must be set, since Gallium doesn't know this and virglrenderer always sets the state of the current sub-context's default fb. Now, when virglrenderer enters vrend_renderer_blit_int it references the original textures with there original formats, and when GL_FRAMEBUFFER_SRGB is disabled things go wrong. The only approach for guessing the right state I could think of is, to change Gallium to send the changed GL_FRAMEBUFFER_SRGB state, and when a blit comes up, to check whether the target texture is sRGB has a texture view that has the corresponding linear format and has otherwise the same parameters. Thanks for any comments and for reviewing the patch, Gert src/vrend_blitter.c | 19 ++++++++++++++++++- src/vrend_renderer.c | 9 ++++++++- src/vrend_renderer.h | 3 ++- 3 files changed, 28 insertions(+), 3 deletions(-) diff --git a/src/vrend_blitter.c b/src/vrend_blitter.c index 68b5bab..1439c9b 100644 --- a/src/vrend_blitter.c +++ b/src/vrend_blitter.c @@ -686,7 +686,8 @@ static void calc_dst_deltas_from_src(const struct pipe_blit_info *info, void vrend_renderer_blit_gl(UNUSED struct vrend_context *ctx, struct vrend_resource *src_res, struct vrend_resource *dst_res, - const struct pipe_blit_info *info) + const struct pipe_blit_info *info, + bool use_gles) { struct vrend_blitter_ctx *blit_ctx = &vrend_blit_ctx; GLuint buffers; @@ -787,6 +788,8 @@ void vrend_renderer_blit_gl(UNUSED struct vrend_context *ctx, to_gl_swizzle(src_entry->swizzle[3])); } + if (!use_gles && util_format_is_srgb(src_res->base.format)) + glTexParameteri(src_res->target, GL_TEXTURE_SRGB_DECODE_EXT, GL_DECODE_EXT); glTexParameteri(src_res->target, GL_TEXTURE_WRAP_S, GL_CLAMP_TO_EDGE); glTexParameteri(src_res->target, GL_TEXTURE_WRAP_T, GL_CLAMP_TO_EDGE); @@ -820,6 +823,13 @@ void vrend_renderer_blit_gl(UNUSED struct vrend_context *ctx, glBindFramebuffer(GL_FRAMEBUFFER_EXT, blit_ctx->fb_id); vrend_fb_bind_texture(dst_res, 0, info->dst.level, layer); + if (!use_gles) { + if (util_format_is_srgb(dst_res->base.format)) + glEnable(GL_FRAMEBUFFER_SRGB); + else + glDisable(GL_FRAMEBUFFER_SRGB); + } + buffers = GL_COLOR_ATTACHMENT0_EXT; glDrawBuffers(1, &buffers); blitter_set_texcoords(blit_ctx, src_res, info->src.level, @@ -829,4 +839,11 @@ void vrend_renderer_blit_gl(UNUSED struct vrend_context *ctx, glBufferData(GL_ARRAY_BUFFER, sizeof(blit_ctx->vertices), blit_ctx->vertices, GL_STATIC_DRAW); glDrawArrays(GL_TRIANGLE_FAN, 0, 4); } + if (!use_gles) { + if (util_format_is_srgb(dst_res->base.format)) + glDisable(GL_FRAMEBUFFER_SRGB); + + if (util_format_is_srgb(src_res->base.format)) + glTexParameteri(src_res->target, GL_TEXTURE_SRGB_DECODE_EXT, GL_DECODE_EXT); + } } diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c index e357bae..8b3e513 100644 --- a/src/vrend_renderer.c +++ b/src/vrend_renderer.c @@ -6571,7 +6571,7 @@ static void vrend_renderer_blit_int(struct vrend_context *ctx, use_gl = !vrend_formats_can_blit(tex_conv_table, src_res->base.format, dst_res->base.format); if (use_gl) { - vrend_renderer_blit_gl(ctx, src_res, dst_res, info); + vrend_renderer_blit_gl(ctx, src_res, dst_res, info, vrend_state.use_gles); vrend_clicbs->make_current(0, ctx->sub->gl_context); return; } @@ -6690,6 +6690,13 @@ static void vrend_renderer_blit_int(struct vrend_context *ctx, vrend_fb_bind_texture(dst_res, 0, info->dst.level, info->dst.box.z + i); glBindFramebuffer(GL_DRAW_FRAMEBUFFER, ctx->sub->blit_fb_ids[1]); + if (!vrend_state.use_gles) { + if (util_format_is_srgb(dst_res->base.format)) + glEnable(GL_FRAMEBUFFER_SRGB); + else + glDisable(GL_FRAMEBUFFER_SRGB); + } + glBindFramebuffer(GL_READ_FRAMEBUFFER, intermediate_fbo); glBlitFramebuffer(info->src.box.x, diff --git a/src/vrend_renderer.h b/src/vrend_renderer.h index 28e2819..c873c8c 100644 --- a/src/vrend_renderer.h +++ b/src/vrend_renderer.h @@ -388,7 +388,8 @@ bool vrend_formats_can_blit(struct vrend_format_table *table, enum virgl_formats void vrend_renderer_blit_gl(struct vrend_context *ctx, struct vrend_resource *src_res, struct vrend_resource *dst_res, - const struct pipe_blit_info *info); + const struct pipe_blit_info *info, + bool use_gles); void vrend_renderer_reset(void); int vrend_renderer_get_poll_fd(void); -- 2.17.1 _______________________________________________ virglrenderer-devel mailing list <a href="mailto:virglrenderer-devel@lists.freedesktop.org" target="_blank">virglrenderer-devel@lists.freedesktop.org</a> <a href="https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel" rel="noreferrer" target="_blank">https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel</a> </blockquote></div>