Luca Barbieri wrote:
> Changes in v3:
> - Use positive caps instead of negative ones
> 
> Changes in v2:
> - Updated formatting
> 
> The state tracker will use the TGSI convention properties if the hardware
> exposes the appropriate capability, and otherwise adjust WPOS itself.
> 
> This will also fix some drivers that were previously broken due to their
> incorrect, inadvertent, use of conventions other than upper_left+half_integer.

Minor comments below.


> ---
>  src/mesa/state_tracker/st_extensions.c   |    1 +
>  src/mesa/state_tracker/st_mesa_to_tgsi.c |   74 ++++++++++++++++++++++++++++-
>  2 files changed, 72 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/state_tracker/st_extensions.c 
> b/src/mesa/state_tracker/st_extensions.c
> index 60732f3..982a1fb 100644
> --- a/src/mesa/state_tracker/st_extensions.c
> +++ b/src/mesa/state_tracker/st_extensions.c
> @@ -147,6 +147,7 @@ void st_init_extensions(struct st_context *st)
>      * Extensions that are supported by all Gallium drivers:
>      */
>     ctx->Extensions.ARB_copy_buffer = GL_TRUE;
> +   ctx->Extensions.ARB_fragment_coord_conventions = GL_TRUE;

We can't really expose this extension until we've updated GLSL to 
understand layout qualifiers.  And, unfortunately, that builds on GLSL 
1.40 which we don't support yet.


>     ctx->Extensions.ARB_fragment_program = GL_TRUE;
>     ctx->Extensions.ARB_half_float_vertex = GL_TRUE;
>     ctx->Extensions.ARB_map_buffer_range = GL_TRUE;
> diff --git a/src/mesa/state_tracker/st_mesa_to_tgsi.c 
> b/src/mesa/state_tracker/st_mesa_to_tgsi.c
> index 05b56c9..366729a 100644
> --- a/src/mesa/state_tracker/st_mesa_to_tgsi.c
> +++ b/src/mesa/state_tracker/st_mesa_to_tgsi.c
> @@ -34,8 +34,10 @@
>  #include "pipe/p_compiler.h"
>  #include "pipe/p_shader_tokens.h"
>  #include "pipe/p_state.h"
> +#include "pipe/p_context.h"
>  #include "tgsi/tgsi_ureg.h"
>  #include "st_mesa_to_tgsi.h"
> +#include "st_context.h"
>  #include "shader/prog_instruction.h"
>  #include "shader/prog_parameter.h"
>  #include "shader/prog_print.h"
> @@ -665,6 +667,22 @@ compile_instruction(
>     }
>  }
>  
> +/**
> + * Emit the TGSI instructions to adjust the WPOS pixel center convention
> + */
> +static void
> +emit_adjusted_wpos( struct st_translate *t,
> +                    const struct gl_program *program, GLfloat value)
> +{
> +   struct ureg_program *ureg = t->ureg;
> +   struct ureg_dst wpos_temp = ureg_DECL_temporary(ureg);
> +   struct ureg_src wpos_input = t->inputs[t->inputMapping[FRAG_ATTRIB_WPOS]];
> +
> +   ureg_ADD(ureg, ureg_writemask(wpos_temp, TGSI_WRITEMASK_X | 
> TGSI_WRITEMASK_Y),
> +                wpos_input, ureg_imm1f(ureg, value));
> +
> +   t->inputs[t->inputMapping[FRAG_ATTRIB_WPOS]] = ureg_src(wpos_temp);
> +}
>  
>  /**
>   * Emit the TGSI instructions for inverting the WPOS y coordinate.
> @@ -690,12 +708,17 @@ emit_inverted_wpos( struct st_translate *t,
>                                                         winSizeState);
>  
>     struct ureg_src winsize = ureg_DECL_constant( ureg, winHeightConst );
> -   struct ureg_dst wpos_temp = ureg_DECL_temporary( ureg );
> +   struct ureg_dst wpos_temp;
>     struct ureg_src wpos_input = t->inputs[t->inputMapping[FRAG_ATTRIB_WPOS]];
>  
>     /* MOV wpos_temp, input[wpos]
>      */
> -   ureg_MOV( ureg, wpos_temp, wpos_input );
> +   if (wpos_input.File == TGSI_FILE_TEMPORARY)
> +      wpos_temp = ureg_dst(wpos_input);
> +   else {
> +      wpos_temp = ureg_DECL_temporary( ureg );
> +      ureg_MOV( ureg, wpos_temp, wpos_input );
> +   }
>  
>     /* SUB wpos_temp.y, winsize_const, wpos_input
>      */
> @@ -801,6 +824,7 @@ st_translate_mesa_program(
>      * Declare input attributes.
>      */
>     if (procType == TGSI_PROCESSOR_FRAGMENT) {
> +      struct gl_fragment_program* fp = (struct gl_fragment_program*)program;
>        for (i = 0; i < numInputs; i++) {
>           t->inputs[i] = ureg_DECL_fs_input(ureg,
>                                             inputSemanticName[i],
> @@ -812,7 +836,51 @@ st_translate_mesa_program(
>           /* Must do this after setting up t->inputs, and before
>            * emitting constant references, below:
>            */
> -         emit_inverted_wpos( t, program );
> +         struct pipe_screen* pscreen = st_context(ctx)->pipe->screen;
> +         int invert = 0;

This should be "boolean invert = FALSE".


> +         if (!fp->OriginUpperLeft) {
> +            if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_ORIGIN_LOWER_LEFT))
> +               ureg_property_fs_coord_origin(ureg, 
> TGSI_FS_COORD_ORIGIN_LOWER_LEFT);
> +            else if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_ORIGIN_UPPER_LEFT))
> +               invert = 1;
> +            else
> +               assert(0);
> +         }
> +         else {
> +            if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_ORIGIN_UPPER_LEFT)) {
> +            }
> +            else if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_ORIGIN_LOWER_LEFT)) {
> +               ureg_property_fs_coord_origin(ureg, 
> TGSI_FS_COORD_ORIGIN_LOWER_LEFT);
> +               invert = 1;
> +            }
> +            else
> +               assert(0);
> +         }

In an if/else like this I think it's a little easier to read if the 
condition is "if (fp->OriginUpperLeft) {".  That is, switch the 
true/false clauses and remove the "not".  As you do in the next piece 
of code:


> +
> +         if (fp->PixelCenterInteger) {
> +            if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_INTEGER))
> +               ureg_property_fs_coord_pixel_center(ureg, 
> TGSI_FS_COORD_PIXEL_CENTER_INTEGER);
> +            else if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_HALF_INTEGER))
> +               emit_adjusted_wpos(t, program, invert ? 0.5f : -0.5f);
> +            else
> +               assert(0);
> +         }
> +         else {
> +            if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_HALF_INTEGER)) {
> +            }
> +            else if (pscreen->get_param(pscreen, 
> PIPE_CAP_TGSI_FS_COORD_PIXEL_CENTER_INTEGER)) {
> +               ureg_property_fs_coord_pixel_center(ureg, 
> TGSI_FS_COORD_PIXEL_CENTER_INTEGER);
> +               emit_adjusted_wpos(t, program, invert ? -0.5f : 0.5f);
> +            }
> +            else
> +               assert(0);
> +         }
> +
> +         /* we invert after adjustment so that we avoid the MOV to temporary,
> +          * and reuse the adjustment ADD instead */
> +         if (invert)
> +            emit_inverted_wpos(t, program);
>        }
>  
>        if (program->InputsRead & FRAG_BIT_FACE) {


------------------------------------------------------------------------------
The Planet: dedicated and managed hosting, cloud storage, colocation
Stay online with enterprise data centers and the best network in the business
Choose flexible plans and management services without long-term contracts
Personal 24x7 support from experience hosting pros just a phone call away.
http://p.sf.net/sfu/theplanet-com
_______________________________________________
Mesa3d-dev mailing list
Mesa3d-dev@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/mesa3d-dev

Reply via email to