[Mesa-dev] [Bug 91099] [llvmpipe] piglit glsl-max-varyings >max_varying_components regression
https://bugs.freedesktop.org/show_bug.cgi?id=91099 José Fonseca changed: What|Removed |Added CC||i...@freedesktop.org Blocks||90539 --- Comment #2 from José Fonseca --- This is the same as bug 90539. And the change in http://cgit.freedesktop.org/~jrfonseca/mesa/commit/?h=max-varyings fixes the assertion. -- You are receiving this mail because: You are the QA Contact for the bug. 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 90539] [softpipe] piglit varying-packing-simple dmat3 array regression
https://bugs.freedesktop.org/show_bug.cgi?id=90539 José Fonseca changed: What|Removed |Added Depends on||91099 -- You are receiving this mail because: You are the QA Contact for the bug. 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] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
Hi, The ARB asm programs only allow indirect addressing into the constant buffer. Only GLSL allows indirect addressing everywhere and GLSL-to-TGSI already generates proper array declarations, so we should be fine. I think we can just assume that indirect addressing into TEMP, INPUT, or OUTPUT with ArrayID==0 is an invalid instruction and should be disallowed and guarded by assert(). Of course, all drivers must support PIPE_SHADER_CAP_TGSI_ANY_INOUT_DECL_RANGE first, so that GLSL-to-TGSI can use the array declarations. Marek On Thu, Jun 25, 2015 at 5:17 AM, Rob Clark wrote: > I'd be a big fan of letting it go.. if needed for old ARB shader > stuff we can give entire IN/OUT file ArrayID==1.. if someone points me > in the right direction I don't mind going and poking at it since the > whole assuming-the-worst thing if ArrayID==0 is not so nice for > register allocation.. > > BR, > -R > > On Wed, Jun 24, 2015 at 10:38 PM, Roland Scheidegger > wrote: >> I thought this was needed by something in the gl state tracker. relative >> addressing with ARB_vp maybe, can't remember... >> I dunno if all places where this is used are fixed up it can go. >> >> Roland >> >> >> Am 25.06.2015 um 01:31 schrieb Rob Clark: >>> tgsi.rst currently says: >>> >>> >>> If no ArrayID is specified with an indirect addressing operand the whole >>> register file might be accessed by this operand. This is strongly >>> discouraged >>> and will prevent packing of scalar/vec2 arrays and effective alias analysis. >>> >>> >>> I'd be pretty happy to remove it and replace it w/ something to the >>> effect of indirect addressing where no ArrayID is specified is >>> undefined :-) >>> >>> BR, >>> -R >>> >>> On Wed, Jun 24, 2015 at 6:17 PM, Marek Olšák wrote: Indirect addressing without the ArrayID is undefined in the general case. You need the ArrayID to be able to tell where the array declaration starts and what its semantic name is. Marek On Wed, Jun 24, 2015 at 8:35 PM, Rob Clark wrote: > On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote: >> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact >> way to add declarations. > > ok.. hmm, I guess tgsi.rst needs an update then.. > >> I recently added the array support for inputs and outputs, we just >> need to enable it on non-radeon non-swrast drivers: >> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5 > > ok, in principle (after fixing ttn based on the assumption that we > won't get indirect access if ArrayID==0, and now that tgsi f/e is > dropped) freedreno should be ok for array in/out's.. I'll enable the > cap and see what happens after fixing ttn and debugging some indirect > issues (seems either there is something I don't understand about the > hw yet, or maybe an a4xx bug.. right now shaders that I think should > work aren't and I'm in android hell trying to get some more traces > from blob :-/) > > BR, > -R > >> It would be nice to enable the arrays on all drivers instead of >> working around it indefinitely. >> >> Marek >> >> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote: >>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it >>> is safe to assume that we always get ArrayID that makes it much >>> easier. >>> >>> BR, >>> -R >>> >>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote: It's not an array, because the ArrayID is 0. It's a valid non-array declaration. If any TGSI user doesn't understand it, that user should be fixed. Marek On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote: > From: Rob Clark > > Ok, so actually there is a ttn issue here to fix as well.. but it > brought up a question in my mind. When ttn sees something like > > DCL IN[0..1] > > it will treat that as an array (which in the end will result in > constraints about where the registers get allocated. Which is not > really ideal. > > With glsl we don't actually get input arrays (but instead a bunch > of MOV's to a TEMP array) currently. So I'm not quite sure how > an actual input array should look. (But my preference would be > IN[a..b] for arrays and IN[c] otherwise) > --- > src/gallium/auxiliary/hud/hud_context.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/src/gallium/auxiliary/hud/hud_context.c > b/src/gallium/auxiliary/hud/hud_context.c > index 6a124f7..2b6d3a7 100644 > --- a/src/gallium/auxiliary/hud/hud_context.c > +++ b/src/gallium/auxiliary/hud/hud_context.c > @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct >
Re: [Mesa-dev] [PATCH 1/5] mesa: Fix errors values returned by glShaderBinary()
On 06/09/2015 07:41 PM, Ian Romanick wrote:
> On 06/04/2015 06:38 PM, Ben Widawsky wrote:
>> On Wed, Mar 11, 2015 at 10:01:24AM +0100, Eduardo Lima Mitev wrote:
>>> Page 68, section 7.2 'Shader Binaries" of the of the OpenGL ES 3.1,
>>> and page 88 of the OpenGL 4.5 specs state:
>>>
>>> "An INVALID_VALUE error is generated if count or length is negative.
>>> An INVALID_ENUM error is generated if binaryformat is not a supported
>>> format returned in SHADER_BINARY_FORMATS."
>>>
>>> Currently, an INVALID_OPERATION error is returned for all cases.
>>>
>>> Fixes 1 dEQP test:
>>> * dEQP-GLES3.functional.negative_api.shader.shader_binary
>>> ---
>>> src/mesa/main/shaderapi.c | 15 ++-
>>> 1 file changed, 14 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/mesa/main/shaderapi.c b/src/mesa/main/shaderapi.c
>>> index 5731d58..49a9c80 100644
>>> --- a/src/mesa/main/shaderapi.c
>>> +++ b/src/mesa/main/shaderapi.c
>>> @@ -1711,7 +1711,20 @@ _mesa_ShaderBinary(GLint n, const GLuint* shaders,
>>> GLenum binaryformat,
>>> (void) binaryformat;
>>> (void) binary;
>>> (void) length;
>>> - _mesa_error(ctx, GL_INVALID_OPERATION, "glShaderBinary");
>>> +
>>> + /* Page 68, section 7.2 'Shader Binaries" of the of the OpenGL ES 3.1,
>>> and
>>> +* page 88 of the OpenGL 4.5 specs state:
>>> +*
>>> +* "An INVALID_VALUE error is generated if count or length is
>>> negative.
>>> +* An INVALID_ENUM error is generated if binaryformat is not a
>>> supported
>>> +* format returned in SHADER_BINARY_FORMATS."
>>> +*/
>>> + if (n < 0 || length < 0) {
>>> + _mesa_error(ctx, GL_INVALID_VALUE, "glShaderBinary(count or length <
>>> 0)");
>>> + return;
>>> + }
>>> +
>>> + _mesa_error(ctx, GL_INVALID_ENUM, "glShaderBinary(format)");
>>> }
>>>
>
> There are some generic errors that apply to all GL functions. These are
> listed in section 2.3.1 (Errors). The full text from section 7.2 (from
> the GL 4.4 spec) says:
>
> An INVALID_VALUE error is generated if count or length is negative.
>
> An INVALID_ENUM error is generated if binaryformat is not a
> supported format returned in SHADER_BINARY_FORMATS.
>
> An INVALID_VALUE error is generated if the data pointed to by binary
> does not match the specified binaryformat.
>
> An INVALID_VALUE error is generated if any of the handles in
> shaders is not the name of either a program or shader object.
>
> An INVALID_OPERATION error is generated if any of the handles in
> shader is the name of a program object.
>
> An INVALID_OPERATION error is generated if more than one of the
> handles in shaders refers to the same type of shader object.
>
> Additional errors corresponding to specific binary formats may be
> generated as specified by the extensions defining those formats.
>
> If we're going to add code to generate two of the specified error codes,
> we probably ought to add code to generate the other as well.
>
I agree with this in general, but in this particular case since loading
binary shaders is not supported by mesa, we will never reach past the
2nd error check on that list:
An INVALID_ENUM error is generated if binaryformat is not a
supported format returned in SHADER_BINARY_FORMATS.
So it is pointless to validate further.
The original patch adds the only check that, per the spec, goes earlier.
cheers,
Eduardo
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/5] mesa: Fix error returned by glCopyTexImage2D() upon an invalid internal format
On 06/05/2015 03:43 AM, Ben Widawsky wrote:
> On Wed, Mar 11, 2015 at 10:01:25AM +0100, Eduardo Lima Mitev wrote:
>>
>> Fixes 1 dEQP test:
>> * dEQP-GLES3.functional.negative_api.texture.copyteximage2d_invalid_format
>> ---
>> src/mesa/main/teximage.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
>> index 7b1a0e6..6382114 100644
>> --- a/src/mesa/main/teximage.c
>> +++ b/src/mesa/main/teximage.c
>> @@ -2619,7 +2619,7 @@ copytexture_error_check( struct gl_context *ctx,
>> GLuint dimensions,
>>
>> rb = _mesa_get_read_renderbuffer_for_format(ctx, internalFormat);
>> if (rb == NULL) {
>> - _mesa_error(ctx, GL_INVALID_OPERATION,
>> + _mesa_error(ctx, GL_INVALID_ENUM,
>>"glCopyTexImage%dD(read buffer)", dimensions);
>>return GL_TRUE;
>> }
>
> Hmm. Now I wonder if I am reading the wrong docs.
> (https://www.khronos.org/opengles/sdk/docs/man/xhtml/glCopyTexImage2D.xml)
>
> GL_INVALID_ENUM is generated if internalformat is not an accepted format.
>
> internalformat
> Specifies the internal format of the texture.
> Must be one of the following symbolic constants:
> GL_ALPHA,
> GL_LUMINANCE,
> GL_LUMINANCE_ALPHA,
> GL_RGB, or
> GL_RGBA.
>
>
> Would have expected this:
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index 7bc1da7..5ca774b 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -2648,7 +2648,7 @@ copytexture_error_check( struct gl_context *ctx, GLuint
> dimensions,
>case GL_LUMINANCE_ALPHA:
> break;
>default:
> - _mesa_error(ctx, GL_INVALID_VALUE,
> + _mesa_error(ctx, GL_INVALID_ENUM,
> "glCopyTexImage%dD(internalFormat=%s)", dimensions,
> _mesa_lookup_enum_by_nr(internalFormat));
>
Indeed. The exact issue addressed by this patch has been fixed already
at some point after the patch was sent back in March. That's why you see
a GL_INVALID_VALUE instead of the GL_INVALID_OPERATION from my patch.
However, the dEQP test in question keeps failing because this code was
added before checking for validity of internalformat:
rb = _mesa_get_read_renderbuffer_for_format(ctx, internalFormat);
if (rb == NULL) {
_mesa_error(ctx, GL_INVALID_OPERATION, "glCopyTexImage%dD(read
buffer)", dimensions);
return GL_TRUE;
}
So, I will write a new patch to fix the failing test. Current patch is
no longer relevant.
Thanks!
Eduardo
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 05/14] meta: Abort meta pbo path if readpixels need signed-unsigned conversion
On Wed, 2015-06-24 at 18:39 -0700, Anuj Phogat wrote:
> Meta pbo path for ReadPixels rely on BlitFramebuffer which doesn't support
> signed to unsigned integer conversions and vice versa.
>
> Without this patch, piglit test fbo_integer_readpixels_sint_uint fails, when
> forced to use the meta pbo path.
>
> v2: Make need_rgb_to_luminance_conversion() a static function. (Iago)
I think you meant "need_signed_unsigned_int_conversion()" here
Other than that:
Reviewed-by: Iago Toral Quiroga
> Bump up the comment and the commit message. (Jason)
>
> Signed-off-by: Anuj Phogat
> Reviewed-by: Jason Ekstrand
> Cc: Iago Toral
> Cc:
> ---
> src/mesa/drivers/common/meta_tex_subimage.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
> b/src/mesa/drivers/common/meta_tex_subimage.c
> index 00364f8..a617b77 100644
> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> @@ -248,6 +248,23 @@ fail:
> return success;
> }
>
> +static bool
> +need_signed_unsigned_int_conversion(mesa_format rbFormat,
> +GLenum format, GLenum type)
> +{
> + const GLenum srcType = _mesa_get_format_datatype(rbFormat);
> + return (srcType == GL_INT &&
> + _mesa_is_enum_format_integer(format) &&
> + (type == GL_UNSIGNED_INT ||
> +type == GL_UNSIGNED_SHORT ||
> +type == GL_UNSIGNED_BYTE)) ||
> + (srcType == GL_UNSIGNED_INT &&
> + _mesa_is_enum_format_integer(format) &&
> + (type == GL_INT ||
> +type == GL_SHORT ||
> +type == GL_BYTE));
> +}
> +
> bool
> _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
>struct gl_texture_image *tex_image,
> @@ -283,6 +300,14 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
> GLuint dims,
>
>if (_mesa_need_rgb_to_luminance_conversion(rb->Format, format))
> return false;
> +
> + /* This function rely on BlitFramebuffer to fill in the pixel data for
> + * ReadPixels. But, BlitFrameBuffer doesn't support signed to unsigned
> + * or unsigned to signed integer conversions. OpenGL spec expects an
> + * invalid operation in that case.
> + */
> + if (need_signed_unsigned_int_conversion(rb->Format, format, type))
> + return false;
> }
>
> /* For arrays, use a tall (height * depth) 2D texture but taking into
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radeonsi: add support for viewport array (v2)
From: Dave Airlie
This isn't pretty and I'd suggest it the pm4 interface builder
could be tweaked to do this more efficently, but I'd need
guidance on how that would look.
This seems to pass the few piglit tests I threw at it.
v2: handle passing layer/viewport index to fragment shader.
fix crash in blit changes,
add support to io_get_unique_index for layer/viewport index
update docs.
Signed-off-by: Dave Airlie
---
docs/GL3.txt| 4 +-
docs/relnotes/10.7.0.html | 3 ++
src/gallium/drivers/radeonsi/si_blit.c | 8 +--
src/gallium/drivers/radeonsi/si_pipe.c | 2 +-
src/gallium/drivers/radeonsi/si_shader.c| 26 +++---
src/gallium/drivers/radeonsi/si_state.c | 66 +++--
src/gallium/drivers/radeonsi/si_state.h | 4 +-
src/gallium/drivers/radeonsi/si_state_shaders.c | 2 -
8 files changed, 73 insertions(+), 42 deletions(-)
diff --git a/docs/GL3.txt b/docs/GL3.txt
index 220bcc8..df913bd 100644
--- a/docs/GL3.txt
+++ b/docs/GL3.txt
@@ -128,7 +128,7 @@ GL 4.1, GLSL 4.10:
GL_ARB_separate_shader_objects DONE (all drivers)
GL_ARB_shader_precision started (Micah)
GL_ARB_vertex_attrib_64bit DONE (nvc0, softpipe)
- GL_ARB_viewport_arrayDONE (i965, nv50, nvc0,
r600, llvmpipe)
+ GL_ARB_viewport_arrayDONE (i965, nv50, nvc0,
r600, radeonsi, llvmpipe)
GL 4.2, GLSL 4.20:
@@ -156,7 +156,7 @@ GL 4.3, GLSL 4.30:
GL_ARB_copy_imageDONE (i965) (gallium -
in progress, VMware)
GL_KHR_debug DONE (all drivers)
GL_ARB_explicit_uniform_location DONE (all drivers that
support GLSL)
- GL_ARB_fragment_layer_viewport DONE (nv50, nvc0, r600,
llvmpipe)
+ GL_ARB_fragment_layer_viewport DONE (nv50, nvc0, r600,
radeonsi, llvmpipe)
GL_ARB_framebuffer_no_attachmentsDONE (i965)
GL_ARB_internalformat_query2 not started
GL_ARB_invalidate_subdataDONE (all drivers)
diff --git a/docs/relnotes/10.7.0.html b/docs/relnotes/10.7.0.html
index e089889..fcc5081 100644
--- a/docs/relnotes/10.7.0.html
+++ b/docs/relnotes/10.7.0.html
@@ -44,8 +44,11 @@ Note: some of the new features are only available with
certain drivers.
+GL_AMD_vertex_shader_viewport_index on radeonsi
GL_ARB_framebuffer_no_attachments on i965
GL_ARB_shader_stencil_export on llvmpipe
+GL_ARB_viewport_array on radeonsi
+GL_ARB_fragment_layer_viewport on radeonsi
Bug fixes
diff --git a/src/gallium/drivers/radeonsi/si_blit.c
b/src/gallium/drivers/radeonsi/si_blit.c
index 1f2c408..6c7b383 100644
--- a/src/gallium/drivers/radeonsi/si_blit.c
+++ b/src/gallium/drivers/radeonsi/si_blit.c
@@ -63,11 +63,11 @@ static void si_blitter_begin(struct pipe_context *ctx, enum
si_blitter_op op)
util_blitter_save_sample_mask(sctx->blitter,
sctx->queued.named.sample_mask->sample_mask);
}
- if (sctx->queued.named.viewport) {
- util_blitter_save_viewport(sctx->blitter,
&sctx->queued.named.viewport->viewport);
+ if (sctx->queued.named.viewport[0]) {
+ util_blitter_save_viewport(sctx->blitter,
&sctx->queued.named.viewport[0]->viewport);
}
- if (sctx->queued.named.scissor) {
- util_blitter_save_scissor(sctx->blitter,
&sctx->queued.named.scissor->scissor);
+ if (sctx->queued.named.scissor[0]) {
+ util_blitter_save_scissor(sctx->blitter,
&sctx->queued.named.scissor[0]->scissor);
}
util_blitter_save_vertex_buffer_slot(sctx->blitter,
sctx->vertex_buffer);
util_blitter_save_so_targets(sctx->blitter,
sctx->b.streamout.num_targets,
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c
b/src/gallium/drivers/radeonsi/si_pipe.c
index 53ae71a..480a301 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -335,7 +335,7 @@ static int si_get_param(struct pipe_screen* pscreen, enum
pipe_cap param)
return 8;
case PIPE_CAP_MAX_VIEWPORTS:
- return 1;
+ return 16;
/* Timer queries, present when the clock frequency is non zero. */
case PIPE_CAP_QUERY_TIMESTAMP:
diff --git a/src/gallium/drivers/radeonsi/si_shader.c
b/src/gallium/drivers/radeonsi/si_shader.c
index 47e5f96..87608a1 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -125,12 +125,16 @@ unsigned si_shader_io_get_unique_index(unsigned
semantic_name, unsigned index)
return 0;
case TGSI_SEMANTIC_PSIZE:
return 1;
+ case TGSI_SEMANTIC_LAYER:
+
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
I'd be a big fan of letting it go.. if needed for old ARB shader
stuff we can give entire IN/OUT file ArrayID==1.. if someone points me
in the right direction I don't mind going and poking at it since the
whole assuming-the-worst thing if ArrayID==0 is not so nice for
register allocation..
BR,
-R
On Wed, Jun 24, 2015 at 10:38 PM, Roland Scheidegger wrote:
> I thought this was needed by something in the gl state tracker. relative
> addressing with ARB_vp maybe, can't remember...
> I dunno if all places where this is used are fixed up it can go.
>
> Roland
>
>
> Am 25.06.2015 um 01:31 schrieb Rob Clark:
>> tgsi.rst currently says:
>>
>>
>> If no ArrayID is specified with an indirect addressing operand the whole
>> register file might be accessed by this operand. This is strongly discouraged
>> and will prevent packing of scalar/vec2 arrays and effective alias analysis.
>>
>>
>> I'd be pretty happy to remove it and replace it w/ something to the
>> effect of indirect addressing where no ArrayID is specified is
>> undefined :-)
>>
>> BR,
>> -R
>>
>> On Wed, Jun 24, 2015 at 6:17 PM, Marek Olšák wrote:
>>> Indirect addressing without the ArrayID is undefined in the general
>>> case. You need the ArrayID to be able to tell where the array
>>> declaration starts and what its semantic name is.
>>>
>>> Marek
>>>
>>> On Wed, Jun 24, 2015 at 8:35 PM, Rob Clark wrote:
On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
> way to add declarations.
ok.. hmm, I guess tgsi.rst needs an update then..
> I recently added the array support for inputs and outputs, we just
> need to enable it on non-radeon non-swrast drivers:
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
ok, in principle (after fixing ttn based on the assumption that we
won't get indirect access if ArrayID==0, and now that tgsi f/e is
dropped) freedreno should be ok for array in/out's.. I'll enable the
cap and see what happens after fixing ttn and debugging some indirect
issues (seems either there is something I don't understand about the
hw yet, or maybe an a4xx bug.. right now shaders that I think should
work aren't and I'm in android hell trying to get some more traces
from blob :-/)
BR,
-R
> It would be nice to enable the arrays on all drivers instead of
> working around it indefinitely.
>
> Marek
>
> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
>> is safe to assume that we always get ArrayID that makes it much
>> easier.
>>
>> BR,
>> -R
>>
>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
>>> It's not an array, because the ArrayID is 0. It's a valid non-array
>>> declaration. If any TGSI user doesn't understand it, that user should
>>> be fixed.
>>>
>>> Marek
>>>
>>> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
From: Rob Clark
Ok, so actually there is a ttn issue here to fix as well.. but it
brought up a question in my mind. When ttn sees something like
DCL IN[0..1]
it will treat that as an array (which in the end will result in
constraints about where the registers get allocated. Which is not
really ideal.
With glsl we don't actually get input arrays (but instead a bunch
of MOV's to a TEMP array) currently. So I'm not quite sure how
an actual input array should look. (But my preference would be
IN[a..b] for arrays and IN[c] otherwise)
---
src/gallium/auxiliary/hud/hud_context.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/hud/hud_context.c
b/src/gallium/auxiliary/hud/hud_context.c
index 6a124f7..2b6d3a7 100644
--- a/src/gallium/auxiliary/hud/hud_context.c
+++ b/src/gallium/auxiliary/hud/hud_context.c
@@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
cso_context *cso)
{
static const char *vertex_shader_text = {
"VERT\n"
- "DCL IN[0..1]\n"
+ "DCL IN[0]\n"
+ "DCL IN[1]\n"
"DCL OUT[0], POSITION\n"
"DCL OUT[1], COLOR[0]\n" /* color */
"DCL OUT[2], GENERIC[0]\n" /* texcoord */
--
2.4.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>> ___
>> mesa-dev mailing list
>> mesa-dev@lists.freedes
[Mesa-dev] [PATCH] i965/skl: Use more compact hiz dimensions
gen8 had some special restrictions which don't seem to carry over to gen9.
Quoting the spec for SKL:
"The Z_Height and Z_Width values must equal those present in
3DSTATE_DEPTH_BUFFER incremented by one."
This fixes nothing in piglit (and regresses nothing).
Cc: Jordan Justen
Signed-off-by: Ben Widawsky
---
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 32 ++-
1 file changed, 17 insertions(+), 15 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 6aa969a..432a47c 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -1550,21 +1550,23 @@ intel_gen8_hiz_buf_create(struct brw_context *brw,
/* Gen7 PRM Volume 2, Part 1, 11.5.3 "Hierarchical Depth Buffer" documents
* adjustments required for Z_Height and Z_Width based on multisampling.
*/
- switch (mt->num_samples) {
- case 0:
- case 1:
- break;
- case 2:
- case 4:
- z_width *= 2;
- z_height *= 2;
- break;
- case 8:
- z_width *= 4;
- z_height *= 2;
- break;
- default:
- unreachable("unsupported sample count");
+ if (brw->gen < 9) {
+ switch (mt->num_samples) {
+ case 0:
+ case 1:
+ break;
+ case 2:
+ case 4:
+ z_width *= 2;
+ z_height *= 2;
+ break;
+ case 8:
+ z_width *= 4;
+ z_height *= 2;
+ break;
+ default:
+ unreachable("unsupported sample count");
+ }
}
const unsigned vertical_align = 8; /* 'j' in the docs */
--
2.4.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] radeonsi: add support for viewport array
From: Dave Airlie
This isn't pretty and I'd suggest it the pm4 interface builder
could be tweaked to do this more efficently, but I'd need
guidance on how that would look.
This seems to pass the few piglit tests I threw at it.
Signed-off-by: Dave Airlie
---
src/gallium/drivers/radeonsi/si_blit.c | 4 +-
src/gallium/drivers/radeonsi/si_pipe.c | 2 +-
src/gallium/drivers/radeonsi/si_shader.c | 12 +-
src/gallium/drivers/radeonsi/si_state.c | 66
src/gallium/drivers/radeonsi/si_state.h | 4 +-
5 files changed, 56 insertions(+), 32 deletions(-)
diff --git a/src/gallium/drivers/radeonsi/si_blit.c
b/src/gallium/drivers/radeonsi/si_blit.c
index 1f2c408..4560bf7 100644
--- a/src/gallium/drivers/radeonsi/si_blit.c
+++ b/src/gallium/drivers/radeonsi/si_blit.c
@@ -64,10 +64,10 @@ static void si_blitter_begin(struct pipe_context *ctx, enum
si_blitter_op op)
sctx->queued.named.sample_mask->sample_mask);
}
if (sctx->queued.named.viewport) {
- util_blitter_save_viewport(sctx->blitter,
&sctx->queued.named.viewport->viewport);
+ util_blitter_save_viewport(sctx->blitter,
&sctx->queued.named.viewport[0]->viewport);
}
if (sctx->queued.named.scissor) {
- util_blitter_save_scissor(sctx->blitter,
&sctx->queued.named.scissor->scissor);
+ util_blitter_save_scissor(sctx->blitter,
&sctx->queued.named.scissor[0]->scissor);
}
util_blitter_save_vertex_buffer_slot(sctx->blitter,
sctx->vertex_buffer);
util_blitter_save_so_targets(sctx->blitter,
sctx->b.streamout.num_targets,
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c
b/src/gallium/drivers/radeonsi/si_pipe.c
index 53ae71a..480a301 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -335,7 +335,7 @@ static int si_get_param(struct pipe_screen* pscreen, enum
pipe_cap param)
return 8;
case PIPE_CAP_MAX_VIEWPORTS:
- return 1;
+ return 16;
/* Timer queries, present when the clock frequency is non zero. */
case PIPE_CAP_QUERY_TIMESTAMP:
diff --git a/src/gallium/drivers/radeonsi/si_shader.c
b/src/gallium/drivers/radeonsi/si_shader.c
index 47e5f96..3cd439c 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -1128,7 +1128,7 @@ static void si_llvm_export_vs(struct
lp_build_tgsi_context *bld_base,
&si_shader_ctx->radeon_bld.soa.bld_base.uint_bld;
LLVMValueRef args[9];
LLVMValueRef pos_args[4][9] = { { 0 } };
- LLVMValueRef psize_value = NULL, edgeflag_value = NULL, layer_value =
NULL;
+ LLVMValueRef psize_value = NULL, edgeflag_value = NULL, layer_value =
NULL, viewport_index_value = NULL;
unsigned semantic_name, semantic_index;
unsigned target;
unsigned param_count = 0;
@@ -1155,6 +1155,9 @@ handle_semantic:
case TGSI_SEMANTIC_LAYER:
layer_value = outputs[i].values[0];
continue;
+ case TGSI_SEMANTIC_VIEWPORT_INDEX:
+ viewport_index_value = outputs[i].values[0];
+ continue;
case TGSI_SEMANTIC_POSITION:
target = V_008DFC_SQ_EXP_POS;
break;
@@ -1220,11 +1223,13 @@ handle_semantic:
/* Write the misc vector (point size, edgeflag, layer, viewport). */
if (shader->selector->info.writes_psize ||
shader->selector->info.writes_edgeflag ||
+ shader->selector->info.writes_viewport_index ||
shader->selector->info.writes_layer) {
pos_args[1][0] = lp_build_const_int32(base->gallivm, /*
writemask */
shader->selector->info.writes_psize |
(shader->selector->info.writes_edgeflag << 1) |
-
(shader->selector->info.writes_layer << 2));
+
(shader->selector->info.writes_layer << 2) |
+
(shader->selector->info.writes_viewport_index << 3));
pos_args[1][1] = uint->zero; /* EXEC mask */
pos_args[1][2] = uint->zero; /* last export? */
pos_args[1][3] = lp_build_const_int32(base->gallivm,
V_008DFC_SQ_EXP_POS + 1);
@@ -1255,6 +1260,9 @@ handle_semantic:
if (shader->selector->info.writes_layer)
pos_args[1][7] = layer_value;
+
+ if (shader->selector->info.writes_viewport_index)
+ pos_args[1][8] = viewport_index_value;
}
for (i = 0; i < 4; i++)
diff --git a/src/gallium/drivers/radeonsi/si_state.c
Re: [Mesa-dev] [PATCH] configure: use $target_cpu, not $host_cpu when setting asm_arch
On 24.06.2015 22:16, Emil Velikov wrote: > On 24 June 2015 at 10:28, Marek Olšák wrote: >> >> This is how I build 32-bit Mesa: >> >> # Mandatory for 32-bit >> dir=i386-linux-gnu >> build=i686-linux-gnu >> export CFLAGS="-m32 -O2 -g" >> export CXXFLAGS="$CFLAGS" >> export LDFLAGS="-L/usr/lib/$dir" > On Arch/Gentoo (and maybe others) don't need the LDFLAGS, but I'd > suspect it's essential for Debian based distros. I don't need to set LDFLAGS on Debian. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
I thought this was needed by something in the gl state tracker. relative
addressing with ARB_vp maybe, can't remember...
I dunno if all places where this is used are fixed up it can go.
Roland
Am 25.06.2015 um 01:31 schrieb Rob Clark:
> tgsi.rst currently says:
>
>
> If no ArrayID is specified with an indirect addressing operand the whole
> register file might be accessed by this operand. This is strongly discouraged
> and will prevent packing of scalar/vec2 arrays and effective alias analysis.
>
>
> I'd be pretty happy to remove it and replace it w/ something to the
> effect of indirect addressing where no ArrayID is specified is
> undefined :-)
>
> BR,
> -R
>
> On Wed, Jun 24, 2015 at 6:17 PM, Marek Olšák wrote:
>> Indirect addressing without the ArrayID is undefined in the general
>> case. You need the ArrayID to be able to tell where the array
>> declaration starts and what its semantic name is.
>>
>> Marek
>>
>> On Wed, Jun 24, 2015 at 8:35 PM, Rob Clark wrote:
>>> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
way to add declarations.
>>>
>>> ok.. hmm, I guess tgsi.rst needs an update then..
>>>
I recently added the array support for inputs and outputs, we just
need to enable it on non-radeon non-swrast drivers:
http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>>>
>>> ok, in principle (after fixing ttn based on the assumption that we
>>> won't get indirect access if ArrayID==0, and now that tgsi f/e is
>>> dropped) freedreno should be ok for array in/out's.. I'll enable the
>>> cap and see what happens after fixing ttn and debugging some indirect
>>> issues (seems either there is something I don't understand about the
>>> hw yet, or maybe an a4xx bug.. right now shaders that I think should
>>> work aren't and I'm in android hell trying to get some more traces
>>> from blob :-/)
>>>
>>> BR,
>>> -R
>>>
It would be nice to enable the arrays on all drivers instead of
working around it indefinitely.
Marek
On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
> is safe to assume that we always get ArrayID that makes it much
> easier.
>
> BR,
> -R
>
> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
>> It's not an array, because the ArrayID is 0. It's a valid non-array
>> declaration. If any TGSI user doesn't understand it, that user should
>> be fixed.
>>
>> Marek
>>
>> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
>>> From: Rob Clark
>>>
>>> Ok, so actually there is a ttn issue here to fix as well.. but it
>>> brought up a question in my mind. When ttn sees something like
>>>
>>> DCL IN[0..1]
>>>
>>> it will treat that as an array (which in the end will result in
>>> constraints about where the registers get allocated. Which is not
>>> really ideal.
>>>
>>> With glsl we don't actually get input arrays (but instead a bunch
>>> of MOV's to a TEMP array) currently. So I'm not quite sure how
>>> an actual input array should look. (But my preference would be
>>> IN[a..b] for arrays and IN[c] otherwise)
>>> ---
>>> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>>> b/src/gallium/auxiliary/hud/hud_context.c
>>> index 6a124f7..2b6d3a7 100644
>>> --- a/src/gallium/auxiliary/hud/hud_context.c
>>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>>> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> {
>>>static const char *vertex_shader_text = {
>>> "VERT\n"
>>> - "DCL IN[0..1]\n"
>>> + "DCL IN[0]\n"
>>> + "DCL IN[1]\n"
>>> "DCL OUT[0], POSITION\n"
>>> "DCL OUT[1], COLOR[0]\n" /* color */
>>> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
>>> --
>>> 2.4.3
>>>
>>> ___
>>> 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
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] softpipe, llvmpipe: fix PIPE_SHADER_CAP_MAX_INPUTS value
Reviewed-by: Roland Scheidegger And probably reference the other two too (91100,91101) if it fixes them. Roland Am 25.06.2015 um 01:09 schrieb Brian Paul: > Reviewed-by: Brian Paul > > Want to reference bug 91099 in the commit msg? > > > On 06/24/2015 04:59 PM, Marek Olšák wrote: >> From: Marek Olšák >> >> PIPE_MAX_SHADER_INPUTS was recently bumped to 80 because of tessellation. >> --- >> src/gallium/auxiliary/gallivm/lp_bld_limits.h | 2 +- >> src/gallium/auxiliary/tgsi/tgsi_exec.h| 2 +- >> 2 files changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/src/gallium/auxiliary/gallivm/lp_bld_limits.h >> b/src/gallium/auxiliary/gallivm/lp_bld_limits.h >> index 49064fe..050076e 100644 >> --- a/src/gallium/auxiliary/gallivm/lp_bld_limits.h >> +++ b/src/gallium/auxiliary/gallivm/lp_bld_limits.h >> @@ -96,7 +96,7 @@ gallivm_get_shader_param(enum pipe_shader_cap param) >> case PIPE_SHADER_CAP_MAX_CONTROL_FLOW_DEPTH: >> return LP_MAX_TGSI_NESTING; >> case PIPE_SHADER_CAP_MAX_INPUTS: >> - return PIPE_MAX_SHADER_INPUTS; >> + return 32; >> case PIPE_SHADER_CAP_MAX_OUTPUTS: >> return 32; >> case PIPE_SHADER_CAP_MAX_CONST_BUFFER_SIZE: >> diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.h >> b/src/gallium/auxiliary/tgsi/tgsi_exec.h >> index 208640c..e8ee256 100644 >> --- a/src/gallium/auxiliary/tgsi/tgsi_exec.h >> +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.h >> @@ -213,7 +213,7 @@ struct tgsi_sampler >>* input register files, this is the stride between two 1D >>* arrays. >>*/ >> -#define TGSI_EXEC_MAX_INPUT_ATTRIBS PIPE_MAX_SHADER_INPUTS >> +#define TGSI_EXEC_MAX_INPUT_ATTRIBS 32 >> >> /* The maximum number of bytes per constant buffer. >>*/ >> > > ___ > 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 v2 05/14] meta: Abort meta pbo path if readpixels need signed-unsigned conversion
On Wed, Jun 24, 2015 at 6:39 PM, Anuj Phogat wrote:
> Meta pbo path for ReadPixels rely on BlitFramebuffer which doesn't support
> signed to unsigned integer conversions and vice versa.
>
> Without this patch, piglit test fbo_integer_readpixels_sint_uint fails, when
> forced to use the meta pbo path.
>
> v2: Make need_rgb_to_luminance_conversion() a static function. (Iago)
> Bump up the comment and the commit message. (Jason)
>
> Signed-off-by: Anuj Phogat
> Reviewed-by: Jason Ekstrand
> Cc: Iago Toral
> Cc:
> ---
> src/mesa/drivers/common/meta_tex_subimage.c | 25 +
> 1 file changed, 25 insertions(+)
>
> diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
> b/src/mesa/drivers/common/meta_tex_subimage.c
> index 00364f8..a617b77 100644
> --- a/src/mesa/drivers/common/meta_tex_subimage.c
> +++ b/src/mesa/drivers/common/meta_tex_subimage.c
> @@ -248,6 +248,23 @@ fail:
> return success;
> }
>
> +static bool
> +need_signed_unsigned_int_conversion(mesa_format rbFormat,
> +GLenum format, GLenum type)
> +{
> + const GLenum srcType = _mesa_get_format_datatype(rbFormat);
> + return (srcType == GL_INT &&
> + _mesa_is_enum_format_integer(format) &&
> + (type == GL_UNSIGNED_INT ||
> +type == GL_UNSIGNED_SHORT ||
> +type == GL_UNSIGNED_BYTE)) ||
> + (srcType == GL_UNSIGNED_INT &&
> + _mesa_is_enum_format_integer(format) &&
> + (type == GL_INT ||
> +type == GL_SHORT ||
> +type == GL_BYTE));
> +}
> +
> bool
> _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
>struct gl_texture_image *tex_image,
> @@ -283,6 +300,14 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
> GLuint dims,
>
>if (_mesa_need_rgb_to_luminance_conversion(rb->Format, format))
> return false;
> +
> + /* This function rely on BlitFramebuffer to fill in the pixel data for
> + * ReadPixels. But, BlitFrameBuffer doesn't support signed to unsigned
> + * or unsigned to signed integer conversions. OpenGL spec expects an
> + * invalid operation in that case.
> + */
> + if (need_signed_unsigned_int_conversion(rb->Format, format, type))
> + return false;
We should add this to TexSubImage as well. Other than that, R-B still applies.
> }
>
> /* For arrays, use a tall (height * depth) 2D texture but taking into
> --
> 1.9.3
>
> ___
> 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
[Mesa-dev] [PATCH v2 05/14] meta: Abort meta pbo path if readpixels need signed-unsigned conversion
Meta pbo path for ReadPixels rely on BlitFramebuffer which doesn't support
signed to unsigned integer conversions and vice versa.
Without this patch, piglit test fbo_integer_readpixels_sint_uint fails, when
forced to use the meta pbo path.
v2: Make need_rgb_to_luminance_conversion() a static function. (Iago)
Bump up the comment and the commit message. (Jason)
Signed-off-by: Anuj Phogat
Reviewed-by: Jason Ekstrand
Cc: Iago Toral
Cc:
---
src/mesa/drivers/common/meta_tex_subimage.c | 25 +
1 file changed, 25 insertions(+)
diff --git a/src/mesa/drivers/common/meta_tex_subimage.c
b/src/mesa/drivers/common/meta_tex_subimage.c
index 00364f8..a617b77 100644
--- a/src/mesa/drivers/common/meta_tex_subimage.c
+++ b/src/mesa/drivers/common/meta_tex_subimage.c
@@ -248,6 +248,23 @@ fail:
return success;
}
+static bool
+need_signed_unsigned_int_conversion(mesa_format rbFormat,
+GLenum format, GLenum type)
+{
+ const GLenum srcType = _mesa_get_format_datatype(rbFormat);
+ return (srcType == GL_INT &&
+ _mesa_is_enum_format_integer(format) &&
+ (type == GL_UNSIGNED_INT ||
+type == GL_UNSIGNED_SHORT ||
+type == GL_UNSIGNED_BYTE)) ||
+ (srcType == GL_UNSIGNED_INT &&
+ _mesa_is_enum_format_integer(format) &&
+ (type == GL_INT ||
+type == GL_SHORT ||
+type == GL_BYTE));
+}
+
bool
_mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx, GLuint dims,
struct gl_texture_image *tex_image,
@@ -283,6 +300,14 @@ _mesa_meta_pbo_GetTexSubImage(struct gl_context *ctx,
GLuint dims,
if (_mesa_need_rgb_to_luminance_conversion(rb->Format, format))
return false;
+
+ /* This function rely on BlitFramebuffer to fill in the pixel data for
+ * ReadPixels. But, BlitFrameBuffer doesn't support signed to unsigned
+ * or unsigned to signed integer conversions. OpenGL spec expects an
+ * invalid operation in that case.
+ */
+ if (need_signed_unsigned_int_conversion(rb->Format, format, type))
+ return false;
}
/* For arrays, use a tall (height * depth) 2D texture but taking into
--
1.9.3
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder
On Jun 24, 2015 4:29 AM, "Francisco Jerez" wrote:
>
> Jason Ekstrand writes:
>
> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez
wrote:
> >> Jason Ekstrand writes:
> >>
> >>> We want to move these into the builder so that they know the current
> >>> builder's dispatch width. This will be needed by a later commit.
> >>
> >> I very much like the idea of this series, but, why do you need to move
> >> these register manipulators into the builder? The builder is an object
> >> you can use to:
> >> - Manipulate and query parameters affecting code generation.
> >> - Create instructions into the program (::emit and friends).
> >> - Allocate virtual registers from the program (::vgrf and friends).
> >>
> >> offset() and half() logically perform an action on a given register
> >> object (or rather, compute a function of a given register object), not
> >> on a builder object, the builder is only required as an auxiliary
> >> parameter -- Any reason you didn't just pass it as a third parameter?
> >
> > What's required as a third parameter is the current execution size. I
> > could have passed that directly, but I figured that, especially for
> > half(), it would get messed up. I could pass the builder in but I
> > don't see a whole lot of difference between that and what I'm doing
> > right now.
>
> Assembly-wise there's no difference, but it seems inconsistent with both
> the remaining register manipulators and remaining builder methods, and
> IMHO it's kind of an anti-pattern to make something a method that
> doesn't need access to any internal details of the object.
>
> > As is, it's not entirely obvious whether you should call
> > half(reg) on the half-width or full-width builder. I'm not 100% sure
> > what to do about that.
> >
> Actually, does half() really need to know about the builder? AFAICT it
> only needs it because of dispatch_width(), and before doing anything
> useful with it it asserts that it's equal to 16, what points at the
> parameter being redundant. By convention a "half" is a group of 8
> channels (we may want to revise this convention when we implement SIMD32
> -- E.g. make half a group of 16 channels and quarter a group of 8
> channels), so 'half(reg)' could simply be implemented as
> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As
> additional paranoia to catch half() being called on a non-16-aligned
> register you could assert that either 'stride == 0' or 16 divides
> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why
> don't we have a reg_offset already in bytes again?) -- That would also
> catch cases in which the register and builder "widths" get out of sync,
> e.g. if half is called in an already halved register but the builder
> used happens to be of the correct exec_size.
OK, fine, we can pull half() back out. Should offset() stay in the
builder? If not, where should it get its dispatch width.
> >> As offset() and half() don't require access to any private details of
> >> the builder, that would actually improve encapsulation, and would avoid
> >> the dubious overloading of fs_builder::half() with two methods with
> >> completely different semantics.
> >
> > Yeah, I don't really like that either. I just couldn't come up with
> > anything better at the time.
> >
> > Suggestions are very much welcome. But I would like to settle on
> > whatever we do fairly quickly so as to limit the amount of
> > refactoring.
> > --Jason
> >
> >> Thanks.
> >>
> >>> ---
> >>> src/mesa/drivers/dri/i965/brw_fs.cpp | 52 ++
> >>> src/mesa/drivers/dri/i965/brw_fs_builder.h | 46 +
> >>> src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 2 +-
> >>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 60 +--
> >>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 149
++-
> >>> src/mesa/drivers/dri/i965/brw_ir_fs.h| 51 -
> >>> 6 files changed, 182 insertions(+), 178 deletions(-)
> >>>
> >>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >>> index 4f98d63..c13ac7d 100644
> >>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
> >>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
> >>> @@ -267,7 +267,7 @@ fs_visitor::VARYING_PULL_CONSTANT_LOAD(const
fs_builder &bld,
> >>> inst->mlen = 1 + dispatch_width / 8;
> >>> }
> >>>
> >>> - bld.MOV(dst, offset(vec4_result, (const_offset & 3) * scale));
> >>> + bld.MOV(dst, bld.offset(vec4_result, (const_offset & 3) * scale));
> >>> }
> >>>
> >>> /**
> >>> @@ -361,7 +361,12 @@ fs_inst::is_copy_payload(const
brw::simple_allocator &grf_alloc) const
> >>>reg.width = this->src[i].width;
> >>>if (!this->src[i].equals(reg))
> >>> return false;
> >>> - reg = ::offset(reg, 1);
> >>> +
> >>> + if (i < this->header_size) {
> >>> + reg.reg_offset += 1;
> >>> + } else {
> >>> + reg.reg_offset += this->exec_size / 8;
> >>> + }
> >>> }
> >>>
Re: [Mesa-dev] [PATCH 1/4] i965/bxt: Add basic Broxton infrastructure
I Have successfully tested and validate patch 1,3,4 on BXT Regards, Philippe Intel Corporation NV/SA Kings Square, Veldkant 31 2550 Kontich RPM (Bruxelles) 0415.497.718. Citibank, Brussels, account 570/1031255/09 This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/gen9: use an unreserved surface alignment value
On Wed, Jun 24, 2015 at 3:51 PM, Nanley Chery wrote:
> From: Nanley Chery
>
> Although the horizontal and vertical alignment fields are ignored here,
> 0 is a reserved value for them and may cause undefined behavior. Change
> the default value to an abitrary valid one.
>
> Signed-off-by: Nanley Chery
> ---
> src/mesa/drivers/dri/i965/gen8_surface_state.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> index b2d1a57..22ae960 100644
> --- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
> +++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
> @@ -93,7 +93,7 @@ vertical_alignment(const struct brw_context *brw,
> if (brw->gen > 8 &&
> (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
> surf_type == BRW_SURFACE_1D))
> - return 0;
> + return GEN8_SURFACE_VALIGN_4;
>
> switch (mt->align_h) {
> case 4:
> @@ -118,7 +118,7 @@ horizontal_alignment(const struct brw_context *brw,
> if (brw->gen > 8 &&
> (mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
> gen9_use_linear_1d_layout(brw, mt)))
> - return 0;
> + return GEN8_SURFACE_HALIGN_4;
>
> switch (mt->align_w) {
> case 4:
> --
> 2.4.4
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Good find Nanley. We had no known issues with value 0 but it's
always nice to avoid undefined behavior :).
Reviewed-by: Anuj Phogat
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: remove unnecessary checks in _mesa_readpixels_needs_slow_path
On Tue, Jun 23, 2015 at 3:34 AM, Iago Toral Quiroga wrote:
> readpixels_can_use_memcpy will later call _mesa_format_matches_format_and_type
> which does much tighter checks than these to decide if we can use
> memcpy for readpixels.
>
> Also, the checks do not seem to be extensive enough anyway, since we are
> checking for signed/unsigned conversion only when the framebuffer has
> integers,
> but the same checks could be done for other types anyway, since as long as
> there is a signed/unsigned conversion we can't memcpy.
>
> No regressions observed on i965/llvmpipe.
> ---
> The way gallium uses _mesa_format_matches_format_and_type and
> _mesa_readpixels_needs_slow_path is a bit different, they call these
> functions to decide if they want to fallback to Mesa's implementation
> of ReadPixels, not exactly to check if we can memcpy... so it is unclear
> to me if some combinations may still need these checks to make the right
> decision. I did not see any regressions with llvmpipe at least, so I am
> assuming that they are not needed, but maybe someone wants to give this
> patch a test run on nouveau or radeon, just in case. If they are needed
> I guess the right thing would be to move the checks to st_cb_readpixels.c.
>
> src/mesa/main/readpix.c | 16
> 1 file changed, 16 deletions(-)
>
> diff --git a/src/mesa/main/readpix.c b/src/mesa/main/readpix.c
> index a3357cd..e256695 100644
> --- a/src/mesa/main/readpix.c
> +++ b/src/mesa/main/readpix.c
> @@ -128,7 +128,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context
> *ctx, GLenum format,
> {
> struct gl_renderbuffer *rb =
> _mesa_get_read_renderbuffer_for_format(ctx, format);
> - GLenum srcType;
>
> assert(rb);
>
> @@ -153,21 +152,6 @@ _mesa_readpixels_needs_slow_path(const struct gl_context
> *ctx, GLenum format,
> return GL_TRUE;
>}
>
> - /* Conversion between signed and unsigned integers needs masking
> - * (it isn't just memcpy). */
> - srcType = _mesa_get_format_datatype(rb->Format);
> -
> - if ((srcType == GL_INT &&
> - (type == GL_UNSIGNED_INT ||
> -type == GL_UNSIGNED_SHORT ||
> -type == GL_UNSIGNED_BYTE)) ||
> - (srcType == GL_UNSIGNED_INT &&
> - (type == GL_INT ||
> -type == GL_SHORT ||
> -type == GL_BYTE))) {
> - return GL_TRUE;
> - }
> -
>/* And finally, see if there are any transfer ops. */
>return get_readpixels_transfer_ops(ctx, rb->Format, format, type,
> uses_blit) != 0;
> --
> 1.9.1
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Thanks for the patch Iago.
Reviewed-by: Anuj Phogat
You might want to wait few days to hear any comments on the gallium
usage of the function. If no one objects you can push it upstream.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91101] [softpipe] piglit glsl-1.50@execution@geometry@max-input-components regression
https://bugs.freedesktop.org/show_bug.cgi?id=91101 Bug ID: 91101 Summary: [softpipe] piglit glsl-1.50@execution@geometry@max-input-components regression Product: Mesa Version: git Hardware: x86-64 (AMD64) OS: Linux (All) Status: NEW Keywords: bisected, regression Severity: normal Priority: medium Component: Mesa core Assignee: mesa-dev@lists.freedesktop.org Reporter: v...@freedesktop.org QA Contact: mesa-dev@lists.freedesktop.org CC: mar...@gmail.com, srol...@vmware.com mesa: e31bce4041122cd00712b60b4dc1eae6486f6579 (master 10.7.0-devel) $ ./bin/shader_runner tests/spec/glsl-1.50/execution/geometry/max-input-components.shader_test -auto shader_runner: tgsi/tgsi_exec.c:1265: fetch_src_file_channel: Assertion `pos < 6 * 32' failed. Aborted (core dumped) (gdb) bt #0 0x7fc5c4efa267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 #1 0x7fc5c4efbeca in __GI_abort () at abort.c:89 #2 0x7fc5c4ef303d in __assert_fail_base (fmt=0x7fc5c5055028 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7fc5bfd54891 "pos < 6 * 32", file=file@entry=0x7fc5bfd5484f "tgsi/tgsi_exec.c", line=line@entry=1265, function=function@entry=0x7fc5bfd55230 <__PRETTY_FUNCTION__.7359> "fetch_src_file_channel") at assert.c:92 #3 0x7fc5c4ef30f2 in __GI___assert_fail (assertion=assertion@entry=0x7fc5bfd54891 "pos < 6 * 32", file=file@entry=0x7fc5bfd5484f "tgsi/tgsi_exec.c", line=line@entry=1265, function=function@entry=0x7fc5bfd55230 <__PRETTY_FUNCTION__.7359> "fetch_src_file_channel") at assert.c:101 #4 0x7fc5bfc0b9f7 in fetch_src_file_channel (mach=mach@entry=0x7fc5bdd36010, file=, swizzle=, index=index@entry=0x7ffd29c9e400, index2D=index2D@entry=0x7ffd29c9e410, chan=chan@entry=0x7ffd29c9e4d0, chan_index=0) at tgsi/tgsi_exec.c:1265 #5 0x7fc5bfc0c12d in fetch_source_d (mach=mach@entry=0x7fc5bdd36010, chan=chan@entry=0x7ffd29c9e4d0, reg=reg@entry=0x25bcf90, chan_index=chan_index@entry=0, src_datatype=TGSI_EXEC_DATA_FLOAT) at tgsi/tgsi_exec.c:1474 #6 0x7fc5bfc0c2d2 in fetch_source (mach=mach@entry=0x7fc5bdd36010, chan=chan@entry=0x7ffd29c9e4d0, reg=reg@entry=0x25bcf90, chan_index=chan_index@entry=0, src_datatype=src_datatype@entry=TGSI_EXEC_DATA_FLOAT) at tgsi/tgsi_exec.c:1490 #7 0x7fc5bfc0cdb8 in exec_vector_unary (mach=0x7fc5bdd36010, inst=inst@entry=0x25bcf60, op=op@entry=0x7fc5bfc09880 , src_datatype=src_datatype@entry=TGSI_EXEC_DATA_FLOAT, dst_datatype=TGSI_EXEC_DATA_UINT) at tgsi/tgsi_exec.c:2768 #8 0x7fc5bfc117e7 in exec_instruction (mach=mach@entry=0x7fc5bdd36010, inst=0x25bcf60, pc=pc@entry=0x7ffd29c9e734) at tgsi/tgsi_exec.c:4162 #9 0x7fc5bfc13827 in tgsi_exec_machine_run (mach=mach@entry=0x7fc5bdd36010) at tgsi/tgsi_exec.c:5151 #10 0x7fc5bfbdc615 in tgsi_gs_run (shader=, input_primitives=) at draw/draw_gs.c:216 #11 0x7fc5bfbdc6f0 in gs_flush (shader=0x25a3a20) at draw/draw_gs.c:415 #12 0x7fc5bfbdc9b8 in gs_tri_adj (shader=0x25a3a20, i0=, i1=, i2=, i3=, i4=, i5=5) at draw/draw_gs.c:517 #13 0x7fc5bfbdcd06 in gs_run (input_prims=, input_prims=, input_prims=, input_prims=, input_verts=, output_prims=, output_verts=, gs=) at draw/draw_decompose_tmp.h:342 #14 draw_geometry_shader_run (shader=0x25a3a20, constants=0x46f5, constants_size=0x6, input_verts=0x1, input_prim=0x7ffd29c9e9e0, input_info=0xb, output_verts=0x7ffd29c9e8c0, output_prims=0x7ffd29c9e900) at draw/draw_gs.c:638 #15 0x7fc5bfbee7a7 in fetch_pipeline_generic (middle=0x1e02080, fetch_info=0x46f5, fetch_info@entry=0x7ffd29c9e9c0, in_prim_info=0x7ffd29c9e9e0) at draw/draw_pt_fetch_shade_pipeline.c:290 #16 0x7fc5bfbeec16 in fetch_pipeline_linear_run (middle=, start=, count=6, prim_flags=) at draw/draw_pt_fetch_shade_pipeline.c:416 #17 0x7fc5bfbf4149 in vsplit_segment_simple_linear (vsplit=0x1eb6c40, vsplit=0x1eb6c40, icount=6, istart=0, flags=0) at draw/draw_pt_vsplit_tmp.h:240 #18 vsplit_run_linear (frontend=0x1eb6c40, start=0, count=6) at draw/draw_split_tmp.h:60 #19 0x7fc5bfbebb15 in draw_pt_arrays (draw=draw@entry=0x1df9d80, prim=12, start=0, count=count@entry=6) at draw/draw_pt.c:149 #20 0x7fc5bfbec143 in draw_vbo (draw=draw@entry=0x1df9d80, info=0x7ffd29c9eb10, info@entry=0x7ffd29c9ec10) at draw/draw_pt.c:564 #21 0x7fc5bfc8ff5a in softpipe_draw_vbo (pipe=0x1dedf10, info=0x7ffd29c9ec10) at sp_draw_arrays.c:147 #22 0x7fc5bfadbec0 in st_draw_vbo (ctx=, prims=, nr_prims=, ib=0x0, index_bounds_valid=, min_index=0, max_index=5, tfb_vertcount=0x0, indirect=0x0) at state_tracker/st_draw.c:286 #23 0x7fc5bfaa4de3 in vbo_draw_arrays (ctx=0x7fc5c5a5d010, mode=12, start=0, count=6, numInstances=1, baseInstance=0) at vbo/vbo_exec_array.c:645 #24 0x7fc5c553b103 in stub_g
Re: [Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations
> -fno-strict-aliasing:with strict aliasing: > libGL.so 699188 699188(no change) > *_dri.so 9575876 9563104(-2772) > Use the size command to get the actual text segment size, otherwise debugging symbols can drown changes. Dave. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] i965/bxt: Add basic Broxton infrastructure
On Wed, Jun 24, 2015 at 08:12:36PM +, Lecluse, Philippe wrote: > I Have successfully tested and validate patch 1,3,4 on BXT > Regards, > Philippe > Intel Corporation NV/SA > Kings Square, Veldkant 31 > 2550 Kontich > RPM (Bruxelles) 0415.497.718. > Citibank, Brussels, account 570/1031255/09 > > This e-mail and any attachments may contain confidential material for the > sole use of the intended recipient(s). Any review or distribution by others > is strictly prohibited. If you are not the intended recipient, please contact > the sender and delete all copies. > Thanks. Patch 3 was already pushed. I've squashed 1, and 4, and pushed that with Mark's review. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH v2] glsls: Modify exec_list to avoid strict-aliasing violations
This is an alternative to my earlier patch [1] (and it is now constructed
properly using git format-patch).
Quick background:
There is a problem in exec_list due to it directly including a trio
of 'struct exec_node *' members to implement two overlapping sentinel
nodes. The sentinel nodes do not exist as exec_node objects and so
should not be accessed as such, according to C99 6.5 paragraph 7.
When this strict aliasing rule is violated the compiler may re-order
reads and writes in unexpected ways, such as demonstrated in another
email [2].
The problem only manifests if compiling without -fno-strict-aliasing.
This patch addresses the issue by introducing some new methods for
setting the 'next' and 'prev' members of the exec_node structure, which
avoid the aliasing restrictions by way of casting the exec_node pointer
(to an exec_node-pointer-pointer) whenever it may actual point to a
sentinel node. Essentially an exec_node can be seen as an array of two
exec_node pointers, and this view is compatible with the sentinel
structure in exec_list.
Compared to the previous patch, this patch is much less intrusive, and
does not increase the storage requirements of the exec_list structure.
While I'm not proposing that -fno-strict-aliasing no longer be used for
Mesa builds, this patch represents a step in that direction. With this
patch applied, a working Mesa library can be built, although bugs may
be present (and could be triggered only when using particular compiler
versions and options). FWIW file sizes with and without strict aliasing:
(gcc 4.8.4, -O3 -fomit-frame-pointer -march=corei7).
-fno-strict-aliasing:with strict aliasing:
libGL.so 699188 699188(no change)
*_dri.so 9575876 9563104(-2772)
(dri bundle includes r300, r600, kms_swrast and swrast).
So, not a huge win, size-wise. Dave Airlie reports a 30K difference in
his r600_dri.so build however [3].
In terms of performance:
(export LIBGL_ALWAYS_SOFTWARE=1; time glmark2)
-fno-strict-aliasing:
glmark2 Score: 244
real5m34.707s
user11m36.192s
sys0m29.596s
with strict aliasing:
glmark2 Score: 247
real5m34.438s
user11m29.904s
sys0m29.556s
Again, only a very small improvement when strict aliasing is enabled.
With the above in mind it is reasonable to question whether this patch
is worthwhile. However, it's done, and it seems to work, so I offer it
for review.
[1] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087179.html
[2] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087246.html
[3] http://lists.freedesktop.org/archives/mesa-dev/2015-June/087206.html
---
src/glsl/list.h | 123
1 file changed, 80 insertions(+), 43 deletions(-)
diff --git a/src/glsl/list.h b/src/glsl/list.h
index 15fcd4a..cfbe5a9 100644
--- a/src/glsl/list.h
+++ b/src/glsl/list.h
@@ -76,6 +76,12 @@
#include "util/ralloc.h"
struct exec_node {
+ /**
+* Accessing these fields directly may be ill-advised; if the
'exec_node'
+* is actually a sentinel node embedded in the exec_list structure,
it may
+* be a strict-aliasing violation (C99 6.5 paragraph 7). Use the methods
+* provided instead.
+*/
struct exec_node *next;
struct exec_node *prev;
@@ -140,35 +146,55 @@ exec_node_init(struct exec_node *n)
n->prev = NULL;
}
+/**
+ * Strict-aliasing safe method for setting the next pointer for any
+ * node, including sentinel nodes.
+ */
+static inline void
+exec_node_set_next(struct exec_node *n, struct exec_node *next)
+{
+ ((struct exec_node **)n)[0] = next;
+}
+
+/**
+ * Strict-aliasing safe method for setting the next pointer for any
+ * node, including sentinel nodes.
+ */
+static inline void
+exec_node_set_prev(struct exec_node *n, struct exec_node *next)
+{
+ ((struct exec_node **)n)[1] = next;
+}
+
static inline const struct exec_node *
exec_node_get_next_const(const struct exec_node *n)
{
- return n->next;
+ return ((const struct exec_node **)n)[0];
}
static inline struct exec_node *
exec_node_get_next(struct exec_node *n)
{
- return n->next;
+ return ((struct exec_node **)n)[0];
}
static inline const struct exec_node *
exec_node_get_prev_const(const struct exec_node *n)
{
- return n->prev;
+ return ((const struct exec_node **)n)[1];
}
static inline struct exec_node *
exec_node_get_prev(struct exec_node *n)
{
- return n->prev;
+ return ((struct exec_node **)n)[1];
}
static inline void
exec_node_remove(struct exec_node *n)
{
- n->next->prev = n->prev;
- n->prev->next = n->next;
+ exec_node_set_prev(n->next, n->prev);
+ exec_node_set_next(n->prev, n->next);
n->next = NULL;
n->prev = NULL;
}
@@ -213,13 +239,15 @@ exec_node_replace_with(struct exec_node *n, struct
exec_node *replacement)
static inline bool
exec_node_is_tail_sentinel(const struct exec_node *n)
{
- return n->next == NULL;
+ // Use
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
tgsi.rst currently says:
If no ArrayID is specified with an indirect addressing operand the whole
register file might be accessed by this operand. This is strongly discouraged
and will prevent packing of scalar/vec2 arrays and effective alias analysis.
I'd be pretty happy to remove it and replace it w/ something to the
effect of indirect addressing where no ArrayID is specified is
undefined :-)
BR,
-R
On Wed, Jun 24, 2015 at 6:17 PM, Marek Olšák wrote:
> Indirect addressing without the ArrayID is undefined in the general
> case. You need the ArrayID to be able to tell where the array
> declaration starts and what its semantic name is.
>
> Marek
>
> On Wed, Jun 24, 2015 at 8:35 PM, Rob Clark wrote:
>> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
>>> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
>>> way to add declarations.
>>
>> ok.. hmm, I guess tgsi.rst needs an update then..
>>
>>> I recently added the array support for inputs and outputs, we just
>>> need to enable it on non-radeon non-swrast drivers:
>>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>>
>> ok, in principle (after fixing ttn based on the assumption that we
>> won't get indirect access if ArrayID==0, and now that tgsi f/e is
>> dropped) freedreno should be ok for array in/out's.. I'll enable the
>> cap and see what happens after fixing ttn and debugging some indirect
>> issues (seems either there is something I don't understand about the
>> hw yet, or maybe an a4xx bug.. right now shaders that I think should
>> work aren't and I'm in android hell trying to get some more traces
>> from blob :-/)
>>
>> BR,
>> -R
>>
>>> It would be nice to enable the arrays on all drivers instead of
>>> working around it indefinitely.
>>>
>>> Marek
>>>
>>> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
is safe to assume that we always get ArrayID that makes it much
easier.
BR,
-R
On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
> It's not an array, because the ArrayID is 0. It's a valid non-array
> declaration. If any TGSI user doesn't understand it, that user should
> be fixed.
>
> Marek
>
> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
>> From: Rob Clark
>>
>> Ok, so actually there is a ttn issue here to fix as well.. but it
>> brought up a question in my mind. When ttn sees something like
>>
>> DCL IN[0..1]
>>
>> it will treat that as an array (which in the end will result in
>> constraints about where the registers get allocated. Which is not
>> really ideal.
>>
>> With glsl we don't actually get input arrays (but instead a bunch
>> of MOV's to a TEMP array) currently. So I'm not quite sure how
>> an actual input array should look. (But my preference would be
>> IN[a..b] for arrays and IN[c] otherwise)
>> ---
>> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>> b/src/gallium/auxiliary/hud/hud_context.c
>> index 6a124f7..2b6d3a7 100644
>> --- a/src/gallium/auxiliary/hud/hud_context.c
>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
>> cso_context *cso)
>> {
>>static const char *vertex_shader_text = {
>> "VERT\n"
>> - "DCL IN[0..1]\n"
>> + "DCL IN[0]\n"
>> + "DCL IN[1]\n"
>> "DCL OUT[0], POSITION\n"
>> "DCL OUT[1], COLOR[0]\n" /* color */
>> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
>> --
>> 2.4.3
>>
>> ___
>> 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
[Mesa-dev] [Bug 91100] [softpipe] piglit egl-create-pbuffer-surface regression
https://bugs.freedesktop.org/show_bug.cgi?id=91100 Bug ID: 91100 Summary: [softpipe] piglit egl-create-pbuffer-surface regression Product: Mesa Version: git Hardware: x86-64 (AMD64) OS: Linux (All) Status: NEW Keywords: bisected, regression Severity: normal Priority: medium Component: Mesa core Assignee: mesa-dev@lists.freedesktop.org Reporter: v...@freedesktop.org QA Contact: mesa-dev@lists.freedesktop.org CC: bri...@vmware.com, mar...@gmail.com, martin.pe...@free.fr mesa: e31bce4041122cd00712b60b4dc1eae6486f6579 (master 10.7.0-devel) $ ./bin/egl-create-pbuffer-surface -auto egl-create-pbuffer-surface: sp_texture.c:360: softpipe_transfer_map: Assertion `box->x + box->width <= (int) u_minify(resource->width0, level)' failed. Aborted (core dumped) (gdb) bt #0 0x7f154073e267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 #1 0x7f154073feca in __GI_abort () at abort.c:89 #2 0x7f154073703d in __assert_fail_base (fmt=0x7f1540899028 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7f153bb067b8 "box->x + box->width <= (int) u_minify(resource->width0, level)", file=file@entry=0x7f153bb06663 "sp_texture.c", line=line@entry=360, function=function@entry=0x7f153bb06a00 <__PRETTY_FUNCTION__.8823> "softpipe_transfer_map") at assert.c:92 #3 0x7f15407370f2 in __GI___assert_fail (assertion=0x7f153bb067b8 "box->x + box->width <= (int) u_minify(resource->width0, level)", file=0x7f153bb06663 "sp_texture.c", line=360, function=0x7f153bb06a00 <__PRETTY_FUNCTION__.8823> "softpipe_transfer_map") at assert.c:101 #4 0x7f153ba4dc10 in softpipe_transfer_map (pipe=0x417c, resource=0x1b1a1e0, level=0, usage=2, box=0x7ffd8acf97e0, transfer=0x1) at sp_texture.c:360 #5 0x7f153b97200b in pipe_transfer_map (transfer=0x7ffd8acf97d8, h=, w=, y=, x=, access=2, layer=0, level=0, resource=0x1b1a1e0, context=0x19adb20) at ../../../../src/gallium/auxiliary/util/u_inlines.h:447 #6 drisw_update_tex_buffer (drawable=, ctx=, res=0x1b1a1e0) at drisw.c:309 #7 0x7f153b9709a8 in dri_set_tex_buffer2 (pDRICtx=, target=3553, format=8410, dPriv=) at dri_drawable.c:245 #8 0x7f15410204a2 in dri2_bind_tex_image (drv=0x199fe50, disp=0x199f810, surf=0x1ac4720, buffer=12420) at egl_dri2.c:1288 #9 0x7f154101991a in eglBindTexImage (dpy=0x199f810, surface=, buffer=12420) at eglapi.c:948 #10 0x00402507 in draw (state=0x7ffd8acf9af0) at piglit/tests/egl/egl-create-pbuffer-surface.c:64 #11 0x00401d55 in event_loop (state=0x7ffd8acf9af0, test=0x7ffd8acf9b70) at piglit/tests/egl/egl-util.c:156 #12 0x004022fb in egl_util_run (test=0x7ffd8acf9b70, argc=2, argv=0x7ffd8acf9c88) at piglit/tests/egl/egl-util.c:302 #13 0x004026ef in main (argc=2, argv=0x7ffd8acf9c88) at piglit/tests/egl/egl-create-pbuffer-surface.c:100 (gdb) frame 4 #4 0x7f153ba4dc10 in softpipe_transfer_map (pipe=0x417c, resource=0x1b1a1e0, level=0, usage=2, box=0x7ffd8acf97e0, transfer=0x1) at sp_texture.c:360 360 assert(box->x + box->width <= (int) u_minify(resource->width0, level)); (gdb) print box->x $1 = 1 (gdb) print box->width $2 = 256 (gdb) print resource->width0 $3 = 256 (gdb) print level $4 = 0 184e4de3a126fa21945fe59f68b8a29977919fc4 is the first bad commit commit 184e4de3a126fa21945fe59f68b8a29977919fc4 Author: Martin Peres Date: Fri Jun 5 15:03:19 2015 +0300 main/version: make sure all the output variables get set in get_gl_override This fixes 2 warnings in gcc 5.1. Reviewed-by: Brian Paul Reviewed-by: Marek Olšák Signed-off-by: Martin Peres :04 04 d79f536961876f490be51ac2137ba8f18662bc49 cca6b9455a77b64fb2d684a9dd7b3edb20d533eb Msrc bisect run success -- You are receiving this mail because: You are the QA Contact for the bug. 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] softpipe, llvmpipe: fix PIPE_SHADER_CAP_MAX_INPUTS value
Reviewed-by: Brian Paul Want to reference bug 91099 in the commit msg? On 06/24/2015 04:59 PM, Marek Olšák wrote: From: Marek Olšák PIPE_MAX_SHADER_INPUTS was recently bumped to 80 because of tessellation. --- src/gallium/auxiliary/gallivm/lp_bld_limits.h | 2 +- src/gallium/auxiliary/tgsi/tgsi_exec.h| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/auxiliary/gallivm/lp_bld_limits.h b/src/gallium/auxiliary/gallivm/lp_bld_limits.h index 49064fe..050076e 100644 --- a/src/gallium/auxiliary/gallivm/lp_bld_limits.h +++ b/src/gallium/auxiliary/gallivm/lp_bld_limits.h @@ -96,7 +96,7 @@ gallivm_get_shader_param(enum pipe_shader_cap param) case PIPE_SHADER_CAP_MAX_CONTROL_FLOW_DEPTH: return LP_MAX_TGSI_NESTING; case PIPE_SHADER_CAP_MAX_INPUTS: - return PIPE_MAX_SHADER_INPUTS; + return 32; case PIPE_SHADER_CAP_MAX_OUTPUTS: return 32; case PIPE_SHADER_CAP_MAX_CONST_BUFFER_SIZE: diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.h b/src/gallium/auxiliary/tgsi/tgsi_exec.h index 208640c..e8ee256 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.h +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.h @@ -213,7 +213,7 @@ struct tgsi_sampler * input register files, this is the stride between two 1D * arrays. */ -#define TGSI_EXEC_MAX_INPUT_ATTRIBS PIPE_MAX_SHADER_INPUTS +#define TGSI_EXEC_MAX_INPUT_ATTRIBS 32 /* The maximum number of bytes per constant buffer. */ ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] softpipe, llvmpipe: fix PIPE_SHADER_CAP_MAX_INPUTS value
From: Marek Olšák PIPE_MAX_SHADER_INPUTS was recently bumped to 80 because of tessellation. --- src/gallium/auxiliary/gallivm/lp_bld_limits.h | 2 +- src/gallium/auxiliary/tgsi/tgsi_exec.h| 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/auxiliary/gallivm/lp_bld_limits.h b/src/gallium/auxiliary/gallivm/lp_bld_limits.h index 49064fe..050076e 100644 --- a/src/gallium/auxiliary/gallivm/lp_bld_limits.h +++ b/src/gallium/auxiliary/gallivm/lp_bld_limits.h @@ -96,7 +96,7 @@ gallivm_get_shader_param(enum pipe_shader_cap param) case PIPE_SHADER_CAP_MAX_CONTROL_FLOW_DEPTH: return LP_MAX_TGSI_NESTING; case PIPE_SHADER_CAP_MAX_INPUTS: - return PIPE_MAX_SHADER_INPUTS; + return 32; case PIPE_SHADER_CAP_MAX_OUTPUTS: return 32; case PIPE_SHADER_CAP_MAX_CONST_BUFFER_SIZE: diff --git a/src/gallium/auxiliary/tgsi/tgsi_exec.h b/src/gallium/auxiliary/tgsi/tgsi_exec.h index 208640c..e8ee256 100644 --- a/src/gallium/auxiliary/tgsi/tgsi_exec.h +++ b/src/gallium/auxiliary/tgsi/tgsi_exec.h @@ -213,7 +213,7 @@ struct tgsi_sampler * input register files, this is the stride between two 1D * arrays. */ -#define TGSI_EXEC_MAX_INPUT_ATTRIBS PIPE_MAX_SHADER_INPUTS +#define TGSI_EXEC_MAX_INPUT_ATTRIBS 32 /* The maximum number of bytes per constant buffer. */ -- 2.1.0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/gen9: use an unreserved surface alignment value
From: Nanley Chery
Although the horizontal and vertical alignment fields are ignored here,
0 is a reserved value for them and may cause undefined behavior. Change
the default value to an abitrary valid one.
Signed-off-by: Nanley Chery
---
src/mesa/drivers/dri/i965/gen8_surface_state.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/gen8_surface_state.c
b/src/mesa/drivers/dri/i965/gen8_surface_state.c
index b2d1a57..22ae960 100644
--- a/src/mesa/drivers/dri/i965/gen8_surface_state.c
+++ b/src/mesa/drivers/dri/i965/gen8_surface_state.c
@@ -93,7 +93,7 @@ vertical_alignment(const struct brw_context *brw,
if (brw->gen > 8 &&
(mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
surf_type == BRW_SURFACE_1D))
- return 0;
+ return GEN8_SURFACE_VALIGN_4;
switch (mt->align_h) {
case 4:
@@ -118,7 +118,7 @@ horizontal_alignment(const struct brw_context *brw,
if (brw->gen > 8 &&
(mt->tr_mode != INTEL_MIPTREE_TRMODE_NONE ||
gen9_use_linear_1d_layout(brw, mt)))
- return 0;
+ return GEN8_SURFACE_HALIGN_4;
switch (mt->align_w) {
case 4:
--
2.4.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 13/18] i965: use ALIGN_NPOT for setting ASTC mipmap layouts
On Mon, Jun 22, 2015 at 4:02 PM, Nanley Chery wrote:
> From: Nanley Chery
>
> ALIGN is changed to ALIGN_NPOT because alignment values are sometimes not
> powers of two when working with ASTC.
>
> Signed-off-by: Nanley Chery
> ---
> src/mesa/drivers/dri/i965/brw_tex_layout.c| 12 ++--
> src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 4 ++--
> 2 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> index 998d8c4..4007697 100644
> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> @@ -367,7 +367,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> mt->total_width = mt->physical_width0;
>
> if (mt->compressed) {
> - mt->total_width = ALIGN(mt->physical_width0, mt->align_w);
> + mt->total_width = ALIGN_NPOT(mt->physical_width0, mt->align_w);
> }
>
> /* May need to adjust width to accommodate the placement of
> @@ -379,10 +379,10 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> unsigned mip1_width;
>
> if (mt->compressed) {
> - mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
> - ALIGN(minify(mt->physical_width0, 2), bw);
> + mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1),
> mt->align_w) +
> + ALIGN_NPOT(minify(mt->physical_width0, 2), bw);
> } else {
> - mip1_width = ALIGN(minify(mt->physical_width0, 1), mt->align_w) +
> + mip1_width = ALIGN_NPOT(minify(mt->physical_width0, 1),
> mt->align_w) +
> minify(mt->physical_width0, 2);
> }
>
> @@ -398,7 +398,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>
>intel_miptree_set_level_info(mt, level, x, y, depth);
>
> - img_height = ALIGN(height, mt->align_h);
> + img_height = ALIGN_NPOT(height, mt->align_h);
>if (mt->compressed)
> img_height /= bh;
>
> @@ -415,7 +415,7 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>/* Layout_below: step right after second mipmap.
> */
>if (level == mt->first_level + 1) {
> -x += ALIGN(width, mt->align_w);
> +x += ALIGN_NPOT(width, mt->align_w);
>} else {
> y += img_height;
>}
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 6aa969a..b47f49d0 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1213,8 +1213,8 @@ intel_miptree_copy_slice(struct brw_context *brw,
> if (dst_mt->compressed) {
>unsigned int i, j;
>_mesa_get_format_block_size(dst_mt->format, &i, &j);
> - height = ALIGN(height, j) / j;
> - width = ALIGN(width, i);
> + height = ALIGN_NPOT(height, j) / j;
> + width = ALIGN_NPOT(width, i);
> }
>
> /* If it's a packed depth/stencil buffer with separate stencil, the blit
> --
> 2.4.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Reviewed-by: Anuj Phogat
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 12/18] mesa/macros: move ALIGN_NPOT to macros.h
On Mon, Jun 22, 2015 at 4:02 PM, Nanley Chery wrote:
> From: Nanley Chery
>
> Aligning with a non-power-of-two number is a general task that can be used in
> various places. This commit is required for the next one.
>
> Signed-off-by: Nanley Chery
> ---
> src/mesa/drivers/dri/i965/intel_upload.c | 6 --
> src/mesa/main/macros.h | 6 ++
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_upload.c
> b/src/mesa/drivers/dri/i965/intel_upload.c
> index 870aabc..deaae6c 100644
> --- a/src/mesa/drivers/dri/i965/intel_upload.c
> +++ b/src/mesa/drivers/dri/i965/intel_upload.c
> @@ -44,12 +44,6 @@
>
> #define INTEL_UPLOAD_SIZE (64*1024)
>
> -/**
> - * Like ALIGN(), but works with a non-power-of-two alignment.
> - */
> -#define ALIGN_NPOT(value, alignment) \
> - (((value) + (alignment) - 1) / (alignment) * (alignment))
> -
> void
> intel_upload_finish(struct brw_context *brw)
> {
> diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
> index 4a640ad..4a08130 100644
> --- a/src/mesa/main/macros.h
> +++ b/src/mesa/main/macros.h
> @@ -708,6 +708,12 @@ ALIGN(uintptr_t value, uintptr_t alignment)
> }
>
> /**
> + * Like ALIGN(), but works with a non-power-of-two alignment.
> + */
> +#define ALIGN_NPOT(value, alignment) \
> + (((value) + (alignment) - 1) / (alignment) * (alignment))
> +
> +/**
Add an assert for the 0 alignment?
> * Align a value down to an alignment value
> *
> * If \c value is not already aligned to the requested alignment value, it
> --
> 2.4.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
R
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91099] [llvmpipe] piglit glsl-max-varyings >max_varying_components regression
https://bugs.freedesktop.org/show_bug.cgi?id=91099 Vinson Lee changed: What|Removed |Added Keywords||bisected CC||mar...@gmail.com --- Comment #1 from Vinson Lee --- 7ffc1fb928268f8493e88d45e9a006208d05f0f6 is the first bad commit commit 7ffc1fb928268f8493e88d45e9a006208d05f0f6 Author: Marek Olšák Date: Thu Mar 19 23:27:10 2015 +0100 gallium: bump shader input and output limits Reviewed-by: Roland Scheidegger Signed-off-by: Marek Olšák :04 04 c8ba9549231eff63c36137320a597d586a53298a a53d5640b434fc002eca189ffcb19aec79e1755a Msrc bisect run success -- You are receiving this mail because: You are the QA Contact for the bug. 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 v3 11/18] mesa/macros: add power-of-two assertions for alignment macros
On Mon, Jun 22, 2015 at 4:02 PM, Nanley Chery wrote:
> From: Nanley Chery
>
> ALIGN and ROUND_DOWN_TO both require that the alignment value passed
> into the macro be a power of two in the comments. Using software assertions
> verifies this to be the case.
>
> v2: use static inline functions instead of gcc-specific statement expressions.
>
> Signed-off-by: Nanley Chery
> ---
> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 2 +-
> src/mesa/main/macros.h | 16 +---
> 2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> index 59081ea..1a57784 100644
> --- a/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> +++ b/src/mesa/drivers/dri/i965/brw_fs_nir.cpp
> @@ -134,7 +134,7 @@ fs_visitor::nir_setup_outputs(nir_shader *shader)
> : var->type->vector_elements;
>
>if (stage == MESA_SHADER_VERTEX) {
> - for (int i = 0; i < ALIGN(type_size(var->type), 4) / 4; i++) {
> + for (unsigned int i = 0; i < ALIGN(type_size(var->type), 4) / 4;
> i++) {
> int output = var->data.location + i;
> this->outputs[output] = offset(reg, 4 * i);
> this->output_components[output] = vector_elements;
> diff --git a/src/mesa/main/macros.h b/src/mesa/main/macros.h
> index 0608650..4a640ad 100644
> --- a/src/mesa/main/macros.h
> +++ b/src/mesa/main/macros.h
> @@ -684,7 +684,7 @@ minify(unsigned value, unsigned levels)
> * Note that this considers 0 a power of two.
> */
> static inline bool
> -is_power_of_two(unsigned value)
> +is_power_of_two(uintptr_t value)
> {
> return (value & (value - 1)) == 0;
> }
> @@ -700,7 +700,12 @@ is_power_of_two(unsigned value)
> *
> * \sa ROUND_DOWN_TO()
> */
> -#define ALIGN(value, alignment) (((value) + (alignment) - 1) &
> ~((alignment) - 1))
> +static inline uintptr_t
> +ALIGN(uintptr_t value, uintptr_t alignment)
> +{
> + assert(is_power_of_two(alignment));
Also handle the 0 alignment. is_power_of_two() returns true for 0.
Use assert(alignment > 0 && is_power_of_two(alignment))?
> + return (((value) + (alignment) - 1) & ~((alignment) - 1));
> +}
>
> /**
> * Align a value down to an alignment value
> @@ -713,7 +718,12 @@ is_power_of_two(unsigned value)
> *
> * \sa ALIGN()
> */
> -#define ROUND_DOWN_TO(value, alignment) ((value) & ~(alignment - 1))
> +static inline uintptr_t
> +ROUND_DOWN_TO(uintptr_t value, uintptr_t alignment)
> +{
> + assert(is_power_of_two(alignment));
Here as well.
> + return ((value) & ~(alignment - 1));
> +}
>
>
> /** Cross product of two 3-element vectors */
> --
> 2.4.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
With above changes and indentation fixes:
Reviewed-by: Anuj Phogat
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
Indirect addressing without the ArrayID is undefined in the general
case. You need the ArrayID to be able to tell where the array
declaration starts and what its semantic name is.
Marek
On Wed, Jun 24, 2015 at 8:35 PM, Rob Clark wrote:
> On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
>> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
>> way to add declarations.
>
> ok.. hmm, I guess tgsi.rst needs an update then..
>
>> I recently added the array support for inputs and outputs, we just
>> need to enable it on non-radeon non-swrast drivers:
>> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
>
> ok, in principle (after fixing ttn based on the assumption that we
> won't get indirect access if ArrayID==0, and now that tgsi f/e is
> dropped) freedreno should be ok for array in/out's.. I'll enable the
> cap and see what happens after fixing ttn and debugging some indirect
> issues (seems either there is something I don't understand about the
> hw yet, or maybe an a4xx bug.. right now shaders that I think should
> work aren't and I'm in android hell trying to get some more traces
> from blob :-/)
>
> BR,
> -R
>
>> It would be nice to enable the arrays on all drivers instead of
>> working around it indefinitely.
>>
>> Marek
>>
>> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
>>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
>>> is safe to assume that we always get ArrayID that makes it much
>>> easier.
>>>
>>> BR,
>>> -R
>>>
>>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
It's not an array, because the ArrayID is 0. It's a valid non-array
declaration. If any TGSI user doesn't understand it, that user should
be fixed.
Marek
On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
> From: Rob Clark
>
> Ok, so actually there is a ttn issue here to fix as well.. but it
> brought up a question in my mind. When ttn sees something like
>
> DCL IN[0..1]
>
> it will treat that as an array (which in the end will result in
> constraints about where the registers get allocated. Which is not
> really ideal.
>
> With glsl we don't actually get input arrays (but instead a bunch
> of MOV's to a TEMP array) currently. So I'm not quite sure how
> an actual input array should look. (But my preference would be
> IN[a..b] for arrays and IN[c] otherwise)
> ---
> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/hud/hud_context.c
> b/src/gallium/auxiliary/hud/hud_context.c
> index 6a124f7..2b6d3a7 100644
> --- a/src/gallium/auxiliary/hud/hud_context.c
> +++ b/src/gallium/auxiliary/hud/hud_context.c
> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
> cso_context *cso)
> {
>static const char *vertex_shader_text = {
> "VERT\n"
> - "DCL IN[0..1]\n"
> + "DCL IN[0]\n"
> + "DCL IN[1]\n"
> "DCL OUT[0], POSITION\n"
> "DCL OUT[1], COLOR[0]\n" /* color */
> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
> --
> 2.4.3
>
> ___
> 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 v3 04/18] mesa: return bool instead of GLboolean in compressedteximage_only_format()
On Mon, Jun 22, 2015 at 4:02 PM, Nanley Chery wrote:
> From: Nanley Chery
>
> In agreement with the coding style, functions that aren't directly visible
> to the GL API should prefer the use of bool over GLboolean.
>
> Suggested-by: Ian Romanick
> Signed-off-by: Nanley Chery
> ---
> src/mesa/main/teximage.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/main/teximage.c b/src/mesa/main/teximage.c
> index 86ef407..0e0488a 100644
> --- a/src/mesa/main/teximage.c
> +++ b/src/mesa/main/teximage.c
> @@ -1763,7 +1763,7 @@ _mesa_test_proxy_teximage(struct gl_context *ctx,
> GLenum target, GLint level,
> /**
> * Return true if the format is only valid for glCompressedTexImage.
> */
> -static GLboolean
> +static bool
> compressedteximage_only_format(const struct gl_context *ctx, GLenum format)
> {
> switch (format) {
> @@ -1806,9 +1806,9 @@ compressedteximage_only_format(const struct gl_context
> *ctx, GLenum format)
> case GL_COMPRESSED_SRGB8_ALPHA8_ASTC_10x10_KHR:
> case GL_COMPRESSED_SRGB8_ALPHA8_ASTC_12x10_KHR:
> case GL_COMPRESSED_SRGB8_ALPHA8_ASTC_12x12_KHR:
> - return GL_TRUE;
> + return true;
> default:
> - return GL_FALSE;
> + return false;
> }
> }
>
> --
> 2.4.2
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Reviewed-by: Anuj Phogat
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91098] vmwgfx null ptr dereference at vmw_screen_ioctl.c:76 due to ioctl failure
https://bugs.freedesktop.org/show_bug.cgi?id=91098 --- Comment #1 from Thomas Hellström --- Hi. Thanks for the bug report. I think the root problem is gnome-shell(gdm) dropping its master privileges and then trying to render. The reason this is not allowed in the vmwgfx driver is the following scenario: 1) A user switches away the X server VT and gets a console terminal. 2) User launches a DRM-aware malicious app that becomes master and authenticates itself. 3) The user switches back the X server 4) The malicious app can now open exported buffer objects at will and access or manipulate user private data. This is correctly blocked in the vmwgfx driver. So this is AFAICT a gnome-shell (gdm mode) bug. It shouldn't render when it drops its master privileges, so the gnome bugzilla would be good to start with. We'll follow up. -- You are receiving this mail because: You are the QA Contact for the bug. 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 91099] [llvmpipe] piglit glsl-max-varyings >max_varying_components regression
https://bugs.freedesktop.org/show_bug.cgi?id=91099 Bug ID: 91099 Summary: [llvmpipe] piglit glsl-max-varyings >max_varying_components regression Product: Mesa Version: git Hardware: x86-64 (AMD64) OS: Linux (All) Status: NEW Keywords: regression Severity: normal Priority: medium Component: Mesa core Assignee: mesa-dev@lists.freedesktop.org Reporter: v...@freedesktop.org QA Contact: mesa-dev@lists.freedesktop.org CC: jfons...@vmware.com, srol...@vmware.com mesa: e31bce4041122cd00712b60b4dc1eae6486f6579 (master 10.7.0-devel) $ ./bin/glsl-max-varyings --exceed-limits -auto Vertical axis: Increasing numbers of varyings. Horizontal axis: Which of the varyings contains the color. GL_MAX_VARYING_FLOATS = 128 glsl-max-varyings: src/mesa/state_tracker/st_glsl_to_tgsi.cpp:4655: ureg_dst dst_register(st_translate*, gl_register_file, unsigned int, unsigned int): Assertion `index < VARYING_SLOT_MAX' failed. Aborted (core dumped) (gdb) bt #0 0x7fc07799b267 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:55 #1 0x7fc07799ceca in __GI_abort () at abort.c:89 #2 0x7fc07799403d in __assert_fail_base (fmt=0x7fc077af6028 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n", assertion=assertion@entry=0x7fc076a3e67b "index < VARYING_SLOT_MAX", file=file@entry=0x7fc076a3d708 "src/mesa/state_tracker/st_glsl_to_tgsi.cpp", line=line@entry=4655, function=function@entry=0x7fc076a3ff00 "ureg_dst dst_register(st_translate*, gl_register_file, unsigned int, unsigned int)") at assert.c:92 #3 0x7fc0779940f2 in __GI___assert_fail (assertion=0x7fc076a3e67b "index < VARYING_SLOT_MAX", file=0x7fc076a3d708 "src/mesa/state_tracker/st_glsl_to_tgsi.cpp", line=4655, function=0x7fc076a3ff00 "ureg_dst dst_register(st_translate*, gl_register_file, unsigned int, unsigned int)") at assert.c:101 #4 0x7fc075bebc69 in dst_register (t=0xc31420, file=PROGRAM_OUTPUT, index=56, array_id=0) at src/mesa/state_tracker/st_glsl_to_tgsi.cpp:4655 #5 0x7fc075bec3aa in translate_dst (t=0xc31420, dst_reg=0xcda7d8, saturate=false, clamp_color=false) at src/mesa/state_tracker/st_glsl_to_tgsi.cpp:4756 #6 0x7fc075becb15 in compile_tgsi_instruction (t=0xc31420, inst=0xcda7c0, clamp_dst_color_output=false) at src/mesa/state_tracker/st_glsl_to_tgsi.cpp:4904 #7 0x7fc075beef45 in st_translate_program (ctx=0x7fc0784fe010, procType=1, ureg=0xe79180, program=0xe26870, proginfo=0xf08070, numInputs=3, inputMapping=0xf08448, inputSlotToAttr=0x0, inputSemanticName=0x0, inputSemanticIndex=0x0, interpMode=0x0, interpLocation=0x0, numOutputs=33, outputMapping=0xf08610, outputSlotToAttr=0xf086f0, outputSemanticName=0xf087d0 "", outputSemanticIndex=0xf08808 "", passthrough_edgeflags=0 '\000', clamp_color=0 '\000') at src/mesa/state_tracker/st_glsl_to_tgsi.cpp:5581 #8 0x7fc075a68de5 in st_translate_vertex_program (st=0xa64210, stvp=0xf08070, key=0x7ffe56dacd60) at src/mesa/state_tracker/st_program.c:348 #9 0x7fc075a69027 in st_get_vp_variant (st=0xa64210, stvp=0xf08070, key=0x7ffe56dacd60) at src/mesa/state_tracker/st_program.c:440 #10 0x7fc075bbe1f2 in update_vp (st=0xa64210) at src/mesa/state_tracker/st_atom_shader.c:158 #11 0x7fc075bb925b in st_validate_state (st=0xa64210) at src/mesa/state_tracker/st_atom.c:214 #12 0x7fc075a63e73 in st_draw_vbo (ctx=0x7fc0784fe010, prims=0x7ffe56dacf70, nr_prims=1, ib=0x0, index_bounds_valid=1 '\001', min_index=0, max_index=3, tfb_vertcount=0x0, indirect=0x0) at src/mesa/state_tracker/st_draw.c:199 #13 0x7fc075ba6212 in vbo_draw_arrays (ctx=0x7fc0784fe010, mode=5, start=0, count=4, numInstances=1, baseInstance=0) at src/mesa/vbo/vbo_exec_array.c:645 #14 0x7fc075ba6ca0 in vbo_exec_DrawArrays (mode=5, start=0, count=4) at src/mesa/vbo/vbo_exec_array.c:797 #15 0x00401ef2 in draw (num_varyings=33) at piglit/tests/shaders/glsl-max-varyings.c:233 #16 0x0040208c in piglit_display () at piglit/tests/shaders/glsl-max-varyings.c:273 #17 0x7fc078067f8e in run_test (gl_fw=0x976980, argc=2, argv=0x7ffe56dad448) at piglit/tests/util/piglit-framework-gl/piglit_winsys_framework.c:79 #18 0x7fc07804c999 in piglit_gl_test_run (argc=2, argv=0x7ffe56dad448, config=0x7ffe56dad300) at piglit/tests/util/piglit-framework-gl.c:191 #19 0x004014fd in main (argc=2, argv=0x7ffe56dad448) at piglit/tests/shaders/glsl-max-varyings.c:48 (gdb) frame 4 #4 0x7fc075bebc69 in dst_register (t=0xc31420, file=PROGRAM_OUTPUT, index=56, array_id=0) at src/mesa/state_tracker/st_glsl_to_tgsi.cpp:4655 4655assert(index < VARYING_SLOT_MAX); (gdb) print /d VARYING_SLOT_MAX $1 = 56 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. _
Re: [Mesa-dev] [PATCH] i965: Drop brw->depthstencil.stencil_offset from gen8_depth_state.c.
On Wed, Jun 24, 2015 at 12:04 AM, Kenneth Graunke wrote: > This is always 0 - only brw_workaround_depthstencil_alignment ever sets > it, and that doesn't run on Gen6+. My initial Broadwell depth state > commit had this mistake. > > Signed-off-by: Kenneth Graunke > --- > src/mesa/drivers/dri/i965/gen8_depth_state.c |7 ++- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c > b/src/mesa/drivers/dri/i965/gen8_depth_state.c > index 7c4ec06..76ba09c 100644 > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c > @@ -41,7 +41,6 @@ emit_depth_packets(struct brw_context *brw, > bool depth_writable, > struct intel_mipmap_tree *stencil_mt, > bool stencil_writable, > - uint32_t stencil_offset, > bool hiz, > uint32_t width, > uint32_t height, > @@ -127,8 +126,7 @@ emit_depth_packets(struct brw_context *brw, >OUT_BATCH(HSW_STENCIL_ENABLED | mocs_wb << 22 | > (2 * stencil_mt->pitch - 1)); >OUT_RELOC64(stencil_mt->bo, > - I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, > - stencil_offset); > + I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0); >OUT_BATCH(stencil_mt ? stencil_mt->qpitch >> 2 : 0); >ADVANCE_BATCH(); > } > @@ -220,7 +218,6 @@ gen8_emit_depth_stencil_hiz(struct brw_context *brw, > emit_depth_packets(brw, depth_mt, brw_depthbuffer_format(brw), surftype, >ctx->Depth.Mask != 0, >stencil_mt, ctx->Stencil._WriteEnabled, > - brw->depthstencil.stencil_offset, >hiz, width, height, depth, lod, min_array_element); > } > > @@ -439,7 +436,7 @@ gen8_hiz_exec(struct brw_context *brw, struct > intel_mipmap_tree *mt, >brw_depth_format(brw, mt->format), >BRW_SURFACE_2D, >true, /* depth writes */ > - NULL, false, 0, /* no stencil for now */ > + NULL, false, /* no stencil for now */ >true, /* hiz */ >surface_width, >surface_height, > -- > 1.7.10.4 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev Reviewed-by: Anuj Phogat ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91098] vmwgfx null ptr dereference at vmw_screen_ioctl.c:76 due to ioctl failure
https://bugs.freedesktop.org/show_bug.cgi?id=91098 Bug ID: 91098 Summary: vmwgfx null ptr dereference at vmw_screen_ioctl.c:76 due to ioctl failure Product: Mesa Version: 10.6 Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: freedesk...@pargon.nl QA Contact: mesa-dev@lists.freedesktop.org I'm experiencing occasional crashes of gnome-shell (3.16) due to failed ioctls requested by Mesa's vmware DRI backend. Any pointers on where I should take the apparent root issue would be appreciated - I have no idea whether Mesa or the kernel driver is at fault for the ioctl failing in the first place. Kernel logs (v4.0.5) report an ioctl failure: [15949.294396] [drm:vmw_generic_ioctl [vmwgfx]] *ERROR* Dropped master trying to access ioctl that requires authentication. [15949.294400] [drm] IOCTL ERROR Command 65, Error -13. [15949.296209] [drm:vmw_generic_ioctl [vmwgfx]] *ERROR* Dropped master trying to access ioctl that requires authentication. [15949.296214] [drm] IOCTL ERROR Command 65, Error -13. [15949.296468] [drm:vmw_generic_ioctl [vmwgfx]] *ERROR* Dropped master trying to access ioctl that requires authentication. [15949.296470] [drm] IOCTL ERROR Command 87, Error -13. [15949.296478] gnome-shell[337]: segfault at 20 ip 7ff502cb4680 sp 7ffeaeea96e8 error 4 in vmwgfx_dri.so[7ff50293f000+506000] Followed by a crash of gnome-shell, due to an apparent null pointer dereference: Core was generated by `gnome-shell --mode=gdm --wayland --display-server'. Program terminated with signal SIGSEGV, Segmentation fault. #0 vmw_region_size (region=0x0) at vmw_screen_ioctl.c:76 76 return region->size; (gdb) bt #0 vmw_region_size (region=0x0) at vmw_screen_ioctl.c:76 #1 0x7ff502cb65cc in vmw_svga_winsys_surface_create (sws=0x145aaa0, flags=(SVGA3D_SURFACE_HINT_TEXTURE | SVGA3D_SURFACE_HINT_RENDERTARGET), format=SVGA3D_A8R8G8B8, usage=0, size=..., numFaces=1, numMipLevels=1) at vmw_screen_svga.c:222 #2 0x7ff502cc0d46 in svga_screen_surface_create (svgascreen=svgascreen@entry=0x145b9c0, key=key@entry=0x1cc8c90) at svga_screen_cache.c:449 #3 0x7ff502cbf810 in svga_texture_create (screen=0x145b9c0, template=0x7ffeaeea9840) at svga_resource_texture.c:729 #4 0x7ff502b0797b in st_texture_create (st=st@entry=0x1536270, target=, format=format@entry=PIPE_FORMAT_B8G8R8A8_UNORM, last_level=last_level@entry=0, width0=width0@entry=16, height0=height0@entry=16, depth0=1, layers=1, nr_samples=0, bind=10) at state_tracker/st_texture.c:97 #5 0x7ff502ada27d in guess_and_alloc_texture (st=st@entry=0x1536270, stObj=stObj@entry=0x268bc00, stImage=stImage@entry=0x16b5060) at state_tracker/st_cb_texture.c:464 #6 0x7ff502ada3a5 in st_AllocTextureImageBuffer (ctx=0x150c200, texImage=0x16b5060) at state_tracker/st_cb_texture.c:517 #7 0x7ff502adcb9c in st_TexImage (ctx=0x150c200, dims=2, texImage=0x16b5060, format=6408, type=5121, pixels=0x1f3bc80, unpack=0x15273f8) at state_tracker/st_cb_texture.c:875 #8 0x7ff502a72e00 in teximage (ctx=0x150c200, compressed=compressed@entry=0 '\000', dims=dims@entry=2, target=3553, level=, internalFormat=, width=16, height=16, depth=1, border=0, format=6408, type=5121, imageSize=0, pixels=0x1f3bc80) at main/teximage.c:3364 #9 0x7ff502a740e0 in _mesa_TexImage2D (target=, level=, internalFormat=, width=, height=, border=, format=6408, type=5121, pixels=0x1f3bc80) at main/teximage.c:3403 #10 0x7ff513f093a3 in ?? () from /usr/lib/libcogl.so.20 #11 0x7ff513efed94 in ?? () from /usr/lib/libcogl.so.20 #12 0x7ff513f3008b in cogl_texture_allocate () from /usr/lib/libcogl.so.20 #13 0x7ff513f31880 in cogl_texture_2d_new_from_data () from /usr/lib/libcogl.so.20 #14 0x7ff5191a6b98 in pixbuf_to_cogl_texture (pixbuf=pixbuf@entry=0x2779de0) at st/st-texture-cache.c:473 #15 0x7ff5191a6bf1 in finish_texture_load (data=data@entry=0x2568a50, pixbuf=pixbuf@entry=0x2779de0) at st/st-texture-cache.c:518 #16 0x7ff5191a6daa in on_symbolic_icon_loaded (source=0x15f20f0, result=, user_data=0x2568a50) at st/st-texture-cache.c:553 #17 0x7ff516fcc1f3 in ?? () from /usr/lib/libgio-2.0.so.0 #18 0x7ff516fcc229 in ?? () from /usr/lib/libgio-2.0.so.0 #19 0x7ff51682490d in g_main_context_dispatch () from /usr/lib/libglib-2.0.so.0 #20 0x7ff516824ce0 in ?? () from /usr/lib/libglib-2.0.so.0 #21 0x7ff516825002 in g_main_loop_run () from /usr/lib/libglib-2.0.so.0 #22 0x7ff518183326 in meta_run () from /usr/lib/libmutter.so.0 #23 0x0040208d in main (argc=1, argv=0x7ffeaeeaa028) at main.c:463 -- You are receiving this mail because: You are the QA Contact for the bug. You are the assignee for the bug. ___ mesa-dev mailing list mesa-d
Re: [Mesa-dev] [PATCH] i965/skl: Fix aligning mt->total_width to the block size
On Wed, Jun 24, 2015 at 02:29:09PM +0100, Neil Roberts wrote:
> Ben Widawsky writes:
>
> > I think this is beginning to infringe upon the definition of align_w.
> > The total width is a function of it's miptree properties and not the
> > compressed block properties, right?
> >
> > In other words, if there is a case where align_w != bw, I think
> > total_width should be aligned to align_w, NOT bw.
>
> I don't think it's so clear cut. In practice the mt->total_width doesn't
> really need to be aligned to anything because as far as I can tell it is
> only used to calculate the row stride. The row stride is separately
> aligned to whatever constraints necessary by libdrm so it doesn't really
> matter what we pick here.
>
> The reason I think that the intention was to align it to the block width
> rather than the horizontal alignment is that in the non-compressed case
> the total width isn't aligned to anything at all.
>
> It's probably not worth making too much of a fuss over this patch seeing
> as it doesn't make any practical difference. I'm happy to forget about
> it and pretend I never noticed the inconsistency.
>
> Regards,
> - Neil
>
I'm not opposed, I guess I just wanted a clear understanding of how it should
work. If you think this is the right thing to do, I trust you.
Reviewed-by: Ben Widawsky
> >
> > (I'm not opposed to the patch, just making sure I understand.)
> >
> >> ---
> >> src/mesa/drivers/dri/i965/brw_tex_layout.c | 5 ++---
> >> 1 file changed, 2 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> index 1e7d8a1..dbb6cef 100644
> >> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
> >> @@ -366,9 +366,8 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
> >>
> >> mt->total_width = mt->physical_width0;
> >>
> >> - if (mt->compressed) {
> >> - mt->total_width = ALIGN(mt->physical_width0, mt->align_w);
> >> - }
> >> + if (mt->compressed)
> >> + mt->total_width = ALIGN(mt->total_width, bw);
> >>
> >> /* May need to adjust width to accommodate the placement of
> >> * the 2nd mipmap. This occurs when the alignment
> >> --
> >> 1.9.3
> >>
> >> ___
> >> mesa-dev mailing list
> >> mesa-dev@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
--
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] gallium/hud: prevent NULL pointer dereference with pipe_query functions
The HUD doesn't check if query_create() fails and it calls other
pipe_query functions with NULL pointer instead of a valid query object.
Signed-off-by: Samuel Pitoiset
---
src/gallium/auxiliary/hud/hud_driver_query.c | 14 --
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/src/gallium/auxiliary/hud/hud_driver_query.c
b/src/gallium/auxiliary/hud/hud_driver_query.c
index 603aba7..ee71678 100644
--- a/src/gallium/auxiliary/hud/hud_driver_query.c
+++ b/src/gallium/auxiliary/hud/hud_driver_query.c
@@ -62,7 +62,8 @@ query_new_value(struct hud_graph *gr)
uint64_t now = os_time_get();
if (info->last_time) {
- pipe->end_query(pipe, info->query[info->head]);
+ if (info->query[info->head])
+ pipe->end_query(pipe, info->query[info->head]);
/* read query results */
while (1) {
@@ -70,7 +71,7 @@ query_new_value(struct hud_graph *gr)
union pipe_query_result result;
uint64_t *res64 = (uint64_t *)&result;
- if (pipe->get_query_result(pipe, query, FALSE, &result)) {
+ if (query && pipe->get_query_result(pipe, query, FALSE, &result)) {
info->results_cumulative += res64[info->result_index];
info->num_results++;
@@ -88,7 +89,8 @@ query_new_value(struct hud_graph *gr)
"gallium_hud: all queries are busy after %i frames, "
"can't add another query\n",
NUM_QUERIES);
- pipe->destroy_query(pipe, info->query[info->head]);
+ if (info->query[info->head])
+ pipe->destroy_query(pipe, info->query[info->head]);
info->query[info->head] =
pipe->create_query(pipe, info->query_type, 0);
}
@@ -113,15 +115,15 @@ query_new_value(struct hud_graph *gr)
info->results_cumulative = 0;
info->num_results = 0;
}
-
- pipe->begin_query(pipe, info->query[info->head]);
}
else {
/* initialize */
info->last_time = now;
info->query[info->head] = pipe->create_query(pipe, info->query_type, 0);
- pipe->begin_query(pipe, info->query[info->head]);
}
+
+ if (info->query[info->head])
+ pipe->begin_query(pipe, info->query[info->head]);
}
static void
--
2.4.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation
On Wed, Jun 24, 2015 at 3:59 PM, Davin McCall wrote:
> Hi Ian,
>
> On 23/06/15 23:26, Ian Romanick wrote:
>
> On 06/23/2015 02:36 PM, Thomas Helland wrote:
>
> 2015-06-24 23:05 GMT+02:00 Davin McCall :
>
> Hi - I'm new here.
>
> I've recently started poking the Mesa codebase for little reason other than
> personal interest. In the "help wanted" section of the website it mentions
> aliasing violations as a target for newcomers to fix, so with that in mind
> I've attached a patch (against git head) which resolves a few of them, by
> targeting the linked list implementation (list.h) used in the GLSL
> compiler/optimizers. This change slightly increases the storage requirements
> for a list (adds one word) but resolves the blatant aliasing violation that
> was caused by the trick used to conserve that word in the first place.
>
> (I toyed with another approach - using a single sentinel node for both the
> head and tail of a list - but this was much more invasive, and meant that
> you could no longer check whether a particular node was a sentinel node
> unless you had a reference to the list, so I gave up and went with this
> simpler approach).
>
> The most essential change is in the 'exec_list' structure. Three fields
> 'head', 'tail' and 'tail_pred' are removed, and two separate sentinel nodes
> are inserted in their place. The old 'head' is replaced by
> 'head_sentinel.next', 'tail_pred' by 'tail_sentinel.prev', and tail (always
> NULL) by 'head_sentinel.prev' and 'tail_sentinel.next' (both always NULL).
>
> NAK. The datastructure is correct as-is. It has been in common use
> since at least 1985. See the references in the header file.
>
>
> I understand the data structure and how it is supposed to work; the issue is
> that the trick it employs cannot be implemented in C without breaking the
> strict aliasing rules (or at least, the current implementation in Mesa
> breaks the strict aliasing rules). The current code works correctly only
> with the -fno-strict-aliasing compiler option. The issue is that a pair of
> 'exec_node *' do not constitute an exec_node in the eyes of the language
> spec, even though exec_node is declared as holding two such pointers.
> Consider (from src/glsl/list.h):
>
> static inline void
> exec_list_make_empty(struct exec_list *list)
> {
>list->head = (struct exec_node *) & list->tail;
>list->tail = NULL;
>list->tail_pred = (struct exec_node *) & list->head;
> }
>
>
> 'list->head' is of type 'struct exec_node *', and so should point at a
> 'struct exec_node'. In the code above it is instead co-erced to point at a
> 'struct exec_node *' (list->tail). That in itself doesn't break the alias
> rules, but then:
>
> static inline bool
> exec_node_is_tail_sentinel(const struct exec_node *n)
> {
>return n->next == NULL;
> }
>
>
> In 'exec_node_is_tail_sentinel', the sentinel is not actually an exec_node -
> it is &list->tail. So, if the parameter n does refer to the sentinel, then
> it does not point to an actual exec_node structure. However, it is
> de-referenced (by 'n->next') which breaks the strict aliasing rules. This
> means that the method above can only ever return false, unless it violates
> the aliasing rules.
>
> (The above method could be fixed by casting n to an 'struct exec_node **'
> and then comparing '**n' against NULL. However, there are other similar
> examples throughout the code that I do not think would be so trivial).
>
> I can quote the relevant parts of the standard if necessary, but your tone
> somewhat implies that you wouldn't even consider this patch?
>
> Davin
>
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
(I couldn't find your original message for some reason, so I'm
replying to this one...)
I think a better solution would be to use __attribute((__may_alias__))
for this case, which is supported at least on GCC and clang (I'm not
sure how recent though) and fall back to -fno-strict-aliasing if it
isn't available. It seems reasonable to require the use of an escape
hatch if we want to use a compiler that takes advantage of the strict
aliasing rules.
On the other hand, I share the concern of other people that it's going
to be very, very difficult to tell whether we're violating strict
aliasing rules. GCC does have -Wstrict-aliasing, but that won't catch
everything, and sometimes these sorts of issues can fester for a while
before a specific combination of compiler flags/code paths/phase of
the moon etc. causes it to break. And the fact that a lot of
developers don't even know about these rules makes things even worse.
We'd need to do some *serious* testing before enabling it both to
justify the perf gain and to root out any bugs, and even then there's
no guarantee that we won't get bitten by it years down the line.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http:
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
On Wed, Jun 24, 2015 at 12:31 PM, Marek Olšák wrote:
> Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
> way to add declarations.
ok.. hmm, I guess tgsi.rst needs an update then..
> I recently added the array support for inputs and outputs, we just
> need to enable it on non-radeon non-swrast drivers:
> http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
ok, in principle (after fixing ttn based on the assumption that we
won't get indirect access if ArrayID==0, and now that tgsi f/e is
dropped) freedreno should be ok for array in/out's.. I'll enable the
cap and see what happens after fixing ttn and debugging some indirect
issues (seems either there is something I don't understand about the
hw yet, or maybe an a4xx bug.. right now shaders that I think should
work aren't and I'm in android hell trying to get some more traces
from blob :-/)
BR,
-R
> It would be nice to enable the arrays on all drivers instead of
> working around it indefinitely.
>
> Marek
>
> On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
>> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
>> is safe to assume that we always get ArrayID that makes it much
>> easier.
>>
>> BR,
>> -R
>>
>> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
>>> It's not an array, because the ArrayID is 0. It's a valid non-array
>>> declaration. If any TGSI user doesn't understand it, that user should
>>> be fixed.
>>>
>>> Marek
>>>
>>> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
From: Rob Clark
Ok, so actually there is a ttn issue here to fix as well.. but it
brought up a question in my mind. When ttn sees something like
DCL IN[0..1]
it will treat that as an array (which in the end will result in
constraints about where the registers get allocated. Which is not
really ideal.
With glsl we don't actually get input arrays (but instead a bunch
of MOV's to a TEMP array) currently. So I'm not quite sure how
an actual input array should look. (But my preference would be
IN[a..b] for arrays and IN[c] otherwise)
---
src/gallium/auxiliary/hud/hud_context.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/src/gallium/auxiliary/hud/hud_context.c
b/src/gallium/auxiliary/hud/hud_context.c
index 6a124f7..2b6d3a7 100644
--- a/src/gallium/auxiliary/hud/hud_context.c
+++ b/src/gallium/auxiliary/hud/hud_context.c
@@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
cso_context *cso)
{
static const char *vertex_shader_text = {
"VERT\n"
- "DCL IN[0..1]\n"
+ "DCL IN[0]\n"
+ "DCL IN[1]\n"
"DCL OUT[0], POSITION\n"
"DCL OUT[1], COLOR[0]\n" /* color */
"DCL OUT[2], GENERIC[0]\n" /* texcoord */
--
2.4.3
___
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] ARB_arrays_of_arrays GLSL ES
On Sat, Jun 20, 2015 at 5:32 AM, Timothy Arceri wrote: > Hi all, > > The restrictions in ES make the extension easier to implement so > I thought I'd try get this stuff reviewed an committed before finishing > up the full extension. > The bits that I'm still working on for the desktop version are AoA inputs > outputs, and interface blocks. > > The only thing I know is definatly missing in this series for ES is > support for indirect indexing of samplers, but that didn't seem like > something that should hold up the series. > > Once the SSBO series lands (with a patch that restricts unsized arrays) > then all the AoA ES conformance tests will pass. > > There are already a bunch of piglit tests in git but I've just sent a > series with all the patches still waiting review here: > http://lists.freedesktop.org/archives/piglit/2015-June/016312.html > > I haven't made a patch marking this as done yet because currently > the i965 backend takes a very long time trying to optimise some of the > conformance tests. They still pass but they are taking 15-minutes+ just > to compile so this really needs to be sorted out first. If someone with > more knowledge in this area than me wants to take a look at this I would > be greatful for being pointed in the right direction. Can you try it with this patch: http://lists.freedesktop.org/archives/mesa-dev/2015-June/086011.html > If useful the series is in the 'gles4' branch of the repo here: > https://github.com/tarceri/Mesa_arrays_of_arrays.git > > Thanks, > Tim > ___ > 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] mesa: fold duplicated GL/GL_CORE/GLES3 entry in get_hash_params.py
On Wed, Jun 24, 2015 at 6:07 AM, Emil Velikov wrote: > Signed-off-by: Emil Velikov > --- > src/mesa/main/get_hash_params.py | 8 +++- > 1 file changed, 3 insertions(+), 5 deletions(-) Doh! Reviewed-by: Matt Turner ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
Yes, ArrayID != 0 means it's an array, otherwise it's just a compact
way to add declarations.
I recently added the array support for inputs and outputs, we just
need to enable it on non-radeon non-swrast drivers:
http://cgit.freedesktop.org/mesa/mesa/commit/?id=b6ebe7eabf54936a02acc0968e718e0c264a73f5
It would be nice to enable the arrays on all drivers instead of
working around it indefinitely.
Marek
On Wed, Jun 24, 2015 at 1:31 PM, Rob Clark wrote:
> Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
> is safe to assume that we always get ArrayID that makes it much
> easier.
>
> BR,
> -R
>
> On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
>> It's not an array, because the ArrayID is 0. It's a valid non-array
>> declaration. If any TGSI user doesn't understand it, that user should
>> be fixed.
>>
>> Marek
>>
>> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
>>> From: Rob Clark
>>>
>>> Ok, so actually there is a ttn issue here to fix as well.. but it
>>> brought up a question in my mind. When ttn sees something like
>>>
>>> DCL IN[0..1]
>>>
>>> it will treat that as an array (which in the end will result in
>>> constraints about where the registers get allocated. Which is not
>>> really ideal.
>>>
>>> With glsl we don't actually get input arrays (but instead a bunch
>>> of MOV's to a TEMP array) currently. So I'm not quite sure how
>>> an actual input array should look. (But my preference would be
>>> IN[a..b] for arrays and IN[c] otherwise)
>>> ---
>>> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>>> b/src/gallium/auxiliary/hud/hud_context.c
>>> index 6a124f7..2b6d3a7 100644
>>> --- a/src/gallium/auxiliary/hud/hud_context.c
>>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>>> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
>>> cso_context *cso)
>>> {
>>>static const char *vertex_shader_text = {
>>> "VERT\n"
>>> - "DCL IN[0..1]\n"
>>> + "DCL IN[0]\n"
>>> + "DCL IN[1]\n"
>>> "DCL OUT[0], POSITION\n"
>>> "DCL OUT[1], COLOR[0]\n" /* color */
>>> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
>>> --
>>> 2.4.3
>>>
>>> ___
>>> 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 07/17] i965/fs: Move offset() and half() to the fs_builder
On Wed, Jun 24, 2015 at 7:56 AM, Francisco Jerez wrote: > Jason Ekstrand writes: > >> On Wed, Jun 24, 2015 at 6:44 AM, Francisco Jerez >> wrote: >>> Jason Ekstrand writes: >>> On Jun 24, 2015 6:29 AM, "Francisco Jerez" wrote: > > Jason Ekstrand writes: > > > On Jun 24, 2015 4:29 AM, "Francisco Jerez" wrote: > >> > >> Jason Ekstrand writes: > >> > >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez < curroje...@riseup.net> > > wrote: > >> >> Jason Ekstrand writes: > >> >> > >> >>> We want to move these into the builder so that they know the current > >> >>> builder's dispatch width. This will be needed by a later commit. > >> >> > >> >> I very much like the idea of this series, but, why do you need to move > >> >> these register manipulators into the builder? The builder is an object > >> >> you can use to: > >> >> - Manipulate and query parameters affecting code generation. > >> >> - Create instructions into the program (::emit and friends). > >> >> - Allocate virtual registers from the program (::vgrf and friends). > >> >> > >> >> offset() and half() logically perform an action on a given register > >> >> object (or rather, compute a function of a given register object), not > >> >> on a builder object, the builder is only required as an auxiliary > >> >> parameter -- Any reason you didn't just pass it as a third parameter? > >> > > >> > What's required as a third parameter is the current execution size. I > >> > could have passed that directly, but I figured that, especially for > >> > half(), it would get messed up. I could pass the builder in but I > >> > don't see a whole lot of difference between that and what I'm doing > >> > right now. > >> > >> Assembly-wise there's no difference, but it seems inconsistent with both > >> the remaining register manipulators and remaining builder methods, and > >> IMHO it's kind of an anti-pattern to make something a method that > >> doesn't need access to any internal details of the object. > >> > >> > As is, it's not entirely obvious whether you should call > >> > half(reg) on the half-width or full-width builder. I'm not 100% sure > >> > what to do about that. > >> > > >> Actually, does half() really need to know about the builder? AFAICT it > >> only needs it because of dispatch_width(), and before doing anything > >> useful with it it asserts that it's equal to 16, what points at the > >> parameter being redundant. By convention a "half" is a group of 8 > >> channels (we may want to revise this convention when we implement SIMD32 > >> -- E.g. make half a group of 16 channels and quarter a group of 8 > >> channels), so 'half(reg)' could simply be implemented as > >> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As > >> additional paranoia to catch half() being called on a non-16-aligned > >> register you could assert that either 'stride == 0' or 16 divides > >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why > >> don't we have a reg_offset already in bytes again?) -- That would also > >> catch cases in which the register and builder "widths" get out of sync, > >> e.g. if half is called in an already halved register but the builder > >> used happens to be of the correct exec_size. > > > > OK, fine, we can pull half() back out. Should offset() stay in the > > builder? If not, where should it get its dispatch width. > > > I'm for leaving it as a stand-alone function (like all other register > manipulators), and add a third argument to pass the 'fs_builder' it can > take the dispatch width from? I'm not a big fan. However, in the interest of keeping the builder clean, >>> >>> It also keeps the register interface consistent IMHO. Why do you say >>> you're not a big fan? >> >> Unfortunately, there's really no good place to put it. Ideally, we'd >> have more/better information in the allocator or somewhere and we >> could do offset() type operations with just the register. >> Unfortunately, that's not an option in the current setup and I don't >> think keeping reg.width around just for offset() is worth it. That >> means it has to pull it from somewhere. It can't pull it from the >> visitor because it needs to be the width we're currently using for >> building. That means it needs to pull it from the builder. >> > I think that part of the problem here is that width was really used for > two subtly different purposes: > - Determining the distance between individual components of a vector. > - Determining the execution size of new instructions based on a rather >complex function of the above. > > The builder makes the second function unnecessary, but not the first, so > it's not surpr
Re: [Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder
Jason Ekstrand writes: > On Wed, Jun 24, 2015 at 6:44 AM, Francisco Jerez > wrote: >> Jason Ekstrand writes: >> >>> On Jun 24, 2015 6:29 AM, "Francisco Jerez" wrote: Jason Ekstrand writes: > On Jun 24, 2015 4:29 AM, "Francisco Jerez" >>> wrote: >> >> Jason Ekstrand writes: >> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez < >>> curroje...@riseup.net> > wrote: >> >> Jason Ekstrand writes: >> >> >> >>> We want to move these into the builder so that they know the >>> current >> >>> builder's dispatch width. This will be needed by a later commit. >> >> >> >> I very much like the idea of this series, but, why do you need to >>> move >> >> these register manipulators into the builder? The builder is an >>> object >> >> you can use to: >> >> - Manipulate and query parameters affecting code generation. >> >> - Create instructions into the program (::emit and friends). >> >> - Allocate virtual registers from the program (::vgrf and friends). >> >> >> >> offset() and half() logically perform an action on a given register >> >> object (or rather, compute a function of a given register object), >>> not >> >> on a builder object, the builder is only required as an auxiliary >> >> parameter -- Any reason you didn't just pass it as a third >>> parameter? >> > >> > What's required as a third parameter is the current execution size. >>> I >> > could have passed that directly, but I figured that, especially for >> > half(), it would get messed up. I could pass the builder in but I >> > don't see a whole lot of difference between that and what I'm doing >> > right now. >> >> Assembly-wise there's no difference, but it seems inconsistent with >>> both >> the remaining register manipulators and remaining builder methods, and >> IMHO it's kind of an anti-pattern to make something a method that >> doesn't need access to any internal details of the object. >> >> > As is, it's not entirely obvious whether you should call >> > half(reg) on the half-width or full-width builder. I'm not 100% sure >> > what to do about that. >> > >> Actually, does half() really need to know about the builder? AFAICT it >> only needs it because of dispatch_width(), and before doing anything >> useful with it it asserts that it's equal to 16, what points at the >> parameter being redundant. By convention a "half" is a group of 8 >> channels (we may want to revise this convention when we implement >>> SIMD32 >> -- E.g. make half a group of 16 channels and quarter a group of 8 >> channels), so 'half(reg)' could simply be implemented as >> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As >> additional paranoia to catch half() being called on a non-16-aligned >> register you could assert that either 'stride == 0' or 16 divides >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why >> don't we have a reg_offset already in bytes again?) -- That would also >> catch cases in which the register and builder "widths" get out of sync, >> e.g. if half is called in an already halved register but the builder >> used happens to be of the correct exec_size. > > OK, fine, we can pull half() back out. Should offset() stay in the > builder? If not, where should it get its dispatch width. > I'm for leaving it as a stand-alone function (like all other register manipulators), and add a third argument to pass the 'fs_builder' it can take the dispatch width from? >>> >>> I'm not a big fan. However, in the interest of keeping the builder clean, >> >> It also keeps the register interface consistent IMHO. Why do you say >> you're not a big fan? > > Unfortunately, there's really no good place to put it. Ideally, we'd > have more/better information in the allocator or somewhere and we > could do offset() type operations with just the register. > Unfortunately, that's not an option in the current setup and I don't > think keeping reg.width around just for offset() is worth it. That > means it has to pull it from somewhere. It can't pull it from the > visitor because it needs to be the width we're currently using for > building. That means it needs to pull it from the builder. > I think that part of the problem here is that width was really used for two subtly different purposes: - Determining the distance between individual components of a vector. - Determining the execution size of new instructions based on a rather complex function of the above. The builder makes the second function unnecessary, but not the first, so it's not surprising to see some awkwardness while trying to remove it regardless (need for an additional parameter, either as an explicit function argument or as "this"). > Yes, it
Re: [Mesa-dev] [PATCH v2 23/82] glsl: Do not do CSE for expressions involving SSBO loads
On Wed, 2015-06-24 at 15:43 +0300, Francisco Jerez wrote:
> Iago Toral writes:
>
> > On Wed, 2015-06-17 at 17:20 -0700, Jordan Justen wrote:
> >> I wanted to question whether this was required, based on this text
> >> from the extension spec:
> >>
> >> "The ability to write to buffer objects creates the potential for
> >> multiple independent shader invocations to read and write the same
> >> underlying memory. The same issue exists with the
> >> ARB_shader_image_load_store extension provided in OpenGL 4.2, which
> >> can write to texture objects and buffers. In both cases, the
> >> specification makes few guarantees related to the relative order of
> >> memory reads and writes performed by the shader invocations."
> >>
> >> But I'm not sure if we can reconcile CSE with 'memoryBarrier' and
> >> 'barrier'. curro, any thoughts from image load/store?
> >
> > I think the problem is within the same thread, that text above talks
> > about multiple invocations reading from and writing to the same
> > location, but within the same invocation, the order of reads and writes
> > must be preserved:
> >
> > "Buffer variable memory reads and writes within a single shader
> > invocation are processed in order. However, the order of reads and
> > writes performed in one invocation relative to those performed by
> > another invocation is largely undefined."
> >
> > For example, if X is a shader storage buffer variable and we have code
> > like this with just one invocation:
> >
> > ssbo_store(X, 1);
> > a = ssbo_load(X) + 1 // a = 2
> > ssbo_store(X, 2);
> > b = ssbo_load(X) + 1; // b = 3
> >
> > CSE could mess it up like this:
> >
> > ssbo_store(X, 1);
> > tmp = ssbo_load(X) + 1 // tmp = 2
> > a = tmp;
> > ssbo_store(X, 2);
> > b = tmp;
> >
> > which would be incorrect. I think I wrote this patch after seeing
> > something like this happening. The CSE pass clearly states that it does
> > not support write variables after all.
> >
> > Also, notice the same would apply if there are multiple invocations but
> > the shader code used something like gl_VertexID or gl_FragCoord to make
> > each invocation read from/write to a different address within the SSBO
> > buffer (I imagine this is the usual way to operate with SSBOs). In these
> > cases, even if we have multiple invocations, keeping the relative order
> > of reads and writes within each one is necessary.
> >
>
> AFAICT the reason why this (and many of the other changes in GLSL
> optimization passes) is needed is because SSBO loads have been
> implemented as ir_expression nodes instead of being lowered into
> intrinsics (as other side-effectful operations do like
> ARB_shader_image_load_store and ARB_shader_atomic_counters). This
> surely broke the assumption of a number of optimization passes that
> ir_expression nodes behave as pure functions. I guess the reason why
> you've done it this way is because UBO loads were already being
> represented as expressions, so I see why you may have wanted to use the
> same approach for SSBOs even though there is a fundamental difference
> between the two: UBO loads have no side effects and are constant for a
> given set of arguments and a given shader execution, SSBO loads and
> stores are not. SSBO stores couldn't be accommodated into the same
> framework so easily, and you decided to create a separate ir node for
> them, what seems inconsistent with loads. Intrinsics would probably
> have been a good fit for both loads and stores, and would have made all
> these optimization changes unnecessary...
>
> P.S.: Sorry for the late reply, I was on vacation when I was CC'ed.
Right, your assessment about the reasons behind the current
implementation is correct. I did not realize of these issues when I
decided to go with the current implementation, now it does look like
going with GLSL intrinsics would have made things a bit easier. I
suppose it would make sense to revisit the implementation in the near
future taking your work on arb_shader_image_load_store as a reference.
Iago
> > Iago
> >
> >> -Jordan
> >>
> >> On 2015-06-03 00:01:13, Iago Toral Quiroga wrote:
> >> > SSBOs are read/write and this CSE pass only handles read-only variables.
> >> > ---
> >> > src/glsl/opt_cse.cpp | 33 -
> >> > 1 file changed, 32 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/src/glsl/opt_cse.cpp b/src/glsl/opt_cse.cpp
> >> > index 4b8e9a0..a05ab46 100644
> >> > --- a/src/glsl/opt_cse.cpp
> >> > +++ b/src/glsl/opt_cse.cpp
> >> > @@ -245,6 +245,28 @@ contains_rvalue(ir_rvalue *haystack, ir_rvalue
> >> > *needle)
> >> > }
> >> >
> >> > static bool
> >> > +expression_contains_ssbo_load(ir_expression *expr)
> >> > +{
> >> > + if (expr->operation == ir_binop_ssbo_load)
> >> > + return true;
> >> > +
> >> > + for (unsigned i = 0; i < expr->get_num_operands(); i++) {
> >> > + ir_rvalue *op = expr->operands[i];
> >> > + if (op->ir_type == ir_type_expression &&
> >> > +
Re: [Mesa-dev] [PATCH v2 26/82] glsl: Don't do copy propagation on buffer variables
On Tue, 2015-06-23 at 15:45 -0700, Jordan Justen wrote:
> On 2015-06-22 23:38:14, Iago Toral wrote:
> > On Mon, 2015-06-22 at 14:28 -0700, Jordan Justen wrote:
> > > 24-26 once again makes me wonder if these optimization *can* be used
> > > with SSBOs based on the same ext spec wording I referenced before:
> > >
> > > "The ability to write to buffer objects creates the potential for
> > > multiple independent shader invocations to read and write the same
> > > underlying memory. The same issue exists with the
> > > ARB_shader_image_load_store extension provided in OpenGL 4.2, which
> > > can write to texture objects and buffers. In both cases, the
> > > specification makes few guarantees related to the relative order of
> > > memory reads and writes performed by the shader invocations."
> > >
> > > In these patches "other threads" were specifically mentioned.
> > >
> > > Did these patches also prevent bad things from happening in generated
> > > code? (Like mentioned for patch 23.)
> >
> > I think the problem here is the possibility for shaders to use
> > memoryBarrier():
> >
> > "SHADER_STORAGE_BARRIER_BIT: Memory accesses using shader buffer
> > variables issued after the barrier will reflect data written by
> > shaders prior to the barrier. Additionally, assignments to and atomic
> > operations performed on shader buffer variables after the barrier will
> > not execute until all memory accesses (e.g., loads, stores, texture
> > fetches, vertex fetches) initiated prior to the barrier complete."
> >
> > I think in these cases we can't allow these optimizations to kick in.
> >
> > That said, maybe we can check if we are using any memorybarriers in the
> > shader code and decide if we want to enable these optimizations for
> > ssbos based on that. I think we can try to do that in a later patch.
>
> Ok. What do you think about updating the commit messages on these
> three patches?
>
> For example, currently you have:
>
> "Since the backing storage for these is shared we cannot ensure that
> the value won't change by writes from other threads."
>
> How does something like this sound?
>
> "Since the backing storage for these is shared we cannot ensure that
> the value won't change by writes from other threads. Normally SSBO
> accesses are not guaranteed to be syncronized with other threads,
> except when memoryBarrier is used. So, we might be able to optimize
> some SSBO accesses, but for now we always take the safe path and emit
> the SSBO access."
Sure, that makes things more clear.
> With a change like that to these commit messages, you can add
> Reviewed-by: Jordan Justen
> to all 3 patches.
Thanks Jordan!
Iago
> > > On 2015-06-03 00:01:16, Iago Toral Quiroga wrote:
> > > > Since the backing storage for these is shared we cannot ensure that the
> > > > value won't change by writes from other threads.
> > > > ---
> > > > src/glsl/opt_copy_propagation.cpp | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/src/glsl/opt_copy_propagation.cpp
> > > > b/src/glsl/opt_copy_propagation.cpp
> > > > index 806027b..f206995 100644
> > > > --- a/src/glsl/opt_copy_propagation.cpp
> > > > +++ b/src/glsl/opt_copy_propagation.cpp
> > > > @@ -330,7 +330,7 @@ ir_copy_propagation_visitor::add_copy(ir_assignment
> > > > *ir)
> > > > */
> > > > ir->condition = new(ralloc_parent(ir)) ir_constant(false);
> > > > this->progress = true;
> > > > - } else {
> > > > + } else if (lhs_var->data.mode != ir_var_shader_storage) {
> > > > entry = new(this->acp) acp_entry(lhs_var, rhs_var);
> > > > this->acp->push_tail(entry);
> > > >}
> > > > --
> > > > 1.9.1
> > > >
> > > > ___
> > > > 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] u_vbuf: fix src_offset alignment in u_vbuf_create_vertex_elements()
The cherry-picking to 1.6 is okay with me. Marek On Wed, Jun 24, 2015 at 3:52 PM, Brian Paul wrote: > On 06/24/2015 06:49 AM, Emil Velikov wrote: >> >> Hello gents, >> >> On 19 June 2015 at 15:39, Brian Paul wrote: >>> >>> If the driver says >>> PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY=1, >>> the driver should never receive a pipe_vertex_element::src_offset value >>> that's not a multiple of four. But the vbuf code wasn't actually >>> adjusting >>> the src_offset value when creating the vertex element state object. >>> >>> We just need to align the src_offset values put in the driver_attribs[] >>> array. >>> >>> See the piglit gl-1.5-vertex-buffer-offsets test. >> >> Is this something we want in the 10.6 stable branch ? According to the >> comment in the piglit test, the spec does not explicitly state what >> should happen in the particular case, but I'd assume that we want to >> provide some safe behaviour ? > > > I think the patch fixes a pretty rare situation so I wasn't too concerned > about porting to the 10.6 branch. But I don't think there'd be any harm in > cherry-picking it to 10.6. > > -Brian > > > > ___ > 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] mesa: move ARB_gs5 enums to core, EXT_polygon_offset_clamp to desktop
It seemed minor enough to not really matter, but I wouldn't be against this going into 10.5/10.6 On Jun 24, 2015 08:57, "Emil Velikov" wrote: > Hi Ilia, > On 19 June 2015 at 17:23, Ilia Mirkin wrote: > > When adding EXT_polygon_offset_clamp, I first made it core-only, and > > never moved the enum getter back to the GL/GL_CORE section. Similarly, > > ARB_gs5 is a core-only extension, so move its getters to the GL_CORE > > section. > > > Would this be worthy for 10.6 ? Afaict without it, attempting to query > POLYGON_OFFSET_CLAMP_EXT will return an error. > > -Emil > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] u_vbuf: fix src_offset alignment in u_vbuf_create_vertex_elements()
On 06/24/2015 06:49 AM, Emil Velikov wrote: Hello gents, On 19 June 2015 at 15:39, Brian Paul wrote: If the driver says PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY=1, the driver should never receive a pipe_vertex_element::src_offset value that's not a multiple of four. But the vbuf code wasn't actually adjusting the src_offset value when creating the vertex element state object. We just need to align the src_offset values put in the driver_attribs[] array. See the piglit gl-1.5-vertex-buffer-offsets test. Is this something we want in the 10.6 stable branch ? According to the comment in the piglit test, the spec does not explicitly state what should happen in the particular case, but I'd assume that we want to provide some safe behaviour ? I think the patch fixes a pretty rare situation so I wasn't too concerned about porting to the 10.6 branch. But I don't think there'd be any harm in cherry-picking it to 10.6. -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder
On Wed, Jun 24, 2015 at 6:44 AM, Francisco Jerez wrote: > Jason Ekstrand writes: > >> On Jun 24, 2015 6:29 AM, "Francisco Jerez" wrote: >>> >>> Jason Ekstrand writes: >>> >>> > On Jun 24, 2015 4:29 AM, "Francisco Jerez" >> wrote: >>> >> >>> >> Jason Ekstrand writes: >>> >> >>> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez < >> curroje...@riseup.net> >>> > wrote: >>> >> >> Jason Ekstrand writes: >>> >> >> >>> >> >>> We want to move these into the builder so that they know the >> current >>> >> >>> builder's dispatch width. This will be needed by a later commit. >>> >> >> >>> >> >> I very much like the idea of this series, but, why do you need to >> move >>> >> >> these register manipulators into the builder? The builder is an >> object >>> >> >> you can use to: >>> >> >> - Manipulate and query parameters affecting code generation. >>> >> >> - Create instructions into the program (::emit and friends). >>> >> >> - Allocate virtual registers from the program (::vgrf and friends). >>> >> >> >>> >> >> offset() and half() logically perform an action on a given register >>> >> >> object (or rather, compute a function of a given register object), >> not >>> >> >> on a builder object, the builder is only required as an auxiliary >>> >> >> parameter -- Any reason you didn't just pass it as a third >> parameter? >>> >> > >>> >> > What's required as a third parameter is the current execution size. >> I >>> >> > could have passed that directly, but I figured that, especially for >>> >> > half(), it would get messed up. I could pass the builder in but I >>> >> > don't see a whole lot of difference between that and what I'm doing >>> >> > right now. >>> >> >>> >> Assembly-wise there's no difference, but it seems inconsistent with >> both >>> >> the remaining register manipulators and remaining builder methods, and >>> >> IMHO it's kind of an anti-pattern to make something a method that >>> >> doesn't need access to any internal details of the object. >>> >> >>> >> > As is, it's not entirely obvious whether you should call >>> >> > half(reg) on the half-width or full-width builder. I'm not 100% sure >>> >> > what to do about that. >>> >> > >>> >> Actually, does half() really need to know about the builder? AFAICT it >>> >> only needs it because of dispatch_width(), and before doing anything >>> >> useful with it it asserts that it's equal to 16, what points at the >>> >> parameter being redundant. By convention a "half" is a group of 8 >>> >> channels (we may want to revise this convention when we implement >> SIMD32 >>> >> -- E.g. make half a group of 16 channels and quarter a group of 8 >>> >> channels), so 'half(reg)' could simply be implemented as >>> >> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As >>> >> additional paranoia to catch half() being called on a non-16-aligned >>> >> register you could assert that either 'stride == 0' or 16 divides >>> >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why >>> >> don't we have a reg_offset already in bytes again?) -- That would also >>> >> catch cases in which the register and builder "widths" get out of sync, >>> >> e.g. if half is called in an already halved register but the builder >>> >> used happens to be of the correct exec_size. >>> > >>> > OK, fine, we can pull half() back out. Should offset() stay in the >>> > builder? If not, where should it get its dispatch width. >>> > >>> I'm for leaving it as a stand-alone function (like all other register >>> manipulators), and add a third argument to pass the 'fs_builder' it can >>> take the dispatch width from? >> >> I'm not a big fan. However, in the interest of keeping the builder clean, > > It also keeps the register interface consistent IMHO. Why do you say > you're not a big fan? Unfortunately, there's really no good place to put it. Ideally, we'd have more/better information in the allocator or somewhere and we could do offset() type operations with just the register. Unfortunately, that's not an option in the current setup and I don't think keeping reg.width around just for offset() is worth it. That means it has to pull it from somewhere. It can't pull it from the visitor because it needs to be the width we're currently using for building. That means it needs to pull it from the builder. Yes, it could take an extra parameter, but it seems kind of awkward to have something that takes a builder that isn't "building" anything. On the other hand, it seemed to me at the time like offset() went together rather well with fs_builder::vgrf(). You get one with vgrf() and you get different components with offset() on the same builder. I'm not that attached to keeping it in the builder. It just seems like it's not the best of the bad options available. --Jason >> I'm willing to go with that. >> --Jason >> >>> >> >> As offset() and half() don't require access to any private details >> of >>> >> >> the builder, that would actually improve encapsulati
Re: [Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation
On 24/06/15 03:35, Ian Romanick wrote:
I'd also like to see assembly dumps with and without
-fno-strict-aliasing of some place where this goes wrong. If you,
Davin, or someone else can point out a specific function that actually
does the wrong thing, I can generate assembly myself. For that
matter... how the heck is the ralloc (or any memory allocator)
implementation valid?
Ok, got one. In src/glsl/link_varying.cpp, function
"canonicalize_shader_io". I compiled with gcc 4.8.4 on x86, with
CFLAGS="-O3 -fomit-frame-pointer -march=corei7". The method has a loop
that looks like this:
/* Remove the variable from it's current location in the IR, and put
it at
* the front.
*/
for (unsigned i = 0; i < num_variables; i++) {
var_table[i]->remove();
ir->push_head(var_table[i]);
}
With -fno-strict-aliasing (i.e. "correctly compiled") this becomes
(with my comments added):
.p2align 4,,10
.p2align 3
# at this point: %ecx = var_table, %eax = ir,
#%edi = var_table + num_variables
.L59:
movl(%ecx), %edx # %edx = var_table[i]
# call to remove() is inlined beginning here:
movl4(%edx), %ebp # %ebp = (%edx)->next
movl8(%edx), %esi # %esi = (%edx)->prev
movl%esi, 4(%ebp) # (%edx)->next->prev = (%edx)->prev
movl4(%edx), %esi # %esi = (%edx)->next(reload)
movl8(%edx), %ebp # %ebp = (%edx)->prev (reload)
movl%esi, 0(%ebp) # (%edx)->prev->next = (%edx)->next
movl$0, %esi
movl$0, 4(%edx) # (%edx)->next = NULL
movl$0, 8(%edx) # (%edx)->prev = NULL
# inlined call to remove() completes.
movl(%ecx), %ebp # %ebp = var_table[i]
leal4(%ebp), %edx # %edx = (struct exec_node *)(%ebp)
testl %ebp, %ebp# NULL check(?!)
movl(%eax), %ebp # Note that %eax = ir: %ebp=ir->head
cmove %esi, %edx# %edx = NULL if var_table[i]==NULL
addl$4, %ecx # (i++)
cmpl%edi, %ecx# (i < num_variables?)
# ir->push_head(var_table[i])
movl%eax, 4(%edx) # var_table[i]->prev = & ir->head
movl%ebp, (%edx) # var_table[i]->next = ir->head
movl%edx, 4(%ebp) # ir->head->prev = var_table[i]
movl%edx, (%eax) # ir->head = var_table[i]
jne .L59
This more-or-less makes sense (there is an unnecessary NULL check
generated by the implicit type conversion of ir_variable* to
exec_node*). Now, without -fno-strict-aliasing it's:
jmp .L59 # note this jump
.p2align 4,,10
.p2align 3
.L74:
movl%esi, %edi
# %ecx = var_table, %eax = ir, %edi = ir->head
.L59:
# inlined remove():
movl(%ecx), %edx # %edx = var_table[i]
movl4(%edx), %esi # %esi = (%edx)->next
movl8(%edx), %ebp # %ebp = (%edx)->prev
movl%ebp, 4(%esi) # (%esi)->next->prev = (%edx)->prev
movl8(%edx), %ebp # re-load (%edx)->prev
# in this case reload of ->next is omitted
movl%esi, 0(%ebp) #(%edx)->prev->next = (%edx)->next
movl$0, 4(%edx) # (%edx)->next = NULL
movl$0, 8(%edx) # (%edx)->prev = NULL
movl(%ecx), %edx# %edx = var_table[i]
leal4(%edx), %esi # %esi = (struct exec node *)%edx
testl %edx, %edx # NULL check
movl$0, %edx
cmove %edx, %esi
addl$4, %ecx# (i++)
cmpl28(%esp), %ecx # (i < num_variables?)
movl%edi, (%esi)# var_table[i]->next = ir->head
movl%eax, 4(%esi) # var_table[i]->prev = & ir->head
movl%esi, 4(%edi) #ir->head->prev = var_table[i]
jne .L74
movl%esi, (%eax)# ir->head = var_table[i]
Note that it doesn't update ir->head in memory until right at the end of
the loop (i.e. it collapses all writes to ir->head into a single write).
The current value of ir->head is kept in %edi, and is neither stored in
nor re-loaded from memory during the loop. This is the problem: the
line that sets "(%edx)->prev->next = (%edx)->next" can in fact store to
ir->head (which is an aliasing violation), so it does need to be
reloaded after that line (and stored before it).
Davin
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/17] i965/fs: Move offset() and half() to the fs_builder
Jason Ekstrand writes: > On Jun 24, 2015 6:29 AM, "Francisco Jerez" wrote: >> >> Jason Ekstrand writes: >> >> > On Jun 24, 2015 4:29 AM, "Francisco Jerez" > wrote: >> >> >> >> Jason Ekstrand writes: >> >> >> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez < > curroje...@riseup.net> >> > wrote: >> >> >> Jason Ekstrand writes: >> >> >> >> >> >>> We want to move these into the builder so that they know the > current >> >> >>> builder's dispatch width. This will be needed by a later commit. >> >> >> >> >> >> I very much like the idea of this series, but, why do you need to > move >> >> >> these register manipulators into the builder? The builder is an > object >> >> >> you can use to: >> >> >> - Manipulate and query parameters affecting code generation. >> >> >> - Create instructions into the program (::emit and friends). >> >> >> - Allocate virtual registers from the program (::vgrf and friends). >> >> >> >> >> >> offset() and half() logically perform an action on a given register >> >> >> object (or rather, compute a function of a given register object), > not >> >> >> on a builder object, the builder is only required as an auxiliary >> >> >> parameter -- Any reason you didn't just pass it as a third > parameter? >> >> > >> >> > What's required as a third parameter is the current execution size. > I >> >> > could have passed that directly, but I figured that, especially for >> >> > half(), it would get messed up. I could pass the builder in but I >> >> > don't see a whole lot of difference between that and what I'm doing >> >> > right now. >> >> >> >> Assembly-wise there's no difference, but it seems inconsistent with > both >> >> the remaining register manipulators and remaining builder methods, and >> >> IMHO it's kind of an anti-pattern to make something a method that >> >> doesn't need access to any internal details of the object. >> >> >> >> > As is, it's not entirely obvious whether you should call >> >> > half(reg) on the half-width or full-width builder. I'm not 100% sure >> >> > what to do about that. >> >> > >> >> Actually, does half() really need to know about the builder? AFAICT it >> >> only needs it because of dispatch_width(), and before doing anything >> >> useful with it it asserts that it's equal to 16, what points at the >> >> parameter being redundant. By convention a "half" is a group of 8 >> >> channels (we may want to revise this convention when we implement > SIMD32 >> >> -- E.g. make half a group of 16 channels and quarter a group of 8 >> >> channels), so 'half(reg)' could simply be implemented as >> >> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As >> >> additional paranoia to catch half() being called on a non-16-aligned >> >> register you could assert that either 'stride == 0' or 16 divides >> >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why >> >> don't we have a reg_offset already in bytes again?) -- That would also >> >> catch cases in which the register and builder "widths" get out of sync, >> >> e.g. if half is called in an already halved register but the builder >> >> used happens to be of the correct exec_size. >> > >> > OK, fine, we can pull half() back out. Should offset() stay in the >> > builder? If not, where should it get its dispatch width. >> > >> I'm for leaving it as a stand-alone function (like all other register >> manipulators), and add a third argument to pass the 'fs_builder' it can >> take the dispatch width from? > > I'm not a big fan. However, in the interest of keeping the builder clean, It also keeps the register interface consistent IMHO. Why do you say you're not a big fan? > I'm willing to go with that. > --Jason > >> >> >> As offset() and half() don't require access to any private details > of >> >> >> the builder, that would actually improve encapsulation, and would > avoid >> >> >> the dubious overloading of fs_builder::half() with two methods with >> >> >> completely different semantics. >> >> > >> >> > Yeah, I don't really like that either. I just couldn't come up with >> >> > anything better at the time. >> >> > >> >> > Suggestions are very much welcome. But I would like to settle on >> >> > whatever we do fairly quickly so as to limit the amount of >> >> > refactoring. >> >> > --Jason >> >> > >> >> >> Thanks. >> >> >> >> >> >>> --- >> >> >>> src/mesa/drivers/dri/i965/brw_fs.cpp | 52 ++ >> >> >>> src/mesa/drivers/dri/i965/brw_fs_builder.h | 46 + >> >> >>> src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 2 +- >> >> >>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 60 +-- >> >> >>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 149 >> > ++- >> >> >>> src/mesa/drivers/dri/i965/brw_ir_fs.h| 51 - >> >> >>> 6 files changed, 182 insertions(+), 178 deletions(-) >> >> >>> >> >> >>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp >> > b/src/mesa/drivers/dri/i965/brw_fs.cpp >> >> >>> index 4f98d6
[Mesa-dev] [Bug 91077] dri2_glx.c:1186: undefined reference to `loader_open_device'
https://bugs.freedesktop.org/show_bug.cgi?id=91077 Emil Velikov changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #4 from Emil Velikov --- I do wonder if/when we can fold some of the conditional compilation mayhem in the loader, but until then I've pushed Julien's patch. Thanks for the prompt fix ! -- You are receiving this mail because: You are the QA Contact for the bug. 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] i965/skl: Fix aligning mt->total_width to the block size
Ben Widawsky writes:
> I think this is beginning to infringe upon the definition of align_w.
> The total width is a function of it's miptree properties and not the
> compressed block properties, right?
>
> In other words, if there is a case where align_w != bw, I think
> total_width should be aligned to align_w, NOT bw.
I don't think it's so clear cut. In practice the mt->total_width doesn't
really need to be aligned to anything because as far as I can tell it is
only used to calculate the row stride. The row stride is separately
aligned to whatever constraints necessary by libdrm so it doesn't really
matter what we pick here.
The reason I think that the intention was to align it to the block width
rather than the horizontal alignment is that in the non-compressed case
the total width isn't aligned to anything at all.
It's probably not worth making too much of a fuss over this patch seeing
as it doesn't make any practical difference. I'm happy to forget about
it and pretend I never noticed the inconsistency.
Regards,
- Neil
>
> (I'm not opposed to the patch, just making sure I understand.)
>
>> ---
>> src/mesa/drivers/dri/i965/brw_tex_layout.c | 5 ++---
>> 1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> index 1e7d8a1..dbb6cef 100644
>> --- a/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> +++ b/src/mesa/drivers/dri/i965/brw_tex_layout.c
>> @@ -366,9 +366,8 @@ brw_miptree_layout_2d(struct intel_mipmap_tree *mt)
>>
>> mt->total_width = mt->physical_width0;
>>
>> - if (mt->compressed) {
>> - mt->total_width = ALIGN(mt->physical_width0, mt->align_w);
>> - }
>> + if (mt->compressed)
>> + mt->total_width = ALIGN(mt->total_width, bw);
>>
>> /* May need to adjust width to accommodate the placement of
>> * the 2nd mipmap. This occurs when the alignment
>> --
>> 1.9.3
>>
>> ___
>> 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 07/17] i965/fs: Move offset() and half() to the fs_builder
Jason Ekstrand writes: > On Jun 24, 2015 4:29 AM, "Francisco Jerez" wrote: >> >> Jason Ekstrand writes: >> >> > On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez > wrote: >> >> Jason Ekstrand writes: >> >> >> >>> We want to move these into the builder so that they know the current >> >>> builder's dispatch width. This will be needed by a later commit. >> >> >> >> I very much like the idea of this series, but, why do you need to move >> >> these register manipulators into the builder? The builder is an object >> >> you can use to: >> >> - Manipulate and query parameters affecting code generation. >> >> - Create instructions into the program (::emit and friends). >> >> - Allocate virtual registers from the program (::vgrf and friends). >> >> >> >> offset() and half() logically perform an action on a given register >> >> object (or rather, compute a function of a given register object), not >> >> on a builder object, the builder is only required as an auxiliary >> >> parameter -- Any reason you didn't just pass it as a third parameter? >> > >> > What's required as a third parameter is the current execution size. I >> > could have passed that directly, but I figured that, especially for >> > half(), it would get messed up. I could pass the builder in but I >> > don't see a whole lot of difference between that and what I'm doing >> > right now. >> >> Assembly-wise there's no difference, but it seems inconsistent with both >> the remaining register manipulators and remaining builder methods, and >> IMHO it's kind of an anti-pattern to make something a method that >> doesn't need access to any internal details of the object. >> >> > As is, it's not entirely obvious whether you should call >> > half(reg) on the half-width or full-width builder. I'm not 100% sure >> > what to do about that. >> > >> Actually, does half() really need to know about the builder? AFAICT it >> only needs it because of dispatch_width(), and before doing anything >> useful with it it asserts that it's equal to 16, what points at the >> parameter being redundant. By convention a "half" is a group of 8 >> channels (we may want to revise this convention when we implement SIMD32 >> -- E.g. make half a group of 16 channels and quarter a group of 8 >> channels), so 'half(reg)' could simply be implemented as >> "horiz_offset(reg, 8 * i)" without any dependency on the builder. As >> additional paranoia to catch half() being called on a non-16-aligned >> register you could assert that either 'stride == 0' or 16 divides >> '(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why >> don't we have a reg_offset already in bytes again?) -- That would also >> catch cases in which the register and builder "widths" get out of sync, >> e.g. if half is called in an already halved register but the builder >> used happens to be of the correct exec_size. > > OK, fine, we can pull half() back out. Should offset() stay in the > builder? If not, where should it get its dispatch width. > I'm for leaving it as a stand-alone function (like all other register manipulators), and add a third argument to pass the 'fs_builder' it can take the dispatch width from? >> >> As offset() and half() don't require access to any private details of >> >> the builder, that would actually improve encapsulation, and would avoid >> >> the dubious overloading of fs_builder::half() with two methods with >> >> completely different semantics. >> > >> > Yeah, I don't really like that either. I just couldn't come up with >> > anything better at the time. >> > >> > Suggestions are very much welcome. But I would like to settle on >> > whatever we do fairly quickly so as to limit the amount of >> > refactoring. >> > --Jason >> > >> >> Thanks. >> >> >> >>> --- >> >>> src/mesa/drivers/dri/i965/brw_fs.cpp | 52 ++ >> >>> src/mesa/drivers/dri/i965/brw_fs_builder.h | 46 + >> >>> src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 2 +- >> >>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 60 +-- >> >>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 149 > ++- >> >>> src/mesa/drivers/dri/i965/brw_ir_fs.h| 51 - >> >>> 6 files changed, 182 insertions(+), 178 deletions(-) >> >>> >> >>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp > b/src/mesa/drivers/dri/i965/brw_fs.cpp >> >>> index 4f98d63..c13ac7d 100644 >> >>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp >> >>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp >> >>> @@ -267,7 +267,7 @@ fs_visitor::VARYING_PULL_CONSTANT_LOAD(const > fs_builder &bld, >> >>> inst->mlen = 1 + dispatch_width / 8; >> >>> } >> >>> >> >>> - bld.MOV(dst, offset(vec4_result, (const_offset & 3) * scale)); >> >>> + bld.MOV(dst, bld.offset(vec4_result, (const_offset & 3) * scale)); >> >>> } >> >>> >> >>> /** >> >>> @@ -361,7 +361,12 @@ fs_inst::is_copy_payload(const > brw::simple_allocator &grf_alloc) const >> >>>reg.width = this->src[i].width; >
Re: [Mesa-dev] [PATCH] configure: use $target_cpu, not $host_cpu when setting asm_arch
On 24 June 2015 at 10:28, Marek Olšák wrote: > Hi, > > This is how I build 32-bit Mesa: > > # Mandatory for 32-bit > dir=i386-linux-gnu > build=i686-linux-gnu > export CFLAGS="-m32 -O2 -g" > export CXXFLAGS="$CFLAGS" > export LDFLAGS="-L/usr/lib/$dir" On Arch/Gentoo (and maybe others) don't need the LDFLAGS, but I'd suspect it's essential for Debian based distros. If any anyone has some tips that might be I'll appreciate the input :-) Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation
Hi, If we are going to have to prod all of the code using this list implementation to solve this problem anyway maybe it would make more sense to switch to a kernel-style list instead. There is already an implementation of this in src/util/list.h. I think this style of list only accesses the pointers through a single struct so there shouldn't be any aliasing issues. It should also end up using one fewer pointer to store the head sentinel. It would be nice to reduce the number of list implementations used in Mesa. Regards, - Neil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] mesa: fold duplicated GL/GL_CORE/GLES3 entry in get_hash_params.py
Signed-off-by: Emil Velikov
---
src/mesa/main/get_hash_params.py | 8 +++-
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/mesa/main/get_hash_params.py b/src/mesa/main/get_hash_params.py
index 74ff3ba..c25e1b6 100644
--- a/src/mesa/main/get_hash_params.py
+++ b/src/mesa/main/get_hash_params.py
@@ -351,6 +351,9 @@ descriptor=[
# GL_ARB_framebuffer_object
[ "MAX_SAMPLES", "CONTEXT_INT(Const.MaxSamples),
extra_ARB_framebuffer_object_EXT_framebuffer_multisample" ],
+# GL_ARB_sampler_objects / GL 3.3 / GLES 3.0
+ [ "SAMPLER_BINDING", "LOC_CUSTOM, TYPE_INT, GL_SAMPLER_BINDING, NO_EXTRA" ],
+
# GL_ARB_sync
[ "MAX_SERVER_WAIT_TIMEOUT", "CONTEXT_INT64(Const.MaxServerWaitTimeout),
extra_ARB_sync" ],
@@ -404,11 +407,6 @@ descriptor=[
[ "TEXTURE_EXTERNAL_OES", "LOC_CUSTOM, TYPE_BOOLEAN, 0,
extra_OES_EGL_image_external" ],
]},
-{ "apis": ["GL", "GL_CORE", "GLES3"], "params": [
-# GL_ARB_sampler_objects / GL 3.3 / GLES 3.0
- [ "SAMPLER_BINDING", "LOC_CUSTOM, TYPE_INT, GL_SAMPLER_BINDING, NO_EXTRA" ],
-]},
-
# Enums in OpenGL Core profile and ES 3.1
{ "apis": ["GL_CORE", "GLES3"], "params": [
# GL_ARB_draw_indirect / GLES 3.1
--
2.4.2
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] mesa: move ARB_gs5 enums to core, EXT_polygon_offset_clamp to desktop
Hi Ilia, On 19 June 2015 at 17:23, Ilia Mirkin wrote: > When adding EXT_polygon_offset_clamp, I first made it core-only, and > never moved the enum getter back to the GL/GL_CORE section. Similarly, > ARB_gs5 is a core-only extension, so move its getters to the GL_CORE > section. > Would this be worthy for 10.6 ? Afaict without it, attempting to query POLYGON_OFFSET_CLAMP_EXT will return an error. -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] u_vbuf: fix src_offset alignment in u_vbuf_create_vertex_elements()
Hello gents, On 19 June 2015 at 15:39, Brian Paul wrote: > If the driver says PIPE_CAP_VERTEX_ELEMENT_SRC_OFFSET_4BYTE_ALIGNED_ONLY=1, > the driver should never receive a pipe_vertex_element::src_offset value > that's not a multiple of four. But the vbuf code wasn't actually adjusting > the src_offset value when creating the vertex element state object. > > We just need to align the src_offset values put in the driver_attribs[] > array. > > See the piglit gl-1.5-vertex-buffer-offsets test. Is this something we want in the 10.6 stable branch ? According to the comment in the piglit test, the spec does not explicitly state what should happen in the particular case, but I'd assume that we want to provide some safe behaviour ? -Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] clover: Implement image attribute getters
Zoltan Gilian writes:
> Image attributes are passed to the kernel as hidden parameters after the
> image attribute itself. An llvm pass replaces the getter builtins to
> the appropriate parameters.
This seems to be doing essentially the same thing as v1? Is it the
right patch?
> ---
> src/gallium/state_trackers/clover/core/kernel.cpp | 26 +++
> src/gallium/state_trackers/clover/core/kernel.hpp | 13 ++--
> src/gallium/state_trackers/clover/core/memory.cpp | 2 +-
> .../state_trackers/clover/llvm/invocation.cpp | 81
> +-
> 4 files changed, 116 insertions(+), 6 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/core/kernel.cpp
> b/src/gallium/state_trackers/clover/core/kernel.cpp
> index 0756f06..291c799 100644
> --- a/src/gallium/state_trackers/clover/core/kernel.cpp
> +++ b/src/gallium/state_trackers/clover/core/kernel.cpp
> @@ -185,6 +185,13 @@ kernel::exec_context::bind(intrusive_ptr
> _q,
>}
> }
>
> + // Bind image attribute args.
> + for (const auto& arg: kern._args) {
> + if (auto img_arg = dynamic_cast(arg.get())) {
> + img_arg->bind_attributes(*this);
> + }
> + }
> +
> // Create a new compute state if anything changed.
> if (!st || q != _q ||
> cs.req_local_mem != mem_local ||
> @@ -465,6 +472,25 @@ kernel::constant_argument::unbind(exec_context &ctx) {
> }
>
> void
> +kernel::image_argument::bind_attributes(exec_context &ctx) {
> + cl_image_format format = img->format();
> + cl_uint attributes[] = {
> + static_cast(img->width()),
> + static_cast(img->height()),
> + static_cast(img->depth()),
> + format.image_channel_data_type,
> + format.image_channel_order};
> + for (unsigned i = 0; i < 5; ++i) {
> + auto v = bytes(attributes[i]);
> +
> + extend(v, module::argument::zero_ext, sizeof(cl_uint));
> + byteswap(v, ctx.q->device().endianness());
> + align(ctx.input, sizeof(cl_uint));
> + insert(ctx.input, v);
> + }
> +}
> +
> +void
> kernel::image_rd_argument::set(size_t size, const void *value) {
> if (size != sizeof(cl_mem))
>throw error(CL_INVALID_ARG_SIZE);
> diff --git a/src/gallium/state_trackers/clover/core/kernel.hpp
> b/src/gallium/state_trackers/clover/core/kernel.hpp
> index d6432a4..8c15b2f 100644
> --- a/src/gallium/state_trackers/clover/core/kernel.hpp
> +++ b/src/gallium/state_trackers/clover/core/kernel.hpp
> @@ -190,7 +190,14 @@ namespace clover {
> pipe_surface *st;
>};
>
> - class image_rd_argument : public argument {
> + class image_argument : public argument {
> + public:
> + void bind_attributes(exec_context &ctx);
> + protected:
> + image *img;
> + };
> +
> + class image_rd_argument : public image_argument {
>public:
> virtual void set(size_t size, const void *value);
> virtual void bind(exec_context &ctx,
> @@ -198,11 +205,10 @@ namespace clover {
> virtual void unbind(exec_context &ctx);
>
>private:
> - image *img;
> pipe_sampler_view *st;
>};
>
> - class image_wr_argument : public argument {
> + class image_wr_argument : public image_argument {
>public:
> virtual void set(size_t size, const void *value);
> virtual void bind(exec_context &ctx,
> @@ -210,7 +216,6 @@ namespace clover {
> virtual void unbind(exec_context &ctx);
>
>private:
> - image *img;
> pipe_surface *st;
>};
>
> diff --git a/src/gallium/state_trackers/clover/core/memory.cpp
> b/src/gallium/state_trackers/clover/core/memory.cpp
> index 055336a..b852e68 100644
> --- a/src/gallium/state_trackers/clover/core/memory.cpp
> +++ b/src/gallium/state_trackers/clover/core/memory.cpp
> @@ -189,7 +189,7 @@ image2d::image2d(clover::context &ctx, cl_mem_flags flags,
> const cl_image_format *format, size_t width,
> size_t height, size_t row_pitch,
> void *host_ptr) :
> - image(ctx, flags, format, width, height, 0,
> + image(ctx, flags, format, width, height, 1,
> row_pitch, 0, height * row_pitch, host_ptr) {
> }
>
> diff --git a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> index 9b91fee..a33d450 100644
> --- a/src/gallium/state_trackers/clover/llvm/invocation.cpp
> +++ b/src/gallium/state_trackers/clover/llvm/invocation.cpp
> @@ -80,6 +80,7 @@
> using namespace clover;
>
> namespace {
> +
> #if 0
> void
> build_binary(const std::string &source, const std::string &target,
> @@ -340,17 +341,65 @@ namespace {
>PM.run(*mod);
> }
>
> + const llvm::MDNode *
> + get_kernel_metadata(const llvm::Function *kernel_func) {
> + auto mod = kernel_func->getParent();
> + auto kernels_node = mod->getNamedMetadata("opencl.kernels");
> + i
Re: [Mesa-dev] [PATCH v2 23/82] glsl: Do not do CSE for expressions involving SSBO loads
Iago Toral writes:
> On Wed, 2015-06-17 at 17:20 -0700, Jordan Justen wrote:
>> I wanted to question whether this was required, based on this text
>> from the extension spec:
>>
>> "The ability to write to buffer objects creates the potential for
>> multiple independent shader invocations to read and write the same
>> underlying memory. The same issue exists with the
>> ARB_shader_image_load_store extension provided in OpenGL 4.2, which
>> can write to texture objects and buffers. In both cases, the
>> specification makes few guarantees related to the relative order of
>> memory reads and writes performed by the shader invocations."
>>
>> But I'm not sure if we can reconcile CSE with 'memoryBarrier' and
>> 'barrier'. curro, any thoughts from image load/store?
>
> I think the problem is within the same thread, that text above talks
> about multiple invocations reading from and writing to the same
> location, but within the same invocation, the order of reads and writes
> must be preserved:
>
> "Buffer variable memory reads and writes within a single shader
> invocation are processed in order. However, the order of reads and
> writes performed in one invocation relative to those performed by
> another invocation is largely undefined."
>
> For example, if X is a shader storage buffer variable and we have code
> like this with just one invocation:
>
> ssbo_store(X, 1);
> a = ssbo_load(X) + 1 // a = 2
> ssbo_store(X, 2);
> b = ssbo_load(X) + 1; // b = 3
>
> CSE could mess it up like this:
>
> ssbo_store(X, 1);
> tmp = ssbo_load(X) + 1 // tmp = 2
> a = tmp;
> ssbo_store(X, 2);
> b = tmp;
>
> which would be incorrect. I think I wrote this patch after seeing
> something like this happening. The CSE pass clearly states that it does
> not support write variables after all.
>
> Also, notice the same would apply if there are multiple invocations but
> the shader code used something like gl_VertexID or gl_FragCoord to make
> each invocation read from/write to a different address within the SSBO
> buffer (I imagine this is the usual way to operate with SSBOs). In these
> cases, even if we have multiple invocations, keeping the relative order
> of reads and writes within each one is necessary.
>
AFAICT the reason why this (and many of the other changes in GLSL
optimization passes) is needed is because SSBO loads have been
implemented as ir_expression nodes instead of being lowered into
intrinsics (as other side-effectful operations do like
ARB_shader_image_load_store and ARB_shader_atomic_counters). This
surely broke the assumption of a number of optimization passes that
ir_expression nodes behave as pure functions. I guess the reason why
you've done it this way is because UBO loads were already being
represented as expressions, so I see why you may have wanted to use the
same approach for SSBOs even though there is a fundamental difference
between the two: UBO loads have no side effects and are constant for a
given set of arguments and a given shader execution, SSBO loads and
stores are not. SSBO stores couldn't be accommodated into the same
framework so easily, and you decided to create a separate ir node for
them, what seems inconsistent with loads. Intrinsics would probably
have been a good fit for both loads and stores, and would have made all
these optimization changes unnecessary...
P.S.: Sorry for the late reply, I was on vacation when I was CC'ed.
> Iago
>
>> -Jordan
>>
>> On 2015-06-03 00:01:13, Iago Toral Quiroga wrote:
>> > SSBOs are read/write and this CSE pass only handles read-only variables.
>> > ---
>> > src/glsl/opt_cse.cpp | 33 -
>> > 1 file changed, 32 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/glsl/opt_cse.cpp b/src/glsl/opt_cse.cpp
>> > index 4b8e9a0..a05ab46 100644
>> > --- a/src/glsl/opt_cse.cpp
>> > +++ b/src/glsl/opt_cse.cpp
>> > @@ -245,6 +245,28 @@ contains_rvalue(ir_rvalue *haystack, ir_rvalue
>> > *needle)
>> > }
>> >
>> > static bool
>> > +expression_contains_ssbo_load(ir_expression *expr)
>> > +{
>> > + if (expr->operation == ir_binop_ssbo_load)
>> > + return true;
>> > +
>> > + for (unsigned i = 0; i < expr->get_num_operands(); i++) {
>> > + ir_rvalue *op = expr->operands[i];
>> > + if (op->ir_type == ir_type_expression &&
>> > + expression_contains_ssbo_load(op->as_expression())) {
>> > + return true;
>> > + } else if (op->ir_type == ir_type_swizzle) {
>> > + ir_swizzle *swizzle = op->as_swizzle();
>> > + ir_expression *val = swizzle->val->as_expression();
>> > + if (val && expression_contains_ssbo_load(val))
>> > +return true;
>> > + }
>> > + }
>> > +
>> > + return false;
>> > +}
>> > +
>> > +static bool
>> > is_cse_candidate(ir_rvalue *ir)
>> > {
>> > /* Our temporary variable assignment generation isn't ready to handle
>> > @@ -260,7 +282,16 @@ is_cse_candidate(ir_rvalue *ir)
>> > * to variable-index array dereferenc
Re: [Mesa-dev] [PATCH v3 1/6] mesa/es3.1: enable GL_ARB_shader_image_load_store for gles3.1
small style nits below ..
On 06/15/2015 02:19 PM, Marta Lofstedt wrote:
From: Marta Lofstedt
v3: only expose enums from GL_ARB_shader_image_load_store
for gles 3.1 and GL core
Signed-off-by: Marta Lofstedt
---
src/mesa/main/get.c | 6 ++
src/mesa/main/get_hash_params.py | 16 +---
2 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
index 1bc9b5d..0ab6837 100644
--- a/src/mesa/main/get.c
+++ b/src/mesa/main/get.c
@@ -355,6 +355,12 @@ static const int extra_ARB_draw_indirect_es31[] = {
EXTRA_END
};
+static const int extra_ARB_shader_image_load_store_es31[] = {
+ EXT(ARB_shader_image_load_store),
+ EXTRA_API_ES31,
+ EXTRA_END
+};
+
EXTRA_EXT(ARB_texture_cube_map);
EXTRA_EXT(EXT_texture_array);
EXTRA_EXT(NV_fog_distance);
diff --git a/src/mesa/main/get_hash_params.py b/src/mesa/main/get_hash_params.py
index 513d5d2..7b98fee 100644
--- a/src/mesa/main/get_hash_params.py
+++ b/src/mesa/main/get_hash_params.py
@@ -407,6 +407,14 @@ descriptor=[
{ "apis": ["GL", "GL_CORE", "GLES3"], "params": [
# GL_ARB_sampler_objects / GL 3.3 / GLES 3.0
[ "SAMPLER_BINDING", "LOC_CUSTOM, TYPE_INT, GL_SAMPLER_BINDING, NO_EXTRA" ],
+
whitespace errors on line above
+# GL_ARB_shader_image_load_store / GLES 3.1
+ [ "MAX_IMAGE_UNITS", "CONTEXT_INT(Const.MaxImageUnits),
extra_ARB_shader_image_load_store_es31" ],
+ [ "MAX_COMBINED_IMAGE_UNITS_AND_FRAGMENT_OUTPUTS",
"CONTEXT_INT(Const.MaxCombinedImageUnitsAndFragmentOutputs),
extra_ARB_shader_image_load_store_es31" ],
+ [ "MAX_IMAGE_SAMPLES", "CONTEXT_INT(Const.MaxImageSamples),
extra_ARB_shader_image_load_store_es31" ],
+ [ "MAX_VERTEX_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
+ [ "MAX_FRAGMENT_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
+ [ "MAX_COMBINED_IMAGE_UNIFORMS", "CONTEXT_INT(Const.MaxCombinedImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
]},
# Enums in OpenGL Core profile and ES 3.1
@@ -781,13 +789,7 @@ descriptor=[
[ "MAX_VERTEX_ATTRIB_BINDINGS", "CONTEXT_ENUM(Const.MaxVertexAttribBindings),
NO_EXTRA" ],
# GL_ARB_shader_image_load_store
- [ "MAX_IMAGE_UNITS", "CONTEXT_INT(Const.MaxImageUnits),
extra_ARB_shader_image_load_store"],
- [ "MAX_COMBINED_IMAGE_UNITS_AND_FRAGMENT_OUTPUTS",
"CONTEXT_INT(Const.MaxCombinedImageUnitsAndFragmentOutputs),
extra_ARB_shader_image_load_store"],
- [ "MAX_IMAGE_SAMPLES", "CONTEXT_INT(Const.MaxImageSamples),
extra_ARB_shader_image_load_store"],
- [ "MAX_VERTEX_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxImageUniforms),
extra_ARB_shader_image_load_store"],
- [ "MAX_GEOMETRY_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxImageUniforms),
extra_ARB_shader_image_load_store_and_geometry_shader"],
- [ "MAX_FRAGMENT_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxImageUniforms),
extra_ARB_shader_image_load_store"],
- [ "MAX_COMBINED_IMAGE_UNIFORMS", "CONTEXT_INT(Const.MaxCombinedImageUniforms),
extra_ARB_shader_image_load_store"],
+[ "MAX_GEOMETRY_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxImageUniforms),
extra_ARB_shader_image_load_store_and_geometry_shader"],
This line should be just left as it is, your changes modify indentation.
# GL_ARB_compute_shader
[ "MAX_COMPUTE_WORK_GROUP_INVOCATIONS",
"CONTEXT_INT(Const.MaxComputeWorkGroupInvocations), extra_ARB_compute_shader" ],
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 2/6] mesa/es3.1: enable GL_ARB_shader_atomic_counters for GLES 3.1
On 06/15/2015 02:19 PM, Marta Lofstedt wrote:
From: Marta Lofstedt
v3 : only expose ARB_shader_atomic_counters enums
for gles 3.1 and GL core.
Signed-off-by: Marta Lofstedt
---
src/mesa/main/get.c | 6 ++
src/mesa/main/get_hash_params.py | 22 +-
2 files changed, 19 insertions(+), 9 deletions(-)
diff --git a/src/mesa/main/get.c b/src/mesa/main/get.c
index 0ab6837..4a0537d 100644
--- a/src/mesa/main/get.c
+++ b/src/mesa/main/get.c
@@ -361,6 +361,12 @@ static const int extra_ARB_shader_image_load_store_es31[]
= {
EXTRA_END
};
+static const int extra_ARB_shader_atomic_counters_es31[] = {
+ EXT(ARB_shader_atomic_counters),
+ EXTRA_API_ES31,
+ EXTRA_END
+};
+
EXTRA_EXT(ARB_texture_cube_map);
EXTRA_EXT(EXT_texture_array);
EXTRA_EXT(NV_fog_distance);
diff --git a/src/mesa/main/get_hash_params.py b/src/mesa/main/get_hash_params.py
index 7b98fee..7de9169 100644
--- a/src/mesa/main/get_hash_params.py
+++ b/src/mesa/main/get_hash_params.py
@@ -415,6 +415,19 @@ descriptor=[
[ "MAX_VERTEX_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
[ "MAX_FRAGMENT_IMAGE_UNIFORMS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
[ "MAX_COMBINED_IMAGE_UNIFORMS", "CONTEXT_INT(Const.MaxCombinedImageUniforms),
extra_ARB_shader_image_load_store_es31" ],
+
+# GL_ARB_shader_atomic_counters / GLES 3.1
+ [ "ATOMIC_COUNTER_BUFFER_BINDING", "LOC_CUSTOM, TYPE_INT, 0,
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_ATOMIC_COUNTER_BUFFER_BINDINGS",
"CONTEXT_INT(Const.MaxAtomicBufferBindings), extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_ATOMIC_COUNTER_BUFFER_SIZE", "CONTEXT_INT(Const.MaxAtomicBufferSize),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_VERTEX_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_VERTEX_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxAtomicCounters),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_FRAGMENT_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_FRAGMENT_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxAtomicCounters),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_GEOMETRY_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_GEOMETRY_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxAtomicCounters),
extra_ARB_shader_atomic_counters_es31" ],
2 GEOMETRY enums above should not be here, they are only for atomic
buffers + desktop GL (you have left them in correct place in this
patch), remove them from here.
+ [ "MAX_COMBINED_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.MaxCombinedAtomicBuffers), extra_ARB_shader_atomic_counters_es31" ],
+ [ "MAX_COMBINED_ATOMIC_COUNTERS", "CONTEXT_INT(Const.MaxCombinedAtomicCounters),
extra_ARB_shader_atomic_counters_es31" ],
]},
# Enums in OpenGL Core profile and ES 3.1
@@ -772,17 +785,8 @@ descriptor=[
[ "PROGRAM_PIPELINE_BINDING", "LOC_CUSTOM, TYPE_INT,
GL_PROGRAM_PIPELINE_BINDING, NO_EXTRA" ],
# GL_ARB_shader_atomic_counters
- [ "ATOMIC_COUNTER_BUFFER_BINDING", "LOC_CUSTOM, TYPE_INT, 0,
extra_ARB_shader_atomic_counters" ],
- [ "MAX_ATOMIC_COUNTER_BUFFER_BINDINGS",
"CONTEXT_INT(Const.MaxAtomicBufferBindings), extra_ARB_shader_atomic_counters" ],
- [ "MAX_ATOMIC_COUNTER_BUFFER_SIZE", "CONTEXT_INT(Const.MaxAtomicBufferSize),
extra_ARB_shader_atomic_counters" ],
- [ "MAX_VERTEX_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters" ],
- [ "MAX_VERTEX_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_VERTEX].MaxAtomicCounters),
extra_ARB_shader_atomic_counters" ],
- [ "MAX_FRAGMENT_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters" ],
- [ "MAX_FRAGMENT_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_FRAGMENT].MaxAtomicCounters),
extra_ARB_shader_atomic_counters" ],
[ "MAX_GEOMETRY_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxAtomicBuffers),
extra_ARB_shader_atomic_counters_and_geometry_shader" ],
[ "MAX_GEOMETRY_ATOMIC_COUNTERS",
"CONTEXT_INT(Const.Program[MESA_SHADER_GEOMETRY].MaxAtomicCounters),
extra_ARB_shader_atomic_counters_and_geometry_shader" ],
- [ "MAX_COMBINED_ATOMIC_COUNTER_BUFFERS",
"CONTEXT_INT(Const.MaxCombinedAtomicBuffers), extra_ARB_shader_atomic_counters" ],
- [ "MAX_COMBINED_ATOMIC_COUNTERS", "CONTEXT_INT(Const.MaxCombinedAtomicCounters),
extra_ARB_shader_atomic_counters" ],
# GL_ARB_vertex_attrib_binding
[
Re: [Mesa-dev] [PATCH v3 0/6] Continue enabling OpenGL ES 3.1
Hi Marta; In all of the commits you state 'for gles 3.1 and GL core', however we move enums to a section where they are available for both desktop core and compatibility contexts (as expected), please change 'GL core' to 'desktop GL' in the message. I've sent some small comments to patches 1 and 2, with commit messages and those small things fixed this series is Reviewed-by: Tapani Pälli (I also run Piglit with these, no regressions.) On 06/15/2015 02:19 PM, Marta Lofstedt wrote: This is the V3 versions of my previous patch-set. Also, please note that Tapani's patch that was originally part of this patch-set was merged: git@83624c141d3 Marta Lofstedt (6): mesa/es3.1: enable GL_ARB_shader_image_load_store for gles3.1 mesa/es3.1: enable GL_ARB_shader_atomic_counters for GLES 3.1 mesa/es3.1: enable GL_ARB_texture_multisample for GLES 3.1 mesa/es3.1: enable GL_ARB_texture_gather for GLES 3.1 mesa/es3.1: enable GL_ARB_compute_shader for GLES 3.1 mesa/es3.1: enable GL_ARB_explicit_uniform_location for GLES 3.1 src/mesa/main/get.c | 36 src/mesa/main/get_hash_params.py | 90 ++-- 2 files changed, 85 insertions(+), 41 deletions(-) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
Ok, I *thought* we didn't get ArrayID on IN/OUT, but only TEMP? If it
is safe to assume that we always get ArrayID that makes it much
easier.
BR,
-R
On Wed, Jun 24, 2015 at 5:39 AM, Marek Olšák wrote:
> It's not an array, because the ArrayID is 0. It's a valid non-array
> declaration. If any TGSI user doesn't understand it, that user should
> be fixed.
>
> Marek
>
> On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
>> From: Rob Clark
>>
>> Ok, so actually there is a ttn issue here to fix as well.. but it
>> brought up a question in my mind. When ttn sees something like
>>
>> DCL IN[0..1]
>>
>> it will treat that as an array (which in the end will result in
>> constraints about where the registers get allocated. Which is not
>> really ideal.
>>
>> With glsl we don't actually get input arrays (but instead a bunch
>> of MOV's to a TEMP array) currently. So I'm not quite sure how
>> an actual input array should look. (But my preference would be
>> IN[a..b] for arrays and IN[c] otherwise)
>> ---
>> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/gallium/auxiliary/hud/hud_context.c
>> b/src/gallium/auxiliary/hud/hud_context.c
>> index 6a124f7..2b6d3a7 100644
>> --- a/src/gallium/auxiliary/hud/hud_context.c
>> +++ b/src/gallium/auxiliary/hud/hud_context.c
>> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
>> cso_context *cso)
>> {
>>static const char *vertex_shader_text = {
>> "VERT\n"
>> - "DCL IN[0..1]\n"
>> + "DCL IN[0]\n"
>> + "DCL IN[1]\n"
>> "DCL OUT[0], POSITION\n"
>> "DCL OUT[1], COLOR[0]\n" /* color */
>> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
>> --
>> 2.4.3
>>
>> ___
>> 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 07/17] i965/fs: Move offset() and half() to the fs_builder
Jason Ekstrand writes:
> On Tue, Jun 23, 2015 at 9:22 AM, Francisco Jerez
> wrote:
>> Jason Ekstrand writes:
>>
>>> We want to move these into the builder so that they know the current
>>> builder's dispatch width. This will be needed by a later commit.
>>
>> I very much like the idea of this series, but, why do you need to move
>> these register manipulators into the builder? The builder is an object
>> you can use to:
>> - Manipulate and query parameters affecting code generation.
>> - Create instructions into the program (::emit and friends).
>> - Allocate virtual registers from the program (::vgrf and friends).
>>
>> offset() and half() logically perform an action on a given register
>> object (or rather, compute a function of a given register object), not
>> on a builder object, the builder is only required as an auxiliary
>> parameter -- Any reason you didn't just pass it as a third parameter?
>
> What's required as a third parameter is the current execution size. I
> could have passed that directly, but I figured that, especially for
> half(), it would get messed up. I could pass the builder in but I
> don't see a whole lot of difference between that and what I'm doing
> right now.
Assembly-wise there's no difference, but it seems inconsistent with both
the remaining register manipulators and remaining builder methods, and
IMHO it's kind of an anti-pattern to make something a method that
doesn't need access to any internal details of the object.
> As is, it's not entirely obvious whether you should call
> half(reg) on the half-width or full-width builder. I'm not 100% sure
> what to do about that.
>
Actually, does half() really need to know about the builder? AFAICT it
only needs it because of dispatch_width(), and before doing anything
useful with it it asserts that it's equal to 16, what points at the
parameter being redundant. By convention a "half" is a group of 8
channels (we may want to revise this convention when we implement SIMD32
-- E.g. make half a group of 16 channels and quarter a group of 8
channels), so 'half(reg)' could simply be implemented as
"horiz_offset(reg, 8 * i)" without any dependency on the builder. As
additional paranoia to catch half() being called on a non-16-aligned
register you could assert that either 'stride == 0' or 16 divides
'(REG_SIZE * reg_offset + subreg_offset) / (stride * type_size)' (why
don't we have a reg_offset already in bytes again?) -- That would also
catch cases in which the register and builder "widths" get out of sync,
e.g. if half is called in an already halved register but the builder
used happens to be of the correct exec_size.
>> As offset() and half() don't require access to any private details of
>> the builder, that would actually improve encapsulation, and would avoid
>> the dubious overloading of fs_builder::half() with two methods with
>> completely different semantics.
>
> Yeah, I don't really like that either. I just couldn't come up with
> anything better at the time.
>
> Suggestions are very much welcome. But I would like to settle on
> whatever we do fairly quickly so as to limit the amount of
> refactoring.
> --Jason
>
>> Thanks.
>>
>>> ---
>>> src/mesa/drivers/dri/i965/brw_fs.cpp | 52 ++
>>> src/mesa/drivers/dri/i965/brw_fs_builder.h | 46 +
>>> src/mesa/drivers/dri/i965/brw_fs_cse.cpp | 2 +-
>>> src/mesa/drivers/dri/i965/brw_fs_nir.cpp | 60 +--
>>> src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 149
>>> ++-
>>> src/mesa/drivers/dri/i965/brw_ir_fs.h| 51 -
>>> 6 files changed, 182 insertions(+), 178 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> index 4f98d63..c13ac7d 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>>> @@ -267,7 +267,7 @@ fs_visitor::VARYING_PULL_CONSTANT_LOAD(const fs_builder
>>> &bld,
>>> inst->mlen = 1 + dispatch_width / 8;
>>> }
>>>
>>> - bld.MOV(dst, offset(vec4_result, (const_offset & 3) * scale));
>>> + bld.MOV(dst, bld.offset(vec4_result, (const_offset & 3) * scale));
>>> }
>>>
>>> /**
>>> @@ -361,7 +361,12 @@ fs_inst::is_copy_payload(const brw::simple_allocator
>>> &grf_alloc) const
>>>reg.width = this->src[i].width;
>>>if (!this->src[i].equals(reg))
>>> return false;
>>> - reg = ::offset(reg, 1);
>>> +
>>> + if (i < this->header_size) {
>>> + reg.reg_offset += 1;
>>> + } else {
>>> + reg.reg_offset += this->exec_size / 8;
>>> + }
>>> }
>>>
>>> return true;
>>> @@ -963,7 +968,7 @@ fs_visitor::emit_fragcoord_interpolation(bool
>>> pixel_center_integer,
>>> } else {
>>>bld.ADD(wpos, this->pixel_x, fs_reg(0.5f));
>>> }
>>> - wpos = offset(wpos, 1);
>>> + wpos = bld.offset(wpos, 1);
>>>
>>> /* gl_FragCoord.y */
>>> if (!flip && pixel_center_integer) {
>>> @@
Re: [Mesa-dev] [PATCH] nir: Use a switch statement for detecting move-like operations.
Reviewed-by: Samuel Iglesias Gonsálvez
Sam
On Tuesday 23 June 2015 23:17:53 Kenneth Graunke wrote:
> Suggested by Jason Ekstrand.
>
> Signed-off-by: Kenneth Graunke
> ---
> src/glsl/nir/nir_opt_peephole_select.c | 20 ++--
> 1 file changed, 14 insertions(+), 6 deletions(-)
>
> diff --git a/src/glsl/nir/nir_opt_peephole_select.c
> b/src/glsl/nir/nir_opt_peephole_select.c index ef7c977..6620e5d 100644
> --- a/src/glsl/nir/nir_opt_peephole_select.c
> +++ b/src/glsl/nir/nir_opt_peephole_select.c
> @@ -82,14 +82,22 @@ block_check_for_allowed_instrs(nir_block *block)
> break;
>
>case nir_instr_type_alu: {
> - /* It must be a move operation */
> nir_alu_instr *mov = nir_instr_as_alu(instr);
> - if (mov->op != nir_op_fmov && mov->op != nir_op_imov &&
> - mov->op != nir_op_fneg && mov->op != nir_op_ineg &&
> - mov->op != nir_op_fabs && mov->op != nir_op_iabs &&
> - mov->op != nir_op_vec2 && mov->op != nir_op_vec3 &&
> - mov->op != nir_op_vec4)
> + switch (mov->op) {
> + case nir_op_fmov:
> + case nir_op_imov:
> + case nir_op_fneg:
> + case nir_op_ineg:
> + case nir_op_fabs:
> + case nir_op_iabs:
> + case nir_op_vec2:
> + case nir_op_vec3:
> + case nir_op_vec4:
> +/* It must be a move-like operation. */
> +break;
> + default:
> return false;
> + }
>
> /* Can't handle saturate */
> if (mov->dest.saturate)
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [RFC] gallium/hud: fix issue w/ tgsi_to_nir
It's not an array, because the ArrayID is 0. It's a valid non-array
declaration. If any TGSI user doesn't understand it, that user should
be fixed.
Marek
On Tue, Jun 23, 2015 at 3:20 PM, Rob Clark wrote:
> From: Rob Clark
>
> Ok, so actually there is a ttn issue here to fix as well.. but it
> brought up a question in my mind. When ttn sees something like
>
> DCL IN[0..1]
>
> it will treat that as an array (which in the end will result in
> constraints about where the registers get allocated. Which is not
> really ideal.
>
> With glsl we don't actually get input arrays (but instead a bunch
> of MOV's to a TEMP array) currently. So I'm not quite sure how
> an actual input array should look. (But my preference would be
> IN[a..b] for arrays and IN[c] otherwise)
> ---
> src/gallium/auxiliary/hud/hud_context.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/auxiliary/hud/hud_context.c
> b/src/gallium/auxiliary/hud/hud_context.c
> index 6a124f7..2b6d3a7 100644
> --- a/src/gallium/auxiliary/hud/hud_context.c
> +++ b/src/gallium/auxiliary/hud/hud_context.c
> @@ -1163,7 +1163,8 @@ hud_create(struct pipe_context *pipe, struct
> cso_context *cso)
> {
>static const char *vertex_shader_text = {
> "VERT\n"
> - "DCL IN[0..1]\n"
> + "DCL IN[0]\n"
> + "DCL IN[1]\n"
> "DCL OUT[0], POSITION\n"
> "DCL OUT[1], COLOR[0]\n" /* color */
> "DCL OUT[2], GENERIC[0]\n" /* texcoord */
> --
> 2.4.3
>
> ___
> 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] configure: use $target_cpu, not $host_cpu when setting asm_arch
Hi, This is how I build 32-bit Mesa: # Mandatory for 32-bit dir=i386-linux-gnu build=i686-linux-gnu export CFLAGS="-m32 -O2 -g" export CXXFLAGS="$CFLAGS" export LDFLAGS="-L/usr/lib/$dir" export PKG_CONFIG_PATH="/usr/lib/$dir/pkgconfig" # Optional export USER_CFLAGS="-fno-omit-frame-pointer" export USER_CXXFLAGS="$USER_CFLAGS" export CPP="ccache cpp" export CC="ccache gcc" export CXX="ccache g++" ./autogen.sh \ --build=$build --prefix=/usr --libdir=/usr/lib/$dir --with-llvm-prefix=/usr/llvm/$dir \ --enable-glx-tls --enable-texture-float --enable-debug --enable-vdpau --disable-xvmc \ --with-gallium-drivers=r600,radeonsi,swrast --with-dri-drivers= \ --with-egl-platforms=x11,drm --enable-gles1 --enable-gles2 Marek On Wed, Jun 24, 2015 at 4:40 AM, Brian Paul wrote: > On 06/23/2015 04:56 PM, Brian Paul wrote: >> >> On 06/23/2015 04:25 PM, Matt Turner wrote: >>> >>> On Tue, Jun 23, 2015 at 3:04 PM, Brian Paul wrote: Otherwise, if we're trying to build a 32-bit Mesa on a 64-bit host we wind up with -DUSE_X86_64_ASM, which is incorrect. --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index ddc757e..b12f5f9 100644 --- a/configure.ac +++ b/configure.ac @@ -605,7 +605,7 @@ if test "x$enable_asm" = xyes -a "x$cross_compiling" = xyes; then fi # check for supported arches if test "x$enable_asm" = xyes; then -case "$host_cpu" in +case "$target_cpu" in i?86) case "$host_os" in linux* | *freebsd* | dragonfly* | *netbsd* | openbsd* | gnu*) -- 1.9.1 >>> >>> >>> According to [1], host is "the machine that you are building for" and >>> target is "the machine that GCC will produce code for". In the context >>> of building GCC, I think this means that the resulting GCC binaries >>> will run on $host and will produce code for $target. In the context of >>> Mesa, I can't come up with a way that host != target makes sense. >>> >>> docs/autoconf.html suggests using --build=x86_64-pc-linux-gnu >>> --host=i686-pc-linux-gnu to build on x86_64 for i686. Is that what >>> you're doing? >> >> >> Thanks for the pointer to the docs! I forgot this was mentioned there. >> I basically had my --host and --build mixed up. >> >> Also, I was using "i686-linux-gnu" instead of "i686-pc-linux-gnu" (is >> there really a difference)? >> >> Let me see how far I get now that you set me straight... > > > OK, so the next problem is Mesa is trying to link with 64-bit libdrm instead > of the 32-bit one. > > The command is: > > libtool: link: g++ -fPIC -DPIC -shared -nostdlib > /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../../lib32/crti.o > /usr/lib/gcc/x86_64-linux-gnu/4.9/32/crtbeginS.o -Wl,--whole-archive > ../../.libs/libmesa.a common/.libs/libmegadriver_stub.a > common/.libs/libdricommon.a common/.libs/libxmlconfig.a > swrast/.libs/libswrast_dri.a -Wl,--no-whole-archive > /usr/lib/x86_64-linux-gnu/libdrm.so -ludev /usr/lib/i386-linux-gnu/libdrm.so > /usr/lib/x86_64-linux-gnu/libexpat.so -lpthread -ldl > -L/usr/lib/gcc/x86_64-linux-gnu/4.9/32 > -L/usr/lib/gcc/x86_64-linux-gnu/4.9/../../../i386-linux-gnu > -L/usr/lib/gcc/x86_64-linux-gnu/4.9/../../../../lib32 -L/lib/i386-linux-gnu > -L/lib/../lib32 -L/usr/lib/i386-linux-gnu -L/usr/lib/../lib32 > -L/usr/lib/gcc/x86_64-linux-gnu/4.9 > -L/usr/lib/gcc/x86_64-linux-gnu/4.9/../../.. -lstdc++ -lm -lc -lgcc_s > /usr/lib/gcc/x86_64-linux-gnu/4.9/32/crtendS.o > /usr/lib/gcc/x86_64-linux-gnu/4.9/../../../../lib32/crtn.o -O0 -m32 > -Wl,-Bsymbolic -Wl,--gc-sections -Wl,-soname -Wl,mesa_dri_drivers.so -o > .libs/mesa_dri_drivers.so > /usr/lib/x86_64-linux-gnu/libdrm.so: error adding symbols: File in wrong > format > collect2: error: ld returned 1 exit status > > Note near the middle: > > /usr/lib/x86_64-linux-gnu/libdrm.so > > shortly followed by: > > /usr/lib/i386-linux-gnu/libdrm.so > > That's after I added PKG_CONFIG_PATH=/usr/lib/i386-linux-gnu/pkgconfig > during configuring. Without that, it only tries to link with the 64-bit > libdrm. > > Any ideas? > > > -Brian > > ___ > 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] [RFC shader-db] Add support for shadertoy tests
On Wed, Jun 24, 2015 at 2:18 AM, Rob Clark wrote: > On Tue, Jun 23, 2015 at 7:27 PM, Dylan Baker wrote: >> I have a couple of python pointers for you, feel free to take them or >> leave them. > > cool, thanks.. > > What do others think about including shadertoy in shader-db? If it is > a useful thing, I'll clean up my script and re-submit.. It's a pretty neat resource, but AFAIK it has a few (probably minor) potential problems: 1) The data-set changes over time. This might not be a real-world problem, as measurements are presumably taken with two identical snap-shots. 2) It contains a *lot* of basically the same thing; raymarched raytracers. So I worry a bit that pulling in all of the Shadertoy-shaders ends up punishing "normal" shaders, by making them seem more fringe. The raymarchers on Shadertoy tends to be extremely ALU heavy compared to games shaders, and do very long loops. I have no doubt it's healthy to have *some* such shaders in shader-db, but perhaps all is overkill? 3) Since all use the same vertex shader, won't changes to that get artificially high scores? I'm not saying these are showstoppers, just something that might need addressing. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] Fix strict-aliasing violations in GLSL shader list implementation
On 24/06/15 03:35, Ian Romanick wrote: On 06/23/2015 07:01 PM, Dave Airlie wrote: Oh we've always had aliasing problems this is just one, you can't expect one person to fix them all at once. But "With this patch, I can build a working (though perhaps not 100% bug-free) Mesa without using -fno-strict-aliasing during compilation." is a pretty strong statement that doesn't completely jibe many years of Mesa using -fno-strict-aliasing before exec_list was added. I suppose the reason for that is that an aliasing violation doesn't necessarily cause the compiler to do something other than what the programmer wanted - it just raises the potential for that to happen. It so happens that the exec_list implementation caused issues in my build. Having said that, the statement wasn't intended to be particularly strong. Read it as "I, personally, with my particular compiler version and options, was able to produce a binary that appeared to work". I wasn't necessarily proposing that -fno-strict-aliasing should be removed from all Mesa builds after applying my patch, thought it could certainly be considered. (There's also the fact that GCC makes considerably more effort now to behave "correctly", when it detects an aliasing violation, than it did several years back. It used to be trivial to demonstrate aliasing issues by showing the assembly output of a small piece of code; it's no longer so easy). But making headway is a good thing. You can't have struct exec_list *p; struct exec_node *p2 = (struct exec_list *)p And do things with p2 and hope that p will get them, because the compiler wants to store things its doing to p in registers, and when you go and do something in p2 it can't work out it's the same thing, so it has to spill/reload. Which I think is different from what Davin was saying, but I may be misunderstanding the whole thing. That's why I'd like to see spec language. Dave has it right, though he uses a slightly different example to me. In his example, p and p2 (if they are de-referenced) must not alias according to the C99 rules. So, If they are de-referenced, the compiler can assume that they do not alias, which means it can re-order reads and writes to the two pointers. Consider: p2->next = NULL; p->head = p2; (I know this code doesn't make much sense in the context of actual use of exec_list, but bear with me). Both are trying to assign to p->head, but one does so directly where the other does it via an exec_node *. These assignments might be re-ordered by the compiler, because it believes the pointers cannot alias. The part that really gets me is that this is across a function boundary... that's generally a sacred line, so I'm surprised that the compiler is allowed to disregard what it's told in that scenario. In the language standard, function boundaries really don't mean much. Module boundaries used to be much stronger, but link-time optimization weakens those, too. I'd also like to see assembly dumps with and without -fno-strict-aliasing of some place where this goes wrong. If you, Davin, or someone else can point out a specific function that actually does the wrong thing, I can generate assembly myself. I'll see if I can pinpoint where it's going wrong. It may take me a little time. Bear in mind that the compiler version and options have a significant effect. For that matter... how the heck is the ralloc (or any memory allocator) implementation valid? There are some ways to work around the aliasing restrictions. 'char *' is allowed to alias anything. Unions can be used for aliasing, with some restrictions. 'memcpy' can copy bytes from any object into any other object. Davin ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Don't use GCC extension for ?: with only two operands.
On Wednesday 24 June 2015 00:34:04 Kenneth Graunke wrote:
> From the "apparently I don't know C" files...GCC apparently supports:
>
> x ?: y
>
> which is equivalent to
>
> x ? x : y
>
> except that it doesn't cause side-effects to occur twice. See:
> https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals
>
> This was confusing and looked like a typo. It doesn't really buy us
> anything, so just write the obvious code in normal C.
>
> Signed-off-by: Kenneth Graunke
> ---
> src/mesa/drivers/dri/i965/intel_fbo.c |8 +---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
> b/src/mesa/drivers/dri/i965/intel_fbo.c index 1b3a72f..d1f5770 100644
> --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> @@ -551,10 +551,12 @@ intel_renderbuffer_update_wrapper(struct brw_context
> *brw,
>
> irb->mt_layer = layer_multiplier * layer;
>
> - if (layered) {
> - irb->layer_count = image->TexObject->NumLayers ?:
> mt->level[level].depth / layer_multiplier; - } else {
> + if (!layered) {
>irb->layer_count = 1;
> + } else if (image->TexObject->NumLayers > 0) {
> + irb->layer_count = image->TexObject->NumLayers;
> + } else {
> + irb->layer_count = mt->level[level].depth / layer_multiplier;
> }
>
> intel_miptree_reference(&irb->mt, mt);
Reviewed-by: Samuel Iglesias Gonsálvez
Sam
signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965: Don't use GCC extension for ?: with only two operands.
From the "apparently I don't know C" files...GCC apparently supports:
x ?: y
which is equivalent to
x ? x : y
except that it doesn't cause side-effects to occur twice. See:
https://gcc.gnu.org/onlinedocs/gcc/Conditionals.html#Conditionals
This was confusing and looked like a typo. It doesn't really buy us
anything, so just write the obvious code in normal C.
Signed-off-by: Kenneth Graunke
---
src/mesa/drivers/dri/i965/intel_fbo.c |8 +---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 1b3a72f..d1f5770 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -551,10 +551,12 @@ intel_renderbuffer_update_wrapper(struct brw_context *brw,
irb->mt_layer = layer_multiplier * layer;
- if (layered) {
- irb->layer_count = image->TexObject->NumLayers ?: mt->level[level].depth
/ layer_multiplier;
- } else {
+ if (!layered) {
irb->layer_count = 1;
+ } else if (image->TexObject->NumLayers > 0) {
+ irb->layer_count = image->TexObject->NumLayers;
+ } else {
+ irb->layer_count = mt->level[level].depth / layer_multiplier;
}
intel_miptree_reference(&irb->mt, mt);
--
1.7.10.4
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 91079] New account request
https://bugs.freedesktop.org/show_bug.cgi?id=91079 Marta Löfstedt changed: What|Removed |Added CC||cwo...@cworth.org, ||marta.lofstedt@linux.intel. ||com --- Comment #2 from Marta Löfstedt --- Hi, I would like an account so that I can put up a public branch with my OpenGL ES 3.1 work. BR, Marta -- You are receiving this mail because: You are the QA Contact for the bug. 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 91079] New account request
https://bugs.freedesktop.org/show_bug.cgi?id=91079 --- Comment #1 from Marta Löfstedt --- Created attachment 116684 --> https://bugs.freedesktop.org/attachment.cgi?id=116684&action=edit Public PGP key -- You are receiving this mail because: You are the QA Contact for the bug. 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 91079] New account request
https://bugs.freedesktop.org/show_bug.cgi?id=91079 Bug ID: 91079 Summary: New account request Product: Mesa Version: unspecified Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: marta.lofst...@linux.intel.com QA Contact: mesa-dev@lists.freedesktop.org Created attachment 116683 --> https://bugs.freedesktop.org/attachment.cgi?id=116683&action=edit Public ssh key -- You are receiving this mail because: You are the QA Contact for the bug. 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] [PATCH] i965: Drop brw->depthstencil.stencil_offset from gen8_depth_state.c.
This is always 0 - only brw_workaround_depthstencil_alignment ever sets it, and that doesn't run on Gen6+. My initial Broadwell depth state commit had this mistake. Signed-off-by: Kenneth Graunke --- src/mesa/drivers/dri/i965/gen8_depth_state.c |7 ++- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c b/src/mesa/drivers/dri/i965/gen8_depth_state.c index 7c4ec06..76ba09c 100644 --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c @@ -41,7 +41,6 @@ emit_depth_packets(struct brw_context *brw, bool depth_writable, struct intel_mipmap_tree *stencil_mt, bool stencil_writable, - uint32_t stencil_offset, bool hiz, uint32_t width, uint32_t height, @@ -127,8 +126,7 @@ emit_depth_packets(struct brw_context *brw, OUT_BATCH(HSW_STENCIL_ENABLED | mocs_wb << 22 | (2 * stencil_mt->pitch - 1)); OUT_RELOC64(stencil_mt->bo, - I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, - stencil_offset); + I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, 0); OUT_BATCH(stencil_mt ? stencil_mt->qpitch >> 2 : 0); ADVANCE_BATCH(); } @@ -220,7 +218,6 @@ gen8_emit_depth_stencil_hiz(struct brw_context *brw, emit_depth_packets(brw, depth_mt, brw_depthbuffer_format(brw), surftype, ctx->Depth.Mask != 0, stencil_mt, ctx->Stencil._WriteEnabled, - brw->depthstencil.stencil_offset, hiz, width, height, depth, lod, min_array_element); } @@ -439,7 +436,7 @@ gen8_hiz_exec(struct brw_context *brw, struct intel_mipmap_tree *mt, brw_depth_format(brw, mt->format), BRW_SURFACE_2D, true, /* depth writes */ - NULL, false, 0, /* no stencil for now */ + NULL, false, /* no stencil for now */ true, /* hiz */ surface_width, surface_height, -- 1.7.10.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
