[Mesa-dev] [PATCH 2/2] st/mesa: fix CTS regression caused by fcbb93e86024
Samuel Pitoiset
samuel.pitoiset at gmail.com
Tue Aug 8 09:21:53 UTC 2017
On 08/08/2017 07:34 AM, Timothy Arceri wrote:
> On 01/08/17 19:37, Timothy Arceri wrote:
>> On 01/08/17 18:07, Samuel Pitoiset wrote:
>>> Don't you think it's just safer to revert the bad commit for 17.2 and
>>> fix it later on?
>>
>> I'm not overly worried. If you really want to go that way we can, but
>> I don't think it's necessary.
>
> So how do we move forward on this? Can we have the revert applied to
> 17.2 and commit this to master?
Sorry, missed that. I would prefer to revert yes, but opinions are
welcome. :-)
>
> Ccing Emil for answers :)
>
>>
>>>
>>> On 08/01/2017 09:35 AM, Timothy Arceri wrote:
>>>> When generation the storage offset for struct members we need
>>>> to skip opaque types as they no longer have backing storage.
>>>>
>>>> Fixes: fcbb93e86024 ("mesa: stop assigning unused storage for
>>>> non-bindless opaque types")
>>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101983
>>>> ---
>>>> src/mesa/Makefile.sources | 2 +
>>>> src/mesa/state_tracker/st_glsl_to_tgsi.cpp | 16 ++++-
>>>> src/mesa/state_tracker/st_glsl_types.cpp | 105
>>>> +++++++++++++++++++++++++++++
>>>> src/mesa/state_tracker/st_glsl_types.h | 44 ++++++++++++
>>>> 4 files changed, 165 insertions(+), 2 deletions(-)
>>>> create mode 100644 src/mesa/state_tracker/st_glsl_types.cpp
>>>> create mode 100644 src/mesa/state_tracker/st_glsl_types.h
>>>>
>>>> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
>>>> index 97c8287acb..2c557c3952 100644
>>>> --- a/src/mesa/Makefile.sources
>>>> +++ b/src/mesa/Makefile.sources
>>>> @@ -500,20 +500,22 @@ STATETRACKER_FILES = \
>>>> state_tracker/st_extensions.c \
>>>> state_tracker/st_extensions.h \
>>>> state_tracker/st_format.c \
>>>> state_tracker/st_format.h \
>>>> state_tracker/st_gen_mipmap.c \
>>>> state_tracker/st_gen_mipmap.h \
>>>> state_tracker/st_gl_api.h \
>>>> state_tracker/st_glsl_to_nir.cpp \
>>>> state_tracker/st_glsl_to_tgsi.cpp \
>>>> state_tracker/st_glsl_to_tgsi.h \
>>>> +- state_tracker/st_glsl_types.cpp \
>>>> +- state_tracker/st_glsl_types.h \
>>>> state_tracker/st_manager.c \
>>>> state_tracker/st_manager.h \
>>>> state_tracker/st_mesa_to_tgsi.c \
>>>> state_tracker/st_mesa_to_tgsi.h \
>>>> state_tracker/st_nir.h \
>>>> state_tracker/st_nir_lower_builtin.c \
>>>> state_tracker/st_nir_lower_tex_src_plane.c \
>>>> state_tracker/st_pbo.c \
>>>> state_tracker/st_pbo.h \
>>>> state_tracker/st_program.c \
>>>> diff --git a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> index 2fca398a51..54a749d388 100644
>>>> --- a/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> +++ b/src/mesa/state_tracker/st_glsl_to_tgsi.cpp
>>>> @@ -42,20 +42,21 @@
>>>> #include "main/shaderapi.h"
>>>> #include "main/shaderimage.h"
>>>> #include "program/prog_instruction.h"
>>>> #include "pipe/p_context.h"
>>>> #include "pipe/p_screen.h"
>>>> #include "tgsi/tgsi_ureg.h"
>>>> #include "tgsi/tgsi_info.h"
>>>> #include "util/u_math.h"
>>>> #include "util/u_memory.h"
>>>> +#include "st_glsl_types.h"
>>>> #include "st_program.h"
>>>> #include "st_mesa_to_tgsi.h"
>>>> #include "st_format.h"
>>>> #include "st_nir.h"
>>>> #include "st_shader_cache.h"
>>>> #include "util/hash_table.h"
>>>> #include <algorithm>
>>>> #define PROGRAM_ANY_CONST ((1 << PROGRAM_STATE_VAR) | \
>>>> @@ -2825,22 +2826,30 @@ shrink_array_declarations(struct inout_decl
>>>> *decls, unsigned count,
>>>> *usage_mask |= BITFIELD64_BIT(decl->mesa_index + j);
>>>> }
>>>> }
>>>> }
>>>> void
>>>> glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
>>>> {
>>>> ir_constant *index;
>>>> st_src_reg src;
>>>> - int element_size = type_size(ir->type);
>>>> bool is_2D = false;
>>>> + ir_variable *var = ir->variable_referenced();
>>>> +
>>>> + /* We only need the logic provided by st_glsl_storage_type_size()
>>>> + * for arrays of structs. Indirect sampler and image indexing is
>>>> handled
>>>> + * elsewhere.
>>>> + */
>>>> + int element_size = ir->type->without_array()->is_record() ?
>>>> + st_glsl_storage_type_size(ir->type, var->data.bindless) :
>>>> + type_size(ir->type);
>>>> index = ir->array_index->constant_expression_value();
>>>> ir->array->accept(this);
>>>> src = this->result;
>>>> if (ir->array->ir_type != ir_type_dereference_array) {
>>>> switch (this->prog->Target) {
>>>> case GL_TESS_CONTROL_PROGRAM_NV:
>>>> is_2D = (src.file == PROGRAM_INPUT || src.file ==
>>>> PROGRAM_OUTPUT) &&
>>>> @@ -2916,28 +2925,31 @@
>>>> glsl_to_tgsi_visitor::visit(ir_dereference_array *ir)
>>>> src.type = ir->type->base_type;
>>>> this->result = src;
>>>> }
>>>> void
>>>> glsl_to_tgsi_visitor::visit(ir_dereference_record *ir)
>>>> {
>>>> unsigned int i;
>>>> const glsl_type *struct_type = ir->record->type;
>>>> + ir_variable *var = ir->record->variable_referenced();
>>>> int offset = 0;
>>>> ir->record->accept(this);
>>>> + assert(var);
>>>> for (i = 0; i < struct_type->length; i++) {
>>>> if (strcmp(struct_type->fields.structure[i].name, ir->field)
>>>> == 0)
>>>> break;
>>>> - offset += type_size(struct_type->fields.structure[i].type);
>>>> + const glsl_type *member_type =
>>>> struct_type->fields.structure[i].type;
>>>> + offset += st_glsl_storage_type_size(member_type,
>>>> var->data.bindless);
>>>> }
>>>> /* If the type is smaller than a vec4, replicate the last
>>>> channel out. */
>>>> if (ir->type->is_scalar() || ir->type->is_vector())
>>>> this->result.swizzle =
>>>> swizzle_for_size(ir->type->vector_elements);
>>>> else
>>>> this->result.swizzle = SWIZZLE_NOOP;
>>>> this->result.index += offset;
>>>> this->result.type = ir->type->base_type;
>>>> diff --git a/src/mesa/state_tracker/st_glsl_types.cpp
>>>> b/src/mesa/state_tracker/st_glsl_types.cpp
>>>> new file mode 100644
>>>> index 0000000000..50936025d9
>>>> --- /dev/null
>>>> +++ b/src/mesa/state_tracker/st_glsl_types.cpp
>>>> @@ -0,0 +1,105 @@
>>>> +/*
>>>> + * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
>>>> + * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
>>>> + * Copyright © 2010 Intel Corporation
>>>> + * Copyright © 2011 Bryan Cain
>>>> + *
>>>> + * 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 "st_glsl_types.h"
>>>> +
>>>> +/**
>>>> + * Returns the number of places to offset the uniform index, given
>>>> the type of
>>>> + * a struct member. We use this because samplers and images have
>>>> backing
>>>> + * storeage only when they are bindless.
>>>> + */
>>>> +int
>>>> +st_glsl_storage_type_size(const struct glsl_type *type, bool
>>>> is_bindless)
>>>> +{
>>>> + unsigned int i;
>>>> + int size;
>>>> +
>>>> + switch (type->base_type) {
>>>> + case GLSL_TYPE_UINT:
>>>> + case GLSL_TYPE_INT:
>>>> + case GLSL_TYPE_FLOAT:
>>>> + case GLSL_TYPE_BOOL:
>>>> + if (type->is_matrix()) {
>>>> + return type->matrix_columns;
>>>> + } else {
>>>> + /* Regardless of size of vector, it gets a vec4. This is bad
>>>> + * packing for things like floats, but otherwise arrays
>>>> become a
>>>> + * mess. Hopefully a later pass over the code can pack
>>>> scalars
>>>> + * down if appropriate.
>>>> + */
>>>> + return 1;
>>>> + }
>>>> + break;
>>>> + case GLSL_TYPE_DOUBLE:
>>>> + if (type->is_matrix()) {
>>>> + if (type->vector_elements <= 2)
>>>> + return type->matrix_columns;
>>>> + else
>>>> + return type->matrix_columns * 2;
>>>> + } else {
>>>> + /* For doubles if we have a double or dvec2 they fit in one
>>>> + * vec4, else they need 2 vec4s.
>>>> + */
>>>> + if (type->vector_elements <= 2)
>>>> + return 1;
>>>> + else
>>>> + return 2;
>>>> + }
>>>> + break;
>>>> + case GLSL_TYPE_UINT64:
>>>> + case GLSL_TYPE_INT64:
>>>> + if (type->vector_elements <= 2)
>>>> + return 1;
>>>> + else
>>>> + return 2;
>>>> + case GLSL_TYPE_ARRAY:
>>>> + assert(type->length > 0);
>>>> + return st_glsl_storage_type_size(type->fields.array,
>>>> is_bindless) *
>>>> + type->length;
>>>> + case GLSL_TYPE_STRUCT:
>>>> + size = 0;
>>>> + for (i = 0; i < type->length; i++) {
>>>> + size +=
>>>> st_glsl_storage_type_size(type->fields.structure[i].type,
>>>> + is_bindless);
>>>> + }
>>>> + return size;
>>>> + case GLSL_TYPE_SAMPLER:
>>>> + case GLSL_TYPE_IMAGE:
>>>> + if (!is_bindless)
>>>> + return 0;
>>>> + /* fall through */
>>>> + case GLSL_TYPE_SUBROUTINE:
>>>> + return 1;
>>>> + case GLSL_TYPE_ATOMIC_UINT:
>>>> + case GLSL_TYPE_INTERFACE:
>>>> + case GLSL_TYPE_VOID:
>>>> + case GLSL_TYPE_ERROR:
>>>> + case GLSL_TYPE_FUNCTION:
>>>> + assert(!"Invalid type in type_size");
>>>> + break;
>>>> + }
>>>> + return 0;
>>>> +}
>>>> diff --git a/src/mesa/state_tracker/st_glsl_types.h
>>>> b/src/mesa/state_tracker/st_glsl_types.h
>>>> new file mode 100644
>>>> index 0000000000..915816d1fa
>>>> --- /dev/null
>>>> +++ b/src/mesa/state_tracker/st_glsl_types.h
>>>> @@ -0,0 +1,44 @@
>>>> +/*
>>>> + * Copyright (C) 2005-2007 Brian Paul All Rights Reserved.
>>>> + * Copyright (C) 2008 VMware, Inc. All Rights Reserved.
>>>> + * Copyright © 2010 Intel Corporation
>>>> + * Copyright © 2011 Bryan Cain
>>>> + *
>>>> + * 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.
>>>> + */
>>>> +
>>>> +#ifndef __ST_GLSL_TYPES_H__
>>>> +#define __ST_GLSL_TYPES_H__
>>>> +
>>>> +#include "compiler/glsl_types.h"
>>>> +
>>>> +#ifdef __cplusplus
>>>> +extern "C" {
>>>> +#endif
>>>> +
>>>> +int st_glsl_storage_type_size(const struct glsl_type *type,
>>>> + bool is_bindless);
>>>> +
>>>> +
>>>> +#ifdef __cplusplus
>>>> +}
>>>> +#endif
>>>> +
>>>> +#endif /* __ST_GLSL_TYPES_H__ */
>>>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list