Re: [Mesa-dev] Gallium debugging help

[Marek Olšák]

Hi James,

1) Try: GALLIUM_TRACE=~/gallium.trace ./gbmtest

2) Trace is an optional component, so make sure it's built in your
driver. See for example src/gallium/targets/dri-r600/Makefile.am to
know how to add it.

3) The easiest way to debug gallium is to use a debugger.

Marek



On Wed, Sep 18, 2013 at 2:08 AM, James Simmons  wrote:
>
> Hello.
>
> I'm the main developer from the Openchrome project and we have
> reached the point were work is being started for the Gallium driver for
> VIA hardware. To help develope a stable simple start I decided to go for
> bare bones libgbm support using a gallium backend. So like any start my
> simple libgbm app fails at the start. Well to figure it out I need to
> debug my gallium driver. So I started to read up on how to debug gallium
> and so I attempted to use the trace pipe driver. So the command I used
> was
>
> GALLIUM_TRACE=~/gallium.trace;./gbmtest
>
> but no XML file was generated. What am I doing wrong? How do I properly
> debug? Thanks for your help. Once I get this working I will push to
> master.
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/3] util/u_blit: Implement util_blit_pixels via pipe_context::blit.

[Jose Fonseca]

- Original Message -
> Isn't u_blit a candidate for removal considering it has no user in Mesa?

u_blit still has users outside mesa. And it provides features that 
pipe_context::blit does not:

- util_blit_pixels() will call pipe_context::resource_copy for non-stretch 
blits, which is potentially faster than pipe_context::blit
- util_blit_pixels_tex() allows to specify pipe_sampler_view::swizzle_?

Ie., the ability to do textured quad blits from state tracker is something that 
will be often handy.

The reasons I need custom pipe_sampler_view::swizzle_? is to simulate DX9 
formats (which have ?, 1, 1, 1 swizlle; while gallium/OpenGL/D3D10 formats has 
?, 0, 0, 1 swizzle).  The only way to use pipe_context::blit would be to
a) extend pipe_context::blit to receive pipe_sampler_view::swizzle_?
b) add ?, 1, 1, 1 variants for all DX9 formats...

Both seemed more work than what was worth.

Jose
 
> In any case, for the series:
> 
> Reviewed-by: Marek Olšák 
> 
> Marek
> 
> On Tue, Sep 17, 2013 at 8:33 PM,   wrote:
> > From: José Fonseca 
> >
> > This removes a lot of code, but not everything, as util_blit_pixels_tex
> > is still useful when one needs to override pipe_sampler_view::swizzle_?.
> > ---
> >  src/gallium/auxiliary/util/u_blit.c | 447
> >  +++-
> >  1 file changed, 37 insertions(+), 410 deletions(-)
> >
> > diff --git a/src/gallium/auxiliary/util/u_blit.c
> > b/src/gallium/auxiliary/util/u_blit.c
> > index e9bec4a..4ba71b9 100644
> > --- a/src/gallium/auxiliary/util/u_blit.c
> > +++ b/src/gallium/auxiliary/util/u_blit.c
> > @@ -57,29 +57,20 @@ struct blit_state
> > struct pipe_context *pipe;
> > struct cso_context *cso;
> >
> > -   struct pipe_blend_state blend_write_color, blend_keep_color;
> > +   struct pipe_blend_state blend_write_color;
> > struct pipe_depth_stencil_alpha_state dsa_keep_depthstencil;
> > -   struct pipe_depth_stencil_alpha_state dsa_write_depthstencil;
> > -   struct pipe_depth_stencil_alpha_state dsa_write_depth;
> > -   struct pipe_depth_stencil_alpha_state dsa_write_stencil;
> > struct pipe_rasterizer_state rasterizer;
> > struct pipe_sampler_state sampler;
> > struct pipe_viewport_state viewport;
> > struct pipe_vertex_element velem[2];
> > -   enum pipe_texture_target internal_target;
> >
> > void *vs;
> > void *fs[PIPE_MAX_TEXTURE_TYPES][TGSI_WRITEMASK_XYZW + 1];
> > -   void *fs_depthstencil[PIPE_MAX_TEXTURE_TYPES];
> > -   void *fs_depth[PIPE_MAX_TEXTURE_TYPES];
> > -   void *fs_stencil[PIPE_MAX_TEXTURE_TYPES];
> >
> > struct pipe_resource *vbuf;  /**< quad vertices */
> > unsigned vbuf_slot;
> >
> > float vertices[4][2][4];   /**< vertex/texcoords for quad */
> > -
> > -   boolean has_stencil_export;
> >  };
> >
> >
> > @@ -103,20 +94,6 @@ util_create_blit(struct pipe_context *pipe, struct
> > cso_context *cso)
> > /* disabled blending/masking */
> > ctx->blend_write_color.rt[0].colormask = PIPE_MASK_RGBA;
> >
> > -   /* depth stencil states */
> > -   ctx->dsa_write_depth.depth.enabled = 1;
> > -   ctx->dsa_write_depth.depth.writemask = 1;
> > -   ctx->dsa_write_depth.depth.func = PIPE_FUNC_ALWAYS;
> > -   ctx->dsa_write_stencil.stencil[0].enabled = 1;
> > -   ctx->dsa_write_stencil.stencil[0].func = PIPE_FUNC_ALWAYS;
> > -   ctx->dsa_write_stencil.stencil[0].fail_op = PIPE_STENCIL_OP_REPLACE;
> > -   ctx->dsa_write_stencil.stencil[0].zpass_op = PIPE_STENCIL_OP_REPLACE;
> > -   ctx->dsa_write_stencil.stencil[0].zfail_op = PIPE_STENCIL_OP_REPLACE;
> > -   ctx->dsa_write_stencil.stencil[0].valuemask = 0xff;
> > -   ctx->dsa_write_stencil.stencil[0].writemask = 0xff;
> > -   ctx->dsa_write_depthstencil.depth = ctx->dsa_write_depth.depth;
> > -   ctx->dsa_write_depthstencil.stencil[0] =
> > ctx->dsa_write_stencil.stencil[0];
> > -
> > /* rasterizer */
> > ctx->rasterizer.cull_face = PIPE_FACE_NONE;
> > ctx->rasterizer.half_pixel_center = 1;
> > @@ -147,14 +124,6 @@ util_create_blit(struct pipe_context *pipe, struct
> > cso_context *cso)
> >ctx->vertices[i][1][3] = 1.0f; /* q */
> > }
> >
> > -   if(pipe->screen->get_param(pipe->screen, PIPE_CAP_NPOT_TEXTURES))
> > -  ctx->internal_target = PIPE_TEXTURE_2D;
> > -   else
> > -  ctx->internal_target = PIPE_TEXTURE_RECT;
> > -
> > -   ctx->has_stencil_export =
> > -  pipe->screen->get_param(pipe->screen,
> > PIPE_CAP_SHADER_STENCIL_EXPORT);
> > -
> > return ctx;
> >  }
> >
> > @@ -178,18 +147,6 @@ util_destroy_blit(struct blit_state *ctx)
> >}
> > }
> >
> > -   for (i = 0; i < PIPE_MAX_TEXTURE_TYPES; i++) {
> > -  if (ctx->fs_depthstencil[i]) {
> > - pipe->delete_fs_state(pipe, ctx->fs_depthstencil[i]);
> > -  }
> > -  if (ctx->fs_depth[i]) {
> > - pipe->delete_fs_state(pipe, ctx->fs_depth[i]);
> > -  }
> > -  if (ctx->fs_stencil[i]) {
> > - pipe->delete_fs_state(pipe, ctx->fs_stencil[i]);
> > -  }
> > -   }
> > -
> > pipe_resourc

Re: [Mesa-dev] [PATCH 2/3] util/u_blit: Support blits from cubemaps.

[Jose Fonseca]

- Original Message -
> Am 17.09.2013 20:32, schrieb jfons...@vmware.com:
> > From: José Fonseca 
> > 
> > By calling util_map_texcoords2d_onto_cubemap.
> > 
> > A new parameter for util_blit_pixels_tex is necessary, as
> > pipe_sampler_view::first_layer is always supposed to point to the first
> > face when sampling from cubemaps.
> > ---
> >  src/gallium/auxiliary/util/u_blit.c | 34
> >  +++---
> >  src/gallium/auxiliary/util/u_blit.h |  1 +
> >  2 files changed, 32 insertions(+), 3 deletions(-)
> > 
> > diff --git a/src/gallium/auxiliary/util/u_blit.c
> > b/src/gallium/auxiliary/util/u_blit.c
> > index 8cc080c..e9bec4a 100644
> > --- a/src/gallium/auxiliary/util/u_blit.c
> > +++ b/src/gallium/auxiliary/util/u_blit.c
> > @@ -46,6 +46,7 @@
> >  #include "util/u_math.h"
> >  #include "util/u_memory.h"
> >  #include "util/u_sampler.h"
> > +#include "util/u_texture.h"
> >  #include "util/u_simple_shaders.h"
> >  
> >  #include "cso_cache/cso_context.h"
> > @@ -143,7 +144,6 @@ util_create_blit(struct pipe_context *pipe, struct
> > cso_context *cso)
> > /* init vertex data that doesn't change */
> > for (i = 0; i < 4; i++) {
> >ctx->vertices[i][0][3] = 1.0f; /* w */
> > -  ctx->vertices[i][1][2] = 0.0f; /* r */
> >ctx->vertices[i][1][3] = 1.0f; /* q */
> > }
> >  
> > @@ -327,6 +327,8 @@ get_next_slot( struct blit_state *ctx )
> >   */
> >  static unsigned
> >  setup_vertex_data_tex(struct blit_state *ctx,
> > +  unsigned src_target,
> > +  unsigned src_face,
> >float x0, float y0, float x1, float y1,
> >float s0, float t0, float s1, float t1,
> >float z)
> > @@ -338,24 +340,37 @@ setup_vertex_data_tex(struct blit_state *ctx,
> > ctx->vertices[0][0][2] = z;
> > ctx->vertices[0][1][0] = s0; /*s*/
> > ctx->vertices[0][1][1] = t0; /*t*/
> > +   ctx->vertices[0][1][2] = 0;  /*r*/
> >  
> > ctx->vertices[1][0][0] = x1;
> > ctx->vertices[1][0][1] = y0;
> > ctx->vertices[1][0][2] = z;
> > ctx->vertices[1][1][0] = s1; /*s*/
> > ctx->vertices[1][1][1] = t0; /*t*/
> > +   ctx->vertices[1][1][2] = 0;  /*r*/
> >  
> > ctx->vertices[2][0][0] = x1;
> > ctx->vertices[2][0][1] = y1;
> > ctx->vertices[2][0][2] = z;
> > ctx->vertices[2][1][0] = s1;
> > ctx->vertices[2][1][1] = t1;
> > +   ctx->vertices[3][1][2] = 0;
> >  
> > ctx->vertices[3][0][0] = x0;
> > ctx->vertices[3][0][1] = y1;
> > ctx->vertices[3][0][2] = z;
> > ctx->vertices[3][1][0] = s0;
> > ctx->vertices[3][1][1] = t1;
> > +   ctx->vertices[3][1][2] = 0;
> > +
> > +   if (src_target == PIPE_TEXTURE_CUBE ||
> > +   src_target == PIPE_TEXTURE_CUBE_ARRAY) {
> > +  /* Map cubemap texture coordinates inplace. */
> > +  const unsigned stride = sizeof ctx->vertices[0] / sizeof
> > ctx->vertices[0][0][0];
> > +  util_map_texcoords2d_onto_cubemap(src_face,
> > +&ctx->vertices[0][1][0], stride,
> > +&ctx->vertices[0][1][0], stride);
> > +   }
> >  
> > offset = get_next_slot( ctx );
> >  
> > @@ -770,6 +785,8 @@ util_blit_pixels(struct blit_state *ctx,
> >  
> > /* draw quad */
> > offset = setup_vertex_data_tex(ctx,
> > +  sampler_view->texture->target,
> > +  srcZ0 % 6,
> >(float) dstX0 / dst_surface->width *
> >2.0f - 1.0f,
> >(float) dstY0 / dst_surface->height *
> >2.0f - 1.0f,
> >(float) dstX1 / dst_surface->width *
> >2.0f - 1.0f,
> > @@ -811,16 +828,25 @@ util_blit_pixels(struct blit_state *ctx,
> >  
> >  
> >  /**
> > - * Copy pixel block from src texture to dst surface.
> > + * Copy pixel block from src sampler view to dst surface.
> > + *
> >   * The sampler view's first_level field indicates the source
> >   * mipmap level to use.
> > - * XXX need some control over blitting Z and/or stencil.
> > + *
> > + * The sampler view's first_layer indicate the layer to use, but for
> > + * cube maps it must point to the first face.  Face is passed in src_face.
> > + *
> > + * The main advantage over util_blit_pixels is that it allows to specify
> > swizzles in
> > + * pipe_sampler_view::swizzle_?.
> > + *
> > + * But there is no control over blitting Z and/or stencil.
> >   */
> >  void
> >  util_blit_pixels_tex(struct blit_state *ctx,
> >   struct pipe_sampler_view *src_sampler_view,
> >   int srcX0, int srcY0,
> >   int srcX1, int srcY1,
> > + unsigned src_face,
> >   struct pipe_surface *dst,
> >   int dstX0, int dstY0,
> >

[Mesa-dev] [Bug 55817] Torchlight: Texture renders as garbage

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=55817

--- Comment #10 from Sven Arvidsson  ---
Is this bug fixed? I can't seem to reproduce it with Mesa 9.2, but I might be
looking in the wrong places?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 3/3] util/u_blit: Implement util_blit_pixels via pipe_context::blit.

[Marek Olšák]

On Wed, Sep 18, 2013 at 12:36 PM, Jose Fonseca  wrote:
> - Original Message -
>> Isn't u_blit a candidate for removal considering it has no user in Mesa?
>
> u_blit still has users outside mesa. And it provides features that 
> pipe_context::blit does not:
>
> - util_blit_pixels() will call pipe_context::resource_copy for non-stretch 
> blits, which is potentially faster than pipe_context::blit

The decision whether resource_copy_region should be used has been
moved to drivers since pipe->blit was added. All that drivers need to
do is to call util_try_blit_via_copy_region.

> - util_blit_pixels_tex() allows to specify pipe_sampler_view::swizzle_?
>
> Ie., the ability to do textured quad blits from state tracker is something 
> that will be often handy.
>
> The reasons I need custom pipe_sampler_view::swizzle_? is to simulate DX9 
> formats (which have ?, 1, 1, 1 swizlle; while gallium/OpenGL/D3D10 formats 
> has ?, 0, 0, 1 swizzle).  The only way to use pipe_context::blit would be to
> a) extend pipe_context::blit to receive pipe_sampler_view::swizzle_?
> b) add ?, 1, 1, 1 variants for all DX9 formats...
>
> Both seemed more work than what was worth.

Actually I think adding ?,1,1,1 variants would be really easy. You can
name the formats e.g. PIPE_FORMAT_R8_UNORM_DX9, etc. I think you only
need to add one line to u_format_csv, one line to p_format.h, and
support for your driver. Then you would be able to use pipe->blit and
you don't have to rely on pipe_sampler_view::swizzle_?.

Marek
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 55817] Torchlight: Texture renders as garbage

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=55817

--- Comment #11 from Tim Allen  ---
Luckily, I still have the same character I used for taking the attached
screenshots, and the game hasn't regenerated the levels in the intervening
months. I don't have Mesa 9.2, I'm just using Debian's Mesa 9.1.6 packages, but
I can report that yes, everything seems to look just fine now.

I guess it got accidentally fixed?

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 55817] Torchlight: Texture renders as garbage

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=55817

Sven Arvidsson  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #12 from Sven Arvidsson  ---
(In reply to comment #11)
> Luckily, I still have the same character I used for taking the attached
> screenshots, and the game hasn't regenerated the levels in the intervening
> months. I don't have Mesa 9.2, I'm just using Debian's Mesa 9.1.6 packages,
> but I can report that yes, everything seems to look just fine now.
> 
> I guess it got accidentally fixed?

Thanks for confirming! It would be interesting to know where/how it was fixed,
but not so interesting that I'm going to bisect for the fix :)

Let's mark this as fixed for now.

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Ian Romanick]

On 09/15/2013 02:10 AM, Francisco Jerez wrote:
> The _mesa_glsl_parse_state object relies on the memory allocator
> zeroing out its contents before it's initialized, which seems rather
> risky.  One of the following commits will homogenize implementations
> of the new operator in a way that would break this assumption leaving
> some of the member variables of this object uninitialized.

This was an intentional design decision that we made, after a fair
amount of consideration, in the early days of the compiler.  A number of
factors came together:

 * Use of ralloc et al. meant that we could never have stack-allocated
objects.

 * Use of rzalloc meant that objects would always be pre-allocated to zero.

 * Uninitialized class member bugs are hard to track down.

 * It's easy to forget to add 'this->foo = 0;' when you add a field foo
to a class.

 * A dozen lines of 'this->foo = 0;' is hard to properly review when a
new class is added.  Are there actually 13 fields that need to be
initialized?

 * Writing a dozen lines of 'this->foo = 0;' is annoying. :)

In the end, we decided to just rely on the allocator always providing
cleared memory.

I'm not very excited about adding a bunch of redundant code to the these
constructors... especially since that code probably won't get maintained
(e.g., when the next member gets added to those classes).

I'd like to hear Ken and Eric weigh in.

> ---
>  src/glsl/glsl_parser_extras.cpp | 16 ++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/src/glsl/glsl_parser_extras.cpp b/src/glsl/glsl_parser_extras.cpp
> index cac5a18..772933f 100644
> --- a/src/glsl/glsl_parser_extras.cpp
> +++ b/src/glsl/glsl_parser_extras.cpp
> @@ -55,7 +55,7 @@ static unsigned known_desktop_glsl_versions[] =
>  
>  _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
>  GLenum target, void *mem_ctx)
> - : ctx(_ctx)
> +   : ctx(_ctx), switch_state()
>  {
> switch (target) {
> case GL_VERTEX_SHADER:   this->target = vertex_shader; break;
> @@ -66,10 +66,14 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct 
> gl_context *_ctx,
> this->scanner = NULL;
> this->translation_unit.make_empty();
> this->symbols = new(mem_ctx) glsl_symbol_table;
> +
> +   this->num_uniform_blocks = 0;
> +   this->uniform_block_array_size = 0;
> +   this->uniform_blocks = NULL;
> +
> this->info_log = ralloc_strdup(mem_ctx, "");
> this->error = false;
> this->loop_nesting_ast = NULL;
> -   this->switch_state.switch_nesting_ast = NULL;
>  
> this->struct_specifier_depth = 0;
> this->num_builtins_to_link = 0;
> @@ -105,6 +109,13 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct 
> gl_context *_ctx,
>  
> this->Const.MaxDrawBuffers = ctx->Const.MaxDrawBuffers;
>  
> +   this->current_function = NULL;
> +   this->toplevel_ir = NULL;
> +   this->found_return = false;
> +   this->all_invariant = false;
> +   this->user_structures = NULL;
> +   this->num_user_structures = 0;
> +
> /* Populate the list of supported GLSL versions */
> /* FINISHME: Once the OpenGL 3.0 'forward compatible' context or
>  * the OpenGL 3.2 Core context is supported, this logic will need
> @@ -163,6 +174,7 @@ _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct 
> gl_context *_ctx,
>  
> this->gs_input_prim_type_specified = false;
> this->gs_input_prim_type = GL_POINTS;
> +   this->gs_input_size = 0;
> this->out_qualifier = new(this) ast_type_qualifier();
>  }
>  
> 

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Ian Romanick]

On 09/15/2013 02:10 AM, Francisco Jerez wrote:
> ---
>  src/glsl/ast.h| 15 ++
>  src/glsl/ast_to_hir.cpp   | 68 
> +--
>  src/glsl/ast_type.cpp | 13 +++--
>  src/glsl/glsl_lexer.ll|  2 +-
>  src/glsl/glsl_parser.yy   | 13 +++--
>  src/glsl/glsl_parser_extras.h | 10 +++
>  6 files changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 26c4701..8a5d3fc 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -405,6 +405,12 @@ struct ast_type_qualifier {
>*/
>   unsigned explicit_binding:1;
>  
> + /**
> +  * Flag set if GL_ARB_shader_atomic counter "offset" layout
> +  * qualifier is used.
> +  */
> + unsigned explicit_offset:1;
> +
>   /** \name Layout qualifiers for GL_AMD_conservative_depth */
>   /** \{ */
>   unsigned depth_any:1;
> @@ -468,6 +474,15 @@ struct ast_type_qualifier {
> int binding;
>  
> /**
> +* Offset specified via GL_ARB_shader_atomic_counter's "offset"
> +* keyword.
> +*
> +* \note
> +* This field is only valid if \c explicit_offset is set.
> +*/
> +   int offset;
> +
> +   /**
>  * Return true if and only if an interpolation qualifier is present.
>  */
> bool has_interpolation() const;
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index fcca5df..7edbee4 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1197,6 +1197,9 @@ ast_expression::hir(exec_list *instructions,
>   !state->check_version(120, 300, &loc,
> "array comparisons forbidden")) {
>error_emitted = true;
> +  } else if ((op[0]->type->atomic_size() || op[1]->type->atomic_size())) 
> {
> +  _mesa_glsl_error(&loc, state, "atomic counter comparisons forbidden");
> +  error_emitted = true;
>}
>  
>if (error_emitted) {
> @@ -1952,10 +1955,19 @@ validate_binding_qualifier(struct 
> _mesa_glsl_parse_state *state,
>  
>   return false;
>}
> +   } else if (var->type->atomic_size()) {
> +  if (unsigned(qual->binding) >= ctx->Const.MaxAtomicBufferBindings) {
> + _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds the "
> +  " maximum number of atomic counter buffer bindings"
> +  "(%d)", qual->binding,
> +  ctx->Const.MaxAtomicBufferBindings);
> +
> + return false;
> +  }
> } else {
>_mesa_glsl_error(loc, state,
> "the \"binding\" qualifier only applies to uniform "
> -   "blocks, samplers, or arrays of samplers");
> +   "blocks, samplers, atomic counters, or arrays 
> thereof");
>return false;
> }
>  
> @@ -1983,7 +1995,7 @@ apply_type_qualifier_to_variable(const struct 
> ast_type_qualifier *qual,
> }
>  
> if (qual->flags.q.constant || qual->flags.q.attribute
> -   || qual->flags.q.uniform
> +   || (qual->flags.q.uniform && var->type != glsl_type::atomic_uint_type)
> || (qual->flags.q.varying && (state->target == fragment_shader)))
>var->read_only = 1;
>  
> @@ -2225,6 +2237,35 @@ apply_type_qualifier_to_variable(const struct 
> ast_type_qualifier *qual,
>var->binding = qual->binding;
> }
>  
> +   if (var->type->atomic_size()) {
> +  if (var->mode == ir_var_uniform) {
> + if (var->explicit_binding) {
> +_mesa_glsl_parse_state::atomic_counter_binding &binding =
> +   state->atomic_counter_bindings[var->binding];
> +
> +if (binding.next_offset % ATOMIC_COUNTER_SIZE)
> +   _mesa_glsl_error(loc, state,
> +"misaligned atomic counter offset");
> +
> +if (binding.offsets.count(binding.next_offset))
> +   _mesa_glsl_error(loc, state,
> +"atomic counter offsets must be unique");
> +
> +var->atomic.offset = binding.next_offset;
> +binding.offsets.insert(binding.next_offset);
> +binding.next_offset += var->type->atomic_size();
> +
> + } else {
> +_mesa_glsl_error(loc, state,
> + "atomic counters require explicit binding 
> point");
> + }
> +  } else if (var->mode != ir_var_function_in) {
> + _mesa_glsl_error(loc, state, "atomic counters may only be declared 
> as "
> +  "function parameters or uniform-qualified "
> +  "global variables");
> +  }
> +   }
> +
> /* Does the declaration use the deprecated 'attribute' or 'varying'
>  * keywords?
>  */
> @@ -2725,6 +2766,18 @@ ast_declarator_list::hir(exec_list *instructions,
> (void) this->type->specifier->hir(instructions, state);

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Ian Romanick]

On 09/17/2013 03:51 PM, Paul Berry wrote:
> On 15 September 2013 00:10, Francisco Jerez  > wrote:
> 
> ---
>  src/glsl/ast.h| 15 ++
>  src/glsl/ast_to_hir.cpp   | 68
> +--
>  src/glsl/ast_type.cpp | 13 +++--
>  src/glsl/glsl_lexer.ll|  2 +-
>  src/glsl/glsl_parser.yy   | 13 +++--
>  src/glsl/glsl_parser_extras.h | 10 +++
>  6 files changed, 114 insertions(+), 7 deletions(-)
> 
> diff --git a/src/glsl/ast.h b/src/glsl/ast.h
> index 26c4701..8a5d3fc 100644
> --- a/src/glsl/ast.h
> +++ b/src/glsl/ast.h
> @@ -405,6 +405,12 @@ struct ast_type_qualifier {
>*/
>   unsigned explicit_binding:1;
> 
> + /**
> +  * Flag set if GL_ARB_shader_atomic counter "offset" layout
> +  * qualifier is used.
> +  */
> + unsigned explicit_offset:1;
> +
>   /** \name Layout qualifiers for GL_AMD_conservative_depth */
>   /** \{ */
>   unsigned depth_any:1;
> @@ -468,6 +474,15 @@ struct ast_type_qualifier {
> int binding;
> 
> /**
> +* Offset specified via GL_ARB_shader_atomic_counter's "offset"
> +* keyword.
> +*
> +* \note
> +* This field is only valid if \c explicit_offset is set.
> +*/
> +   int offset;
> +
> +   /**
>  * Return true if and only if an interpolation qualifier is present.
>  */
> bool has_interpolation() const;
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index fcca5df..7edbee4 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -1197,6 +1197,9 @@ ast_expression::hir(exec_list *instructions,
>   !state->check_version(120, 300, &loc,
> "array comparisons
> forbidden")) {
>  error_emitted = true;
> +  } else if ((op[0]->type->atomic_size() ||
> op[1]->type->atomic_size())) {
> +_mesa_glsl_error(&loc, state, "atomic counter comparisons
> forbidden");
> +error_emitted = true;
> 
> 
> Do we have spec text to back this up?  I looked around and couldn't find

Section 4.1.7 (Opaque Types) of the GLSL 4.40 spec says:

"Except for array indexing, structure member selection, and
parentheses, opaque variables are not allowed to be operands
in expressions; such use results in a compile-time error."

We shouldn't allow comparisons of samplers either. :)

> anything.  It seems like doing equality comparisons on atomic counters
> is ill-defined, though (do two counters compare equal if they have equal
> values, or if they point to the same counter?  Both possibilities seem
> dodgy).  Maybe we should file a spec bug so we can get clarification
> from khronos about what's intended.
> 
> Note that we currently permit other comparisons that are similarly dodgy
> (e.g. comparing samplers).  It would be nice to get clarification from
> khronos about this too.

Given the above spec quote, I think the intention is clear.  We're just
doing the wrong thing.  I'm starting to feel better about my is_opaque /
contains_opaque predicate idea...

>}
> 
>if (error_emitted) {
> @@ -1952,10 +1955,19 @@ validate_binding_qualifier(struct
> _mesa_glsl_parse_state *state,
> 
>   return false;
>}
> +   } else if (var->type->atomic_size()) {
> +  if (unsigned(qual->binding) >=
> ctx->Const.MaxAtomicBufferBindings) {
> + _mesa_glsl_error(loc, state, "layout(binding = %d) exceeds
> the "
> +  " maximum number of atomic counter buffer
> bindings"
> +  "(%d)", qual->binding,
> +  ctx->Const.MaxAtomicBufferBindings);
> +
> + return false;
> +  }
> } else {
>_mesa_glsl_error(loc, state,
> "the \"binding\" qualifier only applies to
> uniform "
> -   "blocks, samplers, or arrays of samplers");
> +   "blocks, samplers, atomic counters, or
> arrays thereof");
>return false;
> }
> 
> @@ -1983,7 +1995,7 @@ apply_type_qualifier_to_variable(const struct
> ast_type_qualifier *qual,
> }
> 
> if (qual->flags.q.constant || qual->flags.q.attribute
> -   || qual->flags.q.uniform
> +   || (qual->flags.q.uniform && var->type !=
> glsl_type::atomic_uint_type)
> || (qual->flags.q.varying && (state->target ==
> fragment_shader)))
>var->read_only = 1;
> 
> 
> I'm not entirely convinced this is right.  An atomic counter, like a
> sampler, is a handle to an underlying object, not the under

Re: [Mesa-dev] [PATCH] android: Remove builtin_compiler

[Chad Versace]

On 09/13/2013 07:15 AM, Adrian M Negreanu wrote:

On Fri, Sep 13, 2013 at 5:01 PM, Ian Romanick  wrote:

Getting rid of the builtin_compiler from the Android build is a good
idea... but I'll let Chad or someone who knows /anything/ about the
Android build system actually review the patch.

Acked-by: Ian Romanick 

I don't think I want to remove any of the other bits of the builtin
compiler or standalone compiler.  I've been working on resurrecting the
standalone compiler for use as a developer tool.

http://cgit.freedesktop.org/~idr/mesa/log/?h=standalone-compiler



(add Chad to cc:)
The parts removed from Android.gen.mk were using generate_builtins.py,
which was also removed; that will trigger a compile failure.


This patch looks correct to me, though I didn't test it.
Reviewed-by: Chad Versace 
I'll commit it now.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 08/24] glsl: Add new atomic_uint built-in GLSL type.

[Ian Romanick]

On 09/17/2013 02:18 PM, Paul Berry wrote:
> On 15 September 2013 00:10, Francisco Jerez  > wrote:
> 
> ---
>  src/glsl/ast_to_hir.cpp  |  1 +
>  src/glsl/builtin_type_macros.h   |  2 ++
>  src/glsl/builtin_types.cpp   |  6 ++
>  src/glsl/glsl_types.cpp  |  2 ++
>  src/glsl/glsl_types.h| 14 ++
>  src/glsl/ir_clone.cpp|  1 +
>  src/glsl/link_uniform_initializers.cpp   |  1 +
>  src/glsl/tests/uniform_initializer_utils.cpp |  3 +++
>  src/mesa/program/ir_to_mesa.cpp  |  2 ++
>  src/mesa/state_tracker/st_glsl_to_tgsi.cpp   |  1 +
>  10 files changed, 33 insertions(+)
> 
> diff --git a/src/glsl/ast_to_hir.cpp b/src/glsl/ast_to_hir.cpp
> index 2316cf8..fcca5df 100644
> --- a/src/glsl/ast_to_hir.cpp
> +++ b/src/glsl/ast_to_hir.cpp
> @@ -902,6 +902,7 @@ do_comparison(void *mem_ctx, int operation,
> ir_rvalue *op0, ir_rvalue *op1)
> case GLSL_TYPE_VOID:
> case GLSL_TYPE_SAMPLER:
> case GLSL_TYPE_INTERFACE:
> +   case GLSL_TYPE_ATOMIC_UINT:
>/* I assume a comparison of a struct containing a sampler just
> * ignores the sampler present in the type.
> */
> diff --git a/src/glsl/builtin_type_macros.h
> b/src/glsl/builtin_type_macros.h
> index fec38da..263fd83 100644
> --- a/src/glsl/builtin_type_macros.h
> +++ b/src/glsl/builtin_type_macros.h
> @@ -110,6 +110,8 @@ DECL_TYPE(sampler2DRectShadow,  
>  GL_SAMPLER_2D_RECT_SHADOW,GLSL_SAMPLER
> 
>  DECL_TYPE(samplerExternalOES, GL_SAMPLER_EXTERNAL_OES,
>  GLSL_SAMPLER_DIM_EXTERNAL, 0, 0, GLSL_TYPE_FLOAT)
> 
> +DECL_TYPE(atomic_uint, GL_UNSIGNED_INT_ATOMIC_COUNTER,
> GLSL_TYPE_ATOMIC_UINT, 1, 1)
> +
>  STRUCT_TYPE(gl_DepthRangeParameters)
>  STRUCT_TYPE(gl_PointParameters)
>  STRUCT_TYPE(gl_MaterialParameters)
> diff --git a/src/glsl/builtin_types.cpp b/src/glsl/builtin_types.cpp
> index 722eda2..8311a91 100644
> --- a/src/glsl/builtin_types.cpp
> +++ b/src/glsl/builtin_types.cpp
> @@ -203,6 +203,8 @@ const static struct builtin_type_versions {
> T(sampler2DRectShadow, 140, 999)
> 
> T(struct_gl_DepthRangeParameters,  110, 100)
> +
> +   T(atomic_uint, 130, 999)
> 
> 
> This should be the GLSL version in which the type became available
> without the use of an extension, so it should be 420, not 130.
>  
> 
>  };
> 
>  const glsl_type *const deprecated_types[] = {
> @@ -284,5 +286,9 @@ _mesa_glsl_initialize_types(struct
> _mesa_glsl_parse_state *state)
> if (state->OES_texture_3D_enable) {
>add_type(symbols, glsl_type::sampler3D_type);
> }
> +
> +   if (state->ARB_shader_atomic_counters_enable) {
> +  add_type(symbols, glsl_type::atomic_uint_type);
> +   }
>  }
>  /** @} */
> diff --git a/src/glsl/glsl_types.cpp b/src/glsl/glsl_types.cpp
> index 3c396dd..e1fe153 100644
> --- a/src/glsl/glsl_types.cpp
> +++ b/src/glsl/glsl_types.cpp
> @@ -586,6 +586,7 @@ glsl_type::component_slots() const
>return this->length * this->fields.array->component_slots();
> 
> case GLSL_TYPE_SAMPLER:
> +   case GLSL_TYPE_ATOMIC_UINT:
> case GLSL_TYPE_VOID:
> case GLSL_TYPE_ERROR:
>break;
> @@ -874,6 +875,7 @@ glsl_type::count_attribute_slots() const
>return this->length *
> this->fields.array->count_attribute_slots();
> 
> case GLSL_TYPE_SAMPLER:
> +   case GLSL_TYPE_ATOMIC_UINT:
> case GLSL_TYPE_VOID:
> case GLSL_TYPE_ERROR:
>break;
> diff --git a/src/glsl/glsl_types.h b/src/glsl/glsl_types.h
> index acdf48f..d0274e6 100644
> --- a/src/glsl/glsl_types.h
> +++ b/src/glsl/glsl_types.h
> @@ -53,6 +53,7 @@ enum glsl_base_type {
> GLSL_TYPE_FLOAT,
> GLSL_TYPE_BOOL,
> GLSL_TYPE_SAMPLER,
> +   GLSL_TYPE_ATOMIC_UINT,
> GLSL_TYPE_STRUCT,
> GLSL_TYPE_INTERFACE,
> GLSL_TYPE_ARRAY,
> @@ -434,6 +435,19 @@ struct glsl_type {
> }
> 
> /**
> +* Return the amount of atomic counter storage required for a type.
> +*/
> +   unsigned atomic_size() const
> +   {
> +  if (base_type == GLSL_TYPE_ATOMIC_UINT)
> + return ATOMIC_COUNTER_SIZE;
> +  else if (is_array())
> + return length * element_type()->atomic_size();
> +  else
> + return 0;
> +   }
> 
> Can atomic counters appear inside structs?  If so, we probably need an
> is_record() case here.  If not, it would be nice to have a comment
> explaining why it's unnecessary.

The GL_ARB-shader_atomic_counter spec isn't clear, but the GL

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Eric Anholt]

Ian Romanick  writes:

> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
>> The _mesa_glsl_parse_state object relies on the memory allocator
>> zeroing out its contents before it's initialized, which seems rather
>> risky.  One of the following commits will homogenize implementations
>> of the new operator in a way that would break this assumption leaving
>> some of the member variables of this object uninitialized.
>
> This was an intentional design decision that we made, after a fair
> amount of consideration, in the early days of the compiler.  A number of
> factors came together:
>
>  * Use of ralloc et al. meant that we could never have stack-allocated
> objects.
>
>  * Use of rzalloc meant that objects would always be pre-allocated to zero.
>
>  * Uninitialized class member bugs are hard to track down.
>
>  * It's easy to forget to add 'this->foo = 0;' when you add a field foo
> to a class.
>
>  * A dozen lines of 'this->foo = 0;' is hard to properly review when a
> new class is added.  Are there actually 13 fields that need to be
> initialized?
>
>  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
>
> In the end, we decided to just rely on the allocator always providing
> cleared memory.
>
> I'm not very excited about adding a bunch of redundant code to the these
> constructors... especially since that code probably won't get maintained
> (e.g., when the next member gets added to those classes).
>
> I'd like to hear Ken and Eric weigh in.

Agreed completely.


pgpLmiSZnGJJt.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: Delete builtin_builder::shader when destroying built-ins.

[Eric Anholt]

Kenneth Graunke  writes:

> I would use _mesa_delete_shader, but it's declared static, and we don't
> really need any of the stuff in it anyway.
>
> This fixes a memory leak caught by Valgrind.
>
> Signed-off-by: Kenneth Graunke 

Reviewed-by: Eric Anholt 


pgp4R26YuTpnz.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 69541] New: ir_constant_expression.cpp(1384) : error C3861: 'isnormal': identifier not found

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=69541

  Priority: medium
Bug ID: 69541
  Keywords: regression
CC: matts...@gmail.com
  Assignee: mesa-dev@lists.freedesktop.org
   Summary: ir_constant_expression.cpp(1384) : error C3861:
'isnormal': identifier not found
  Severity: blocker
Classification: Unclassified
OS: Windows (All)
  Reporter: v...@freedesktop.org
  Hardware: x86-64 (AMD64)
Status: NEW
   Version: git
 Component: Mesa core
   Product: Mesa

mesa: 602d368446e7c97225f98429ffd54b53522c3b36 (master)


  Compiling src\glsl\ir_constant_expression.cpp ...
ir_constant_expression.cpp
src\glsl\ir_constant_expression.cpp(564) : warning C4244: '=' : conversion from
'int' to 'float', possible loss of data
src\glsl\ir_constant_expression.cpp(1384) : error C3861: 'isnormal': identifier
not found
src\glsl\ir_constant_expression.cpp(1385) : error C3861: 'copysign': identifier
not found


commit b2ab840130677bbe7b67de4727fcd91ee6506bb8
Author: Matt Turner 
Date:   Thu Aug 22 13:31:18 2013 -0700

glsl: Add support for ldexp.

v2: Drop frexp. Rebase on builtins rewrite.
Reviewed-by: Paul Berry 

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Paul Berry]

On 18 September 2013 07:43, Ian Romanick  wrote:

> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
> > The _mesa_glsl_parse_state object relies on the memory allocator
> > zeroing out its contents before it's initialized, which seems rather
> > risky.  One of the following commits will homogenize implementations
> > of the new operator in a way that would break this assumption leaving
> > some of the member variables of this object uninitialized.
>
> This was an intentional design decision that we made, after a fair
> amount of consideration, in the early days of the compiler.  A number of
> factors came together:
>
>  * Use of ralloc et al. meant that we could never have stack-allocated
> objects.
>
>  * Use of rzalloc meant that objects would always be pre-allocated to zero.
>
>  * Uninitialized class member bugs are hard to track down.
>
>  * It's easy to forget to add 'this->foo = 0;' when you add a field foo
> to a class.
>
>  * A dozen lines of 'this->foo = 0;' is hard to properly review when a
> new class is added.  Are there actually 13 fields that need to be
> initialized?
>
>  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
>
> In the end, we decided to just rely on the allocator always providing
> cleared memory.
>
> I'm not very excited about adding a bunch of redundant code to the these
> constructors... especially since that code probably won't get maintained
> (e.g., when the next member gets added to those classes).
>
> I'd like to hear Ken and Eric weigh in.
>

I think I have sufficient development experience and familiarity with the
code base to weigh in as well.  I fall on the opposite side of this
particular fence--I prefer to see class members initialized in the
constructor.  My reasoning is:

* Initializing class members in the constructor costs almost nothing and
decreases the risk of subtle and difficult-to-find bugs.

* Initializing class members in the constructor is common practice in C++,
and therefore less surprising to developers who are new to the Mesa code
base.

* If a class initializes all its members in the constructor, we have the
freedom to allocate it in whatever way is appropriate (stack, rzalloc,
malloc, new operator, or placement new, to name a few examples).  If it
doesn't, we have no choice but to allocate it using rzalloc.

* There's no compile-time mechanism to ensure that all uses of a class use
rzalloc (nor are there any static analysis tools I'm aware of that are
capable of ensuring this).  It's also a pain to verify using grep.  So if a
class doesn't initialize all its members in the constructor, we have to be
vigilant in our code review to make sure that all present and future users
of the class use rzalloc.

* By contrast, Coverity *is* able to ensure that class members are
initialized in the constructor, and I suspect other static analysis tools
are too.


>
> > ---
> >  src/glsl/glsl_parser_extras.cpp | 16 ++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/src/glsl/glsl_parser_extras.cpp
> b/src/glsl/glsl_parser_extras.cpp
> > index cac5a18..772933f 100644
> > --- a/src/glsl/glsl_parser_extras.cpp
> > +++ b/src/glsl/glsl_parser_extras.cpp
> > @@ -55,7 +55,7 @@ static unsigned known_desktop_glsl_versions[] =
> >
> >  _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
> >  GLenum target, void
> *mem_ctx)
> > - : ctx(_ctx)
> > +   : ctx(_ctx), switch_state()
> >  {
> > switch (target) {
> > case GL_VERTEX_SHADER:   this->target = vertex_shader; break;
> > @@ -66,10 +66,14 @@
> _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
> > this->scanner = NULL;
> > this->translation_unit.make_empty();
> > this->symbols = new(mem_ctx) glsl_symbol_table;
> > +
> > +   this->num_uniform_blocks = 0;
> > +   this->uniform_block_array_size = 0;
> > +   this->uniform_blocks = NULL;
> > +
> > this->info_log = ralloc_strdup(mem_ctx, "");
> > this->error = false;
> > this->loop_nesting_ast = NULL;
> > -   this->switch_state.switch_nesting_ast = NULL;
> >
> > this->struct_specifier_depth = 0;
> > this->num_builtins_to_link = 0;
> > @@ -105,6 +109,13 @@
> _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
> >
> > this->Const.MaxDrawBuffers = ctx->Const.MaxDrawBuffers;
> >
> > +   this->current_function = NULL;
> > +   this->toplevel_ir = NULL;
> > +   this->found_return = false;
> > +   this->all_invariant = false;
> > +   this->user_structures = NULL;
> > +   this->num_user_structures = 0;
> > +
> > /* Populate the list of supported GLSL versions */
> > /* FINISHME: Once the OpenGL 3.0 'forward compatible' context or
> >  * the OpenGL 3.2 Core context is supported, this logic will need
> > @@ -163,6 +174,7 @@
> _mesa_glsl_parse_state::_mesa_glsl_parse_state(struct gl_context *_ctx,
> >
> > this->gs_input_prim_type_specified = false;
> > this->gs_input_prim_

Re: [Mesa-dev] [PATCH 08/24] glsl: Add new atomic_uint built-in GLSL type.

[Francisco Jerez]

Paul Berry  writes:

> On 17 September 2013 14:14, Francisco Jerez  wrote:
>
>> Paul Berry  writes:
>>
>> > On 17 September 2013 12:18, Paul Berry  wrote:
>> >[...]
>> > I also notice that most of your uses of atomic_size() in this patch
>> series
>> > are merely to see whether the type contains any atomics.  You might
>> > consider adding a contains_atomic() function (like we've already done
>> with
>> > contains_sampler() and contains_integer()) just for code clarity.
>>
>> Not really, we also need the actual size in a couple of places, like
>> ast_to_hir.cpp:2256, link_atomics.cpp:80 and link_atomics.cpp:123.
>>
>> Thanks.
>>
>
> I wasn't suggesting replacing it.  I was suggesting adding a
> contains_atomic() function so that in those places where we don't need the
> actual size (which are the majority of cases) the code would be clearer.
>
> contains_atomic() might just be an inline function that calls atomic_size().

Oh, right...  I will do that.

Thank you.


pgpPERS1vpeme.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Francisco Jerez]

Ian Romanick  writes:

> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
>> The _mesa_glsl_parse_state object relies on the memory allocator
>> zeroing out its contents before it's initialized, which seems rather
>> risky.  One of the following commits will homogenize implementations
>> of the new operator in a way that would break this assumption leaving
>> some of the member variables of this object uninitialized.
>
> This was an intentional design decision that we made, after a fair
> amount of consideration, in the early days of the compiler.  A number of
> factors came together:
>
>  * Use of ralloc et al. meant that we could never have stack-allocated
> objects.
>
Do you mean it was an intentional decision not to support allocation of
objects from the stack?  In some cases stack allocation is the most
efficient and least error-prone -- Like, in cases where the expected
lifetime of the object you're allocating is precisely the function or
the block you're allocating it from.

I'd vote for lifting this restriction.  In fact it seems you're already
allocating a number of ralloc-allocated objects from the stack in
several cases -- E.g. cfg_t, fs_live_variables, vec4_live_variables as
Paul pointed out.

>  * Use of rzalloc meant that objects would always be pre-allocated to zero.
>
The problem is that this moves part of the initialization semantics of
your classes into the memory allocator, which is problematical in cases
where you would like to use different allocation schemes with the same
object.  It's a common assumption that you're allowed to allocate
constructible types as you please, be it on the stack, within other
non-rzalloc'ed classes and structures, or with any variant of new, new()
or new[].

If we *really* want to break this assumption we should probably declare
all constructors private and use factory methods to enforce rzalloc as
the only usable allocator.

>  * Uninitialized class member bugs are hard to track down.
>
>  * It's easy to forget to add 'this->foo = 0;' when you add a field foo
> to a class.
>
nIt's the first thing any reasonably experienced C++ programmer would
think about when adding a new field to an existing class -- BTW, I
believe in many cases it's more convenient to use the 'foo()'
initialization syntax when you want to zero-initialize a large number of
member variables.

>  * A dozen lines of 'this->foo = 0;' is hard to properly review when a
> new class is added.  Are there actually 13 fields that need to be
> initialized?
>
>  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
>
You can also 'memset(0)' your class from its constructor if it's POD --
though it seems to me that manually initializing all fields to some
specific value is still the clearest way to communicate to the reader
what the intended initial value of some field was.

Thank you for your review.

> In the end, we decided to just rely on the allocator always providing
> cleared memory.
>
> I'm not very excited about adding a bunch of redundant code to the these
> constructors... especially since that code probably won't get maintained
> (e.g., when the next member gets added to those classes).
>
> I'd like to hear Ken and Eric weigh in.
>[...]


pgp4thrV7KVSe.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Francisco Jerez]

Ian Romanick  writes:

> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
>>[...]
>>  
>>  
>> +#include 
>> +#include 
>
> No.  We are not using STL or templates.
>

Any reason for it to be still so?

Actually I started implementing this patch using one of mesa's hash
tables, but at some point I realized I was cluttering the code with
pointer casts, awkward syntax and memory management code spread over
several different places around _mesa_glsl_parse_state.  I'm fine either
way, really, I can revert to my earlier code if you really want -- It's
just not going to be especially fun for whoever gets to maintain it.


pgpCifnhHelY6.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] mt->first_level != BaseLevel

[Eric Anholt]

Until now, whenever someone adjusts BaseLevel, we copy around the miptree
contents to a new miptree that starts from BaseLevel, which is pretty
awful when you think about things like generating mipmaps using FBOs and
BaseLevel/MaxLevel (which our glGenerateMipmap() does).  Disentangling
this code has been hard, though.

I've been on a mission to kill mt->first_level for a while.  It doesn't
fit with other drivers, which has blocked any plans I've come up with to
share miptree management code between drivers (which it seems obvious to
me we ought to be doing, given how common they all look).  As of this
series, the only source of mt->first_level != 0 should be
intel_renderbuffer_move_to_temp() callers.

After this, I think I can extract a series to do color render target
LOD/array index on gen4-6, removing another first_level != 0 case, and
then I get to try and finally figure out why gen6 non-hiz is broken so we
can do depth LOD != 0.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/9] i965: Always look up from the object's mt when setting up texturing state.

[Eric Anholt]

We know that the object's mt is equal to the firstimage's mt because it's
gone through intel_finalize_mipmap_tree().  Saves a lookup of firstimage
on pre-gen7.
---
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c  | 4 +---
 src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 2 +-
 2 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
index 8d87786..b856e24 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -252,8 +252,6 @@ brw_update_texture_surface(struct gl_context *ctx,
struct gl_texture_object *tObj = ctx->Texture.Unit[unit]._Current;
struct intel_texture_object *intelObj = intel_texture_object(tObj);
struct intel_mipmap_tree *mt = intelObj->mt;
-   struct gl_texture_image *firstImage = tObj->Image[0][tObj->BaseLevel];
-   struct intel_texture_image *intel_image = intel_texture_image(firstImage);
struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
uint32_t *surf;
 
@@ -276,7 +274,7 @@ brw_update_texture_surface(struct gl_context *ctx,
 
surf[1] = intelObj->mt->region->bo->offset + intelObj->mt->offset; /* reloc 
*/
 
-   surf[2] = ((intelObj->_MaxLevel - intel_image->mt->first_level) << 
BRW_SURFACE_LOD_SHIFT |
+   surf[2] = ((intelObj->_MaxLevel - mt->first_level) << BRW_SURFACE_LOD_SHIFT 
|
  (mt->logical_width0 - 1) << BRW_SURFACE_WIDTH_SHIFT |
  (mt->logical_height0 - 1) << BRW_SURFACE_HEIGHT_SHIFT);
 
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c 
b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
index 37e3174..531a1a2 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
@@ -326,7 +326,7 @@ gen7_update_texture_surface(struct gl_context *ctx,
 
surf[5] = (SET_FIELD(GEN7_MOCS_L3, GEN7_SURFACE_MOCS) |
   /* mip count */
-  (intelObj->_MaxLevel - intel_image->mt->first_level));
+  (intelObj->_MaxLevel - mt->first_level));
 
if (brw->is_haswell) {
   /* Handling GL_ALPHA as a surface format override breaks 1.30+ style
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/9] i965: Totally switch around how we handle nonzero baselevel-first_level.

[Eric Anholt]

This has no effect currently, because intel_finalize_mipmap_tree() always
makes mt->first_level == tObj->BaseLevel.

The change I made before to handle it
(b1080cfbdb0a084122fcd662cd27b4748c5598fd) got very close to working, but
after fixing some unrelated bugs in the series, it still left
tex-miplevel-selection producing errors when testing textureLod().  The
problem is that for explicit LODs, the sampler's LOD clamping is ignored,
and only the surface's MIP clamping is respected.  So we need to use
surface mip clamping, which applies on top of the sampler's mip clamping,
so the sampler change gets backed out.

Now actually tested with a non-regressing series producing a non-zero
computed baselevel.
---
 src/mesa/drivers/dri/i965/brw_wm_sampler_state.c  | 11 +++
 src/mesa/drivers/dri/i965/brw_wm_surface_state.c  |  5 +++--
 src/mesa/drivers/dri/i965/gen7_sampler_state.c| 11 +++
 src/mesa/drivers/dri/i965/gen7_wm_surface_state.c |  4 +++-
 4 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c 
b/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
index 568dcb5..b716d61 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_sampler_state.c
@@ -202,8 +202,6 @@ static void brw_update_sampler_state(struct brw_context 
*brw,
struct gl_context *ctx = &brw->ctx;
struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
struct gl_texture_object *texObj = texUnit->_Current;
-   struct intel_texture_image *intel_image =
-  intel_texture_image(texObj->Image[0][texObj->BaseLevel]);
struct gl_sampler_object *gl_sampler = _mesa_get_samplerobj(ctx, unit);
bool using_nearest = false;
 
@@ -322,13 +320,10 @@ static void brw_update_sampler_state(struct brw_context 
*brw,
sampler->ss0.lod_preclamp = 1; /* OpenGL mode */
sampler->ss0.default_color_mode = 0; /* OpenGL/DX10 mode */
 
-   int baselevel = texObj->BaseLevel - intel_image->mt->first_level;
-   sampler->ss0.base_level = U_FIXED(baselevel, 1);
+   sampler->ss0.base_level = U_FIXED(0, 1);
 
-   sampler->ss1.max_lod = U_FIXED(CLAMP(baselevel +
-gl_sampler->MaxLod, 0, 13), 6);
-   sampler->ss1.min_lod = U_FIXED(CLAMP(baselevel +
-gl_sampler->MinLod, 0, 13), 6);
+   sampler->ss1.max_lod = U_FIXED(CLAMP(gl_sampler->MaxLod, 0, 13), 6);
+   sampler->ss1.min_lod = U_FIXED(CLAMP(gl_sampler->MinLod, 0, 13), 6);
 
/* On Gen6+, the sampler can handle non-normalized texture
 * rectangle coordinates natively
diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c 
b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
index b856e24..ba77284 100644
--- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c
@@ -274,7 +274,7 @@ brw_update_texture_surface(struct gl_context *ctx,
 
surf[1] = intelObj->mt->region->bo->offset + intelObj->mt->offset; /* reloc 
*/
 
-   surf[2] = ((intelObj->_MaxLevel - mt->first_level) << BRW_SURFACE_LOD_SHIFT 
|
+   surf[2] = ((intelObj->_MaxLevel - tObj->BaseLevel) << BRW_SURFACE_LOD_SHIFT 
|
  (mt->logical_width0 - 1) << BRW_SURFACE_WIDTH_SHIFT |
  (mt->logical_height0 - 1) << BRW_SURFACE_HEIGHT_SHIFT);
 
@@ -283,7 +283,8 @@ brw_update_texture_surface(struct gl_context *ctx,
  (intelObj->mt->region->pitch - 1) <<
  BRW_SURFACE_PITCH_SHIFT);
 
-   surf[4] = brw_get_surface_num_multisamples(intelObj->mt->num_samples);
+   surf[4] = (brw_get_surface_num_multisamples(intelObj->mt->num_samples) |
+  SET_FIELD(tObj->BaseLevel - mt->first_level, 
BRW_SURFACE_MIN_LOD));
 
surf[5] = mt->align_h == 4 ? BRW_SURFACE_VERTICAL_ALIGN_ENABLE : 0;
 
diff --git a/src/mesa/drivers/dri/i965/gen7_sampler_state.c 
b/src/mesa/drivers/dri/i965/gen7_sampler_state.c
index d796fb5..968c410 100644
--- a/src/mesa/drivers/dri/i965/gen7_sampler_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_sampler_state.c
@@ -41,8 +41,6 @@ gen7_update_sampler_state(struct brw_context *brw, int unit, 
int ss_index,
struct gl_context *ctx = &brw->ctx;
struct gl_texture_unit *texUnit = &ctx->Texture.Unit[unit];
struct gl_texture_object *texObj = texUnit->_Current;
-   struct intel_texture_image *intel_image =
-  intel_texture_image(texObj->Image[0][texObj->BaseLevel]);
struct gl_sampler_object *gl_sampler = _mesa_get_samplerobj(ctx, unit);
bool using_nearest = false;
 
@@ -153,13 +151,10 @@ gen7_update_sampler_state(struct brw_context *brw, int 
unit, int ss_index,
sampler->ss0.lod_preclamp = 1; /* OpenGL mode */
sampler->ss0.default_color_mode = 0; /* OpenGL/DX10 mode */
 
-   int baselevel = texObj->BaseLevel - intel_image->mt->first_level;
-   sampler->ss0.base_level = U_FIXED(baselevel, 1);
+   sampler->ss0.base_level = U_FIXED(0, 1);
 
-   sampler->ss1.max_lod = U_FIXED(CLAMP(baselevel +
-   

[Mesa-dev] [PATCH 7/9] i965: Always allocate validated miptrees from level 0.

[Eric Anholt]

No change in copies during a piglit run, but it's one less first_level !=
0 in our codebase.
---
 src/mesa/drivers/dri/i965/intel_tex_validate.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c 
b/src/mesa/drivers/dri/i965/intel_tex_validate.c
index 42533bb..e44c3ca 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
@@ -99,16 +99,15 @@ intel_finalize_mipmap_tree(struct brw_context *brw, GLuint 
unit)
   intel_miptree_get_dimensions_for_image(&firstImage->base.Base,
 &width, &height, &depth);
 
-  perf_debug("Creating new %s %dx%dx%d %d..%d miptree to handle finalized "
- "texture miptree.\n",
+  perf_debug("Creating new %s %dx%dx%d %d-level miptree to handle "
+ "finalized texture miptree.\n",
  _mesa_get_format_name(firstImage->base.Base.TexFormat),
- width, height, depth,
- validate_first_level, validate_last_level);
+ width, height, depth, validate_last_level + 1);
 
   intelObj->mt = intel_miptree_create(brw,
   intelObj->base.Target,
  firstImage->base.Base.TexFormat,
-  validate_first_level,
+  0, /* first_level */
   validate_last_level,
   width,
   height,
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 5/9] i965: Don't allocate a 1-level texture when GL_GENERATE_MIPMAP is set.

[Eric Anholt]

Given that a teximage that calls us with this flag set will immediately
proceed to allocate the other levels, we can probably just go ahead and
allocate those levels now.

Reduces miptree copies in piglit by about .05%.
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index fe274bf..16fed95 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -60,7 +60,8 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
 */
if ((intelObj->base.Sampler.MinFilter == GL_NEAREST ||
 intelObj->base.Sampler.MinFilter == GL_LINEAR) &&
-   intelImage->base.Base.Level == 0) {
+   intelImage->base.Base.Level == 0 &&
+   !intelObj->base.GenerateMipmap) {
   lastLevel = 0;
} else {
   lastLevel = _mesa_get_tex_max_num_levels(intelObj->base.Target,
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/17] mesa: Get GL_MAX_VARYING_FLOATS_ARB from VertexProgram.MaxOutputComponents

[Ian Romanick]

On 09/13/2013 01:45 PM, Paul Berry wrote:
> On 11 September 2013 16:28, Ian Romanick  > wrote:
> 
> On 09/11/2013 04:05 PM, Paul Berry wrote:
> > On 10 September 2013 12:10, Ian Romanick  
> > >> wrote:
> >
> > From: Ian Romanick  
> >  >>
> >
> > Signed-off-by: Ian Romanick  
> >  >>
> > ---
> >  src/mesa/main/get.c  | 4 
> >  src/mesa/main/get_hash_params.py | 2 +-
> >  2 files changed, 1 insertion(+), 5 deletions(-)
> >
> > diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
> > index 34eb6be..ae45bf8 100644
> > --- a/src/mesa/main/get.c
> > +++ b/src/mesa/main/get.c
> > @@ -718,10 +718,6 @@ find_custom_value(struct gl_context *ctx,
> const
> > struct value_desc *d, union valu
> >ASSERT(v->value_int_n.n <=
> ARRAY_SIZE(v->value_int_n.ints));
> >break;
> >
> > -   case GL_MAX_VARYING_FLOATS_ARB:
> > -  v->value_int = ctx->Const.MaxVarying * 4;
> > -  break;
> > -
> > /* Various object names */
> >
> > case GL_TEXTURE_BINDING_1D:
> > diff --git a/src/mesa/main/get_hash_params.py
> > b/src/mesa/main/get_hash_params.py
> > index c0dbf45..3d47443 100644
> > --- a/src/mesa/main/get_hash_params.py
> > +++ b/src/mesa/main/get_hash_params.py
> > @@ -365,7 +365,7 @@ descriptor=[
> >
> >  # GL_ARB_vertex_shader
> >[ "MAX_VERTEX_UNIFORM_COMPONENTS_ARB",
> > "CONTEXT_INT(Const.VertexProgram.MaxUniformComponents),
> > extra_ARB_vertex_shader" ],
> > -  [ "MAX_VARYING_FLOATS_ARB", "LOC_CUSTOM, TYPE_INT, 0,
> > extra_ARB_vertex_shader" ],
> > +  [ "MAX_VARYING_FLOATS_ARB",
> > "CONTEXT_INT(Const.VertexProgram.MaxOutputComponents),
> > extra_ARB_vertex_shader" ],
> >
> >  # GL_EXT_framebuffer_blit
> >  # NOTE: GL_DRAW_FRAMEBUFFER_BINDING_EXT ==
> GL_FRAMEBUFFER_BINDING_EXT
> > --
> > 1.8.1.4
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> 
>  >
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> > Doesn't MAX_VARYING_FLOATS need to be
> MIN2(MAX_VERTEX_OUTPUT_COMPONENTS,
> > MAX_FRAGMENT_INPUT_COMPONENTS)?  I can imagine an implementation where
> > MAX_FRAGMENT_INPUT_COMPONENTS is the smaller constraint (in fact,
> ES3's
> > minimum maximums constitute just such a case).
> 
> It's all so much more horrible than you think. :)
> 
> OpenGL 3.2 sets:
> 
> MAX_VARYING_COMPONENTS: 60
> MAX_VERTEX_OUTPUT_COMPONENTS: 64
> MAX_FRAGMENT_INPUT_COMPONENTS: 128
> 
> OpenGL ES 3.0 sets:
> 
> MAX_VARYING_COMPONENTS: 60
> MAX_VARYING_VECTORS: 15
> MAX_VERTEX_OUTPUT_VECTORS: 16
> MAX_VERTEX_OUTPUT_COMPONENTS: 64
> MAX_FRAGMENT_INPUT_VECTORS: 15
> MAX_FRAGMENT_INPUT_COMPONENTS: 60
> 
> 
> Oddly, I don'tsee MAX_VERTEX_OUTPUT_VECTORS or
> MAX_FRAGMENT_INPUT_VECTORS in the GLES 3 spec; only MAX_VARYING_VECTORS.

Brilliant.  The API spec uses GL_MAX_*_COMPONENTS, but the GLSL built-in
variables are gl_Max*Vectors.

> BUT the description of MAX_VARYING_COMPONENTS in GLES3 says:
> 
> "Number of components for output variables."
> 
> So... OpenGL 3.2 makes no sense, and OpenGL ES 3.0 seems self
> contradictory.  The core problem seems to be the way that each API (and
> each driver) counts gl_Position and gl_FragCoord.  Some say it's
> counted, some say it's not counted, and some say it might be counted.
> Our existing infrastructure may not be sufficient to handle all
> combinations of those cases.
> 
> 
> I've spent a while digging through the specs, and I actually don't think
> it's quite as complicated or counterintuitive as it sounded at first.
> 
> First of all, we don't need to worry about the distinction between
> COMPONENTS and VECTORS, because MAX_foo_COMPONENTS always equals
> MAX_foo_VECTORS * 4.  Similarly, there's no need to distinguish between
> MAX_VARYING_COMPONENTS and MAX_VARYING_FLOATS.

Right.  In fact, the differing names have the same enum values.

> As for the question of whether gl_Position and gl_FragCoord count
> against these limits, all GL specs since 2.0, and all GLES specs since
> 2.0 seem to agree that 

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Ian Romanick]

On 09/18/2013 01:35 PM, Paul Berry wrote:
> On 18 September 2013 07:43, Ian Romanick  > wrote:
> 
> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
> > The _mesa_glsl_parse_state object relies on the memory allocator
> > zeroing out its contents before it's initialized, which seems rather
> > risky.  One of the following commits will homogenize implementations
> > of the new operator in a way that would break this assumption leaving
> > some of the member variables of this object uninitialized.
> 
> This was an intentional design decision that we made, after a fair
> amount of consideration, in the early days of the compiler.  A number of
> factors came together:
> 
>  * Use of ralloc et al. meant that we could never have stack-allocated
> objects.
> 
>  * Use of rzalloc meant that objects would always be pre-allocated
> to zero.
> 
>  * Uninitialized class member bugs are hard to track down.
> 
>  * It's easy to forget to add 'this->foo = 0;' when you add a field foo
> to a class.
> 
>  * A dozen lines of 'this->foo = 0;' is hard to properly review when a
> new class is added.  Are there actually 13 fields that need to be
> initialized?
> 
>  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
> 
> In the end, we decided to just rely on the allocator always providing
> cleared memory.
> 
> I'm not very excited about adding a bunch of redundant code to the these
> constructors... especially since that code probably won't get maintained
> (e.g., when the next member gets added to those classes).
> 
> I'd like to hear Ken and Eric weigh in.
> 
> 
> I think I have sufficient development experience and familiarity with
> the code base to weigh in as well.  I fall on the opposite side of this
> particular fence--I prefer to see class members initialized in the
> constructor.  My reasoning is:
> 
> * Initializing class members in the constructor costs almost nothing and
> decreases the risk of subtle and difficult-to-find bugs.

Except that we forget to add the initialization almost as often as we
remember.  I see from the patch that some of the fields lacking
initializers are ones you most likely added... :)

> * Initializing class members in the constructor is common practice in
> C++, and therefore less surprising to developers who are new to the Mesa
> code base.
> 
> * If a class initializes all its members in the constructor, we have the
> freedom to allocate it in whatever way is appropriate (stack, rzalloc,
> malloc, new operator, or placement new, to name a few examples).  If it
> doesn't, we have no choice but to allocate it using rzalloc.
> 
> * There's no compile-time mechanism to ensure that all uses of a class
> use rzalloc (nor are there any static analysis tools I'm aware of that
> are capable of ensuring this).  It's also a pain to verify using grep. 
> So if a class doesn't initialize all its members in the constructor, we
> have to be vigilant in our code review to make sure that all present and
> future users of the class use rzalloc.

As far as I'm aware, our use of overloaded new / delete with rzalloc
forces us to use new (and therefore rzalloc) for every object or die in
a fire.  None of the destructors deallocate any objects owned by them.
Even if we did switch to a different allocation scheme, we would leak
like a screen door on a submarine.

> * By contrast, Coverity *is* able to ensure that class members are
> initialized in the constructor, and I suspect other static analysis
> tools are too.

Relying on Vinson to run Coverity and submit bugs is not a solid plan.
We have no other plan involving static analysis tools, and we don't have
the staff to filter the flood of false positives that ever static
analysis tool I've ever encountered generates.

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965/gen4: Fix fragment program rectangle texture shadow compares.

[Eric Anholt]

The rescale_texcoord(), if it does something, will return just the
GLSL-sized coordinate, leaving out the 3rd and 4th components where we
were storing our projected shadow compare and the texture projector.
Deref the shadow compare before using the shared rescale-the-coordinate
code to fix the problem.

Fixes piglit tex-shadow2drect.shader_test and txp-shadow2drect.shader_test

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69525
NOTE: This is a candidate for stable branches.
---
 src/mesa/drivers/dri/i965/brw_fs_fp.cpp | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp 
b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
index 68531e3..0594948 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
+++ b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
@@ -490,15 +490,15 @@ fs_visitor::emit_fragment_program_code()
  ir_constant_data junk_data;
  ir->coordinate = new(mem_ctx) ir_constant(coordinate_type, 
&junk_data);
 
- coordinate = rescale_texcoord(ir, coordinate,
-   fpi->TexSrcTarget == TEXTURE_RECT_INDEX,
-   fpi->TexSrcUnit, fpi->TexSrcUnit);
-
  if (fpi->TexShadow) {
 shadow_c = regoffset(coordinate, 2);
 ir->shadow_comparitor = new(mem_ctx) ir_constant(0.0f);
  }
 
+ coordinate = rescale_texcoord(ir, coordinate,
+   fpi->TexSrcTarget == TEXTURE_RECT_INDEX,
+   fpi->TexSrcUnit, fpi->TexSrcUnit);
+
  fs_inst *inst;
  if (brw->gen >= 7) {
 inst = emit_texture_gen7(ir, dst, coordinate, shadow_c, lod, dpdy, 
sample_index);
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 8/9] i965: Add missing license to intel_tex_validate.c.

[Eric Anholt]

I've rewritten a lot of this file.
---
 src/mesa/drivers/dri/i965/intel_tex_validate.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c 
b/src/mesa/drivers/dri/i965/intel_tex_validate.c
index e44c3ca..0b1bfe6 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
@@ -1,3 +1,26 @@
+/*
+ * Copyright © 2013 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
 #include "main/mtypes.h"
 #include "main/macros.h"
 #include "main/samplerobj.h"
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 6/9] i965: Don't relayout a texture just for baselevel changes.

[Eric Anholt]

As long as the baselevel, maxlevel still sit inside the range we had
previously validated, there's no need to reallocate the texture.

I also hope this makes our texture validation logic much more obvious.
It's taken me enough tries to write this change, that's for sure.  Reduces
miptree copy count on a piglit run by 1.3%, though the change in amount of
data moved is much smaller.
---
 src/mesa/drivers/dri/i965/gen7_wm_surface_state.c |  1 -
 src/mesa/drivers/dri/i965/intel_tex_obj.h |  3 ++
 src/mesa/drivers/dri/i965/intel_tex_validate.c| 60 ++-
 3 files changed, 39 insertions(+), 25 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c 
b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
index 115b296..60d26ab 100644
--- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
+++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c
@@ -282,7 +282,6 @@ gen7_update_texture_surface(struct gl_context *ctx,
struct intel_texture_object *intelObj = intel_texture_object(tObj);
struct intel_mipmap_tree *mt = intelObj->mt;
struct gl_texture_image *firstImage = tObj->Image[0][tObj->BaseLevel];
-   struct intel_texture_image *intel_image = intel_texture_image(firstImage);
struct gl_sampler_object *sampler = _mesa_get_samplerobj(ctx, unit);
 
if (tObj->Target == GL_TEXTURE_BUFFER) {
diff --git a/src/mesa/drivers/dri/i965/intel_tex_obj.h 
b/src/mesa/drivers/dri/i965/intel_tex_obj.h
index e30dd8a..2d783c3 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_obj.h
+++ b/src/mesa/drivers/dri/i965/intel_tex_obj.h
@@ -41,6 +41,9 @@ struct intel_texture_object
 */
unsigned int _MaxLevel;
 
+   unsigned int validated_first_level;
+   unsigned int validated_last_level;
+
/* On validation any active images held in main memory or in other
 * regions will be copied to this region and the old storage freed.
 */
diff --git a/src/mesa/drivers/dri/i965/intel_tex_validate.c 
b/src/mesa/drivers/dri/i965/intel_tex_validate.c
index c34c144..42533bb 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_validate.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_validate.c
@@ -11,31 +11,32 @@
 #define FILE_DEBUG_FLAG DEBUG_TEXTURE
 
 /**
- * When validating, we only care about the texture images that could
- * be seen, so for non-mipmapped modes we want to ignore everything
- * but BaseLevel.
+ * Sets our driver-specific variant of tObj->_MaxLevel for later surface state
+ * upload.
+ *
+ * If we're only ensuring that there is storage for the first miplevel of a
+ * texture, then in texture setup we're going to have to make sure we don't
+ * allow sampling beyond level 0.
  */
 static void
 intel_update_max_level(struct intel_texture_object *intelObj,
   struct gl_sampler_object *sampler)
 {
struct gl_texture_object *tObj = &intelObj->base;
-   int maxlevel;
 
if (sampler->MinFilter == GL_NEAREST ||
sampler->MinFilter == GL_LINEAR) {
-  maxlevel = tObj->BaseLevel;
+  intelObj->_MaxLevel = tObj->BaseLevel;
} else {
-  maxlevel = tObj->_MaxLevel;
-   }
-
-   if (intelObj->_MaxLevel != maxlevel) {
-  intelObj->_MaxLevel = maxlevel;
-  intelObj->needs_validate = true;
+  intelObj->_MaxLevel = tObj->_MaxLevel;
}
 }
 
-/*  
+/**
+ * At rendering-from-a-texture time, make sure that the texture object has a
+ * miptree that can hold the entire texture based on
+ * BaseLevel/MaxLevel/filtering, and copy in any texture images that are
+ * stored in other miptrees.
  */
 GLuint
 intel_finalize_mipmap_tree(struct brw_context *brw, GLuint unit)
@@ -53,18 +54,26 @@ intel_finalize_mipmap_tree(struct brw_context *brw, GLuint 
unit)
if (tObj->Target == GL_TEXTURE_BUFFER)
   return true;
 
-   /* We know/require this is true by now: 
+   /* We know that this is true by now, and if it wasn't, we might have
+* mismatched level sizes and the copies would fail.
 */
assert(intelObj->base._BaseComplete);
 
-   /* What levels must the tree include at a minimum?
-*/
intel_update_max_level(intelObj, sampler);
-   if (intelObj->mt && intelObj->mt->first_level != tObj->BaseLevel)
-  intelObj->needs_validate = true;
 
-   if (!intelObj->needs_validate)
+   /* What levels does this validated texture image require? */
+   int validate_first_level = tObj->BaseLevel;
+   int validate_last_level = intelObj->_MaxLevel;
+
+   /* Skip the loop over images in the common case of no images having
+* changed.  But if the GL_BASE_LEVEL / GL_MAX_LEVEL change to something we
+* haven't looked at, then we do need to look at those new images.
+*/
+   if (!intelObj->needs_validate &&
+   validate_first_level >= intelObj->validated_first_level &&
+   validate_last_level <= intelObj->validated_last_level) {
   return true;
+   }
 
firstImage = intel_texture_image(tObj->Image[0][tObj->BaseLevel]);
 
@@ -78,8 +87,8 @@ intel_finalize_mipmap_tree(struct brw_context *brw, GLuint 
unit)
 */

[Mesa-dev] [PATCH 9/9] i965: Add a real native TexStorage path.

[Eric Anholt]

We originally had a path just did the loop and called
ctx->Driver.AllocTextureImageBuffer(), which I moved into Mesa core.  But
we can do better, avoiding incorrect miptree size guesses and later
texture validations by just directly allocating the miptree and setting it
to all the images.
---
 src/mesa/drivers/dri/i965/intel_tex.c | 65 +++
 1 file changed, 65 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_tex.c 
b/src/mesa/drivers/dri/i965/intel_tex.c
index eecffc9..5bc9d25 100644
--- a/src/mesa/drivers/dri/i965/intel_tex.c
+++ b/src/mesa/drivers/dri/i965/intel_tex.c
@@ -111,6 +111,70 @@ intel_alloc_texture_image_buffer(struct gl_context *ctx,
return true;
 }
 
+/**
+ * ctx->Driver.AllocTextureStorage() handler.
+ *
+ * Compare this to _mesa_alloc_texture_storage, which would call into
+ * intel_alloc_texture_image_buffer() above.
+ */
+static GLboolean
+intel_alloc_texture_storage(struct gl_context *ctx,
+struct gl_texture_object *texobj,
+GLsizei levels, GLsizei width,
+GLsizei height, GLsizei depth)
+{
+   struct brw_context *brw = brw_context(ctx);
+   struct intel_texture_object *intel_texobj = intel_texture_object(texobj);
+   struct gl_texture_image *first_image = texobj->Image[0][0];
+   int num_samples = intel_quantize_num_samples(brw->intelScreen,
+first_image->NumSamples);
+   const int numFaces = _mesa_num_tex_faces(texobj->Target);
+   int face;
+   int level;
+
+   printf("storage %d!\n", first_image->NumSamples);
+
+   /* If the object's current miptree doesn't match what we need, make a new
+* one.
+*/
+   if (!intel_texobj->mt ||
+   !intel_miptree_match_image(intel_texobj->mt, first_image) ||
+   intel_texobj->mt->last_level != levels - 1) {
+  intel_miptree_release(&intel_texobj->mt);
+  intel_texobj->mt = intel_miptree_create(brw, texobj->Target,
+  first_image->TexFormat,
+  0, levels - 1,
+  width, height, depth,
+  false, /* expect_accelerated */
+  num_samples,
+  INTEL_MIPTREE_TILING_ANY);
+
+   }
+
+   for (face = 0; face < numFaces; face++) {
+  for (level = 0; level < levels; level++) {
+ struct gl_texture_image *image = texobj->Image[face][level];
+ struct intel_texture_image *intel_image = intel_texture_image(image);
+
+ image->NumSamples = num_samples;
+
+ _swrast_free_texture_image_buffer(ctx, image);
+ if (!_swrast_init_texture_image(image))
+return false;
+
+ intel_miptree_reference(&intel_image->mt, intel_texobj->mt);
+  }
+   }
+
+   /* The miptree is in a validated state, so no need to check later. */
+   intel_texobj->needs_validate = false;
+   intel_texobj->validated_first_level = 0;
+   intel_texobj->validated_last_level = levels - 1;
+
+   return true;
+}
+
+
 static void
 intel_free_texture_image_buffer(struct gl_context * ctx,
struct gl_texture_image *texImage)
@@ -184,6 +248,7 @@ intelInitTextureFuncs(struct dd_function_table *functions)
functions->DeleteTexture = intelDeleteTextureObject;
functions->AllocTextureImageBuffer = intel_alloc_texture_image_buffer;
functions->FreeTextureImageBuffer = intel_free_texture_image_buffer;
+   functions->AllocTextureStorage = intel_alloc_texture_storage;
functions->MapTextureImage = intel_map_texture_image;
functions->UnmapTextureImage = intel_unmap_texture_image;
 }
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/17] mesa: Get GL_MAX_VARYING_FLOATS_ARB from VertexProgram.MaxOutputComponents

[Paul Berry]

On 18 September 2013 13:04, Ian Romanick  wrote:

> On 09/13/2013 01:45 PM, Paul Berry wrote:
> > On 11 September 2013 16:28, Ian Romanick  > > wrote:
> > Another question which we haven't really addressed is how Mesa's front
> > end linker should check these limits, given the fact that some
> > implementations have dedicated locations for certain shader
> > inputs/outputs.  I have a few ideas about this, but it's probably
> > something that we should save for a future patch series.
>
> Maybe the 39th time will be the charm. :(  The last time this came up
> was w.r.t. gl_Color and gl_SecondaryColor.  Most (all?) SM2 hardware
> (i915, r300, and probably others) have special slots that can be used
> for *clamped* fragment shader inputs.  So, gl_Color can use those slots
> if the API setting of CLAMP_VERTEX_COLOR is TRUE.  This control is added
> by the GL_ARB_color_buffer_float extension and OpenGL 3.0
>

So i915, r300, and others even support GL_ARB_color_buffer_float/GL3.0?

If so that's a bit of a difficult wrinkle because that means we can't
determine at link time whether the shader will fit in the available slots.
Given that these older hardware models are frequently pretty tightly
constrained on their number of varyings, and the fact that most apps
probably leave vertex color clamping on, it's probably best to do the link
error checking under the assumption that the special slots can be used.
Kind of lame since it means that a shader tha linked without error might
not be guaranteed to execute correctly in all GL states, but I'm guessin
for these older pieces of hardware that's the least of our worries :)


>
> I think we need a flag for each special variable that says whether or
> not it is counted.  I think Marek implemented something like this for
> gl_Color.
>
>
Yeah, that seems reasonable.  I was thinking along similar lines.  We could
have bitfields called something like ctx->Const.DedicatedFsInputs and
ctx->Const.DedicatedVsOutputs.  The convention would be: set the bit if
your hardware has a dedicated slot for the given input/output (and
therefore it doesn't need to be counted).  That way a default value of 0
would correspond to the conservative assumption that there are no dedicated
slots at all.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH pre-06/17] i965: Set *Program.Max{Input, Output}Components

[Ian Romanick]

From: Ian Romanick 

Now that MaxVaryings is > 16, VertexProgram.MaxOutputComponents,
GeometryProgram.MaxInputComponents, GeometryProgram.MaxOutputComponents,
and FragmentProgram.MaxInputComponents also need to be set.

Signed-off-by: Ian Romanick 
Cc: Paul Berry 
---
Since Paul's "i965/gen6+: Support 128 varying components" series landed
before mine, this additional patch is necessary in my series.

 src/mesa/drivers/dri/i965/brw_context.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 7b38ea3..f60d4df 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -247,8 +247,13 @@ brw_initialize_context_constants(struct brw_context *brw)
ctx->Const.DisableGLSLLineContinuations =
   driQueryOptionb(&brw->optionCache, "disable_glsl_line_continuations");
 
-   if (brw->gen >= 6)
+   if (brw->gen >= 6) {
   ctx->Const.MaxVarying = 32;
+  ctx->Const.VertexProgram.MaxOutputComponents = 128;
+  ctx->Const.GeometryProgram.MaxInputComponents = 128;
+  ctx->Const.GeometryProgram.MaxOutputComponents = 128;
+  ctx->Const.FragmentProgram.MaxInputComponents = 128;
+   }
 
/* We want the GLSL compiler to emit code that uses condition codes */
for (int i = 0; i < MESA_SHADER_TYPES; i++) {
-- 
1.8.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/9] i965: Drop a special case for guessing small miptree levels.

[Eric Anholt]

Let's say you started allocating your 2D texture with level 2 of a tree as
a 1x1 image.  The driver doesn't know if this means that level 0 is 4x4 or
4x1 or 1x4, so we would just allocate a single 1x1 and let it get copied
in to the real location at texture validate time later.

Since this is just a temporary allocation that *will* get copied, the
extra space allocation of just taking the normal path which will happen to
producing a 4x1 level 0, 2x1 level 1, and 1x1 level 2 is the right way to
go, to reduce complexity in the normal case.

No change in miptree copies over the course of a piglit run.
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 73 -
 1 file changed, 30 insertions(+), 43 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index f270862..725d315 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -45,50 +45,37 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
 
DBG("%s\n", __FUNCTION__);
 
-   if (intelImage->base.Base.Level > intelObj->base.BaseLevel &&
-   (width == 1 ||
-(intelObj->base.Target != GL_TEXTURE_1D && height == 1) ||
-(intelObj->base.Target == GL_TEXTURE_3D && depth == 1))) {
-  /* For this combination, we're at some lower mipmap level and
-   * some important dimension is 1.  We can't extrapolate up to a
-   * likely base level width/height/depth for a full mipmap stack
-   * from this info, so just allocate this one level.
-   */
-  firstLevel = intelImage->base.Base.Level;
-  lastLevel = intelImage->base.Base.Level;
-   } else {
-  /* If this image disrespects BaseLevel, allocate from level zero.
-   * Usually BaseLevel == 0, so it's unlikely to happen.
-   */
-  if (intelImage->base.Base.Level < intelObj->base.BaseLevel)
-firstLevel = 0;
-  else
-firstLevel = intelObj->base.BaseLevel;
-
-  /* Figure out image dimensions at start level. */
-  for (i = intelImage->base.Base.Level; i > firstLevel; i--) {
-width <<= 1;
-if (height != 1)
-   height <<= 1;
-if (depth != 1)
-   depth <<= 1;
-  }
+   /* If this image disrespects BaseLevel, allocate from level zero.
+* Usually BaseLevel == 0, so it's unlikely to happen.
+*/
+   if (intelImage->base.Base.Level < intelObj->base.BaseLevel)
+  firstLevel = 0;
+   else
+  firstLevel = intelObj->base.BaseLevel;
+
+   /* Figure out image dimensions at start level. */
+   for (i = intelImage->base.Base.Level; i > firstLevel; i--) {
+  width <<= 1;
+  if (height != 1)
+ height <<= 1;
+  if (depth != 1)
+ depth <<= 1;
+   }
 
-  /* Guess a reasonable value for lastLevel.  This is probably going
-   * to be wrong fairly often and might mean that we have to look at
-   * resizable buffers, or require that buffers implement lazy
-   * pagetable arrangements.
-   */
-  if ((intelObj->base.Sampler.MinFilter == GL_NEAREST ||
-  intelObj->base.Sampler.MinFilter == GL_LINEAR) &&
- intelImage->base.Base.Level == firstLevel &&
- firstLevel == 0) {
-lastLevel = firstLevel;
-  } else {
-lastLevel = (firstLevel +
-  _mesa_get_tex_max_num_levels(intelObj->base.Target,
-   width, height, depth) - 1);
-  }
+   /* Guess a reasonable value for lastLevel.  This is probably going
+* to be wrong fairly often and might mean that we have to look at
+* resizable buffers, or require that buffers implement lazy
+* pagetable arrangements.
+*/
+   if ((intelObj->base.Sampler.MinFilter == GL_NEAREST ||
+intelObj->base.Sampler.MinFilter == GL_LINEAR) &&
+   intelImage->base.Base.Level == firstLevel &&
+   firstLevel == 0) {
+  lastLevel = firstLevel;
+   } else {
+  lastLevel = (firstLevel +
+   _mesa_get_tex_max_num_levels(intelObj->base.Target,
+width, height, depth) - 1);
}
 
return intel_miptree_create(brw,
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 4/9] i965: Stop allocating miptrees with first_level != 0.

[Eric Anholt]

If the caller shows up with GL_BASE_LEVEL != 0, it doesn't mean that the
texture will over the course of its lifetime have that nonzero baselevel,
it means that the caller is filling the texture from the bottom up for
some reason (one could imagine demand-loading detailed texture layers at
runtime, for example).  If we allocate from just the current baselevel, it
means when they come along with the next level up, we'll have to allocate
a new miptree and copy all of our bits out of the first miptree.
---
 src/mesa/drivers/dri/i965/intel_tex_image.c | 23 ++-
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/intel_tex_image.c 
b/src/mesa/drivers/dri/i965/intel_tex_image.c
index 725d315..fe274bf 100644
--- a/src/mesa/drivers/dri/i965/intel_tex_image.c
+++ b/src/mesa/drivers/dri/i965/intel_tex_image.c
@@ -35,7 +35,6 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
  struct intel_texture_image *intelImage,
  bool expect_accelerated_upload)
 {
-   GLuint firstLevel;
GLuint lastLevel;
int width, height, depth;
GLuint i;
@@ -45,16 +44,8 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
 
DBG("%s\n", __FUNCTION__);
 
-   /* If this image disrespects BaseLevel, allocate from level zero.
-* Usually BaseLevel == 0, so it's unlikely to happen.
-*/
-   if (intelImage->base.Base.Level < intelObj->base.BaseLevel)
-  firstLevel = 0;
-   else
-  firstLevel = intelObj->base.BaseLevel;
-
/* Figure out image dimensions at start level. */
-   for (i = intelImage->base.Base.Level; i > firstLevel; i--) {
+   for (i = intelImage->base.Base.Level; i > 0; i--) {
   width <<= 1;
   if (height != 1)
  height <<= 1;
@@ -69,19 +60,17 @@ intel_miptree_create_for_teximage(struct brw_context *brw,
 */
if ((intelObj->base.Sampler.MinFilter == GL_NEAREST ||
 intelObj->base.Sampler.MinFilter == GL_LINEAR) &&
-   intelImage->base.Base.Level == firstLevel &&
-   firstLevel == 0) {
-  lastLevel = firstLevel;
+   intelImage->base.Base.Level == 0) {
+  lastLevel = 0;
} else {
-  lastLevel = (firstLevel +
-   _mesa_get_tex_max_num_levels(intelObj->base.Target,
-width, height, depth) - 1);
+  lastLevel = _mesa_get_tex_max_num_levels(intelObj->base.Target,
+   width, height, depth) - 1;
}
 
return intel_miptree_create(brw,
   intelObj->base.Target,
   intelImage->base.Base.TexFormat,
-  firstLevel,
+  0,
   lastLevel,
   width,
   height,
-- 
1.8.4.rc3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Paul Berry]

On 18 September 2013 13:15, Ian Romanick  wrote:

> On 09/18/2013 01:35 PM, Paul Berry wrote:
> > On 18 September 2013 07:43, Ian Romanick  > > wrote:
> >
> > On 09/15/2013 02:10 AM, Francisco Jerez wrote:
> > > The _mesa_glsl_parse_state object relies on the memory allocator
> > > zeroing out its contents before it's initialized, which seems
> rather
> > > risky.  One of the following commits will homogenize
> implementations
> > > of the new operator in a way that would break this assumption
> leaving
> > > some of the member variables of this object uninitialized.
> >
> > This was an intentional design decision that we made, after a fair
> > amount of consideration, in the early days of the compiler.  A
> number of
> > factors came together:
> >
> >  * Use of ralloc et al. meant that we could never have
> stack-allocated
> > objects.
> >
> >  * Use of rzalloc meant that objects would always be pre-allocated
> > to zero.
> >
> >  * Uninitialized class member bugs are hard to track down.
> >
> >  * It's easy to forget to add 'this->foo = 0;' when you add a field
> foo
> > to a class.
> >
> >  * A dozen lines of 'this->foo = 0;' is hard to properly review when
> a
> > new class is added.  Are there actually 13 fields that need to be
> > initialized?
> >
> >  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
> >
> > In the end, we decided to just rely on the allocator always providing
> > cleared memory.
> >
> > I'm not very excited about adding a bunch of redundant code to the
> these
> > constructors... especially since that code probably won't get
> maintained
> > (e.g., when the next member gets added to those classes).
> >
> > I'd like to hear Ken and Eric weigh in.
> >
> >
> > I think I have sufficient development experience and familiarity with
> > the code base to weigh in as well.  I fall on the opposite side of this
> > particular fence--I prefer to see class members initialized in the
> > constructor.  My reasoning is:
> >
> > * Initializing class members in the constructor costs almost nothing and
> > decreases the risk of subtle and difficult-to-find bugs.
>
> Except that we forget to add the initialization almost as often as we
> remember.  I see from the patch that some of the fields lacking
> initializers are ones you most likely added... :)
>

Only because when I added those fields I was following the prevailing
convention.


>
> > * Initializing class members in the constructor is common practice in
> > C++, and therefore less surprising to developers who are new to the Mesa
> > code base.
> >
> > * If a class initializes all its members in the constructor, we have the
> > freedom to allocate it in whatever way is appropriate (stack, rzalloc,
> > malloc, new operator, or placement new, to name a few examples).  If it
> > doesn't, we have no choice but to allocate it using rzalloc.
> >
> > * There's no compile-time mechanism to ensure that all uses of a class
> > use rzalloc (nor are there any static analysis tools I'm aware of that
> > are capable of ensuring this).  It's also a pain to verify using grep.
> > So if a class doesn't initialize all its members in the constructor, we
> > have to be vigilant in our code review to make sure that all present and
> > future users of the class use rzalloc.
>
> As far as I'm aware, our use of overloaded new / delete with rzalloc
> forces us to use new (and therefore rzalloc) for every object or die in
> a fire.  None of the destructors deallocate any objects owned by them.
> Even if we did switch to a different allocation scheme, we would leak
> like a screen door on a submarine.
>
> > * By contrast, Coverity *is* able to ensure that class members are
> > initialized in the constructor, and I suspect other static analysis
> > tools are too.
>
> Relying on Vinson to run Coverity and submit bugs is not a solid plan.
> We have no other plan involving static analysis tools, and we don't have
> the staff to filter the flood of false positives that ever static
> analysis tool I've ever encountered generates.
>
>
Actually, I believe Vinson just uses the results of the scans that Coverity
runs on open source projects such as Mesa for free.  Anyone can view the
open defects here: https://scan.coverity.com/projects/139, and anyone can
sign up to receive e-mail notifications of new defects.  I'm signed up to
receive the emails too.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Francisco Jerez]

Ian Romanick  writes:

> On 09/17/2013 03:51 PM, Paul Berry wrote:
>> On 15 September 2013 00:10, Francisco Jerez > > wrote:
>>[...]
>> 
>> GLSL 4.20 clarifies that atomic counter offsets must be unique and
>> non-overlapping (see GLSL 4.20 4.4.4.1 "Atomic Counter Layout
>> Qualifiers").  So I tihnk we need a stronger check than this. 
>> Specifically, declarations like the following should be disallowed:
>> 
>> layout(binding=0, offset=16) atomic_uint foo[4]; // Uses offsets 16, 20,
>> 24, 28
>> layout(binding=0, offset=20) atomic_uint bar; // Error: overlaps foo.
>
> I also think this check belongs in the linker.  I don't see a lot of
> value in doing it twice, and we have to verify atomics declared in other
> compilation units.
>
I can move that check back to the linker, you're right that it would
probably be more convenient for us to do so.  The spec isn't very clear
on whether this should be a compile or a link-time error, actually I
implemented this in the linker first but in doubt it seemed desirable to
be able to catch this sort of errors as early as possible, and the spec
mentions explicitly that using atomic buffer bindings over the
implementation's maximum should cause a compile-time error.

>>
>> + if (field_type->atomic_size()) {
>> +YYLTYPE loc = decl_list->get_location();
>> +_mesa_glsl_error(&loc, state, "atomic counter in
>> structure or "
>> + "uniform block");
>> + }
>> +
>> 
>> Are you sure this is not allowed?  I can't find any spec text to back
>> this up.  All I found was this text from ARB_shader_atomic_counters:
>
> Yeah, this is definitely wrong.  See the text I quoted in reply to patch
> 8.[...]
>
Please read my answer to Paul's post.  It's not clear how atomic
counters declared within a structure would get their binding points and
offsets assigned: binding and offset qualifiers are disallowed within
structs, and using a single layout() qualifier for the whole uniform
struct declaration isn't satisfactory either.  I think we should
disallow them for now, the spec is just too inconsistent...

Thanks.


pgpG5adU2FAAS.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Ian Romanick]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

On 09/18/2013 03:43 PM, Francisco Jerez wrote:
> Ian Romanick  writes:
> 
>> On 09/17/2013 03:51 PM, Paul Berry wrote:
>>> On 15 September 2013 00:10, Francisco Jerez
>>> mailto:curroje...@riseup.net>> wrote: 
>>> [...]
>>> 
>>> GLSL 4.20 clarifies that atomic counter offsets must be unique
>>> and non-overlapping (see GLSL 4.20 4.4.4.1 "Atomic Counter
>>> Layout Qualifiers").  So I tihnk we need a stronger check than
>>> this. Specifically, declarations like the following should be
>>> disallowed:
>>> 
>>> layout(binding=0, offset=16) atomic_uint foo[4]; // Uses
>>> offsets 16, 20, 24, 28 layout(binding=0, offset=20) atomic_uint
>>> bar; // Error: overlaps foo.
>> 
>> I also think this check belongs in the linker.  I don't see a lot
>> of value in doing it twice, and we have to verify atomics
>> declared in other compilation units.
>> 
> I can move that check back to the linker, you're right that it
> would probably be more convenient for us to do so.  The spec isn't
> very clear on whether this should be a compile or a link-time
> error, actually I implemented this in the linker first but in doubt
> it seemed desirable to be able to catch this sort of errors as
> early as possible, and the spec mentions explicitly that using
> atomic buffer bindings over the implementation's maximum should
> cause a compile-time error.

There's two checks.  Right?

Check that binding and offset are within implementation limits.  This
can be done in the compiler or the linker.  I generally prefer earlier.

Check that bindings and offsets of multiple variables do not overlap.
 Since you may link multiple compilation units, it is impossible to
know before the linker because you don't have access to the other
compilation units yet.

>>> 
>>> + if (field_type->atomic_size()) { +YYLTYPE
>>> loc = decl_list->get_location(); +
>>> _mesa_glsl_error(&loc, state, "atomic counter in structure or
>>> " + "uniform block"); + } 
>>> +
>>> 
>>> Are you sure this is not allowed?  I can't find any spec text
>>> to back this up.  All I found was this text from
>>> ARB_shader_atomic_counters:
>> 
>> Yeah, this is definitely wrong.  See the text I quoted in reply
>> to patch 8.[...]
>> 
> Please read my answer to Paul's post.  It's not clear how atomic 
> counters declared within a structure would get their binding points
> and offsets assigned: binding and offset qualifiers are disallowed
> within structs, and using a single layout() qualifier for the whole
> uniform struct declaration isn't satisfactory either.  I think we
> should disallow them for now, the spec is just too inconsistent...

Yes... I see the problem now.  I've submitted a spec bug.  Until that
bug gets resolved, let's continue with the assumption that atomic_uint
should not be allowed in structs.

> Thanks.

-BEGIN PGP SIGNATURE-
Version: GnuPG v1.4.13 (GNU/Linux)

iEYEARECAAYFAlI6HXUACgkQX1gOwKyEAw8iXQCdEg2FXG/b9BmT9apdJdgkHIqO
oj4AoJXMkDW85VDHbxiooTDyT6vAGm/3
=cQ8z
-END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 3/5] i965, mesa: Use the new DECLARE_R[Z]ALLOC_CXX_OPERATORS macros.

[Kenneth Graunke]

These classes declared a placement new operator, but didn't declare a
delete operator.  Switching to the macro gives them a delete operator,
which probably is a good idea anyway.

This also eliminates a lot of boilerplate.

Signed-off-by: Kenneth Graunke 
---
 src/mesa/drivers/dri/i965/brw_cfg.h| 20 ++--
 src/mesa/drivers/dri/i965/brw_fs.h | 34 ++--
 src/mesa/drivers/dri/i965/brw_fs_live_variables.h  | 10 +-
 src/mesa/drivers/dri/i965/brw_vec4.h   | 36 ++
 .../drivers/dri/i965/brw_vec4_live_variables.h | 10 +-
 src/mesa/program/ir_to_mesa.cpp| 12 +---
 src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 12 +---
 7 files changed, 12 insertions(+), 122 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h 
b/src/mesa/drivers/dri/i965/brw_cfg.h
index 95a18e9..27bc8b6 100644
--- a/src/mesa/drivers/dri/i965/brw_cfg.h
+++ b/src/mesa/drivers/dri/i965/brw_cfg.h
@@ -39,15 +39,7 @@ public:
 
 class bblock_t {
 public:
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(bblock_t)
 
bblock_link *make_list(void *mem_ctx);
 
@@ -68,15 +60,7 @@ public:
 
 class cfg_t {
 public:
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(cfg_t)
 
cfg_t(backend_visitor *v);
cfg_t(void *mem_ctx, exec_list *instructions);
diff --git a/src/mesa/drivers/dri/i965/brw_fs.h 
b/src/mesa/drivers/dri/i965/brw_fs.h
index b77d4de..b2aa041 100644
--- a/src/mesa/drivers/dri/i965/brw_fs.h
+++ b/src/mesa/drivers/dri/i965/brw_fs.h
@@ -57,17 +57,7 @@ namespace {
 
 class fs_reg {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = ralloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(fs_reg)
 
void init();
 
@@ -122,15 +112,7 @@ static const fs_reg reg_null_d(ARF, BRW_ARF_NULL, 
BRW_REGISTER_TYPE_D);
 
 class ip_record : public exec_node {
 public:
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(ip_record)
 
ip_record(int ip)
{
@@ -142,17 +124,7 @@ public:
 
 class fs_inst : public backend_instruction {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(fs_inst)
 
void init();
 
diff --git a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h 
b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
index 1cde5f4..fa8b61d 100644
--- a/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
+++ b/src/mesa/drivers/dri/i965/brw_fs_live_variables.h
@@ -53,15 +53,7 @@ struct block_data {
 
 class fs_live_variables {
 public:
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(fs_live_variables)
 
fs_live_variables(fs_visitor *v, cfg_t *cfg);
~fs_live_variables();
diff --git a/src/mesa/drivers/dri/i965/brw_vec4.h 
b/src/mesa/drivers/dri/i965/brw_vec4.h
index f0ab53d..689040b 100644
--- a/src/mesa/drivers/dri/i965/brw_vec4.h
+++ b/src/mesa/drivers/dri/i965/brw_vec4.h
@@ -118,17 +118,7 @@ public:
 class src_reg : public reg
 {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = ralloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(src_reg)
 
void init();
 
@@ -156,17 +146,7 @@ public:
 class dst_reg : public reg
 {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = ralloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(dst_reg)
 
void init();
 
@@ -188,17 +168,7 @@ with_writemask(dst_reg const &r, int mask);
 
 class vec4_instruct

[Mesa-dev] [RFC PATCH 4/5] ralloc: Make the C++ macros arrange for class destructors to be called.

[Kenneth Graunke]

Previously, almost all classes didn't actually call their destructors,
which is non-intuitive for C++ code.  Setting them up to be called was
somewhat of a pain, since it required defining a helper function.

With the new macros, we can easily encapsulate this complexity, making
destructors just happen automatically.

Signed-off-by: Kenneth Graunke 
---
 src/glsl/ralloc.h | 8 
 1 file changed, 8 insertions(+)

diff --git a/src/glsl/ralloc.h b/src/glsl/ralloc.h
index 799d3a9..82a3daa 100644
--- a/src/glsl/ralloc.h
+++ b/src/glsl/ralloc.h
@@ -405,15 +405,23 @@ bool ralloc_vasprintf_append(char **str, const char *fmt, 
va_list args);
 #endif
 
 #define _RALLOC_OPS(ALLOC, TYPE) \
+private: \
+   static void _ralloc_##TYPE##_destructor_callback(void *p) \
+   { \
+  reinterpret_cast(p)->~TYPE();  \
+   } \
+public:  \
static void* operator new(size_t size, void *mem_ctx) \
{ \
   void *p = ALLOC(mem_ctx, size);\
   assert(p != NULL); \
+  ralloc_set_destructor(p, _ralloc_##TYPE##_destructor_callback);\
   return p;  \
} \
  \
static void operator delete(void *p)  \
{ \
+  ralloc_set_destructor(p, NULL);\
   ralloc_free(p);\
}
 
-- 
1.8.3.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/5] ralloc: Introduce new macros for defining C++ new/delete operators.

[Kenneth Graunke]

Most of our C++ classes define placement new and delete operators so we
can do convenient allocation via:

   thing *foo = new(mem_ctx) thing(...)

Currently, this is done via a lot of boilerplate.  By adding simple
macros to ralloc, we can condense this to a single line, making it
trivial to add this feature to a new class.

Signed-off-by: Kenneth Graunke 
---
 src/glsl/ralloc.h | 26 ++
 1 file changed, 26 insertions(+)

This series is an alternative to Curro's patch:
http://lists.freedesktop.org/archives/mesa-dev/2013-September/044728.html

diff --git a/src/glsl/ralloc.h b/src/glsl/ralloc.h
index 67eb938..799d3a9 100644
--- a/src/glsl/ralloc.h
+++ b/src/glsl/ralloc.h
@@ -404,4 +404,30 @@ bool ralloc_vasprintf_append(char **str, const char *fmt, 
va_list args);
 } /* end of extern "C" */
 #endif
 
+#define _RALLOC_OPS(ALLOC, TYPE) \
+   static void* operator new(size_t size, void *mem_ctx) \
+   { \
+  void *p = ALLOC(mem_ctx, size);\
+  assert(p != NULL); \
+  return p;  \
+   } \
+ \
+   static void operator delete(void *p)  \
+   { \
+  ralloc_free(p);\
+   }
+
+/**
+ * Declare C++ new and delete operators which use ralloc.
+ *
+ * Placing one of these macros in the body of a class makes it possible to do:
+ *
+ * TYPE *var = new(mem_ctx) TYPE(...);
+ * delete var;
+ *
+ * which is more idiomatic in C++ than calling ralloc or rzalloc.
+ */
+#define DECLARE_RALLOC_CXX_OPERATORS(TYPE)  _RALLOC_OPS(ralloc_size,  TYPE)
+#define DECLARE_RZALLOC_CXX_OPERATORS(TYPE) _RALLOC_OPS(rzalloc_size, TYPE)
+
 #endif
-- 
1.8.3.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/5] glsl: Use the new DECLARE_R[Z]ALLOC_CXX_OPERATORS in a bunch of places.

[Kenneth Graunke]

This eliminates a lot of boilerplate and should be 100% equivalent.

Signed-off-by: Kenneth Graunke 
---
 src/glsl/ast.h| 38 ++-
 src/glsl/glsl_parser_extras.h | 17 +-
 src/glsl/glsl_symbol_table.cpp| 15 +---
 src/glsl/ir_function_detect_recursion.cpp | 20 +---
 src/glsl/list.h   | 38 ++-
 5 files changed, 7 insertions(+), 121 deletions(-)

diff --git a/src/glsl/ast.h b/src/glsl/ast.h
index 1c7fc63..c3361a1 100644
--- a/src/glsl/ast.h
+++ b/src/glsl/ast.h
@@ -49,24 +49,7 @@ struct YYLTYPE;
  */
 class ast_node {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just
-* ralloc_free in that case. */
-   static void operator delete(void *table)
-   {
-  ralloc_free(table);
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(ast_node);
 
/**
 * Print an AST node in something approximating the original GLSL code
@@ -363,24 +346,7 @@ enum {
 };
 
 struct ast_type_qualifier {
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = rzalloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just
-* ralloc_free in that case. */
-   static void operator delete(void *table)
-   {
-  ralloc_free(table);
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(ast_type_qualifier);
 
union {
   struct {
diff --git a/src/glsl/glsl_parser_extras.h b/src/glsl/glsl_parser_extras.h
index 2e2440a..364a983 100644
--- a/src/glsl/glsl_parser_extras.h
+++ b/src/glsl/glsl_parser_extras.h
@@ -73,22 +73,7 @@ struct _mesa_glsl_parse_state {
_mesa_glsl_parse_state(struct gl_context *_ctx, GLenum target,
  void *mem_ctx);
 
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *mem = rzalloc_size(ctx, size);
-  assert(mem != NULL);
-
-  return mem;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just
-* ralloc_free in that case. */
-   static void operator delete(void *mem)
-   {
-  ralloc_free(mem);
-   }
+   DECLARE_RZALLOC_CXX_OPERATORS(_mesa_glsl_parse_state);
 
/**
 * Generate a string representing the GLSL version currently being compiled
diff --git a/src/glsl/glsl_symbol_table.cpp b/src/glsl/glsl_symbol_table.cpp
index 4c96620..6e916b4 100644
--- a/src/glsl/glsl_symbol_table.cpp
+++ b/src/glsl/glsl_symbol_table.cpp
@@ -26,20 +26,7 @@
 
 class symbol_table_entry {
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *entry = ralloc_size(ctx, size);
-  assert(entry != NULL);
-  return entry;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just ralloc_free. */
-   static void operator delete(void *entry)
-   {
-  ralloc_free(entry);
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(symbol_table_entry);
 
bool add_interface(const glsl_type *i, enum ir_variable_mode mode)
{
diff --git a/src/glsl/ir_function_detect_recursion.cpp 
b/src/glsl/ir_function_detect_recursion.cpp
index 280c473..b02c325 100644
--- a/src/glsl/ir_function_detect_recursion.cpp
+++ b/src/glsl/ir_function_detect_recursion.cpp
@@ -139,25 +139,7 @@ public:
   /* empty */
}
 
-
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *node;
-
-  node = ralloc_size(ctx, size);
-  assert(node != NULL);
-
-  return node;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just
-* ralloc_free in that case. */
-   static void operator delete(void *node)
-   {
-  ralloc_free(node);
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(function)
 
ir_function_signature *sig;
 
diff --git a/src/glsl/list.h b/src/glsl/list.h
index 1d46365..5ac17cb 100644
--- a/src/glsl/list.h
+++ b/src/glsl/list.h
@@ -76,24 +76,7 @@ struct exec_node {
struct exec_node *prev;
 
 #ifdef __cplusplus
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator 

[Mesa-dev] [RFC PATCH 5/5] glsl: Switch a few more classes to DECLARE_RALLOC_CXX_OPERATORS.

[Kenneth Graunke]

Now that the macros arrange for the destructors to be called, we can use
them in these cases too.

Signed-off-by: Kenneth Graunke 
---
 src/glsl/glsl_symbol_table.h | 30 +-
 src/glsl/loop_analysis.h | 17 +
 2 files changed, 2 insertions(+), 45 deletions(-)

diff --git a/src/glsl/glsl_symbol_table.h b/src/glsl/glsl_symbol_table.h
index 62d26b8..bd3860f 100644
--- a/src/glsl/glsl_symbol_table.h
+++ b/src/glsl/glsl_symbol_table.h
@@ -43,36 +43,8 @@ class symbol_table_entry;
  * type safe and some symbol table invariants.
  */
 struct glsl_symbol_table {
-private:
-   static void
-   _glsl_symbol_table_destructor (glsl_symbol_table *table)
-   {
-  table->~glsl_symbol_table();
-   }
-
 public:
-   /* Callers of this ralloc-based new need not call delete. It's
-* easier to just ralloc_free 'ctx' (or any of its ancestors). */
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *table;
-
-  table = ralloc_size(ctx, size);
-  assert(table != NULL);
-
-  ralloc_set_destructor(table, (void (*)(void*)) 
_glsl_symbol_table_destructor);
-
-  return table;
-   }
-
-   /* If the user *does* call delete, that's OK, we will just
-* ralloc_free in that case. Here, C++ will have already called the
-* destructor so tell ralloc not to do that again. */
-   static void operator delete(void *table)
-   {
-  ralloc_set_destructor(table, NULL);
-  ralloc_free(table);
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(glsl_symbol_table)

glsl_symbol_table();
~glsl_symbol_table();
diff --git a/src/glsl/loop_analysis.h b/src/glsl/loop_analysis.h
index 769d626..45359e7 100644
--- a/src/glsl/loop_analysis.h
+++ b/src/glsl/loop_analysis.h
@@ -141,22 +141,7 @@ public:
   hash_table_dtor(this->var_hash);
}
 
-   static void* operator new(size_t size, void *ctx)
-   {
-  void *lvs = ralloc_size(ctx, size);
-  assert(lvs != NULL);
-
-  ralloc_set_destructor(lvs, (void (*)(void*)) destructor);
-
-  return lvs;
-   }
-
-private:
-   static void
-   destructor(loop_variable_state *lvs)
-   {
-  lvs->~loop_variable_state();
-   }
+   DECLARE_RALLOC_CXX_OPERATORS(loop_variable_state)
 };
 
 
-- 
1.8.3.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 04/24] ralloc: Unify overloads of the new operator and guarantee object destruction.

[Kenneth Graunke]

On 09/15/2013 12:10 AM, Francisco Jerez wrote:
> This patch introduces a pair of helper functions providing a common
> implementation of the "new" and "delete" operators for all C++ classes
> that are allocated by ralloc via placement new.  The 'ralloc_new'
> helper function takes care of setting up an ralloc destructor callback
> that will call the appropriate destructor before the memory allocated
> to an object is released.
> 
> Until now objects needed to call 'ralloc_set_destructor' explicitly
> with a pointer to a static method which in turn called the actual
> destructor in order to get something that should be transparent to
> them.  After this patch they'll only need to call 'ralloc_new' from
> the new operator and 'ralloc_delete' from the delete operator, turning
> all overloads of new and delete into one-liners.

I really like the idea of reducing the boilerplate.  I'd been meaning to
do that for a while, but was thinking of creating a "rallocable" base
class (which would then introduce multiple inheritance).  I never got
around to trying it.

I can't say I'm crazy about this approach, though:

There's still a fair bit of boilerplate; every class still needs to do:

static void* operator new(size_t size, void *ctx)
{
   return ralloc_new(size, ctx);
}

static void operator delete(void *p)
{
   ralloc_delete(p);
}

I'd like to go all the way and condense that to a single line.

This is also the first usage of templates in core Mesa, which I know
will annoy a lot of people.  While I would be in favor of using
templates to introduce type safety into things like exec_list or
hash_table, I don't think they provide a very compelling advantage in
this case.

I've sent out a new series which accomplishes the same thing, but
doesn't use templates, and reduces the boilerplate even a bit more.
Comments welcome.

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Kenneth Graunke]

On 09/18/2013 07:43 AM, Ian Romanick wrote:
> On 09/15/2013 02:10 AM, Francisco Jerez wrote:
>> The _mesa_glsl_parse_state object relies on the memory allocator
>> zeroing out its contents before it's initialized, which seems rather
>> risky.  One of the following commits will homogenize implementations
>> of the new operator in a way that would break this assumption leaving
>> some of the member variables of this object uninitialized.
> 
> This was an intentional design decision that we made, after a fair
> amount of consideration, in the early days of the compiler.  A number of
> factors came together:
> 
>  * Use of ralloc et al. meant that we could never have stack-allocated
> objects.
> 
>  * Use of rzalloc meant that objects would always be pre-allocated to zero.
> 
>  * Uninitialized class member bugs are hard to track down.
> 
>  * It's easy to forget to add 'this->foo = 0;' when you add a field foo
> to a class.
> 
>  * A dozen lines of 'this->foo = 0;' is hard to properly review when a
> new class is added.  Are there actually 13 fields that need to be
> initialized?
> 
>  * Writing a dozen lines of 'this->foo = 0;' is annoying. :)
> 
> In the end, we decided to just rely on the allocator always providing
> cleared memory.
> 
> I'm not very excited about adding a bunch of redundant code to the these
> constructors... especially since that code probably won't get maintained
> (e.g., when the next member gets added to those classes).
> 
> I'd like to hear Ken and Eric weigh in.

I do like simply using rzalloc or memset to avoid having to fill in
dozens of fields with 0.  It's very concise and convenient.

However, an object's constructor is supposed to initialize its fields.
This is idiomatic for *any* object oriented language.  If we're going to
use the OOP paradigm, we probably ought take this approach.

In my mind, using memset() inside the constructor counts as the
constructor initializing the fields.  It also works with stack
allocation, normal heap allocation, or ralloc allocation.

Uninitialized class member bugs will be caught with Valgrind, and Piglit
makes it very easy to check that:

$ piglit-run.py --valgrind tests/quick.tests results/vg-1

With these tools, I don't think tracking down uninitialized fields is
that difficult.  I also don't think "we'll forget to initialize things"
is a good argument for relying on zero-allocation.  Sometimes fields
need to be initialized to a value other than zero, so it's a good idea
to get in the habit of initializing them.

I would have to say I agree with Paul and Curro, but I can also respect
the current rely-on-rzalloc style, and it doesn't honestly bother me
much.  If people want to explicitly initialize member variables, I
applaud them; if not, I can live with that too.

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 02/24] glsl: Initialize all member variables of _mesa_glsl_parse_state on construction.

[Francisco Jerez]

Ian Romanick  writes:

> On 09/18/2013 01:35 PM, Paul Berry wrote:
>>[...]
>> * Initializing class members in the constructor is common practice in
>> C++, and therefore less surprising to developers who are new to the Mesa
>> code base.
>> 
>> * If a class initializes all its members in the constructor, we have the
>> freedom to allocate it in whatever way is appropriate (stack, rzalloc,
>> malloc, new operator, or placement new, to name a few examples).  If it
>> doesn't, we have no choice but to allocate it using rzalloc.
>> 
>> * There's no compile-time mechanism to ensure that all uses of a class
>> use rzalloc (nor are there any static analysis tools I'm aware of that
>> are capable of ensuring this).  It's also a pain to verify using grep. 
>> So if a class doesn't initialize all its members in the constructor, we
>> have to be vigilant in our code review to make sure that all present and
>> future users of the class use rzalloc.
>
> As far as I'm aware, our use of overloaded new / delete with rzalloc
> forces us to use new (and therefore rzalloc) for every object or die in
> a fire.  None of the destructors deallocate any objects owned by them.
> Even if we did switch to a different allocation scheme, we would leak
> like a screen door on a submarine.
>
But, we're already using other allocation schemes -- and that's fine,
they are useful.  I think those leaks and unpredictable results we get
when using allocators other than rzalloc is something that it would be
worthwhile to fix, having all constructors initialize all class members
is a first step, even if we decide to keep using memset(0) to that
end...

>> * By contrast, Coverity *is* able to ensure that class members are
>> initialized in the constructor, and I suspect other static analysis
>> tools are too.
>
> Relying on Vinson to run Coverity and submit bugs is not a solid plan.
> We have no other plan involving static analysis tools, and we don't have
> the staff to filter the flood of false positives that ever static
> analysis tool I've ever encountered generates.

How about making operator new poison the allocated memory with some
easily recognizable pattern in debug builds to keep future problems of
this kind from going unnoticed when a new member variable is introduced?


pgpNqFL73OBjB.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 11/17] mesa: Get GL_MAX_VARYING_FLOATS_ARB from VertexProgram.MaxOutputComponents

[Marek Olšák]

I don't remember implementing anything in the GLSL linker for colors
not to be counted against the maximum varying count limit.

R500 supports GL_ARB_color_buffer_float, but not GL3.0. R300 and R500
have special slots for colors.

Marek

On Wed, Sep 18, 2013 at 10:30 PM, Paul Berry  wrote:
> On 18 September 2013 13:04, Ian Romanick  wrote:
>>
>> On 09/13/2013 01:45 PM, Paul Berry wrote:
>> > On 11 September 2013 16:28, Ian Romanick > > > wrote:
>> > Another question which we haven't really addressed is how Mesa's front
>> > end linker should check these limits, given the fact that some
>> > implementations have dedicated locations for certain shader
>> > inputs/outputs.  I have a few ideas about this, but it's probably
>> > something that we should save for a future patch series.
>>
>> Maybe the 39th time will be the charm. :(  The last time this came up
>> was w.r.t. gl_Color and gl_SecondaryColor.  Most (all?) SM2 hardware
>> (i915, r300, and probably others) have special slots that can be used
>> for *clamped* fragment shader inputs.  So, gl_Color can use those slots
>> if the API setting of CLAMP_VERTEX_COLOR is TRUE.  This control is added
>> by the GL_ARB_color_buffer_float extension and OpenGL 3.0
>
>
> So i915, r300, and others even support GL_ARB_color_buffer_float/GL3.0?
>
> If so that's a bit of a difficult wrinkle because that means we can't
> determine at link time whether the shader will fit in the available slots.
> Given that these older hardware models are frequently pretty tightly
> constrained on their number of varyings, and the fact that most apps
> probably leave vertex color clamping on, it's probably best to do the link
> error checking under the assumption that the special slots can be used.
> Kind of lame since it means that a shader tha linked without error might not
> be guaranteed to execute correctly in all GL states, but I'm guessin for
> these older pieces of hardware that's the least of our worries :)
>
>>
>>
>> I think we need a flag for each special variable that says whether or
>> not it is counted.  I think Marek implemented something like this for
>> gl_Color.
>>
>
> Yeah, that seems reasonable.  I was thinking along similar lines.  We could
> have bitfields called something like ctx->Const.DedicatedFsInputs and
> ctx->Const.DedicatedVsOutputs.  The convention would be: set the bit if your
> hardware has a dedicated slot for the given input/output (and therefore it
> doesn't need to be counted).  That way a default value of 0 would correspond
> to the conservative assumption that there are no dedicated slots at all.
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 10/24] glsl: Implement parser support for atomic counters.

[Francisco Jerez]

Ian Romanick  writes:

>[...]
 
 + if (field_type->atomic_size()) { +YYLTYPE
 loc = decl_list->get_location(); +
 _mesa_glsl_error(&loc, state, "atomic counter in structure or
 " + "uniform block"); + } 
 +
 
 Are you sure this is not allowed?  I can't find any spec text
 to back this up.  All I found was this text from
 ARB_shader_atomic_counters:
>>> 
>>> Yeah, this is definitely wrong.  See the text I quoted in reply
>>> to patch 8.[...]
>>> 
>> Please read my answer to Paul's post.  It's not clear how atomic 
>> counters declared within a structure would get their binding points
>> and offsets assigned: binding and offset qualifiers are disallowed
>> within structs, and using a single layout() qualifier for the whole
>> uniform struct declaration isn't satisfactory either.  I think we
>> should disallow them for now, the spec is just too inconsistent...
>
> Yes... I see the problem now.  I've submitted a spec bug.  Until that
> bug gets resolved, let's continue with the assumption that atomic_uint
> should not be allowed in structs.
>
Thank you for doing that, do you have a link to the report?

>> Thanks.
>


pgpdmeEdoUpW7.pgp
Description: PGP signature
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH V3 04/11] i965/fs: Add support for ir_tg4

[Kenneth Graunke]

On 09/15/2013 02:58 AM, Chris Forbes wrote:
> Lowers ir_tg4 (from textureGather and textureGatherOffset builtins) to
> SHADER_OPCODE_TG4.
> 
> The usual post-sampling swizzle workaround can't work for ir_tg4,
> so avoid doing that:
> 
> * For R/G/B/A swizzles use the hardware channel select (lives in the
>same dword in the header as the texel offset), and then don't do
>anything afterward in the shader.
> * For 0/1 swizzles blast the appropriate constant over all the output
>channels instead of sampling.
> 
> V2: Avoid duplicating header enabling block
> V3: Avoid sampling at all, for degenerate swizzles.
> 
> Signed-off-by: Chris Forbes 
> ---
>  src/mesa/drivers/dri/i965/brw_fs.h   |  1 +
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 62 
> ++--
>  2 files changed, 60 insertions(+), 3 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h 
> b/src/mesa/drivers/dri/i965/brw_fs.h
> index cb4ac3b..f3a1eeb 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs.h
> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
> @@ -241,6 +241,7 @@ public:
> void visit(ir_emit_vertex *);
> void visit(ir_end_primitive *);
>  
> +   uint32_t gather_channel(ir_texture *ir, int sampler);
> void swizzle_result(ir_texture *ir, fs_reg orig_val, int sampler);
>  
> bool can_do_source_mods(fs_inst *inst);
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index d935c7b..be7aed7 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1163,6 +1163,12 @@ fs_visitor::emit_texture_gen5(ir_texture *ir, fs_reg 
> dst, fs_reg coordinate,
> case ir_lod:
>inst = emit(SHADER_OPCODE_LOD, dst);
>break;
> +   case ir_tg4:
> +  inst = emit(SHADER_OPCODE_TG4, dst);
> +  break;
> +   default:
> +  fail("unrecognized texture opcode");

Maybe assert rather than fail?  Not a huge deal either way.

> +  break;
> }
> inst->base_mrf = base_mrf;
> inst->mlen = mlen;
> @@ -1187,9 +1193,12 @@ fs_visitor::emit_texture_gen7(ir_texture *ir, fs_reg 
> dst, fs_reg coordinate,
> bool header_present = false;
> int offsets[3];
>  
> -   if (ir->offset && ir->op != ir_txf) {
> -  /* The offsets set up by the ir_texture visitor are in the
> +   if (ir->op == ir_tg4 || (ir->offset && ir->op != ir_txf)) {
> +  /* * The offsets set up by the ir_texture visitor are in the
> * m1 header, so we can't go headerless.
> +   *
> +   * * ir4_tg4 needs to place its channel select in the header,
> +   * for interaction with ARB_texture_swizzle

Small typo: ir4_tg4 -> ir_tg4.

Not sure about the two double stars here.  (Guessing they're meant to be
bullets but it looks a little funny.)  Maybe just remove them...

> */
>header_present = true;
>mlen++;
> @@ -1205,6 +1214,7 @@ fs_visitor::emit_texture_gen7(ir_texture *ir, fs_reg 
> dst, fs_reg coordinate,
> switch (ir->op) {
> case ir_tex:
> case ir_lod:
> +   case ir_tg4:
>break;
> case ir_txb:
>emit(MOV(fs_reg(MRF, base_mrf + mlen), lod));
> @@ -1319,6 +1329,7 @@ fs_visitor::emit_texture_gen7(ir_texture *ir, fs_reg 
> dst, fs_reg coordinate,
> case ir_txf_ms: inst = emit(SHADER_OPCODE_TXF_MS, dst); break;
> case ir_txs: inst = emit(SHADER_OPCODE_TXS, dst); break;
> case ir_lod: inst = emit(SHADER_OPCODE_LOD, dst); break;
> +   case ir_tg4: inst = emit(SHADER_OPCODE_TG4, dst); break;
> }
> inst->base_mrf = base_mrf;
> inst->mlen = mlen;
> @@ -1446,6 +1457,24 @@ fs_visitor::visit(ir_texture *ir)
>  */
> int texunit = fp->Base.SamplerUnits[sampler];
>  
> +   if (ir->op == ir_tg4) {
> +  /* When tg4 is used with the degenerate ZERO/ONE swizzles, don't bother
> +   * emitting anything other than setting up the constant result.
> +   */
> +  int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0);
> +  if (swiz == SWIZZLE_ZERO || swiz == SWIZZLE_ONE) {
> +
> + fs_reg res = fs_reg(this, glsl_type::vec4_type);
> + this->result = res;
> +
> + for (int i=0; i<4; i++) {
> +emit(MOV(res, fs_reg(swiz == SWIZZLE_ZERO ? 0.0f : 1.0f)));
> +res.reg_offset++;
> + }
> + return;
> +  }
> +   }
> +
> /* Should be lowered by do_lower_texture_projection */
> assert(!ir->projector);
>  
> @@ -1473,6 +1502,7 @@ fs_visitor::visit(ir_texture *ir)
> switch (ir->op) {
> case ir_tex:
> case ir_lod:
> +   case ir_tg4:
>break;
> case ir_txb:
>ir->lod_info.bias->accept(this);
> @@ -1495,6 +1525,8 @@ fs_visitor::visit(ir_texture *ir)
>ir->lod_info.sample_index->accept(this);
>sample_index = this->result;
>break;
> +   default:
> +  assert(!"Unrecognized texture opcode");
> };
>  
> /* Writemasking doesn't eliminate channels on SIMD8 texture
> @@ -1519,6 +1551

Re: [Mesa-dev] [PATCH V3 06/11] i965: w/a for gather4 green RG32F

[Kenneth Graunke]

On 09/15/2013 02:58 AM, Chris Forbes wrote:
> Signed-off-by: Chris Forbes 
> ---
>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp   | 4 
>  src/mesa/drivers/dri/i965/brw_program.h| 5 +
>  src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 4 
>  src/mesa/drivers/dri/i965/brw_wm.c | 9 +
>  4 files changed, 22 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> index be7aed7..6e1a3f5 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
> @@ -1580,6 +1580,10 @@ uint32_t
>  fs_visitor::gather_channel(ir_texture *ir, int sampler)
>  {
> int swiz = GET_SWZ(c->key.tex.swizzles[sampler], 0 /* red */);
> +   if (c->key.tex.gather_channel_quirk_mask & (1< +  return 2;   /* gather4 sampler is broken for green channel on RG32F --
> +   * we must ask for blue instead.
> +   */
> switch (swiz) {
>case SWIZZLE_X: return 0;
>case SWIZZLE_Y: return 1;
> diff --git a/src/mesa/drivers/dri/i965/brw_program.h 
> b/src/mesa/drivers/dri/i965/brw_program.h
> index e2ddaa8..07be4a0 100644
> --- a/src/mesa/drivers/dri/i965/brw_program.h
> +++ b/src/mesa/drivers/dri/i965/brw_program.h
> @@ -40,6 +40,11 @@ struct brw_sampler_prog_key_data {
>  */
> uint16_t yuvtex_mask;
> uint16_t yuvtex_swap_mask; /**< UV swaped */
> +
> +   /**
> +* For RG32F, gather4's channel select is broken.
> +*/
> +   uint16_t gather_channel_quirk_mask;
>  };
>  
>  #ifdef __cplusplus
> diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp 
> b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> index f23b235..23a6a48 100644
> --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp
> @@ -2374,6 +2374,10 @@ uint32_t
>  vec4_visitor::gather_channel(ir_texture *ir, int sampler)
>  {
> int swiz = GET_SWZ(key->tex.swizzles[sampler], 0 /* red */);
> +   if (key->tex.gather_channel_quirk_mask & (1< +  return 2;   /* gather4 sampler is broken for green channel on RG32F --
> +   * we must ask for blue instead.
> +   */

Ew, right.  I should've kept reading...this function probably is useful
after all...

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965/gen4: Fix fragment program rectangle texture shadow compares.

[Kenneth Graunke]

On 09/18/2013 01:18 PM, Eric Anholt wrote:
> The rescale_texcoord(), if it does something, will return just the
> GLSL-sized coordinate, leaving out the 3rd and 4th components where we
> were storing our projected shadow compare and the texture projector.
> Deref the shadow compare before using the shared rescale-the-coordinate
> code to fix the problem.
> 
> Fixes piglit tex-shadow2drect.shader_test and txp-shadow2drect.shader_test
> 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=69525
> NOTE: This is a candidate for stable branches.
> ---
>  src/mesa/drivers/dri/i965/brw_fs_fp.cpp | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp 
> b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
> index 68531e3..0594948 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_fp.cpp
> @@ -490,15 +490,15 @@ fs_visitor::emit_fragment_program_code()
>   ir_constant_data junk_data;
>   ir->coordinate = new(mem_ctx) ir_constant(coordinate_type, 
> &junk_data);
>  
> - coordinate = rescale_texcoord(ir, coordinate,
> -   fpi->TexSrcTarget == 
> TEXTURE_RECT_INDEX,
> -   fpi->TexSrcUnit, fpi->TexSrcUnit);
> -
>   if (fpi->TexShadow) {
>  shadow_c = regoffset(coordinate, 2);
>  ir->shadow_comparitor = new(mem_ctx) ir_constant(0.0f);
>   }
>  
> + coordinate = rescale_texcoord(ir, coordinate,
> +   fpi->TexSrcTarget == 
> TEXTURE_RECT_INDEX,
> +   fpi->TexSrcUnit, fpi->TexSrcUnit);
> +
>   fs_inst *inst;
>   if (brw->gen >= 7) {
>  inst = emit_texture_gen7(ir, dst, coordinate, shadow_c, lod, 
> dpdy, sample_index);
> 

Pulling out the shadow comparitor first seems obviously correct.

I'm still concerned that rescale_texcoord() might not work since the
SamplerUnits change, though.  It would be nice to use textureSize
instead of this state var uniform stuff.

--Ken
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 69541] ir_constant_expression.cpp(1384) : error C3861: 'isnormal': identifier not found

[bugzilla-daemon]

https://bugs.freedesktop.org/show_bug.cgi?id=69541

--- Comment #1 from Kenneth Graunke  ---
FWIW it looks like MSVC 2013 added isnormal(), but still lacks copysign():

http://blogs.msdn.com/b/vcblog/archive/2013/07/19/c99-library-support-in-visual-studio-2013.aspx

-- 
You are receiving this mail because:
You are the assignee for the bug.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glx: fix compile error in egl_glx.c.

[Kenneth Graunke]

On 09/17/2013 12:46 PM, Gaetan Nadon wrote:
> egl_glx.c:40:22: fatal error: X11/Xlib.h: No such file or directory
> 
> The compiler cannot find the Xlib.h in the installed system headers.
> All supplied include directives point to inside the mesa module.
> The X11_CFLAGS variable is undefined (not defined in config.status).
> 
> It appears the intent was to use X11_INCLUDES defined in configure.ac.
> 
> The Xlib.h file is not installed on my workstation. It is supplied in
> the libx11-dev package. This allows an X developer control over which
> version of this file is used for X development.
> 
> Signed-off-by: Gaetan Nadon 
> ---
>  src/egl/drivers/glx/Makefile.am |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/egl/drivers/glx/Makefile.am b/src/egl/drivers/glx/Makefile.am
> index 6db95b4..5dd5228 100644
> --- a/src/egl/drivers/glx/Makefile.am
> +++ b/src/egl/drivers/glx/Makefile.am
> @@ -23,7 +23,7 @@ AM_CFLAGS = \
>   -I$(top_srcdir)/include \
>   -I$(top_srcdir)/src/egl/main \
>   $(VISIBILITY_CFLAGS) \
> - $(X11_CFLAGS) \
> + $(X11_INCLUDES) \
>   $(DEFINES)
>  
>  noinst_LTLIBRARIES = libegl_glx.la
> 

This looks good to me.  There are a couple of other instances of
X11_CFLAGS in the codebase as well.  Presumably those need to be changed
as well?

Both patches are:
Reviewed-by: Kenneth Graunke 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev