[v2] shader: Do not adjust y coordinate if an application render to a FBO

Submitted by Gurchetan Singh on July 31, 2018, 10:18 p.m.

Details

Message ID CAAfnVBmTvyexCGNnQfsVFnydaLHG_z_z=L4e5vqw-FVR1RaRVw@mail.gmail.com
State New
Headers show
Series "shader: Do not adjust y coordinate if an application render to a FBO" ( rev: 2 ) in Virgil 3D

Not browsing as part of any series.

Commit Message

Gurchetan Singh July 31, 2018, 10:18 p.m.
On Mon, Jul 30, 2018 at 6:46 AM Elie Tournier <tournier.elie@gmail.com> wrote:
>
> v2:
> - Rename the uniform (Gurchetan)
> - Add condition to the uniform emission (Gurchetan)
> - Rebase master
>
> Fixes: 7f615cd "shader: Invert y coordonate if on gles host"
>
> Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
> ---
>  src/vrend_renderer.c |  6 ++++++
>  src/vrend_shader.c   | 11 ++++++++---
>  2 files changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> index 16013cc..4bd5b3c 100644
> --- a/src/vrend_renderer.c
> +++ b/src/vrend_renderer.c
> @@ -250,6 +250,7 @@ struct vrend_linked_shader_program {
>
>     GLuint *ubo_locs[PIPE_SHADER_TYPES];
>     GLuint vs_ws_adjust_loc;
> +   GLuint fs_ws_adjust_loc_gles;
>
>     GLint fs_stipple_loc;
>
> @@ -1232,6 +1233,8 @@ static struct vrend_linked_shader_program *add_shader_program(struct vrend_conte
>     else
>        sprog->fs_stipple_loc = -1;
>     sprog->vs_ws_adjust_loc = glGetUniformLocation(prog_id, "winsys_adjust_y");
> +   if (vrend_state.use_gles)
> +      sprog->fs_ws_adjust_loc_gles = glGetUniformLocation(prog_id, "winsys_adjust_y_gles");
>     for (id = PIPE_SHADER_VERTEX; id <= last_shader; id++) {
>        if (!sprog->ss[id])
>           continue;
> @@ -3519,6 +3522,9 @@ int vrend_draw_vbo(struct vrend_context *ctx,
>     }
>     glUniform1f(ctx->sub->prog->vs_ws_adjust_loc, ctx->sub->viewport_is_negative ? -1.0 : 1.0);
>
> +   if (vrend_state.use_gles)
> +      glUniform1f(ctx->sub->prog->fs_ws_adjust_loc_gles, ctx->sub->viewport_is_negative ? 0.0 : 1.0);
> +
>     if (ctx->sub->rs_state.clip_plane_enable) {
>        for (i = 0 ; i < 8; i++) {
>           glUniform4fv(ctx->sub->prog->clip_locs[i], 1, (const GLfloat *)&ctx->sub->ucp_state.ucp[i]);
> diff --git a/src/vrend_shader.c b/src/vrend_shader.c
> index ca4e18e..f37d56e 100644
> --- a/src/vrend_shader.c
> +++ b/src/vrend_shader.c
> @@ -72,7 +72,7 @@ struct vrend_shader_io {
>     bool glsl_gl_block;
>     bool override_no_wm;
>     bool is_int;
> -   char glsl_name[64];
> +   char glsl_name[128];
>     unsigned stream;
>  };
>
> @@ -751,7 +751,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
>           if (iter->processor.Processor == TGSI_PROCESSOR_FRAGMENT) {
>              if (ctx->key->coord_replace & (1 << ctx->inputs[i].sid)) {
>                 if (ctx->cfg->use_gles)
> -                  name_prefix = "vec4(gl_PointCoord.x, 1.0 - gl_PointCoord.y, 0.0, 1.0)";
> +                  name_prefix = "vec4(gl_PointCoord.x, mix(1.0 - gl_PointCoord.y, gl_PointCoord.y, winsys_adjust_y_gles), 0.0, 1.0)";
>                 else
>                    name_prefix = "vec4(gl_PointCoord, 0.0, 1.0)";
>                 ctx->inputs[i].glsl_predefined_no_emit = true;
> @@ -776,7 +776,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
>        }
>
>        if (ctx->inputs[i].glsl_no_index)
> -         snprintf(ctx->inputs[i].glsl_name, 64, "%s", name_prefix);
> +         snprintf(ctx->inputs[i].glsl_name, 128, "%s", name_prefix);
>        else {
>           if (ctx->inputs[i].name == TGSI_SEMANTIC_FOG)
>              snprintf(ctx->inputs[i].glsl_name, 64, "%s_f%d", name_prefix, ctx->inputs[i].sid);
> @@ -3772,6 +3772,11 @@ static char *emit_ios(struct dump_ctx *ctx, char *glsl_hdr)
>        bcolor_emitted[0] = bcolor_emitted[1] = false;
>     }
>     if (ctx->prog_type == TGSI_PROCESSOR_FRAGMENT) {
> +      if (ctx->cfg->use_gles &&
> +         (ctx->key->coord_replace & (1 << ctx->inputs[i].sid))) {
> +         snprintf(buf, 255, "uniform float winsys_adjust_y_gles;\n");
> +         STRCAT_WITH_RET(glsl_hdr, buf);
> +      }
>        if (fs_emit_layout(ctx)) {
>           bool upper_left = !(ctx->fs_coord_origin ^ ctx->key->invert_fs_origin);
>           char comma = (upper_left && ctx->fs_pixel_center) ? ',' : ' ';
> --
> 2.18.0

Better, since we don't emit winsys_adjust_y_gles every-time, but I
like the approach you mentioned in
https://patchwork.freedesktop.org/patch/223563/ that only uses
winsys_adjust_y:

                else
                   name_prefix = "vec4(gl_PointCoord, 0.0, 1.0)";
                ctx->inputs[i].glsl_predefined_no_emit = true;
@@ -858,7 +858,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
       }

       if (ctx->inputs[i].glsl_no_index)
-         snprintf(ctx->inputs[i].glsl_name, 64, "%s", name_prefix);
+         snprintf(ctx->inputs[i].glsl_name, 128, "%s", name_prefix);
       else {
          if (ctx->inputs[i].name == TGSI_SEMANTIC_FOG)
             snprintf(ctx->inputs[i].glsl_name, 64, "%s_f%d",
name_prefix, ctx->inputs[i].sid);

Doesn't this also fix the test(s) that are failing??

> _______________________________________________
> virglrenderer-devel mailing list
> virglrenderer-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel

Patch hide | download patch | download mbox

diff --git a/src/vrend_shader.c b/src/vrend_shader.c
index 5499faa..2127d6e 100644
--- a/src/vrend_shader.c
+++ b/src/vrend_shader.c
@@ -76,7 +76,7 @@  struct vrend_shader_io {
    bool glsl_gl_block;
    bool override_no_wm;
    bool is_int;
-   char glsl_name[64];
+   char glsl_name[128];
    unsigned stream;
 };

@@ -833,7 +833,7 @@  iter_declaration(struct tgsi_iterate_context *iter,
          if (iter->processor.Processor == TGSI_PROCESSOR_FRAGMENT) {
             if (ctx->key->coord_replace & (1 << ctx->inputs[i].sid)) {
                if (ctx->cfg->use_gles)
-                  name_prefix = "vec4(gl_PointCoord.x, 1.0 -
gl_PointCoord.y, 0.0, 1.0)";
+                  name_prefix = "vec4(gl_PointCoord.x, mix(1.0 -
gl_PointCoord.y, gl_PointCoord.y, clamp(winsys_adjust_y, 0.0, 1.0),
0.0, 1.0)";

Comments

On Tue, Jul 31, 2018 at 03:18:52PM -0700, Gurchetan Singh wrote:
> On Mon, Jul 30, 2018 at 6:46 AM Elie Tournier <tournier.elie@gmail.com> wrote:
> >
> > v2:
> > - Rename the uniform (Gurchetan)
> > - Add condition to the uniform emission (Gurchetan)
> > - Rebase master
> >
> > Fixes: 7f615cd "shader: Invert y coordonate if on gles host"
> >
> > Signed-off-by: Elie Tournier <elie.tournier@collabora.com>
> > ---
> >  src/vrend_renderer.c |  6 ++++++
> >  src/vrend_shader.c   | 11 ++++++++---
> >  2 files changed, 14 insertions(+), 3 deletions(-)
> >
> > diff --git a/src/vrend_renderer.c b/src/vrend_renderer.c
> > index 16013cc..4bd5b3c 100644
> > --- a/src/vrend_renderer.c
> > +++ b/src/vrend_renderer.c
> > @@ -250,6 +250,7 @@ struct vrend_linked_shader_program {
> >
> >     GLuint *ubo_locs[PIPE_SHADER_TYPES];
> >     GLuint vs_ws_adjust_loc;
> > +   GLuint fs_ws_adjust_loc_gles;
> >
> >     GLint fs_stipple_loc;
> >
> > @@ -1232,6 +1233,8 @@ static struct vrend_linked_shader_program *add_shader_program(struct vrend_conte
> >     else
> >        sprog->fs_stipple_loc = -1;
> >     sprog->vs_ws_adjust_loc = glGetUniformLocation(prog_id, "winsys_adjust_y");
> > +   if (vrend_state.use_gles)
> > +      sprog->fs_ws_adjust_loc_gles = glGetUniformLocation(prog_id, "winsys_adjust_y_gles");
> >     for (id = PIPE_SHADER_VERTEX; id <= last_shader; id++) {
> >        if (!sprog->ss[id])
> >           continue;
> > @@ -3519,6 +3522,9 @@ int vrend_draw_vbo(struct vrend_context *ctx,
> >     }
> >     glUniform1f(ctx->sub->prog->vs_ws_adjust_loc, ctx->sub->viewport_is_negative ? -1.0 : 1.0);
> >
> > +   if (vrend_state.use_gles)
> > +      glUniform1f(ctx->sub->prog->fs_ws_adjust_loc_gles, ctx->sub->viewport_is_negative ? 0.0 : 1.0);
> > +
> >     if (ctx->sub->rs_state.clip_plane_enable) {
> >        for (i = 0 ; i < 8; i++) {
> >           glUniform4fv(ctx->sub->prog->clip_locs[i], 1, (const GLfloat *)&ctx->sub->ucp_state.ucp[i]);
> > diff --git a/src/vrend_shader.c b/src/vrend_shader.c
> > index ca4e18e..f37d56e 100644
> > --- a/src/vrend_shader.c
> > +++ b/src/vrend_shader.c
> > @@ -72,7 +72,7 @@ struct vrend_shader_io {
> >     bool glsl_gl_block;
> >     bool override_no_wm;
> >     bool is_int;
> > -   char glsl_name[64];
> > +   char glsl_name[128];
> >     unsigned stream;
> >  };
> >
> > @@ -751,7 +751,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
> >           if (iter->processor.Processor == TGSI_PROCESSOR_FRAGMENT) {
> >              if (ctx->key->coord_replace & (1 << ctx->inputs[i].sid)) {
> >                 if (ctx->cfg->use_gles)
> > -                  name_prefix = "vec4(gl_PointCoord.x, 1.0 - gl_PointCoord.y, 0.0, 1.0)";
> > +                  name_prefix = "vec4(gl_PointCoord.x, mix(1.0 - gl_PointCoord.y, gl_PointCoord.y, winsys_adjust_y_gles), 0.0, 1.0)";
> >                 else
> >                    name_prefix = "vec4(gl_PointCoord, 0.0, 1.0)";
> >                 ctx->inputs[i].glsl_predefined_no_emit = true;
> > @@ -776,7 +776,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
> >        }
> >
> >        if (ctx->inputs[i].glsl_no_index)
> > -         snprintf(ctx->inputs[i].glsl_name, 64, "%s", name_prefix);
> > +         snprintf(ctx->inputs[i].glsl_name, 128, "%s", name_prefix);
> >        else {
> >           if (ctx->inputs[i].name == TGSI_SEMANTIC_FOG)
> >              snprintf(ctx->inputs[i].glsl_name, 64, "%s_f%d", name_prefix, ctx->inputs[i].sid);
> > @@ -3772,6 +3772,11 @@ static char *emit_ios(struct dump_ctx *ctx, char *glsl_hdr)
> >        bcolor_emitted[0] = bcolor_emitted[1] = false;
> >     }
> >     if (ctx->prog_type == TGSI_PROCESSOR_FRAGMENT) {
> > +      if (ctx->cfg->use_gles &&
> > +         (ctx->key->coord_replace & (1 << ctx->inputs[i].sid))) {
> > +         snprintf(buf, 255, "uniform float winsys_adjust_y_gles;\n");
> > +         STRCAT_WITH_RET(glsl_hdr, buf);
> > +      }
> >        if (fs_emit_layout(ctx)) {
> >           bool upper_left = !(ctx->fs_coord_origin ^ ctx->key->invert_fs_origin);
> >           char comma = (upper_left && ctx->fs_pixel_center) ? ',' : ' ';
> > --
> > 2.18.0
> 
> Better, since we don't emit winsys_adjust_y_gles every-time, but I
> like the approach you mentioned in
> https://patchwork.freedesktop.org/patch/223563/ that only uses
> winsys_adjust_y:
Oh sorry, I misread your reply. My bad.
I will submit the fix later today.
> 
> diff --git a/src/vrend_shader.c b/src/vrend_shader.c
> index 5499faa..2127d6e 100644
> --- a/src/vrend_shader.c
> +++ b/src/vrend_shader.c
> @@ -76,7 +76,7 @@ struct vrend_shader_io {
>     bool glsl_gl_block;
>     bool override_no_wm;
>     bool is_int;
> -   char glsl_name[64];
> +   char glsl_name[128];
>     unsigned stream;
>  };
> 
> @@ -833,7 +833,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
>           if (iter->processor.Processor == TGSI_PROCESSOR_FRAGMENT) {
>              if (ctx->key->coord_replace & (1 << ctx->inputs[i].sid)) {
>                 if (ctx->cfg->use_gles)
> -                  name_prefix = "vec4(gl_PointCoord.x, 1.0 -
> gl_PointCoord.y, 0.0, 1.0)";
> +                  name_prefix = "vec4(gl_PointCoord.x, mix(1.0 -
> gl_PointCoord.y, gl_PointCoord.y, clamp(winsys_adjust_y, 0.0, 1.0),
> 0.0, 1.0)";
>                 else
>                    name_prefix = "vec4(gl_PointCoord, 0.0, 1.0)";
>                 ctx->inputs[i].glsl_predefined_no_emit = true;
> @@ -858,7 +858,7 @@ iter_declaration(struct tgsi_iterate_context *iter,
>        }
> 
>        if (ctx->inputs[i].glsl_no_index)
> -         snprintf(ctx->inputs[i].glsl_name, 64, "%s", name_prefix);
> +         snprintf(ctx->inputs[i].glsl_name, 128, "%s", name_prefix);
>        else {
>           if (ctx->inputs[i].name == TGSI_SEMANTIC_FOG)
>              snprintf(ctx->inputs[i].glsl_name, 64, "%s_f%d",
> name_prefix, ctx->inputs[i].sid);
> 
> Doesn't this also fix the test(s) that are failing??
> 
> > _______________________________________________
> > virglrenderer-devel mailing list
> > virglrenderer-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/virglrenderer-devel