Re: [Mesa-dev] [PATCH 00/20] Auto-generate pack/unpack functions
On 18.11.2014 17:43, Iago Toral Quiroga wrote: For software drivers we worked with a trimmed set of piglit tests (related to format conversion), ~5700 tests selected with the following filter: -t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix -t fbo -t frame Any particular reason for not testing at least piglit gpu.py with llvmpipe? Last time I tried that a few months ago, it didn't take much more than ten minutes on a quad-core A10-7850K. -- 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] [PATCH 00/20] Auto-generate pack/unpack functions
On Tue, 2014-11-18 at 13:41 -0800, Jason Ekstrand wrote: Iago, Most of this looks pretty good to me. The one primary concern I have is in the handling of integer formats. I made the comment in a couple of patches, but I'll make it in general here. In a lot of the code, when you convert from integer formats to float, you treat them as if they are normalized. Can you explain why you are doing this? It seems very wrong to me. Right, I have been discussing this with Samuel and it does look wrong. He will change the code and run a new piglit run to verify the changes. One other issue is that I couldn't actually get it to compile. This is probably due to the fact that I always build out-of-tree, so sourcedir and builddir are not the same. Not really sure what's going on there. Mmm... that's weird. I think I remember seeing a patch that added a new file and could be the source of that issue. We will look into it. Other than that, It's looking pretty good. I'll try and get to reviewing your second patch series tomorrow. Since my R-B obviously doesn't mean much on the code I wrote I'll try and dig up a second reviewer as well. Yes, that makes sense. Thanks for looking into these patches so fast! Iago --Jason On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga ito...@igalia.com wrote: This is the fist of two series of patches to address: https://bugs.freedesktop.org/show_bug.cgi?id=84566 The idea is that we have a lot of format conversion code scattered through different files in the repository, a lot of that is redundant / duplicated, so this intends to address that issue. The goal of this first series is to address auto-generation of our pack/unpack functions (format_pack.c and format_unpack.c). Currently, we have a ton of hand-coded pack/unpack functions for lots of formats, but we can auto-generate most of that code instead, so this series handles this. This is based on initial work by Jason Ekstrand. Tested on i965, classic swrast and gallium (radeon, nouveau, llvmpipe) without regressions. For software drivers we worked with a trimmed set of piglit tests (related to format conversion), ~5700 tests selected with the following filter: -t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix -t fbo -t frame Summary of the patches: * Patches 1-7 are general fixes to the current code that were found while working on this. * Patches 8-16 implement auto-generation of pack/unpack functions. * Patches 17-20 make use of the auto-generated pack/unpack functions in various places to simplify the current code. Notice that some of the fixes in patches 1-7 will become obsolete as soon as we auto-generate the pack/unpack functions, but we thought it would make sense to keep them in the patch set anyway since we started from that base and they should be correct fixes to the currently existing code. Iago Toral Quiroga (1): swrast: Remove unused variable. Jason Ekstrand (9): mesa/format_utils: Fix a bug in one of the format helper functions mesa: Fix packing/unpacking of MESA_FORMAT_R5G6B5_UNORM mesa/colormac: Remove an unused macro mesa: Fix A1R5G5B5 packing/unpacking mesa/format_utils: Prefix and expose the conversion helper functions mesa: Add a concept of an array format mesa: Add a _mesa_is_format_color_format helper mesa: Autogenerate most of format_pack.c mesa: Autogenerate format_unpack.c Samuel Iglesias Gonsalvez (10): mesa: Fix get_texbuffer_format(). mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly mesa: Add _mesa_pack_uint_rgba_row() format conversion function mesa: Add non-normalized formats support for ubyte packing functions mesa/format_pack: Add _mesa_pack_int_rgba_row() mesa/formats: add new mesa formats and their pack/unpack functions. mesa: use format conversion functions in swrast mesa/pack: use autogenerated format_pack functions mesa/main/pack_tmp.h: Add float conversion support mesa/pack: refactor _mesa_pack_rgba_span_float() src/mesa/Makefile.am | 18 + src/mesa/Makefile.sources |4 +- src/mesa/main/colormac.h |3 -
Re: [Mesa-dev] [PATCH 01/20] mesa/format_utils: Fix a bug in one of the format helper functions
On 18.11.2014 17:43, Iago Toral Quiroga wrote:
From: Jason Ekstrand jason.ekstr...@intel.com
The commit short log (first line of the commit log) should at least say
'snorm_to_float' instead of 'one of the format helper functions'. It could
probably have a more informative summary of what the change does as well.
---
src/mesa/main/format_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index 93a0cea..b6d0fbc 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -152,7 +152,7 @@ unorm_to_float(unsigned x, unsigned src_bits)
static inline float
snorm_to_float(int x, unsigned src_bits)
{
- if (x == -MAX_INT(src_bits))
+ if (x -MAX_INT(src_bits))
return -1.0f;
else
return x * (1.0f / (float)MAX_INT(src_bits));
Should it be if (x = -MAX_INT(src_bits))? Even if the else case
is guaranteed to produce -1.0f for x == -MAX_INT(src_bits), it seems like
wasted effort.
--
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] [PATCH 00/20] Auto-generate pack/unpack functions
On Wed, 2014-11-19 at 17:09 +0900, Michel Dänzer wrote: On 18.11.2014 17:43, Iago Toral Quiroga wrote: For software drivers we worked with a trimmed set of piglit tests (related to format conversion), ~5700 tests selected with the following filter: -t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix -t fbo -t frame Any particular reason for not testing at least piglit gpu.py with llvmpipe? Last time I tried that a few months ago, it didn't take much more than ten minutes on a quad-core A10-7850K. Not really, we tried to run the full suite but many tests would take forever and we thought we should just cut it down to the tests that seemed more related to the kind of stuff we were working on. Also, since we would ran piglit very often to verify our changes, specially towards the end of the development, we needed something manageable. We will give gpu.py a try too. Thanks! Iago ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 00/20] Auto-generate pack/unpack functions
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 19/11/14 09:25, Iago Toral wrote: On Tue, 2014-11-18 at 13:41 -0800, Jason Ekstrand wrote: Iago, Most of this looks pretty good to me. The one primary concern I have is in the handling of integer formats. I made the comment in a couple of patches, but I'll make it in general here. In a lot of the code, when you convert from integer formats to float, you treat them as if they are normalized. Can you explain why you are doing this? It seems very wrong to me. Right, I have been discussing this with Samuel and it does look wrong. He will change the code and run a new piglit run to verify the changes. As Iago said, I'm going to do some changes and test them. After that I will reply to the mailing list. One other issue is that I couldn't actually get it to compile. This is probably due to the fact that I always build out-of-tree, so sourcedir and builddir are not the same. Not really sure what's going on there. Mmm... that's weird. I think I remember seeing a patch that added a new file and could be the source of that issue. We will look into it. I confirmed that the out-of-tree compilation error is because of the autogeneration of format_pack.c and format_unpack.c files. I'm going to fix it. Thanks! Sam Other than that, It's looking pretty good. I'll try and get to reviewing your second patch series tomorrow. Since my R-B obviously doesn't mean much on the code I wrote I'll try and dig up a second reviewer as well. Yes, that makes sense. Thanks for looking into these patches so fast! Iago --Jason On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga ito...@igalia.com wrote: This is the fist of two series of patches to address: https://bugs.freedesktop.org/show_bug.cgi?id=84566 The idea is that we have a lot of format conversion code scattered through different files in the repository, a lot of that is redundant / duplicated, so this intends to address that issue. The goal of this first series is to address auto-generation of our pack/unpack functions (format_pack.c and format_unpack.c). Currently, we have a ton of hand-coded pack/unpack functions for lots of formats, but we can auto-generate most of that code instead, so this series handles this. This is based on initial work by Jason Ekstrand. Tested on i965, classic swrast and gallium (radeon, nouveau, llvmpipe) without regressions. For software drivers we worked with a trimmed set of piglit tests (related to format conversion), ~5700 tests selected with the following filter: -t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix -t fbo -t frame Summary of the patches: * Patches 1-7 are general fixes to the current code that were found while working on this. * Patches 8-16 implement auto-generation of pack/unpack functions. * Patches 17-20 make use of the auto-generated pack/unpack functions in various places to simplify the current code. Notice that some of the fixes in patches 1-7 will become obsolete as soon as we auto-generate the pack/unpack functions, but we thought it would make sense to keep them in the patch set anyway since we started from that base and they should be correct fixes to the currently existing code. Iago Toral Quiroga (1): swrast: Remove unused variable. Jason Ekstrand (9): mesa/format_utils: Fix a bug in one of the format helper functions mesa: Fix packing/unpacking of MESA_FORMAT_R5G6B5_UNORM mesa/colormac: Remove an unused macro mesa: Fix A1R5G5B5 packing/unpacking mesa/format_utils: Prefix and expose the conversion helper functions mesa: Add a concept of an array format mesa: Add a _mesa_is_format_color_format helper mesa: Autogenerate most of format_pack.c mesa: Autogenerate format_unpack.c Samuel Iglesias Gonsalvez (10): mesa: Fix get_texbuffer_format(). mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly mesa: Add _mesa_pack_uint_rgba_row() format conversion function mesa: Add non-normalized formats support for ubyte packing functions mesa/format_pack: Add _mesa_pack_int_rgba_row() mesa/formats: add new mesa formats and their pack/unpack functions. mesa: use format conversion functions in swrast mesa/pack: use
Re: [Mesa-dev] [PATCH 00/20] Auto-generate pack/unpack functions
On 19/11/14 08:29, Iago Toral wrote:
On Wed, 2014-11-19 at 17:09 +0900, Michel Dänzer wrote:
On 18.11.2014 17:43, Iago Toral Quiroga wrote:
For software drivers we worked with a trimmed set of piglit tests (related to
format conversion), ~5700 tests selected with the following filter:
-t format -t color -t tex -t image -t swizzle -t clamp -t rgb -t lum -t pix
-t fbo -t frame
Any particular reason for not testing at least piglit gpu.py with
llvmpipe? Last time I tried that a few months ago, it didn't take much
more than ten minutes on a quad-core A10-7850K.
Not really, we tried to run the full suite but many tests would take
forever and we thought we should just cut it down to the tests that
seemed more related to the kind of stuff we were working on. Also, since
we would ran piglit very often to verify our changes, specially towards
the end of the development, we needed something manageable.
We will give gpu.py a try too. Thanks!
Iago
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddevd=AAIGaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=AedzeFofUxa1m8ruA88aWS9AZRrJAbQeOAET3aeJXMUs=s-L51NQv5NFJUygqYpmGzRjc-tlQYulkLBoFjBrMKFse=
I have this on my piglit test list for llvmpipe:
# These take too long or too much memory
profile.tests['glean'].pop('pointAtten', None)
profile.tests['glean'].pop('texCombine', None)
profile.tests['shaders'].pop('glsl-fs-inline-explosion', None)
profile.tests['shaders'].pop('glsl-fs-unroll-explosion', None)
profile.tests['shaders'].pop('glsl-vs-inline-explosion', None)
profile.tests['shaders'].pop('glsl-vs-unroll-explosion', None)
profile.tests['spec']['ARB_fragment_program'].pop('fp-indirections',
None)
profile.tests['spec']['ARB_fragment_program'].pop('fp-indirections2',
None)
profile.tests['spec']['!OpenGL 1.1'].pop('streaming-texture-leak', None)
profile.tests['spec']['!OpenGL 1.1'].pop('max-texture-size', None)
but I think the gpu.py seems quite close.
Jose
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)
On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
Add support for decoding the new branch control bit. I saw two things wrong
with
the existing code.
1. It didn't bother trying to decode the bit.
- While we do not *intentionally* emit this bit today, I think it's
interesting
to see if we somehow ended up with the bit set. It may also be useful in
the
future.
2. It seemed to be the wrong bit.
- The docs are pretty poor wrt which bit this actually occupies. To me, it
/looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
verified it should be 28 by looking at the simulator code.
Yeah, it sure looks like 28 to me...I must've botched it when typing up the
tables.
One comment below.
I also added the most basic support for GOTO simply so we don't need to
remember
to change the function in the future.
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/brw_disasm.c | 29 ++---
src/mesa/drivers/dri/i965/brw_inst.h| 2 +-
3 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 53cd75e..ed94bcc 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -820,6 +820,7 @@ enum opcode {
BRW_OPCODE_MSAVE =44, /** Pre-Gen6 */
BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
BRW_OPCODE_PUSH = 46, /** Pre-Gen6 */
+ BRW_OPCODE_GOTO = 46, /** Gen8+*/
BRW_OPCODE_POP = 47, /** Pre-Gen6 */
BRW_OPCODE_WAIT = 48,
BRW_OPCODE_SEND = 49,
diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c
b/src/mesa/drivers/dri/i965/brw_disasm.c
index 53ec767..013058e 100644
--- a/src/mesa/drivers/dri/i965/brw_disasm.c
+++ b/src/mesa/drivers/dri/i965/brw_disasm.c
@@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
}
static bool
+has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
+{
+ if (brw-gen 8)
+ return false;
+
+ return opcode == BRW_OPCODE_IF ||
+ opcode == BRW_OPCODE_ELSE ||
+ opcode == BRW_OPCODE_GOTO ||
+ opcode == BRW_OPCODE_ENDIF;
+}
I don't think ENDIF has BranchCtrl. With that removed, this is:
Reviewed-by: Kenneth Graunke kenn...@whitecape.org
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 1/4] i965: Remove spurious casts in copy_image_with_memcpy()
If a pointer points to raw, untyped memory and is never dereferenced,
then declare it as 'void*' instead of casting it to 'void*'.
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index 341220c..cb44474 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -144,7 +144,7 @@ copy_image_with_memcpy(struct brw_context *brw,
int src_width, int src_height)
{
bool same_slice;
- uint8_t *mapped, *src_mapped, *dst_mapped;
+ void *mapped, *src_mapped, *dst_mapped;
int src_stride, dst_stride, i, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -176,7 +176,7 @@ copy_image_with_memcpy(struct brw_context *brw,
intel_miptree_map(brw, src_mt, src_level, src_z,
map_x1, map_y1, map_x2 - map_x1, map_y2 - map_y1,
GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
-(void **)mapped, src_stride);
+mapped, src_stride);
dst_stride = src_stride;
@@ -188,10 +188,10 @@ copy_image_with_memcpy(struct brw_context *brw,
} else {
intel_miptree_map(brw, src_mt, src_level, src_z,
src_x, src_y, src_width, src_height,
-GL_MAP_READ_BIT, (void **)src_mapped, src_stride);
+GL_MAP_READ_BIT, src_mapped, src_stride);
intel_miptree_map(brw, dst_mt, dst_level, dst_z,
dst_x, dst_y, src_width, src_height,
-GL_MAP_WRITE_BIT, (void **)dst_mapped, dst_stride);
+GL_MAP_WRITE_BIT, dst_mapped, dst_stride);
}
src_width /= (int)src_bw;
--
2.1.0-rc0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/4] i965: Remove spurious casts in copy_image_with_memcpy()
If a pointer points to raw, untyped memory and is never dereferenced,
then declare it as 'void*' instead of casting it to 'void*'.
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index 341220c..cb44474 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -144,7 +144,7 @@ copy_image_with_memcpy(struct brw_context *brw,
int src_width, int src_height)
{
bool same_slice;
- uint8_t *mapped, *src_mapped, *dst_mapped;
+ void *mapped, *src_mapped, *dst_mapped;
int src_stride, dst_stride, i, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -176,7 +176,7 @@ copy_image_with_memcpy(struct brw_context *brw,
intel_miptree_map(brw, src_mt, src_level, src_z,
map_x1, map_y1, map_x2 - map_x1, map_y2 - map_y1,
GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
-(void **)mapped, src_stride);
+mapped, src_stride);
dst_stride = src_stride;
@@ -188,10 +188,10 @@ copy_image_with_memcpy(struct brw_context *brw,
} else {
intel_miptree_map(brw, src_mt, src_level, src_z,
src_x, src_y, src_width, src_height,
-GL_MAP_READ_BIT, (void **)src_mapped, src_stride);
+GL_MAP_READ_BIT, src_mapped, src_stride);
intel_miptree_map(brw, dst_mt, dst_level, dst_z,
dst_x, dst_y, src_width, src_height,
-GL_MAP_WRITE_BIT, (void **)dst_mapped, dst_stride);
+GL_MAP_WRITE_BIT, dst_mapped, dst_stride);
}
src_width /= (int)src_bw;
--
2.1.0-rc0
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
This patch should diminish the likelihood of pointer arithmetic overflow
bugs, like the one fixed by b69c7c5dac.
Change the type of parameter 'out_stride' from int to ptrdiff_t. The
logic is that if you call intel_miptree_map() and use the value of
'out_stride', then you must be doing pointer arithmetic on 'out_ptr'.
Using ptrdiff_t instead of int should make a little bit harder to hit
overflow bugs.
As a side-effect, some function-scope variables needed to be retyped to
avoid compilation errors.
Cc: Ian Romanick i...@freedesktop.org
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 4 ++--
src/mesa/drivers/dri/i965/intel_fbo.c | 4 ++--
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++---
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
src/mesa/drivers/dri/i965/intel_tex.c | 7 +--
5 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index cb44474..f4c7eff 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -145,7 +145,7 @@ copy_image_with_memcpy(struct brw_context *brw,
{
bool same_slice;
void *mapped, *src_mapped, *dst_mapped;
- int src_stride, dst_stride, i, cpp;
+ ptrdiff_t src_stride, dst_stride, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -197,7 +197,7 @@ copy_image_with_memcpy(struct brw_context *brw,
src_width /= (int)src_bw;
src_height /= (int)src_bh;
- for (i = 0; i src_height; ++i) {
+ for (int i = 0; i src_height; ++i) {
memcpy(dst_mapped, src_mapped, src_width * cpp);
src_mapped += src_stride;
dst_mapped += dst_stride;
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 4a03b57..96e7b9e 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -127,7 +127,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
struct intel_renderbuffer *irb = intel_renderbuffer(rb);
struct intel_mipmap_tree *mt;
void *map;
- int stride;
+ ptrdiff_t stride;
if (srb-Buffer) {
/* this is a malloc'd renderbuffer (accum buffer), not an irb */
@@ -189,7 +189,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
stride = -stride;
}
- DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%d\n,
+ DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%PRIdPTR\n,
__FUNCTION__, rb-Name, _mesa_get_format_name(rb-Format),
x, y, w, h, map, stride);
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 7081f1d..f815fbe 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -,7 +,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
int height)
{
void *src, *dst;
- int src_stride, dst_stride;
+ ptrdiff_t src_stride, dst_stride;
int cpp = dst_mt-cpp;
intel_miptree_map(brw, src_mt,
@@ -1129,7 +1129,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
BRW_MAP_DIRECT_BIT,
dst, dst_stride);
- DBG(sw blit %s mt %p %p/%d - %s mt %p %p/%d (%dx%d)\n,
+ DBG(sw blit %s mt %p %p/%PRIdPTR - %s mt %p %p/%PRIdPTR (%dx%d)\n,
_mesa_get_format_name(src_mt-format),
src_mt, src, src_stride,
_mesa_get_format_name(dst_mt-format),
@@ -2259,6 +2259,17 @@ can_blit_slice(struct intel_mipmap_tree *mt,
return true;
}
+/**
+ * Parameter \a out_stride has type ptrdiff_t not because the buffer stride may
+ * exceed 32 bits but to diminish the likelihood subtle bugs in pointer
+ * arithmetic overflow.
+ *
+ * If you call this function and use \a out_stride, then you're doing pointer
+ * arithmetic on \a out_ptr. The type of \a out_stride doesn't prevent all
+ * bugs. The caller must still take care to avoid 32-bit overflow errors in
+ * all arithmetic expressions that contain buffer offsets and pixel sizes,
+ * which usually have type uint32_t or GLuint.
+ */
void
intel_miptree_map(struct brw_context *brw,
struct intel_mipmap_tree *mt,
@@ -2270,7 +2281,7 @@ intel_miptree_map(struct brw_context *brw,
unsigned int h,
GLbitfield mode,
void **out_ptr,
- int *out_stride)
+ ptrdiff_t *out_stride)
{
struct intel_miptree_map *map;
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index f0f6814..44ddc60 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -733,7 +733,7 @@ intel_miptree_map(struct brw_context *brw,
unsigned
[Mesa-dev] [PATCH 0/4] i965: Safer pointer arithmetic
The pointer arithmetic overflow bug that led me to make commit b69c7c5dac, in addition to crashing Google Chrome, had another side-effect: It filled me with paranoia that i965 may be riddled with pointer arithmetic overflow. So I went on a witch hunt. I grepped i965 for -virtual\ and intel_miptree_map, looked closely for code that smelled like pointer arithmetic overflow, and proactively fixed the potential bug. The result is this patch series. No Piglit change on Ivybridge GT2. Patches are on my branch [1] 'i965-safer-pointer-arith'. I think patch 3 is suitable for the stable branches. Let me what you think about that. [1] http://github.com/chadversary/mesa/tree/i965-safer-pointer-arith Chad Versace (4): i965: Remove spurious casts in copy_image_with_memcpy() i965: Fix intel_miptree_map() signature to be more 64-bit safe i965: Use safer pointer arithmetic in intel_texsubimage_tiled_memcpy() i965: Use safer pointer arithmetic in gather_oa_results() src/mesa/drivers/dri/i965/brw_performance_monitor.c | 2 +- src/mesa/drivers/dri/i965/intel_copy_image.c| 12 ++-- src/mesa/drivers/dri/i965/intel_fbo.c | 4 ++-- src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++--- src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +- src/mesa/drivers/dri/i965/intel_tex.c | 7 +-- src/mesa/drivers/dri/i965/intel_tex_subimage.c | 7 --- 7 files changed, 33 insertions(+), 18 deletions(-) -- 2.1.0-rc0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/4] i965: Use safer pointer arithmetic in gather_oa_results()
This patch reduces the likelihood of pointer arithmetic overflow bugs in gather_oa_results(), like the one fixed by b69c7c5dac. I haven't yet encountered any overflow bugs in the wild along this patch's codepath. But I get nervous when I see code patterns like this: (void*) + (int) * (int) I smell 32-bit overflow all over this code. This patch retypes 'snapshot_size' to 'ptrdiff_t', which should fix any potential overflow. Cc: Ian Romanick i...@freedesktop.org Cc: Kenneth Graunke kenn...@whitecape.org Signed-off-by: Chad Versace chad.vers...@linux.intel.com --- src/mesa/drivers/dri/i965/brw_performance_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/i965/brw_performance_monitor.c b/src/mesa/drivers/dri/i965/brw_performance_monitor.c index edfa3d2..e683e40 100644 --- a/src/mesa/drivers/dri/i965/brw_performance_monitor.c +++ b/src/mesa/drivers/dri/i965/brw_performance_monitor.c @@ -907,7 +907,7 @@ gather_oa_results(struct brw_context *brw, return; } - const int snapshot_size = brw-perfmon.entries_per_oa_snapshot; + const ptrdiff_t snapshot_size = brw-perfmon.entries_per_oa_snapshot; /* First, add the contributions from the head interval: * (snapshot taken at BeginPerfMonitor time, -- 2.1.0-rc0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 4/4] i965: Use safer pointer arithmetic in gather_oa_results()
This patch reduces the likelihood of pointer arithmetic overflow bugs in gather_oa_results(), like the one fixed by b69c7c5dac. I haven't yet encountered any overflow bugs in the wild along this patch's codepath. But I get nervous when I see code patterns like this: (void*) + (int) * (int) I smell 32-bit overflow all over this code. This patch retypes 'snapshot_size' to 'ptrdiff_t', which should fix any potential overflow. Cc: Ian Romanick i...@freedesktop.org Cc: Kenneth Graunke kenn...@whitecape.org Signed-off-by: Chad Versace chad.vers...@linux.intel.com --- src/mesa/drivers/dri/i965/brw_performance_monitor.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/drivers/dri/i965/brw_performance_monitor.c b/src/mesa/drivers/dri/i965/brw_performance_monitor.c index edfa3d2..e683e40 100644 --- a/src/mesa/drivers/dri/i965/brw_performance_monitor.c +++ b/src/mesa/drivers/dri/i965/brw_performance_monitor.c @@ -907,7 +907,7 @@ gather_oa_results(struct brw_context *brw, return; } - const int snapshot_size = brw-perfmon.entries_per_oa_snapshot; + const ptrdiff_t snapshot_size = brw-perfmon.entries_per_oa_snapshot; /* First, add the contributions from the head interval: * (snapshot taken at BeginPerfMonitor time, -- 2.1.0-rc0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 0/4] i965: Safer pointer arithmetic
The pointer arithmetic overflow bug that led me to make commit b69c7c5dac, in addition to crashing Google Chrome, had another side-effect: It filled me with paranoia that i965 may be riddled with pointer arithmetic overflow. So I went on a witch hunt. I grepped i965 for -virtual\ and intel_miptree_map, looked closely for code that smelled like pointer arithmetic overflow, and proactively fixed the potential bug. The result is this patch series. No Piglit change on Ivybridge GT2. Patches are on my branch [1] 'i965-safer-pointer-arith'. I think patch 3 is suitable for the stable branches. Let me what you think about that. [1] http://github.com/chadversary/mesa/tree/i965-safer-pointer-arith Chad Versace (4): i965: Remove spurious casts in copy_image_with_memcpy() i965: Fix intel_miptree_map() signature to be more 64-bit safe i965: Use safer pointer arithmetic in intel_texsubimage_tiled_memcpy() i965: Use safer pointer arithmetic in gather_oa_results() src/mesa/drivers/dri/i965/brw_performance_monitor.c | 2 +- src/mesa/drivers/dri/i965/intel_copy_image.c| 12 ++-- src/mesa/drivers/dri/i965/intel_fbo.c | 4 ++-- src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++--- src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +- src/mesa/drivers/dri/i965/intel_tex.c | 7 +-- src/mesa/drivers/dri/i965/intel_tex_subimage.c | 7 --- 7 files changed, 33 insertions(+), 18 deletions(-) -- 2.1.0-rc0 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
This patch should diminish the likelihood of pointer arithmetic overflow
bugs, like the one fixed by b69c7c5dac.
Change the type of parameter 'out_stride' from int to ptrdiff_t. The
logic is that if you call intel_miptree_map() and use the value of
'out_stride', then you must be doing pointer arithmetic on 'out_ptr'.
Using ptrdiff_t instead of int should make a little bit harder to hit
overflow bugs.
As a side-effect, some function-scope variables needed to be retyped to
avoid compilation errors.
Cc: Ian Romanick i...@freedesktop.org
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 4 ++--
src/mesa/drivers/dri/i965/intel_fbo.c | 4 ++--
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++---
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
src/mesa/drivers/dri/i965/intel_tex.c | 7 +--
5 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index cb44474..f4c7eff 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -145,7 +145,7 @@ copy_image_with_memcpy(struct brw_context *brw,
{
bool same_slice;
void *mapped, *src_mapped, *dst_mapped;
- int src_stride, dst_stride, i, cpp;
+ ptrdiff_t src_stride, dst_stride, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -197,7 +197,7 @@ copy_image_with_memcpy(struct brw_context *brw,
src_width /= (int)src_bw;
src_height /= (int)src_bh;
- for (i = 0; i src_height; ++i) {
+ for (int i = 0; i src_height; ++i) {
memcpy(dst_mapped, src_mapped, src_width * cpp);
src_mapped += src_stride;
dst_mapped += dst_stride;
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 4a03b57..96e7b9e 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -127,7 +127,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
struct intel_renderbuffer *irb = intel_renderbuffer(rb);
struct intel_mipmap_tree *mt;
void *map;
- int stride;
+ ptrdiff_t stride;
if (srb-Buffer) {
/* this is a malloc'd renderbuffer (accum buffer), not an irb */
@@ -189,7 +189,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
stride = -stride;
}
- DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%d\n,
+ DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%PRIdPTR\n,
__FUNCTION__, rb-Name, _mesa_get_format_name(rb-Format),
x, y, w, h, map, stride);
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 7081f1d..f815fbe 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -,7 +,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
int height)
{
void *src, *dst;
- int src_stride, dst_stride;
+ ptrdiff_t src_stride, dst_stride;
int cpp = dst_mt-cpp;
intel_miptree_map(brw, src_mt,
@@ -1129,7 +1129,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
BRW_MAP_DIRECT_BIT,
dst, dst_stride);
- DBG(sw blit %s mt %p %p/%d - %s mt %p %p/%d (%dx%d)\n,
+ DBG(sw blit %s mt %p %p/%PRIdPTR - %s mt %p %p/%PRIdPTR (%dx%d)\n,
_mesa_get_format_name(src_mt-format),
src_mt, src, src_stride,
_mesa_get_format_name(dst_mt-format),
@@ -2259,6 +2259,17 @@ can_blit_slice(struct intel_mipmap_tree *mt,
return true;
}
+/**
+ * Parameter \a out_stride has type ptrdiff_t not because the buffer stride may
+ * exceed 32 bits but to diminish the likelihood subtle bugs in pointer
+ * arithmetic overflow.
+ *
+ * If you call this function and use \a out_stride, then you're doing pointer
+ * arithmetic on \a out_ptr. The type of \a out_stride doesn't prevent all
+ * bugs. The caller must still take care to avoid 32-bit overflow errors in
+ * all arithmetic expressions that contain buffer offsets and pixel sizes,
+ * which usually have type uint32_t or GLuint.
+ */
void
intel_miptree_map(struct brw_context *brw,
struct intel_mipmap_tree *mt,
@@ -2270,7 +2281,7 @@ intel_miptree_map(struct brw_context *brw,
unsigned int h,
GLbitfield mode,
void **out_ptr,
- int *out_stride)
+ ptrdiff_t *out_stride)
{
struct intel_miptree_map *map;
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
index f0f6814..44ddc60 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
@@ -733,7 +733,7 @@ intel_miptree_map(struct brw_context *brw,
unsigned
Re: [Mesa-dev] [PATCH v3 0/9] Gallium Nine
Am 18.11.2014 um 23:31 schrieb Emil Velikov emil.l.veli...@gmail.com: Hmm the binaries do not seem to match the source. Am I missing something ? Not sure. As you can see I've last touched them in January. It is possible that I have changed some things in the code and not updated the binaries on the server. I recommend to rebuild them yourself anyway, just for the sake of paranoia. I use Visual Studio on Windows, and you need the last DirectX SDK for the d3dx9 headers and libs, but I guess you can also use mingw with the right include and lib paths. On of the points in my earlier rant was - if wine is interested solely on improvements of the current code, and there is no interest/intensive to include an alternative solution it makes no sense to keep on pestering you guys. We're solely interested on the current code until there's conclusive proof that running a performant d3d implementation on top of OpenGL is not possible by design. So far I have not seen any hard facts to support this claim. Yes, nine/st is faster on some drivers/games, but wined3d is faster on others. Wined3d with my (not yet upstreamed) CSMT patches beats Windows in a big number of games, although it still has performance problems in other games. On the Nvidia blob most performance problems with CSMT are not 3D-related any more, but are caused by expensive IPC that runs through wineserver. The factoids so far suggest that the major problems with OpenGL is the on-demand shader compilation. That's something we can fix in Wine for the average case, although there will still be some corner cases where we have to generate a new shader on the fly (e.g. a texture type mismatch, or a flag change in Pixel Shader 1.x). Some games don't create the D3D shaders until they actually use them. Nvidia has an on-disk shader cache to improve the situation, but that's an ugly hack. The other known problem is uniform updating in GLSL. UBOs may help here. The ARB-style environment constants work better for d3d's purposes than GLSL's per-shader uniforms. I expect that there are more problems, most of them limited in nature and affecting only some games. That could be things like D3D shader instruction X is handled inefficiently by GLSL, or IDirect3DDevice9::ColorFill is slow because we have to build a full FBO in GL. Those are just guesses - hard facts are needed. nine/st is a good tool to extract such small performance problems from real-world games. signature.asc Description: Message signed with OpenPGP using GPGMail ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/20] mesa/format_utils: Fix a bug in one of the format helper functions
-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1
On 19/11/14 09:28, Michel Dänzer wrote:
On 18.11.2014 17:43, Iago Toral Quiroga wrote:
From: Jason Ekstrand jason.ekstr...@intel.com
The commit short log (first line of the commit log) should at least say
'snorm_to_float' instead of 'one of the format helper functions'. It could
probably have a more informative summary of what the change does as well.
---
src/mesa/main/format_utils.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index 93a0cea..b6d0fbc 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -152,7 +152,7 @@ unorm_to_float(unsigned x, unsigned src_bits)
static inline float
snorm_to_float(int x, unsigned src_bits)
{
- if (x == -MAX_INT(src_bits))
+ if (x -MAX_INT(src_bits))
return -1.0f;
else
return x * (1.0f / (float)MAX_INT(src_bits));
Should it be if (x = -MAX_INT(src_bits))? Even if the else case
is guaranteed to produce -1.0f for x == -MAX_INT(src_bits), it seems like
wasted effort.
Agreed, I will fix that when creating the second version of the series.
Thanks,
Sam
-BEGIN PGP SIGNATURE-
Version: GnuPG v1
iQIcBAEBAgAGBQJUbIBUAAoJEH/0ujLxfcNDDQMQALY4nST/oPsMfAh4NFJxFyvC
7YQf9OGYbrRVj797I4M6w/IjMrmXa1kz4OabQD7QWQvCq4d0DlHDqlKLQ71Gx7s0
rBW4pQo//x7sJNkzr4l84COz8rIt/WjYbOtD8qABX6WsRlcCQORf++w8G/RAtx9l
aCSIt5S4y20KZFc73FjhEjUMC4RMwmsxOJovA7FA/hGZtPrIRKCYwbg7EU/01THw
J3NAOPA8OCzPD0oYXBoV/PkXAzUh2s7RFKx/EFeuyHFVgFJmk9yxVImce3s4Cusf
pi6IooQBFcmhqv7GkNDcSU+e2ZFqMMSqXhnrmLaH5jvrqsvDTXsjVlDdCeXMQHwH
5p2QW+RSNwpT2QNBGy73BaqXXhj5ub1KGWHMq3ZVC4yXTVxV3P31vSoG6Lwz+B4J
KFAeBatwXDmmbo9Suk/HnpfB66rNOMmz4fJmzODI4cxArGmtjbpcKTEslorz0hnw
ptP5UVydjaWQK9cCV8QYSJlIZTBswKuC3GBvydUDH5KsS8s6Ki+StPvSDF7rClop
b4j9FrHWqoO1A7kfy+wTDxev34Py+R5ZBqzccjxcaFbPbhkCzg9Lq4hdOdovxKZQ
I+yNMXJWD4hEpZZJR9TOLK7e67EiVe2ObhScX0LZLuuaL9YID58CLaptpCsgI3uf
gNdeMDtzbkkXI2YbPtAr
=LWOR
-END PGP SIGNATURE-
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] i965: Remove spurious casts in copy_image_with_memcpy()
On Tue, Nov 18, 2014 at 9:02 PM, Chad Versace
chad.vers...@linux.intel.com wrote:
If a pointer points to raw, untyped memory and is never dereferenced,
then declare it as 'void*' instead of casting it to 'void*'.
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index 341220c..cb44474 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -144,7 +144,7 @@ copy_image_with_memcpy(struct brw_context *brw,
int src_width, int src_height)
{
bool same_slice;
- uint8_t *mapped, *src_mapped, *dst_mapped;
+ void *mapped, *src_mapped, *dst_mapped;
Making these void * means that this code below:
src_mapped = mapped + ((src_y - map_y1) / src_bh) * src_stride +
((src_x - map_x1) / src_bw) * cpp;
(same for dst_mapped) becomes arithmetic on void pointers. gcc
supports that and treats it as uint8_t pointer arithmetic [1], but I'm
not aware of any official C standard that allows it. I don't think we
rely on that elsewhere and I don't think this patch improves
readability, portability or pointer safety of the code in question.
If you wanted to avoid void * arithmetic and casting to uint8_t, you'd
have to cast to an integer (uintptr_t) do the math there, then cast
back, but that's way more likely to hit a pointer size or 32/64 bit
issue.
Kristian
[1] http://gcc.gnu.org/onlinedocs/gcc-4.8.0/gcc/Pointer-Arith.html
int src_stride, dst_stride, i, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -176,7 +176,7 @@ copy_image_with_memcpy(struct brw_context *brw,
intel_miptree_map(brw, src_mt, src_level, src_z,
map_x1, map_y1, map_x2 - map_x1, map_y2 - map_y1,
GL_MAP_READ_BIT | GL_MAP_WRITE_BIT,
-(void **)mapped, src_stride);
+mapped, src_stride);
dst_stride = src_stride;
@@ -188,10 +188,10 @@ copy_image_with_memcpy(struct brw_context *brw,
} else {
intel_miptree_map(brw, src_mt, src_level, src_z,
src_x, src_y, src_width, src_height,
-GL_MAP_READ_BIT, (void **)src_mapped, src_stride);
+GL_MAP_READ_BIT, src_mapped, src_stride);
intel_miptree_map(brw, dst_mt, dst_level, dst_z,
dst_x, dst_y, src_width, src_height,
-GL_MAP_WRITE_BIT, (void **)dst_mapped, dst_stride);
+GL_MAP_WRITE_BIT, dst_mapped, dst_stride);
}
src_width /= (int)src_bw;
--
2.1.0-rc0
___
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] Using the 'f' suffix to create a float from an integer literal
Hi, I came across a GLSL test that checks that doing something like this in a shader should fail: float value = 1f; However, this works fine in Mesa. Checking the spec I see: Floating-point constants are defined as follows. floating-constant: fractional-constant exponent-part(opt) floating-suffix(opt) digit-sequence exponent-part floating-suffix(opt) fractional-constant: digit-sequence . digit-sequence digit-sequence . . digit-sequence exponent-part: e sign(opt) digit-sequence E sign(opt) digit-sequence sign: one of + - digit-sequence: digit digit-sequence digit floating-suffix: one of f F which suggests that the test is correct and Mesa has a bug. According to the above rules, however, something like this is fine: float f = 1e2f; which I find kind of weird if the other case is not valid, so I wonder if there is a bug in the spec or this is on purpose for some reason that I am missing. Then, to make matters worse, I read this in opengl.org wiki [1]: A numeric literal that uses a decimal is by default of type float. To create a float literal from an integer value, use the suffix f or F as in C/C++. which contradicts the spec and the test and is aligned with the current way Mesa works. So: does anyone know what version is right? Could this be a bug in the spec? and if it is not, do we want to change the behavior to follow the spec as it is now? Thanks, Iago [1] https://www.opengl.org/wiki/Data_Type_%28GLSL%29 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] rtasm,translate: Re-enable SSE on Mingw64.
From: José Fonseca jfons...@vmware.com This reverts f4dd0991719ef3e2606920c5100b372181c60899. The src/gallium/tests/unit/translate_test.c gives the same results on MinGW 64-bits as on Linux 64-bits. And since MinGW is often used for development/testing due to its convenience, it's better not to have this sort of differences relative to MSVC. --- src/gallium/auxiliary/rtasm/rtasm_x86sse.c | 2 +- src/gallium/auxiliary/translate/translate_sse.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/auxiliary/rtasm/rtasm_x86sse.c b/src/gallium/auxiliary/rtasm/rtasm_x86sse.c index 24ff820..f963788 100644 --- a/src/gallium/auxiliary/rtasm/rtasm_x86sse.c +++ b/src/gallium/auxiliary/rtasm/rtasm_x86sse.c @@ -25,7 +25,7 @@ #include pipe/p_config.h #include util/u_cpu_detect.h -#if defined(PIPE_ARCH_X86) || (defined(PIPE_ARCH_X86_64) !defined(__MINGW32__)) +#if defined(PIPE_ARCH_X86) || defined(PIPE_ARCH_X86_64) #include pipe/p_compiler.h #include util/u_debug.h diff --git a/src/gallium/auxiliary/translate/translate_sse.c b/src/gallium/auxiliary/translate/translate_sse.c index c7c53b3..c7b6c36 100644 --- a/src/gallium/auxiliary/translate/translate_sse.c +++ b/src/gallium/auxiliary/translate/translate_sse.c @@ -35,7 +35,7 @@ #include translate.h -#if (defined(PIPE_ARCH_X86) || (defined(PIPE_ARCH_X86_64) !defined(__MINGW32__))) !defined(PIPE_SUBSYSTEM_EMBEDDED) +#if (defined(PIPE_ARCH_X86) || defined(PIPE_ARCH_X86_64)) !defined(PIPE_SUBSYSTEM_EMBEDDED) #include rtasm/rtasm_cpu.h #include rtasm/rtasm_x86sse.h -- 1.9.1 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/9] Gallium Nine
On 19 November 2014 00:26, Emil Velikov emil.l.veli...@gmail.com wrote: From a quick look at MSDN it seems to me that going the DDI (like) route would require substantial rework on the wine side. How much contribution from wine can we expect ? Would you have the chance to help with design/coding, or would you be (no disrespect here) limited to answering questions ? Yeah, it would require a significant amount of work on the wined3d side as well, which is part of the reason we'd like to make sure that work can't be avoided. I'm sure that not everything in mesa is perfect yet I've not seen (m)any bug reports from you guys. If/when you guys spot something broken/extremely slow please bugzilla it or send an email to the ML. I think for a part that's because I prefer sending patches when time allows. E.g. around 2010-2011 I sent a couple of patches, mostly for making the Wine tests pass on r600c and later r600g. These days the amount of time I can spend on Mesa is more limited, but at least r600g generally works pretty well for me. I know Stefan regularly runs tests on r300g and r600g and sends bug reports when something breaks. Speaking of feedback, please consider using GLX_MESA_query_renderer. It should help you (at least a bit) with the massive vendor/device/video_memory tables that you currently have. It's already on our (unfortunately fairly large) todo-list. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v3 0/9] Gallium Nine
On 19 November 2014 01:55, Marek Olšák mar...@gmail.com wrote: Before we start discussing what we can do about the OpenGL API overhead, we must get rid of the on-demand shader compilation. It's unacceptable to compile shaders when we should be rendering. This is one of the things that Nine fixes. I assume Wine does that because there can be several slightly-different variants of the same shader for various reasons. Well then we'll have to figure out how to reduce that number to 1. Pretty much. Although as Stefan mentioned there are likely going to be cases where it can't be avoided. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/20] mesa: Autogenerate most of format_pack.c
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 18/11/14 19:54, Jason Ekstrand wrote: On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga ito...@igalia.com wrote: From: Jason Ekstrand jason.ekstr...@intel.com We were auto-generating it before. The problem was that the autogeneration tool we were using was called copy, paste, and edit. Let's use a more sensible solution. Signed-off-by: Jason Ekstrand jason.ekstr...@intel.com v2 by Samuel Iglesias sigles...@igalia.com - Remove format_pack.c as it is now autogenerated - Add usage of INDENT_FLAGS in Makefile.am - Remove trailing blank line Signed-off-by: Samuel Iglesias Gonsalvez sigles...@igalia.com --- src/mesa/Makefile.am |9 + src/mesa/Makefile.sources|2 +- src/mesa/main/format_convert.py | 71 + src/mesa/main/format_pack.c | 2982 -- src/mesa/main/format_pack.c.mako | 898 src/mesa/main/run_mako.py|7 + 6 files changed, 986 insertions(+), 2983 deletions(-) create mode 100644 src/mesa/main/format_convert.py delete mode 100644 src/mesa/main/format_pack.c create mode 100644 src/mesa/main/format_pack.c.mako create mode 100644 src/mesa/main/run_mako.py diff --git a/src/mesa/Makefile.am b/src/mesa/Makefile.am index e71bccb..76cac4a 100644 --- a/src/mesa/Makefile.am +++ b/src/mesa/Makefile.am @@ -66,6 +66,7 @@ BUILT_SOURCES = \ main/get_hash.h \ main/format_info.c \ $(BUILDDIR)main/git_sha1.h \ + $(BUILDDIR)main/format_pack.c \ $(BUILDDIR)program/program_parse.tab.c \ $(BUILDDIR)program/lex.yy.c CLEANFILES = \ @@ -89,6 +90,14 @@ main/format_info.c: main/formats.csv \ $ $@.tmp; \ mv $@.tmp $@; +$(BUILDDIR)main/format_pack.c: main/format_pack.c.mako main/formats.csv \ + main/run_mako.py main/format_parser.py + $(AM_V_GEN)set -e; \ + $(PYTHON2) $(PYTHON_FLAGS) $(srcdir)/main/run_mako.py \ + $ $(srcdir)/main/formats.csv $@.tmp; \ + cat $@.tmp | $(INDENT) $(INDENT_FLAGS) $@;\ +rm $@.tmp; + main/formats.c: main/format_info.c noinst_LTLIBRARIES = $(ARCH_LIBS) diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources index 4755018..99fc497 100644 --- a/src/mesa/Makefile.sources +++ b/src/mesa/Makefile.sources @@ -50,7 +50,7 @@ MAIN_FILES = \ $(SRCDIR)main/fog.c \ $(SRCDIR)main/formatquery.c \ $(SRCDIR)main/formats.c \ - $(SRCDIR)main/format_pack.c \ + $(BUILDDIR)main/format_pack.c \ $(SRCDIR)main/format_unpack.c \ $(SRCDIR)main/format_utils.c \ $(SRCDIR)main/framebuffer.c \ diff --git a/src/mesa/main/format_convert.py b/src/mesa/main/format_convert.py new file mode 100644 index 000..0423427 --- /dev/null +++ b/src/mesa/main/format_convert.py @@ -0,0 +1,71 @@ +#!/usr/bin/env python +# +# Copyright 2014 Intel Corporation +# All Rights Reserved. +# +# Permission is hereby granted, free of charge, to any person obtaining a +# copy of this software and associated documentation files (the +# Software), to deal in the Software without restriction, including +# without limitation the rights to use, copy, modify, merge, publish, +# distribute, sub license, and/or sell copies of the Software, and to +# permit persons to whom the Software is furnished to do so, subject to +# the following conditions: +# +# The above copyright notice and this permission notice (including the +# next paragraph) shall be included in all copies or substantial portions +# of the Software. +# +# THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS +# OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +# MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NON-INFRINGEMENT. +# IN NO EVENT SHALL VMWARE AND/OR ITS SUPPLIERS BE LIABLE FOR +# ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, +# TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE +# SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE. + +import format_parser as parser + +def __get_datatype(_type, size): + if _type == parser.FLOAT: + if size == 32: + return 'float' + elif size == 16: + return 'uint16_t' + else: + assert False + elif _type == parser.UNSIGNED: + if size = 8: + return 'uint8_t' + elif size = 16: + return 'uint16_t' + elif size = 32: + return 'uint32_t' + else: + assert False + elif _type == parser.SIGNED: + if size = 8: + return 'int8_t' + elif size = 16: + return 'int16_t' + elif size = 32: + return 'int32_t' + else: +
Re: [Mesa-dev] [PATCH] rtasm,translate: Re-enable SSE on Mingw64.
Reviewed-by: Roland Scheidegger srol...@vmware.com Do you know if that's due to MinGW changes or something else why it no longer causes crashes? Roland Am 19.11.2014 um 13:10 schrieb jfons...@vmware.com: From: José Fonseca jfons...@vmware.com This reverts f4dd0991719ef3e2606920c5100b372181c60899. The src/gallium/tests/unit/translate_test.c gives the same results on MinGW 64-bits as on Linux 64-bits. And since MinGW is often used for development/testing due to its convenience, it's better not to have this sort of differences relative to MSVC. --- src/gallium/auxiliary/rtasm/rtasm_x86sse.c | 2 +- src/gallium/auxiliary/translate/translate_sse.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/gallium/auxiliary/rtasm/rtasm_x86sse.c b/src/gallium/auxiliary/rtasm/rtasm_x86sse.c index 24ff820..f963788 100644 --- a/src/gallium/auxiliary/rtasm/rtasm_x86sse.c +++ b/src/gallium/auxiliary/rtasm/rtasm_x86sse.c @@ -25,7 +25,7 @@ #include pipe/p_config.h #include util/u_cpu_detect.h -#if defined(PIPE_ARCH_X86) || (defined(PIPE_ARCH_X86_64) !defined(__MINGW32__)) +#if defined(PIPE_ARCH_X86) || defined(PIPE_ARCH_X86_64) #include pipe/p_compiler.h #include util/u_debug.h diff --git a/src/gallium/auxiliary/translate/translate_sse.c b/src/gallium/auxiliary/translate/translate_sse.c index c7c53b3..c7b6c36 100644 --- a/src/gallium/auxiliary/translate/translate_sse.c +++ b/src/gallium/auxiliary/translate/translate_sse.c @@ -35,7 +35,7 @@ #include translate.h -#if (defined(PIPE_ARCH_X86) || (defined(PIPE_ARCH_X86_64) !defined(__MINGW32__))) !defined(PIPE_SUBSYSTEM_EMBEDDED) +#if (defined(PIPE_ARCH_X86) || defined(PIPE_ARCH_X86_64)) !defined(PIPE_SUBSYSTEM_EMBEDDED) #include rtasm/rtasm_cpu.h #include rtasm/rtasm_x86sse.h ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] Bug in 48157b904a9 found by Coverity
Hey guys, Just got around to looking at the Coverity email about newly introduced bugs, and this seems like a legit issue. Based on the context of the code, I'm guessing you meant to do (input_index - 16) 1. Cheers, -ilia ** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() *** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() 93 if (!(brw-fragment_program-Base.InputsRead BITFIELD64_BIT(attr))) 94 continue; 95 96 if (input_index 16) 97 dw4 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 98 else CID 1251308: Bad bit shift operation (BAD_SHIFT) In expression 3 (input_index 1), left shifting by more than 31 bits has undefined behavior. The shift amount, input_index 1, is 32. 99 dw5 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 100 101 ++input_index; 102 } 103} 104BEGIN_BATCH(sbe_cmd_length); ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965/disasm: Properly decode branch_ctrl (gen8+)
On Wed, Nov 19, 2014 at 01:28:25AM -0800, Kenneth Graunke wrote:
On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
Add support for decoding the new branch control bit. I saw two things wrong
with
the existing code.
1. It didn't bother trying to decode the bit.
- While we do not *intentionally* emit this bit today, I think it's
interesting
to see if we somehow ended up with the bit set. It may also be useful in
the
future.
2. It seemed to be the wrong bit.
- The docs are pretty poor wrt which bit this actually occupies. To me, it
/looks/ like it should be bit 28. I am not sure where Ken got 30 from. I
verified it should be 28 by looking at the simulator code.
Yeah, it sure looks like 28 to me...I must've botched it when typing up the
tables.
One comment below.
I also added the most basic support for GOTO simply so we don't need to
remember
to change the function in the future.
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/brw_disasm.c | 29 ++---
src/mesa/drivers/dri/i965/brw_inst.h| 2 +-
3 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 53cd75e..ed94bcc 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -820,6 +820,7 @@ enum opcode {
BRW_OPCODE_MSAVE = 44, /** Pre-Gen6 */
BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
BRW_OPCODE_PUSH = 46, /** Pre-Gen6 */
+ BRW_OPCODE_GOTO = 46, /** Gen8+*/
BRW_OPCODE_POP =47, /** Pre-Gen6 */
BRW_OPCODE_WAIT = 48,
BRW_OPCODE_SEND = 49,
diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c
b/src/mesa/drivers/dri/i965/brw_disasm.c
index 53ec767..013058e 100644
--- a/src/mesa/drivers/dri/i965/brw_disasm.c
+++ b/src/mesa/drivers/dri/i965/brw_disasm.c
@@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
}
static bool
+has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
+{
+ if (brw-gen 8)
+ return false;
+
+ return opcode == BRW_OPCODE_IF ||
+ opcode == BRW_OPCODE_ELSE ||
+ opcode == BRW_OPCODE_GOTO ||
+ opcode == BRW_OPCODE_ENDIF;
+}
I don't think ENDIF has BranchCtrl. With that removed, this is:
Reviewed-by: Kenneth Graunke kenn...@whitecape.org
Yes. Matt caught this as well. Unfortunately the search for branch_ctrl returns
nothing in bspec, so I had to manually click every instruction in the ISA (I
didn't want to assume anything). Else is next to Endif... I am pretty sure I
clicked Else twice.
Thanks. I'll consider the other comment Matt left and then either resubmit with
a change for him, or push.
--
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] rtasm,translate: Re-enable SSE on Mingw64.
On 19/11/2014 15:25, Jose Fonseca wrote: No idea. But the impression I generally have is MinGW has come a long way since then (3 years ago.) I think there was at least one bug in mesa which prevented this from working, see commit cedfd79b ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Bug in 48157b904a9 found by Coverity
On Wed, Nov 19, 2014 at 12:13:41PM -0500, Ilia Mirkin wrote: Hey guys, Just got around to looking at the Coverity email about newly introduced bugs, and this seems like a legit issue. Based on the context of the code, I'm guessing you meant to do (input_index - 16) 1. Oh my. The proposed fix looks good, mind crafting a patch? -- Damien Cheers, -ilia ** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() *** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() 93 if (!(brw-fragment_program-Base.InputsRead BITFIELD64_BIT(attr))) 94 continue; 95 96 if (input_index 16) 97 dw4 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 98 else CID 1251308: Bad bit shift operation (BAD_SHIFT) In expression 3 (input_index 1), left shifting by more than 31 bits has undefined behavior. The shift amount, input_index 1, is 32. 99 dw5 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 100 101 ++input_index; 102 } 103} 104BEGIN_BATCH(sbe_cmd_length); ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] i965: Remove spurious casts in copy_image_with_memcpy()
On Wed, Nov 19, 2014 at 3:35 AM, Kristian Høgsberg k...@bitplanet.net wrote:
On Tue, Nov 18, 2014 at 9:02 PM, Chad Versace
chad.vers...@linux.intel.com wrote:
If a pointer points to raw, untyped memory and is never dereferenced,
then declare it as 'void*' instead of casting it to 'void*'.
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index 341220c..cb44474 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -144,7 +144,7 @@ copy_image_with_memcpy(struct brw_context *brw,
int src_width, int src_height)
{
bool same_slice;
- uint8_t *mapped, *src_mapped, *dst_mapped;
+ void *mapped, *src_mapped, *dst_mapped;
Making these void * means that this code below:
src_mapped = mapped + ((src_y - map_y1) / src_bh) * src_stride +
((src_x - map_x1) / src_bw) * cpp;
(same for dst_mapped) becomes arithmetic on void pointers. gcc
supports that and treats it as uint8_t pointer arithmetic [1], but I'm
not aware of any official C standard that allows it. I don't think we
rely on that elsewhere
We have in the past, and using gcc extensions are fine. But as you
say, it's probably not what he wanted anyway.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Bug in 48157b904a9 found by Coverity
On Wed, Nov 19, 2014 at 12:28 PM, Damien Lespiau damien.lesp...@intel.com wrote: On Wed, Nov 19, 2014 at 12:13:41PM -0500, Ilia Mirkin wrote: Hey guys, Just got around to looking at the Coverity email about newly introduced bugs, and this seems like a legit issue. Based on the context of the code, I'm guessing you meant to do (input_index - 16) 1. Oh my. The proposed fix looks good, mind crafting a patch? Probably best done by someone with access to the hardware/simulator or who has read the specs (are the gen9 specs even public yet? haven't checked). As an aside, it seems like there can be 32 var varyings, and there are a bunch of additional slots on top of that... not sure if you meant to start at VARYING_SLOT_VAR0 or if you wanted all the varying slots, and there's some sort of additional mechanism that makes sure that at most 32 can be set. [I guess this is for FS, so it can't have a whole lot of other inputs, but gl_FragCoord comes to mind. Perhaps that counts against the varying max.] -- Damien Cheers, -ilia ** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() *** CID 1251308: Bad bit shift operation (BAD_SHIFT) /src/mesa/drivers/dri/i965/gen8_sf_state.c: 99 in upload_sbe() 93 if (!(brw-fragment_program-Base.InputsRead BITFIELD64_BIT(attr))) 94 continue; 95 96 if (input_index 16) 97 dw4 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 98 else CID 1251308: Bad bit shift operation (BAD_SHIFT) In expression 3 (input_index 1), left shifting by more than 31 bits has undefined behavior. The shift amount, input_index 1, is 32. 99 dw5 |= (GEN9_SBE_ACTIVE_COMPONENT_XYZW (input_index 1)); 100 101 ++input_index; 102 } 103} 104BEGIN_BATCH(sbe_cmd_length); ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Bug in 48157b904a9 found by Coverity
On Wed, Nov 19, 2014 at 12:42:45PM -0500, Ilia Mirkin wrote: On Wed, Nov 19, 2014 at 12:28 PM, Damien Lespiau damien.lesp...@intel.com wrote: On Wed, Nov 19, 2014 at 12:13:41PM -0500, Ilia Mirkin wrote: Hey guys, Just got around to looking at the Coverity email about newly introduced bugs, and this seems like a legit issue. Based on the context of the code, I'm guessing you meant to do (input_index - 16) 1. Oh my. The proposed fix looks good, mind crafting a patch? Probably best done by someone with access to the hardware/simulator or who has read the specs (are the gen9 specs even public yet? haven't checked). Nop. As an aside, it seems like there can be 32 var varyings, and there are a bunch of additional slots on top of that... not sure if you meant to start at VARYING_SLOT_VAR0 or if you wanted all the varying slots, and there's some sort of additional mechanism that makes sure that at most 32 can be set. [I guess this is for FS, so it can't have a whole lot of other inputs, but gl_FragCoord comes to mind. Perhaps that counts against the varying max.] It does look fishy. That's probably a question for Ken, a real mesa person though. -- Damien ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 3/3] wgl: Ensure PIXELFORMATDESCRIPTOR members are zeroed.
From: José Fonseca jfons...@vmware.com
I suddenly started seeing many simple GL apps, including wglinfo,
choosing Microsoft GDI OpenGL implementation, even though hardware
accelerated pixel formats were available.
It turned out that:
- the screen was in 16bpp mode (some WHCK tests have the nasty habit
of doing that)
- NVIDIA opengl driver only reports R5G6B5 pixel formats (ie no alpha
bits) in this case
- non-zero cAlphaBits was being passed to ChoosePixelformat (or in the
wglinfo case, garbage, as the structure wasn't being properly zeroed)
- ChoosePixelFormat will choose a SW pixel format, just to honour the
At least on the wglinfo and friends case the alpha bits are not needed,
so this change will make sure that HW accelerated formats will be chosen
before SW ones.
---
src/wgl/sharedtex_mt.c | 6 --
src/wgl/wglinfo.c | 1 +
src/wgl/wglthreads.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/wgl/sharedtex_mt.c b/src/wgl/sharedtex_mt.c
index 161b2bb..073b100 100644
--- a/src/wgl/sharedtex_mt.c
+++ b/src/wgl/sharedtex_mt.c
@@ -118,7 +118,7 @@ initMainthread(void)
{
WNDCLASS wc = {0};
HWND win;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
wc.lpfnWndProc = WndProc;
@@ -147,6 +147,7 @@ initMainthread(void)
Error(Couldn't obtain HDC);
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
@@ -405,7 +406,7 @@ threadRunner (void *arg)
{
struct thread_init_arg *tia = (struct thread_init_arg *) arg;
struct window *win;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
win = Windows[tia-id];
@@ -419,6 +420,7 @@ threadRunner (void *arg)
if(tia-id 0)
WaitForSingleObject(Windows[tia-id - 1].hEventInitialised, INFINITE);
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
diff --git a/src/wgl/wglinfo.c b/src/wgl/wglinfo.c
index 30b1307..b6285ec 100644
--- a/src/wgl/wglinfo.c
+++ b/src/wgl/wglinfo.c
@@ -123,6 +123,7 @@ print_screen_info(HDC _hdc, GLboolean limits, GLboolean
singleLine,
return;
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 3;
pfd.cRedBits = 1;
pfd.cGreenBits = 1;
diff --git a/src/wgl/wglthreads.c b/src/wgl/wglthreads.c
index 27dca10..2ee42e2 100644
--- a/src/wgl/wglthreads.c
+++ b/src/wgl/wglthreads.c
@@ -430,7 +430,7 @@ create_window(struct winthread *wt, HGLRC shareCtx)
int ypos = (wt-Index / 8) * (width + 20);
HWND win;
HDC hdc;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
HGLRC ctx;
@@ -463,6 +463,7 @@ create_window(struct winthread *wt, HGLRC shareCtx)
Error(Couldn't obtain HDC);
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Using the 'f' suffix to create a float from an integer literal
On 11/19/2014 03:47 AM, Iago Toral Quiroga wrote: Hi, I came across a GLSL test that checks that doing something like this in a shader should fail: Is this one of the dEQP tests? float value = 1f; It seems like we have a test related to this in piglit somewhere... it looks like tests/shaders/glsl-floating-constant-120.shader_test uses that syntax, but it's not explicitly testing that feature. However, this works fine in Mesa. Checking the spec I see: Floating-point constants are defined as follows. floating-constant: fractional-constant exponent-part(opt) floating-suffix(opt) digit-sequence exponent-part floating-suffix(opt) fractional-constant: digit-sequence . digit-sequence digit-sequence . . digit-sequence exponent-part: e sign(opt) digit-sequence E sign(opt) digit-sequence sign: one of + - digit-sequence: digit digit-sequence digit floating-suffix: one of f F which suggests that the test is correct and Mesa has a bug. According to the above rules, however, something like this is fine: float f = 1e2f; which I find kind of weird if the other case is not valid, so I wonder if there is a bug in the spec or this is on purpose for some reason that I am missing. Then, to make matters worse, I read this in opengl.org wiki [1]: A numeric literal that uses a decimal is by default of type float. To create a float literal from an integer value, use the suffix f or F as in C/C++. which contradicts the spec and the test and is aligned with the current way Mesa works. So: does anyone know what version is right? Could this be a bug in the spec? and if it is not, do we want to change the behavior to follow the spec as it is now? The $64,000 question: What do other GLSL compilers (including, perhaps, glslang) do? This seems like one of the cases where nobody is likely to follow the spec, and some application will depend on that. If that's the case, I'll submit a spec bug. Thanks, Iago [1] https://www.opengl.org/wiki/Data_Type_%28GLSL%29 ___ 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 2/3] tests, trival, fp, vp: Rename errno with errnum.
From: José Fonseca jfons...@vmware.com
Prevents warnings on MinGW due to conflicting linkage types for errno.
---
src/fp/fp-tri.c | 8
src/tests/arbfpspec.c| 14 +++---
src/tests/arbfptest1.c | 8
src/tests/arbvptest1.c | 8
src/tests/arbvptest3.c | 8
src/tests/arbvptorus.c | 8
src/tests/arbvpwarpmesh.c| 8
src/trivial/draw2arrays.c| 8
src/trivial/drawarrays.c | 8
src/trivial/drawelements-large.c | 8
src/trivial/drawelements.c | 8
src/trivial/drawrange.c | 8
src/trivial/lineloop-elts.c | 8
src/trivial/tri-array-interleaved.c | 8
src/trivial/tri-fp-const-imm.c | 8
src/trivial/tri-fp.c | 8
src/trivial/vbo-drawarrays-2101010.c | 6 +++---
src/trivial/vbo-drawarrays.c | 8
src/trivial/vbo-drawelements.c | 8
src/trivial/vbo-drawrange.c | 8
src/trivial/vbo-noninterleaved.c | 8
src/trivial/vbo-tri.c| 8
src/trivial/vp-array-hf.c| 8
src/trivial/vp-array-int.c | 8
src/trivial/vp-array.c | 8
src/trivial/vp-clip.c| 8
src/trivial/vp-line-clip.c | 8
src/trivial/vp-tri-cb-pos.c | 8
src/trivial/vp-tri-cb-tex.c | 8
src/trivial/vp-tri-cb.c | 8
src/trivial/vp-tri-imm.c | 8
src/trivial/vp-tri-invariant.c | 8
src/trivial/vp-tri-swap.c| 8
src/trivial/vp-tri-tex.c | 8
src/trivial/vp-tri.c | 8
src/trivial/vp-unfilled.c| 8
src/vp/vp-tris.c | 8
37 files changed, 150 insertions(+), 150 deletions(-)
diff --git a/src/fp/fp-tri.c b/src/fp/fp-tri.c
index d063b85..4994d5d 100644
--- a/src/fp/fp-tri.c
+++ b/src/fp/fp-tri.c
@@ -71,7 +71,7 @@ static void args(int argc, char *argv[])
static void Init( void )
{
GLuint Texture;
- GLint errno;
+ GLint errnum;
GLuint prognum;
char buf[5];
GLuint sz;
@@ -101,9 +101,9 @@ static void Init( void )
glProgramStringARB(GL_FRAGMENT_PROGRAM_ARB, GL_PROGRAM_FORMAT_ASCII_ARB,
sz, (const GLubyte *)buf);
- errno = glGetError();
- printf(glGetError = 0x%x\n, errno);
- if (errno != GL_NO_ERROR) {
+ errnum = glGetError();
+ printf(glGetError = 0x%x\n, errnum);
+ if (errnum != GL_NO_ERROR) {
GLint errorpos;
glGetIntegerv(GL_PROGRAM_ERROR_POSITION_ARB, errorpos);
diff --git a/src/tests/arbfpspec.c b/src/tests/arbfpspec.c
index cfa6f3e..fcc5052 100644
--- a/src/tests/arbfpspec.c
+++ b/src/tests/arbfpspec.c
@@ -105,7 +105,7 @@ static void SpecialKey( int key, int x, int y )
static void Init( void )
{
- GLint errno;
+ GLint errnum;
GLuint prognum, fprognum;
static const char prog[] =
@@ -141,9 +141,9 @@ static void Init( void )
strlen(prog), (const GLubyte *) prog);
assert(glIsProgramARB(prognum));
- errno = glGetError();
- printf(glGetError = %d\n, errno);
- if (errno != GL_NO_ERROR)
+ errnum = glGetError();
+ printf(glGetError = %d\n, errnum);
+ if (errnum != GL_NO_ERROR)
{
GLint errorpos;
@@ -155,9 +155,9 @@ static void Init( void )
glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, fprognum);
glProgramStringARB(GL_FRAGMENT_PROGRAM_ARB, GL_PROGRAM_FORMAT_ASCII_ARB,
strlen(fprog), (const GLubyte *) fprog);
- errno = glGetError();
- printf(glGetError = %d\n, errno);
- if (errno != GL_NO_ERROR)
+ errnum = glGetError();
+ printf(glGetError = %d\n, errnum);
+ if (errnum != GL_NO_ERROR)
{
GLint errorpos;
diff --git a/src/tests/arbfptest1.c b/src/tests/arbfptest1.c
index d63e459..40b8aab 100644
--- a/src/tests/arbfptest1.c
+++ b/src/tests/arbfptest1.c
@@ -57,16 +57,16 @@ static void Key( unsigned char key, int x, int y )
static void load_program(const char *prog, GLuint prognum)
{
int a;
- GLint errorpos, errno;
+ GLint errorpos, errnum;
glBindProgramARB(GL_FRAGMENT_PROGRAM_ARB, prognum);
glProgramStringARB(GL_FRAGMENT_PROGRAM_ARB, GL_PROGRAM_FORMAT_ASCII_ARB,
strlen(prog), (const GLubyte *) prog);
assert(glIsProgramARB(prognum));
- errno = glGetError();
- printf(glGetError = %d\n, errno);
- if (errno != GL_NO_ERROR)
+ errnum = glGetError();
+ printf(glGetError = %d\n, errnum);
+ if (errnum != GL_NO_ERROR)
{
glGetIntegerv(GL_PROGRAM_ERROR_POSITION_ARB, errorpos);
printf(errorpos: %d\n, errorpos);
diff --git
Re: [Mesa-dev] [PATCH 03/16] glsl: Move common code to ir_constant_util.h
On 11/16/2014 05:51 PM, Thomas Helland wrote:
This will allow for less code duplication.
I'll be using this in opt_minmax in the coming commits.
---
src/glsl/ir_constant_util.h | 122
src/glsl/opt_algebraic.cpp | 88 +---
src/glsl/opt_minmax.cpp | 17 +-
3 files changed, 124 insertions(+), 103 deletions(-)
create mode 100644 src/glsl/ir_constant_util.h
diff --git a/src/glsl/ir_constant_util.h b/src/glsl/ir_constant_util.h
new file mode 100644
index 000..4e0fede
--- /dev/null
+++ b/src/glsl/ir_constant_util.h
@@ -0,0 +1,122 @@
+/*
+ * Copyright © 2010 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
+ * DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * \file ir_constant_util.h
+ *
+ * A collection of utility functions for use on constants
+ * Shamelessly cut-pasted from opt_algebraic, etc
+ *
+ * Author: Thomas Helland thomashellan...@gmail.com
We don't usually put author tags in comments any more. git-log does
that for us. :)
+ */
+
+#ifndef IR_CONSTANT_UTIL_H_
+#define IR_CONSTANT_UTIL_H_
+
+#include main/macros.h
+#include ir_builder.h
+#include program/prog_instruction.h
+
+using namespace ir_builder;
I don't think putting 'using namespace' in a header file is a good idea.
That could cause surprising results in files that include it.
+
+/* When eliminating an expression and just returning one of its operands,
+ * we may need to swizzle that operand out to a vector if the expression was
+ * vector type.
+ */
+static ir_rvalue *
+swizzle_if_required(ir_expression *expr,
+ ir_rvalue *operand)
+{
+ if (expr-type-is_vector() operand-type-is_scalar()) {
+ return swizzle(operand, SWIZZLE_, expr-type-vector_elements);
+ } else
+ return operand;
+}
+
+static inline bool
+is_vec_zero(ir_constant *ir)
+{
+ return (ir == NULL) ? false : ir-is_zero();
+}
+
+static inline bool
+is_vec_one(ir_constant *ir)
+{
+ return (ir == NULL) ? false : ir-is_one();
+}
+
+static inline bool
+is_vec_two(ir_constant *ir)
+{
+ return (ir == NULL) ? false : ir-is_value(2.0, 2);
+}
+
+static inline bool
+is_vec_negative_one(ir_constant *ir)
+{
+ return (ir == NULL) ? false : ir-is_negative_one();
+}
+
+static inline bool
+is_valid_vec_const(ir_constant *ir)
+{
+ if (ir == NULL)
+ return false;
+
+ if (!ir-type-is_scalar() !ir-type-is_vector())
+ return false;
+
+ return true;
+}
+
+static inline bool
+is_less_than_one(ir_constant *ir)
+{
+ if (!is_valid_vec_const(ir))
+ return false;
+
+ unsigned component = 0;
+ for (int c = 0; c ir-type-vector_elements; c++) {
+ if (ir-get_float_component(c) 1.0f)
+ component++;
+ }
+
+ return (component == ir-type-vector_elements);
+}
+
+static inline bool
+is_greater_than_zero(ir_constant *ir)
+{
+ if (!is_valid_vec_const(ir))
+ return false;
+
+ unsigned component = 0;
+ for (int c = 0; c ir-type-vector_elements; c++) {
+ if (ir-get_float_component(c) 0.0f)
+ component++;
+ }
+
+ return (component == ir-type-vector_elements);
+}
+
+#endif /* IR_CONSTANT_UTIL_H_ */
diff --git a/src/glsl/opt_algebraic.cpp b/src/glsl/opt_algebraic.cpp
index af1f544..8c2f15c 100644
--- a/src/glsl/opt_algebraic.cpp
+++ b/src/glsl/opt_algebraic.cpp
@@ -29,13 +29,10 @@
*/
#include ir.h
-#include ir_visitor.h
#include ir_rvalue_visitor.h
#include ir_optimization.h
-#include ir_builder.h
#include glsl_types.h
-
-using namespace ir_builder;
+#include ir_constant_util.h
namespace {
@@ -68,8 +65,6 @@ public:
int op1,
ir_expression *ir2,
int op2);
- ir_rvalue *swizzle_if_required(ir_expression *expr,
-
Re: [Mesa-dev] [PATCH 04/16] glsl: Expand constant_util
On 11/16/2014 07:41 PM, Matt Turner wrote: On Sun, Nov 16, 2014 at 5:51 PM, Thomas Helland thomashellan...@gmail.com wrote: Add functions: is_greater_than_one is_less_than_zero Add variations like greater_than_or_equal_zero. --- This is not ideal computation-wise, as we are doing two iterations instead of one. The question is wether or not the extra code is worth the duplicaton. Now that all of these functions are in a header, maybe the best thing to do is to actually use macros? Something like IS_CONSTANT(ir, operator, operand) so we can do IS_CONSTANT(ir, =, 0.0f) or something like that. What do other compiler people think? I'm not opposed. ___ 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 05/16] glsl: Change to using switch-case in get_range
On 11/16/2014 05:51 PM, Thomas Helland wrote:
This will make expansion easier and less cluttered.
---
src/glsl/opt_minmax.cpp | 19 ++-
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
index 89970d5..111d183 100644
--- a/src/glsl/opt_minmax.cpp
+++ b/src/glsl/opt_minmax.cpp
@@ -268,11 +268,20 @@ static minmax_range
get_range(ir_rvalue *rval)
{
ir_expression *expr = rval-as_expression();
- if (expr (expr-operation == ir_binop_min ||
-expr-operation == ir_binop_max)) {
- minmax_range r0 = get_range(expr-operands[0]);
- minmax_range r1 = get_range(expr-operands[1]);
- return combine_range(r0, r1, expr-operation == ir_binop_min);
+ minmax_range r0;
+ minmax_range r1;
+
+ if(expr) {
^
Missing space
+ switch(expr-operation) {
^
Missing space
+ case ir_binop_min:
+ case ir_binop_max:
+ r0 = get_range(expr-operands[0]);
+ r1 = get_range(expr-operands[1]);
+ return combine_range(r0, r1, expr-operation == ir_binop_min);
+
+ default:
+ break;
+ }
}
ir_constant *c = rval-as_constant();
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/3] cmake: Don't use gcc specific warnings with g++.
From: José Fonseca jfons...@vmware.com
Avoids warning: command line option ‘-W...’ is valid for Ada/C/ObjC but not for
C++.
---
CMakeLists.txt | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index c3e217f..57a46f8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -72,16 +72,13 @@ if (CMAKE_COMPILER_IS_GNUCC)
add_definitions(
-Wall
-Wpointer-arith
- -Wstrict-prototypes
- -Wmissing-prototypes
-Wmissing-declarations
- -Wnested-externs
-fno-strict-aliasing
- -Wbad-function-cast
#-Wold-style-definition
#-Wdeclaration-after-statement
)
-endif (CMAKE_COMPILER_IS_GNUCC)
+ set (CMAKE_C_FLAGS -Wstrict-prototypes -Wmissing-prototypes
-Wnested-externs -Wbad-function-cast ${CMAKE_CXX_FLAGS})
+endif ()
if (WIN32)
# Nobody likes to include windows.h:
--
1.9.1
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/16] glsl: Add sin, cos and sign to get_range
On 11/16/2014 05:51 PM, Thomas Helland wrote:
They are bound between -1 and 1, so report that.
---
src/glsl/opt_minmax.cpp | 13 +
1 file changed, 13 insertions(+)
diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
index 111d183..341006e 100644
--- a/src/glsl/opt_minmax.cpp
+++ b/src/glsl/opt_minmax.cpp
@@ -271,6 +271,10 @@ get_range(ir_rvalue *rval)
minmax_range r0;
minmax_range r1;
+ void *mem_ctx = ralloc_parent(rval);
I would constify this (as 'void *const mem_ctx') to avoid accidental,
unintentional assignments from being added later. We use mem_ctx a lot
as a name, and this has happened before with nested scopes. I know
others on the project may not agree with me here...
+ ir_constant *low = NULL;
+ ir_constant *high = NULL;
+
if(expr) {
switch(expr-operation) {
case ir_binop_min:
@@ -279,6 +283,15 @@ get_range(ir_rvalue *rval)
r1 = get_range(expr-operands[1]);
return combine_range(r0, r1, expr-operation == ir_binop_min);
+ case ir_unop_sin:
+ case ir_unop_sin_reduced:
+ case ir_unop_cos:
+ case ir_unop_cos_reduced:
+ case ir_unop_sign:
+ high = new(mem_ctx) ir_constant(1.0f);
+ low = new(mem_ctx) ir_constant(-1.0f);
+ return minmax_range(low, high);
+
Why not just
return minmax_range(new(mem_ctx) ir_constant(1.0f),
new(mem_ctx) ir_constant(-1.0f));
default:
break;
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 07/16] glsl: Add saturate to get_range
On 11/16/2014 05:51 PM, Thomas Helland wrote: We can use the intersection function to reduce the range even further if the operand has bounds between 0.0 and 1.0. --- src/glsl/opt_minmax.cpp | 7 +++ 1 file changed, 7 insertions(+) diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp index 341006e..b925aaa 100644 --- a/src/glsl/opt_minmax.cpp +++ b/src/glsl/opt_minmax.cpp @@ -292,6 +292,13 @@ get_range(ir_rvalue *rval) low = new(mem_ctx) ir_constant(-1.0f); return minmax_range(low, high); + case ir_unop_saturate: + high = new(mem_ctx) ir_constant(1.0f); + low = new(mem_ctx) ir_constant(0.0f); + r0 = get_range(expr-operands[0]); + // We can get the intersection here to limit the range even more + return range_intersection(r0, minmax_range(low, high)); + Like in the previous patch, why not return range_intersection(get_range(expr-operands[0]), minmax_range(new(mem_ctx) ir_constant(0.0f), new(mem_ctx) ir_constant(1.0f))); or r0 = minmax_range(new(mem_ctx) ir_constant(0.0f), new(mem_ctx) ir_constant(1.0f)); return range_intersection(get_range(expr-operands[0]), r0); default: break; } ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/16] glsl: Add sin, cos and sign to get_range
On Wed, Nov 19, 2014 at 10:43 AM, Ian Romanick i...@freedesktop.org wrote:
On 11/16/2014 05:51 PM, Thomas Helland wrote:
They are bound between -1 and 1, so report that.
---
src/glsl/opt_minmax.cpp | 13 +
1 file changed, 13 insertions(+)
diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
index 111d183..341006e 100644
--- a/src/glsl/opt_minmax.cpp
+++ b/src/glsl/opt_minmax.cpp
@@ -271,6 +271,10 @@ get_range(ir_rvalue *rval)
minmax_range r0;
minmax_range r1;
+ void *mem_ctx = ralloc_parent(rval);
I would constify this (as 'void *const mem_ctx') to avoid accidental,
unintentional assignments from being added later. We use mem_ctx a lot
as a name, and this has happened before with nested scopes. I know
others on the project may not agree with me here...
+ ir_constant *low = NULL;
+ ir_constant *high = NULL;
+
if(expr) {
switch(expr-operation) {
case ir_binop_min:
@@ -279,6 +283,15 @@ get_range(ir_rvalue *rval)
r1 = get_range(expr-operands[1]);
return combine_range(r0, r1, expr-operation == ir_binop_min);
+ case ir_unop_sin:
+ case ir_unop_sin_reduced:
+ case ir_unop_cos:
+ case ir_unop_cos_reduced:
+ case ir_unop_sign:
+ high = new(mem_ctx) ir_constant(1.0f);
+ low = new(mem_ctx) ir_constant(-1.0f);
+ return minmax_range(low, high);
+
Why not just
return minmax_range(new(mem_ctx) ir_constant(1.0f),
new(mem_ctx) ir_constant(-1.0f));
We can also do a lot better here if we have a small angle. If the angle is
in the range [-pi, pi], then abs(sin(x)) = abs(x). I have no idea if
we're ever going to see a shader in the wild where we would be able to take
advantage of this, but it's something to think about.
default:
break;
}
___
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 08/16] glsl: Add abs to get_range
On 11/16/2014 05:51 PM, Thomas Helland wrote: --- src/glsl/opt_minmax.cpp | 10 ++ 1 file changed, 10 insertions(+) diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp index b925aaa..a48d4d8 100644 --- a/src/glsl/opt_minmax.cpp +++ b/src/glsl/opt_minmax.cpp @@ -283,6 +283,16 @@ get_range(ir_rvalue *rval) r1 = get_range(expr-operands[1]); return combine_range(r0, r1, expr-operation == ir_binop_min); + case ir_unop_abs: + r0 = get_range(expr-operands[0]); + if (is_greater_than_zero(r0.low)) +low = r0.low; + else +low = new(mem_ctx) ir_constant(0.0f); If both low and high are 0, you can get a tighter range. In that case, the new low is abs(old high) and the new high is abs(old low). + if (r0.high r0.low) +high = larger_constant(r0.high, abs(r0.low)-constant_expression_value()); + return minmax_range(low, high); + case ir_unop_sin: case ir_unop_sin_reduced: case ir_unop_cos: ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 11/16] glsl: Add ir_binop_pow to get_range
On 11/16/2014 05:51 PM, Thomas Helland wrote:
The spec states that pow is undefined for x 0.
Just set the range to correspond to a constant 0
if this is the case.
It is technically undefined, but I think all hardware either follows the
SM2.0 rules[1] or the SM5 rules[2]. Since a lot of the applications run
on Mesa are, alas, ports of DX apps, they may depend on one of these
behaviors.
A SM2.0 app will have gone through the HLSL compiler, which I *think*
just lowers pow(x,y) to exp(y, log(abs(x)), so it may not matter too
much there.
I'm not sure what the right answer is.
1:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb147286(v=vs.85).aspx
2:
http://msdn.microsoft.com/en-us/library/windows/desktop/bb509636(v=vs.85).aspx
---
src/glsl/opt_minmax.cpp | 11 +++
1 file changed, 11 insertions(+)
diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp
index 9852dd9..ad8c88a 100644
--- a/src/glsl/opt_minmax.cpp
+++ b/src/glsl/opt_minmax.cpp
@@ -335,6 +335,17 @@ get_range(ir_rvalue *rval)
high = add(r0.high, r1.high)-constant_expression_value();
return minmax_range(low, high);
+ case ir_binop_pow:
+ r0 = get_range(expr-operands[0]);
+ if (is_greater_than_or_equal_zero(r0.low))
+low = new(mem_ctx) ir_constant(0.0f);
+ // Result is undefined so we can set the range to bikeshed.
+ if (is_less_than_zero(r0.high)) {
+low = new(mem_ctx) ir_constant(0.0f);
+high = new(mem_ctx) ir_constant(0.0f);
+ }
+ return minmax_range(low, high);
+
default:
break;
}
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 13/16] glsl: Add ir_binop_sub to get_range
Same whitespace comments as patch 10. With those fixed, Reviewed-by: Ian Romanick ian.d.roman...@intel.com On 11/16/2014 05:51 PM, Thomas Helland wrote: --- src/glsl/opt_minmax.cpp | 9 + 1 file changed, 9 insertions(+) diff --git a/src/glsl/opt_minmax.cpp b/src/glsl/opt_minmax.cpp index 0638a12..9d300d3 100644 --- a/src/glsl/opt_minmax.cpp +++ b/src/glsl/opt_minmax.cpp @@ -335,6 +335,15 @@ get_range(ir_rvalue *rval) high = add(r0.high, r1.high)-constant_expression_value(); return minmax_range(low, high); + case ir_binop_sub: + r0 = get_range(expr-operands[0]); + r1 = get_range(expr-operands[1]); + if (r0.low r1.high) +low = sub(r0.low, r1.high)-constant_expression_value(); + if(r0.high r1.low) +high = sub(r0.high, r1.low)-constant_expression_value(); + return minmax_range(low, high); + case ir_binop_pow: r0 = get_range(expr-operands[0]); if (is_greater_than_or_equal_zero(r0.low)) ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 01/29] mesa: Add an implementation of a master convert function.
By and large, this looks good to me. Most of my comments are cosmetic or
suggestions for added documentation. There is one issue that I think is
subtly wrong with integer format conversion but that should be easy to fix.
--Jason
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
From: Jason Ekstrand jason.ekstr...@intel.com
v2 by Iago Toral ito...@igalia.com:
- When testing if we can directly pack we should use the src format to
check
if we are packing from an RGBA format. The original code used the dst
format
for the ubyte case by mistake.
- Fixed incorrect number of bits for dst, it was computed using the src
format
instead of the dst format.
- If the dst format is an array format, check if it is signed. We were only
checking this for the case where it was not an array format, but we need
to know this in both scenarios.
- Fixed incorrect swizzle transform for the cases where we convert between
array formats.
- Compute is_signed and bits only once and for the dst format. We were
computing these for the src format too but they were overwritten by the
dst values immediately after.
- Be more careful when selecting the integer path. Specifically, check that
both src and dst are integer types. Checking only one of them should
suffice
since OpenGL does not allow conversions between normalized and integer
types,
but putting extra care here makes sense and also makes the actual
requirements
for this path more clear.
- The format argument for pack functions is the destination format we are
packing to, not the source format (which has to be RGBA).
- Expose RGBA_* to other files. These will come in handy when in need
to
test if a given array format is RGBA or in need to pass RGBA formats to
mesa_format_convert.
v3 by Samuel Iglesias sigles...@igalia.com:
- Add an RGBA_INT definition.
---
src/mesa/main/format_utils.c | 378
+++
src/mesa/main/format_utils.h | 10 ++
src/mesa/main/formats.h | 15 +-
3 files changed, 399 insertions(+), 4 deletions(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index fcbbba4..c3815cb 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -25,6 +25,384 @@
#include format_utils.h
#include glformats.h
#include macros.h
+#include format_pack.h
+#include format_unpack.h
+
+mesa_array_format RGBA_FLOAT = {{
+ MESA_ARRAY_FORMAT_TYPE_FLOAT,
+ 0,
+ 4,
+ 0, 1, 2, 3,
+ 0, 1
+}};
+
+mesa_array_format RGBA_UBYTE = {{
+ MESA_ARRAY_FORMAT_TYPE_UBYTE,
+ 1,
+ 4,
+ 0, 1, 2, 3,
+ 0, 1
+}};
+
+mesa_array_format RGBA_UINT = {{
+ MESA_ARRAY_FORMAT_TYPE_UINT,
+ 0,
+ 4,
+ 0, 1, 2, 3,
+ 0, 1
+}};
+
+mesa_array_format RGBA_INT = {{
+ MESA_ARRAY_FORMAT_TYPE_INT,
+ 0,
+ 4,
+ 0, 1, 2, 3,
+ 0, 1
+}};
+
+static void
+invert_swizzle(uint8_t dst[4], const uint8_t src[4])
+{
+ int i, j;
+
+ dst[0] = MESA_FORMAT_SWIZZLE_NONE;
+ dst[1] = MESA_FORMAT_SWIZZLE_NONE;
+ dst[2] = MESA_FORMAT_SWIZZLE_NONE;
+ dst[3] = MESA_FORMAT_SWIZZLE_NONE;
+
+ for (i = 0; i 4; ++i)
+ for (j = 0; j 4; ++j)
+ if (src[j] == i dst[i] == MESA_FORMAT_SWIZZLE_NONE)
+dst[i] = j;
+}
+
+static GLenum
+gl_type_for_array_format_datatype(enum mesa_array_format_datatype type)
+{
+ switch (type) {
+ case MESA_ARRAY_FORMAT_TYPE_UBYTE:
+ return GL_UNSIGNED_BYTE;
+ case MESA_ARRAY_FORMAT_TYPE_USHORT:
+ return GL_UNSIGNED_SHORT;
+ case MESA_ARRAY_FORMAT_TYPE_UINT:
+ return GL_UNSIGNED_INT;
+ case MESA_ARRAY_FORMAT_TYPE_BYTE:
+ return GL_BYTE;
+ case MESA_ARRAY_FORMAT_TYPE_SHORT:
+ return GL_SHORT;
+ case MESA_ARRAY_FORMAT_TYPE_INT:
+ return GL_INT;
+ case MESA_ARRAY_FORMAT_TYPE_HALF:
+ return GL_HALF_FLOAT;
+ case MESA_ARRAY_FORMAT_TYPE_FLOAT:
+ return GL_FLOAT;
+ default:
+ assert(!Invalid datatype);
+ return GL_NONE;
+ }
+}
We should probably just make _mesa_swizzle_and_convert take these instead
of the GL types. That way we can completely remove GL from the format
conversion code. I'm fine if that's a separate patch on top.
Also, it would be good to have a nice descriptive docstring on the function
below.
+
+void
+_mesa_format_convert(void *void_dst, uint32_t dst_format, size_t
dst_stride,
+ void *void_src, uint32_t src_format, size_t
src_stride,
+ size_t width, size_t height)
+{
+ uint8_t *dst = (uint8_t *)void_dst;
+ uint8_t *src = (uint8_t *)void_src;
+ mesa_array_format src_array_format, dst_array_format;
+ uint8_t src2dst[4], src2rgba[4], rgba2dst[4], dst2rgba[4];
+ GLenum src_gl_type, dst_gl_type, common_gl_type;
+ bool normalized, dst_integer, src_integer, is_signed;
+ uint8_t (*tmp_ubyte)[4];
+ float
Re: [Mesa-dev] [PATCH 02/29] mesa: Set normalized=true for float array formats.
I'm not sure what I think about this. Does it make a functional change
other than consistancy?
--Jason
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
In order to check if a format is normalized Mesa does something like this:
normalized = !_mesa_is_enum_format_integer(srcFormat);
So all float types will set normalized to true. Since our mesa_array_format
includes a normalized flag for each type we want to make it consistent with
this.
---
src/mesa/main/format_info.py | 3 ++-
src/mesa/main/format_utils.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/mesa/main/format_info.py b/src/mesa/main/format_info.py
index 315767d..d4bc276 100644
--- a/src/mesa/main/format_info.py
+++ b/src/mesa/main/format_info.py
@@ -220,9 +220,10 @@ for fmat in formats:
print ' {{ {0} }},'.format(', '.join(map(str, fmat.swizzle)))
if fmat.is_array() and fmat.colorspace in ('rgb', 'srgb'):
chan = fmat.array_element()
+ norm = chan.norm or chan.type == parser.FLOAT
print ' {0} ,'.format(', '.join([
get_array_format_datatype(chan),
- str(int(chan.norm)),
+ str(int(norm)),
str(len(fmat.channels)),
str(fmat.swizzle[0]),
str(fmat.swizzle[1]),
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index c3815cb..1d65f2b 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -30,7 +30,7 @@
mesa_array_format RGBA_FLOAT = {{
MESA_ARRAY_FORMAT_TYPE_FLOAT,
- 0,
+ 1,
4,
0, 1, 2, 3,
0, 1
--
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] Removing unused opcodes (TGSI, Mesa IR)
Eric Anholt e...@anholt.net writes: This series removes a bunch of unused opcodes, mostly from TGSI. It doesn't go as far as we could possibly go -- while I welcome discussion for future patch series deleting more, I hope that discussion doesn't derail the review process for these changes. I haven't messed with the subroutine stuff, since I don't know what people are planning with that. I also haven't messed with the pack/unpack opcodes in TGSI, since they might be useful for some of the GLSL packing stuff. Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe. Lots of looks good, no Reviewed-by (other than Ian on the Mesa side). I'm probably going to push soon anyway if somebody doesn't actually give Reviewed-by or NAK. pgp8XAGogg0I2.pgp Description: PGP signature ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 05/29] mesa: Consider internal base format in _mesa_format_convert
A couple of specific comments are below. More generally, why are you only
considering the base format on two cases? Do we never use it for anything
else?
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
Add a dst_internal_format parameter to _mesa_format_convert, that
represents
the base internal format for texture/pixel uploads, so we can do the right
thing when the driver has selected a different internal format for the
target
dst format.
---
src/mesa/main/format_utils.c | 65
+++-
src/mesa/main/format_utils.h | 2 +-
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index fc59e86..5964689 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum inFormat,
GLenum outFormat, GLubyte *map)
void
_mesa_format_convert(void *void_dst, uint32_t dst_format, size_t
dst_stride,
void *void_src, uint32_t src_format, size_t
src_stride,
- size_t width, size_t height)
+ size_t width, size_t height, GLenum
dst_internal_format)
{
uint8_t *dst = (uint8_t *)void_dst;
uint8_t *src = (uint8_t *)void_src;
@@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
if (src_array_format.as_uint dst_array_format.as_uint) {
assert(src_array_format.normalized == dst_array_format.normalized);
+ /* If the base format of our dst is not the same as the provided
base
+ * format it means that we are converting to a different format
+ * than the one originally requested by the client. This can happen
when
+ * the internal base format requested is not supported by the
driver.
+ * In this case we need to consider the requested internal base
format to
+ * compute the correct swizzle operation from src to dst. We will
do this
+ * by computing the swizzle transform src-rgba-base-rgba-dst
instead
+ * of src-rgba-dst.
+ */
+ mesa_format dst_mesa_format;
+ if (dst_format MESA_ARRAY_FORMAT_BIT)
+ dst_mesa_format = _mesa_format_from_array_format(dst_format);
+ else
+ dst_mesa_format = dst_format;
Let's put an extra line here so it doesn't get confused with the below if
statement
+ if (dst_internal_format !=
_mesa_get_format_base_format(dst_mesa_format)) {
+ /* Compute src2rgba as src-rgba-base-rgba */
+ uint8_t rgba2base[4], base2rgba[4], swizzle[4];
+ _mesa_compute_component_mapping(GL_RGBA, dst_internal_format,
rgba2base);
+ _mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
base2rgba);
+
+ for (i = 0; i 4; i++) {
+if (base2rgba[i] MESA_FORMAT_SWIZZLE_W)
+ swizzle[i] = base2rgba[i];
+else
+ swizzle[i] = src2rgba[rgba2base[base2rgba[i]]];
This doesn't work for composing three swizzles. If you get a ZERO or ONE
in rgba2base, you'll read the wrong memory from src2rgba.
+ }
+ memcpy(src2rgba, swizzle, sizeof(src2rgba));
+ }
+
+ /* Compute src2dst from src2rgba */
for (i = 0; i 4; i++) {
if (rgba2dst[i] MESA_FORMAT_SWIZZLE_W) {
src2dst[i] = rgba2dst[i];
@@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
src += src_stride;
}
} else {
+ /* For some conversions, doing src-rgba-dst is not enough and
we
+ * need to consider the base internal format. In these cases a
+ * swizzle operation is required to match the semantics of the
base
+ * internal format requested: src-rgba-swizzle-rgba-dst.
+ *
+ * We can detect these cases by checking if the swizzle transform
+ * for base-rgba-base is 0123. If it is not, then we need
+ * to do the swizzle operation (need_convert = true).
+ */
+ GLubyte rgba2base[4], base2rgba[4], map[4];
+ bool need_convert = false;
+ mesa_format dst_mesa_format;
+ if (dst_format MESA_ARRAY_FORMAT_BIT)
+dst_mesa_format = _mesa_format_from_array_format(dst_format);
+ else
+dst_mesa_format = dst_format;
Blank line again
+ if (dst_internal_format !=
+ _mesa_get_format_base_format(dst_mesa_format)) {
+_mesa_compute_component_mapping(GL_RGBA, dst_internal_format,
+base2rgba);
+_mesa_compute_component_mapping(dst_internal_format, GL_RGBA,
+rgba2base);
+for (i = 0; i 4; ++i) {
+ map[i] = base2rgba[rgba2base[i]];
+ if (map[i] != i)
+
Re: [Mesa-dev] Removing unused opcodes (TGSI, Mesa IR)
On Wed, Nov 19, 2014 at 2:32 PM, Eric Anholt e...@anholt.net wrote: Eric Anholt e...@anholt.net writes: This series removes a bunch of unused opcodes, mostly from TGSI. It doesn't go as far as we could possibly go -- while I welcome discussion for future patch series deleting more, I hope that discussion doesn't derail the review process for these changes. I haven't messed with the subroutine stuff, since I don't know what people are planning with that. I also haven't messed with the pack/unpack opcodes in TGSI, since they might be useful for some of the GLSL packing stuff. Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe. Lots of looks good, no Reviewed-by (other than Ian on the Mesa side). I'm probably going to push soon anyway if somebody doesn't actually give Reviewed-by or NAK. st/nine got merged. It uses ARR. And at the very least references TGSI_OPCODE_NRM even if it doesn't use it. It also has code that uses CND and DP2A although it's presently disabled via #define's. I'm pretty sure that your series will cause st/nine to fail compiling. -ilia ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/29] mesa: Avoid pack/unpack fast paths if base internal format != base format
A couple of general comments on this patch:
1) The prerequisites should be moved to before the first patch in the
series and it should be squashed into the patch that introduces the
function. There are one or two more patches which also modify it and those
should also be squashed in.
2) I wonder if giving _mesa_format_convert an internal swizzle wouldn't be
better than a destination internal format. There are a couple of reasons
for this:
a) It's more general. If it doesn't cost us anything extra to do it
that way, we might as well.
b) It removes the GL enum dependency from the _mesa_format_convert
c) I think this implementation misses the case where we download a
texture that is storred in a different format than its base format. For
instance, if you are downloading a GL_RGB texture as GL_RGBA but it's
storred as GL_RGBA. I think with the current implementaion, you'll get the
junk in the alpha component of the texture's backing storage instead of a
constant alpha value of 1.
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
In general, if the dst base internal format and the selected dst format are
different we can't be sure that directly packing or unpacking to the
destination
format will produce correct results. In these cases we probably want to
fall
back to other paths (like mesa_swizzle_and_convert) that can handle these
situations better.
An example of this includes a luminance internal format for which the
driver
decided to use something like BGRA. In these case, unpacking to BGRA won't
match the requirements of the luminance internal format.
In the future we may want to allow these fast paths for specific cases
where we know the direct pack/unpack functions will do the right thing.
---
src/mesa/main/format_utils.c | 137
+++
1 file changed, 72 insertions(+), 65 deletions(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index 5964689..34c90d9 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -331,65 +331,82 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
dst_array_format.as_uint = _mesa_format_to_array_format(dst_format);
}
- /* Handle the cases where we can directly unpack */
- if (!(src_format MESA_ARRAY_FORMAT_BIT)) {
- if (dst_array_format.as_uint == RGBA_FLOAT.as_uint) {
- for (row = 0; row height; ++row) {
-_mesa_unpack_rgba_row(src_format, width,
- src, (float (*)[4])dst);
-src += src_stride;
-dst += dst_stride;
- }
- return;
- } else if (dst_array_format.as_uint == RGBA_UBYTE.as_uint
- !_mesa_is_format_integer_color(src_format)) {
- for (row = 0; row height; ++row) {
-_mesa_unpack_ubyte_rgba_row(src_format, width,
-src, (uint8_t (*)[4])dst);
-src += src_stride;
-dst += dst_stride;
- }
- return;
- } else if (dst_array_format.as_uint == RGBA_UINT.as_uint
- _mesa_is_format_integer_color(src_format)) {
- for (row = 0; row height; ++row) {
-_mesa_unpack_uint_rgba_row(src_format, width,
- src, (uint32_t (*)[4])dst);
-src += src_stride;
-dst += dst_stride;
+ /* First we see if we can implement the conversion with a direct pack
+* or unpack.
+*
+* In this case we want to be careful when the dst base format and the
+* dst format do not match. In these cases a simple pack/unpack to the
+* dst format from the src format may not match the semantic
requirements
+* of the internal base format. For now we decide to be safe and avoid
+* this path in these scenarios but in the future we may want to enable
+* it for specific combinations that are known to work.
+*/
+ mesa_format dst_mesa_format;
+ if (dst_format MESA_ARRAY_FORMAT_BIT)
+ dst_mesa_format = _mesa_format_from_array_format(dst_format);
+ else
+ dst_mesa_format = dst_format;
+ if (_mesa_get_format_base_format(dst_mesa_format) ==
dst_internal_format) {
+ /* Handle the cases where we can directly unpack */
+ if (!(src_format MESA_ARRAY_FORMAT_BIT)) {
+ if (dst_array_format.as_uint == RGBA_FLOAT.as_uint) {
+for (row = 0; row height; ++row) {
+ _mesa_unpack_rgba_row(src_format, width,
+ src, (float (*)[4])dst);
+ src += src_stride;
+ dst += dst_stride;
+}
+return;
+ } else if (dst_array_format.as_uint == RGBA_UBYTE.as_uint
+!_mesa_is_format_integer_color(src_format)) {
+for (row = 0; row height; ++row) {
+
Re: [Mesa-dev] [PATCH 09/29] mesa: Add _mesa_swap2_copy and _mesa_swap4_copy
Any reason why you added 2 new functions, instead of just altering the ones
we have and updating where they are used?
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
We have _mesa_swap{2,4} but these do in-place byte-swapping only. The new
functions receive an extra parameter so we can swap bytes on a source
input array and store the results in a (possibly different) destination
array.
This is useful to implement byte-swapping in pixel uploads, since in this
case we need to swap bytes on the src data which is owned by the
application so we can't do an in-place byte swap.
---
src/mesa/main/image.c | 25 +
src/mesa/main/image.h | 10 --
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/mesa/main/image.c b/src/mesa/main/image.c
index 4ea5f04..9ad97c5 100644
--- a/src/mesa/main/image.c
+++ b/src/mesa/main/image.c
@@ -41,36 +41,45 @@
/**
- * Flip the order of the 2 bytes in each word in the given array.
+ * Flip the order of the 2 bytes in each word in the given array (src) and
+ * store the result in another array (dst). For in-place byte-swapping
this
+ * function can be called with the same array for src and dst.
*
- * \param p array.
+ * \param dst the array where byte-swapped data will be stored.
+ * \param src the array with the source data we want to byte-swap.
* \param n number of words.
*/
void
-_mesa_swap2( GLushort *p, GLuint n )
+_mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n )
{
GLuint i;
for (i = 0; i n; i++) {
- p[i] = (p[i] 8) | ((p[i] 8) 0xff00);
+ dst[i] = (src[i] 8) | ((src[i] 8) 0xff00);
}
}
/*
- * Flip the order of the 4 bytes in each word in the given array.
+ * Flip the order of the 4 bytes in each word in the given array (src) and
+ * store the result in another array (dst). For in-place byte-swapping
this
+ * function can be called with the same array for src and dst.
+ *
+ * \param dst the array where byte-swapped data will be stored.
+ * \param src the array with the source data we want to byte-swap.
+ * \param n number of words.
*/
void
-_mesa_swap4( GLuint *p, GLuint n )
+_mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n )
{
GLuint i, a, b;
for (i = 0; i n; i++) {
- b = p[i];
+ b = src[i];
a = (b 24)
| ((b 8) 0xff00)
| ((b 8) 0xff)
| ((b 24) 0xff00);
- p[i] = a;
+ dst[i] = a;
}
}
diff --git a/src/mesa/main/image.h b/src/mesa/main/image.h
index abd84bf..79c6e68 100644
--- a/src/mesa/main/image.h
+++ b/src/mesa/main/image.h
@@ -33,10 +33,16 @@ struct gl_context;
struct gl_pixelstore_attrib;
extern void
-_mesa_swap2( GLushort *p, GLuint n );
+_mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n );
extern void
-_mesa_swap4( GLuint *p, GLuint n );
+_mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n );
+
+static inline void
+_mesa_swap2( GLushort *p, GLuint n ) { _mesa_swap2_copy(p, p, n); }
+
+static inline void
+_mesa_swap4( GLuint *p, GLuint n ) { _mesa_swap4_copy(p, p, n); }
extern GLintptr
_mesa_image_offset( GLuint dimensions,
--
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 03/29] mesa: Do not assert on integer-non-integer direct pack/unpack fast paths
Can you remind me again as to what formats hit these paths? I remember you
hitting them, but I'm still not really seeing how it happens.
--Jason
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
We can have conversions from non-integer types to integer types, so remove
the assertions for these in the pack/unpack fast paths. It could be that
we do not have all the necessary pack/unpack functions in these cases
though,
so protect these paths with conditionals and let _mesa_format_convert use
other paths to resolve these kind of conversions if necessary.
---
src/mesa/main/format_utils.c | 16
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/src/mesa/main/format_utils.c b/src/mesa/main/format_utils.c
index 1d65f2b..56a3b8d 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -143,8 +143,8 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
dst += dst_stride;
}
return;
- } else if (dst_array_format.as_uint == RGBA_UBYTE.as_uint) {
- assert(!_mesa_is_format_integer_color(src_format));
+ } else if (dst_array_format.as_uint == RGBA_UBYTE.as_uint
+ !_mesa_is_format_integer_color(src_format)) {
for (row = 0; row height; ++row) {
_mesa_unpack_ubyte_rgba_row(src_format, width,
src, (uint8_t (*)[4])dst);
@@ -152,8 +152,8 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
dst += dst_stride;
}
return;
- } else if (dst_array_format.as_uint == RGBA_UINT.as_uint) {
- assert(_mesa_is_format_integer_color(src_format));
+ } else if (dst_array_format.as_uint == RGBA_UINT.as_uint
+ _mesa_is_format_integer_color(src_format)) {
for (row = 0; row height; ++row) {
_mesa_unpack_uint_rgba_row(src_format, width,
src, (uint32_t (*)[4])dst);
@@ -174,8 +174,8 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
dst += dst_stride;
}
return;
- } else if (src_array_format.as_uint == RGBA_UBYTE.as_uint) {
- assert(!_mesa_is_format_integer_color(dst_format));
+ } else if (src_array_format.as_uint == RGBA_UBYTE.as_uint
+ !_mesa_is_format_integer_color(dst_format)) {
for (row = 0; row height; ++row) {
_mesa_pack_ubyte_rgba_row(dst_format, width,
(const uint8_t (*)[4])src, dst);
@@ -183,8 +183,8 @@ _mesa_format_convert(void *void_dst, uint32_t
dst_format, size_t dst_stride,
dst += dst_stride;
}
return;
- } else if (src_array_format.as_uint == RGBA_UINT.as_uint) {
- assert(_mesa_is_format_integer_color(dst_format));
+ } else if (src_array_format.as_uint == RGBA_UINT.as_uint
+ _mesa_is_format_integer_color(dst_format)) {
for (row = 0; row height; ++row) {
_mesa_pack_uint_rgba_row(dst_format, width,
(const uint32_t (*)[4])src, dst);
--
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 07/29] mesa: Add helper to convert a GL format and type to a mesa (array) format.
General comment: Maybe this would be better in gltypes rather than in
mesa_formats
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
---
src/mesa/main/formats.c | 285
src/mesa/main/formats.h | 3 +
2 files changed, 288 insertions(+)
diff --git a/src/mesa/main/formats.c b/src/mesa/main/formats.c
index 06e8973..7464d89 100644
--- a/src/mesa/main/formats.c
+++ b/src/mesa/main/formats.c
@@ -325,6 +325,291 @@ _mesa_format_from_array_format(uint32_t array_format)
return MESA_FORMAT_NONE;
}
+static void
+_mesa_array_format_set_swizzle(mesa_array_format *array_format,
+ int x, int y, int z, int w)
+{
+ array_format-swizzle_x = x;
+ array_format-swizzle_y = y;
+ array_format-swizzle_z = z;
+ array_format-swizzle_w = w;
+}
+
+static bool
+_mesa_array_format_set_swizzle_from_format(mesa_array_format
*array_format,
+ GLenum format)
+{
+ switch (format) {
+ case GL_RGBA:
+ case GL_RGBA_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 0, 1, 2, 3);
+ return true;
+ case GL_BGRA:
+ case GL_BGRA_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 2, 1, 0, 3);
+ return true;
+ case GL_ABGR_EXT:
+ _mesa_array_format_set_swizzle(array_format, 3, 2, 1, 0);
+ return true;
+ case GL_RGB:
+ case GL_RGB_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 0, 1, 2, 5);
+ return true;
+ case GL_BGR:
+ case GL_BGR_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 2, 1, 0, 5);
+ return true;
+ case GL_LUMINANCE_ALPHA:
+ case GL_LUMINANCE_ALPHA_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 0, 0, 0, 1);
+ return true;
+ case GL_RG:
+ case GL_RG_INTEGER:
+ _mesa_array_format_set_swizzle(array_format, 0, 1, 4, 5);
+ return true;
+ case GL_RED:
+ case GL_RED_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 0, 4, 4, 5);
+ return true;
+ case GL_GREEN:
+ case GL_GREEN_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 4, 0, 4, 5);
+ return true;
+ case GL_BLUE:
+ case GL_BLUE_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 4, 4, 0, 5);
+ return true;
+ case GL_ALPHA:
+ case GL_ALPHA_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 4, 4, 4, 0);
+ return true;
+ case GL_LUMINANCE:
+ case GL_LUMINANCE_INTEGER_EXT:
+ _mesa_array_format_set_swizzle(array_format, 0, 0, 0, 5);
+ return true;
+ case GL_INTENSITY:
+ _mesa_array_format_set_swizzle(array_format, 0, 0, 0, 0);
+ return true;
+ default:
+ return false;
+ }
+}
+
+/**
+* Take an OpenGL format (GL_RGB, GL_RGBA, etc), OpenGL data type (GL_INT,
+* GL_FOAT, etc) and return a matching mesa_array_format or a mesa_format
+* otherwise (for non-array formats).
+*
+* This function will typically be used to compute a mesa format from a GL
type
+* so we can then call _mesa_format_convert. This function does
+* not consider byte swapping, so it returns types assuming that no byte
+* swapping is involved. If byte swapping is involved then clients are
supposed
+* to handle that on their side before calling _mesa_format_convert.
+*
+* This function returns an uint32_t that can pack a mesa_format or a
+* mesa_array_format. Clients must check the mesa array format bit
+* (MESA_ARRAY_FORMAT_BIT) on the return value to know if the returned
+* format is a mesa_array_format or a mesa_format.
+*/
+uint32_t
+_mesa_format_from_format_and_type(GLenum format, GLenum type)
+{
+ mesa_array_format array_format;
+
+ bool is_array_format = true;
+
+ /* Map the OpenGL data type to an array format data type */
+ switch (type) {
+ case GL_UNSIGNED_BYTE:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_UBYTE;
+ break;
+ case GL_BYTE:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_BYTE;
+ break;
+ case GL_UNSIGNED_SHORT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_USHORT;
+ break;
+ case GL_SHORT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_SHORT;
+ break;
+ case GL_UNSIGNED_INT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_UINT;
+ break;
+ case GL_INT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_INT;
+ break;
+ case GL_HALF_FLOAT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_HALF;
+ break;
+ case GL_FLOAT:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_FLOAT;
+ break;
+ case GL_UNSIGNED_INT_8_8_8_8:
+ case GL_UNSIGNED_INT_8_8_8_8_REV:
+ array_format.type = MESA_ARRAY_FORMAT_TYPE_UBYTE;
If you put these in the GL type switch below as returning the
MESA_FORMAT_R8G8B8A8 or whatever, then the code in
mesa_format_get_array_format will fix up
Re: [Mesa-dev] [PATCH 09/29] mesa: Add _mesa_swap2_copy and _mesa_swap4_copy
On Tue, Nov 18, 2014 at 3:23 AM, Iago Toral Quiroga ito...@igalia.com
wrote:
We have _mesa_swap{2,4} but these do in-place byte-swapping only. The new
functions receive an extra parameter so we can swap bytes on a source
input array and store the results in a (possibly different) destination
array.
If this is being split into an in-place and different pointers version,
I think using the restrict keyword would be useful here to improve the
performance. Then, the in-place one cannot be implemented as copy(p,p,n),
but the code isn't overly complicated.
This is useful to implement byte-swapping in pixel uploads, since in this
case we need to swap bytes on the src data which is owned by the
application so we can't do an in-place byte swap.
---
src/mesa/main/image.c | 25 +
src/mesa/main/image.h | 10 --
2 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/src/mesa/main/image.c b/src/mesa/main/image.c
index 4ea5f04..9ad97c5 100644
--- a/src/mesa/main/image.c
+++ b/src/mesa/main/image.c
@@ -41,36 +41,45 @@
/**
- * Flip the order of the 2 bytes in each word in the given array.
+ * Flip the order of the 2 bytes in each word in the given array (src) and
+ * store the result in another array (dst). For in-place byte-swapping
this
+ * function can be called with the same array for src and dst.
*
- * \param p array.
+ * \param dst the array where byte-swapped data will be stored.
+ * \param src the array with the source data we want to byte-swap.
* \param n number of words.
*/
void
-_mesa_swap2( GLushort *p, GLuint n )
+_mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n )
{
GLuint i;
for (i = 0; i n; i++) {
- p[i] = (p[i] 8) | ((p[i] 8) 0xff00);
+ dst[i] = (src[i] 8) | ((src[i] 8) 0xff00);
}
}
/*
- * Flip the order of the 4 bytes in each word in the given array.
+ * Flip the order of the 4 bytes in each word in the given array (src) and
+ * store the result in another array (dst). For in-place byte-swapping
this
+ * function can be called with the same array for src and dst.
+ *
+ * \param dst the array where byte-swapped data will be stored.
+ * \param src the array with the source data we want to byte-swap.
+ * \param n number of words.
*/
void
-_mesa_swap4( GLuint *p, GLuint n )
+_mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n )
{
GLuint i, a, b;
for (i = 0; i n; i++) {
- b = p[i];
+ b = src[i];
a = (b 24)
| ((b 8) 0xff00)
| ((b 8) 0xff)
| ((b 24) 0xff00);
- p[i] = a;
+ dst[i] = a;
}
}
diff --git a/src/mesa/main/image.h b/src/mesa/main/image.h
index abd84bf..79c6e68 100644
--- a/src/mesa/main/image.h
+++ b/src/mesa/main/image.h
@@ -33,10 +33,16 @@ struct gl_context;
struct gl_pixelstore_attrib;
extern void
-_mesa_swap2( GLushort *p, GLuint n );
+_mesa_swap2_copy( GLushort *dst, GLushort *src, GLuint n );
extern void
-_mesa_swap4( GLuint *p, GLuint n );
+_mesa_swap4_copy( GLuint *dst, GLuint *src, GLuint n );
+
+static inline void
+_mesa_swap2( GLushort *p, GLuint n ) { _mesa_swap2_copy(p, p, n); }
+
+static inline void
+_mesa_swap4( GLuint *p, GLuint n ) { _mesa_swap4_copy(p, p, n); }
extern GLintptr
_mesa_image_offset( GLuint dimensions,
--
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] i965/disasm: Properly decode branch_ctrl (gen8+)
On Wednesday, November 19, 2014 09:15:32 AM Ben Widawsky wrote:
On Wed, Nov 19, 2014 at 01:28:25AM -0800, Kenneth Graunke wrote:
On Tuesday, November 18, 2014 12:35:55 PM Ben Widawsky wrote:
Add support for decoding the new branch control bit. I saw two things
wrong with
the existing code.
1. It didn't bother trying to decode the bit.
- While we do not *intentionally* emit this bit today, I think it's
interesting
to see if we somehow ended up with the bit set. It may also be useful
in the
future.
2. It seemed to be the wrong bit.
- The docs are pretty poor wrt which bit this actually occupies. To me,
it
/looks/ like it should be bit 28. I am not sure where Ken got 30 from.
I
verified it should be 28 by looking at the simulator code.
Yeah, it sure looks like 28 to me...I must've botched it when typing up the
tables.
One comment below.
I also added the most basic support for GOTO simply so we don't need to
remember
to change the function in the future.
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Ben Widawsky b...@bwidawsk.net
---
src/mesa/drivers/dri/i965/brw_defines.h | 1 +
src/mesa/drivers/dri/i965/brw_disasm.c | 29
++---
src/mesa/drivers/dri/i965/brw_inst.h| 2 +-
3 files changed, 28 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h
b/src/mesa/drivers/dri/i965/brw_defines.h
index 53cd75e..ed94bcc 100644
--- a/src/mesa/drivers/dri/i965/brw_defines.h
+++ b/src/mesa/drivers/dri/i965/brw_defines.h
@@ -820,6 +820,7 @@ enum opcode {
BRW_OPCODE_MSAVE =44, /** Pre-Gen6 */
BRW_OPCODE_MRESTORE = 45, /** Pre-Gen6 */
BRW_OPCODE_PUSH = 46, /** Pre-Gen6 */
+ BRW_OPCODE_GOTO = 46, /** Gen8+*/
BRW_OPCODE_POP = 47, /** Pre-Gen6 */
BRW_OPCODE_WAIT = 48,
BRW_OPCODE_SEND = 49,
diff --git a/src/mesa/drivers/dri/i965/brw_disasm.c
b/src/mesa/drivers/dri/i965/brw_disasm.c
index 53ec767..013058e 100644
--- a/src/mesa/drivers/dri/i965/brw_disasm.c
+++ b/src/mesa/drivers/dri/i965/brw_disasm.c
@@ -131,6 +131,18 @@ has_uip(struct brw_context *brw, enum opcode opcode)
}
static bool
+has_branch_ctrl(struct brw_context *brw, enum opcode opcode)
+{
+ if (brw-gen 8)
+ return false;
+
+ return opcode == BRW_OPCODE_IF ||
+ opcode == BRW_OPCODE_ELSE ||
+ opcode == BRW_OPCODE_GOTO ||
+ opcode == BRW_OPCODE_ENDIF;
+}
I don't think ENDIF has BranchCtrl. With that removed, this is:
Reviewed-by: Kenneth Graunke kenn...@whitecape.org
Yes. Matt caught this as well. Unfortunately the search for branch_ctrl
returns
nothing in bspec, so I had to manually click every instruction in the ISA (I
didn't want to assume anything). Else is next to Endif... I am pretty sure I
clicked Else twice.
Odd, search for BranchCtrl and branch_ctrl turned up things for me. Just had
to consider the spellings :(
Thanks. I'll consider the other comment Matt left and then either resubmit
with
a change for him, or push.
Oh, I hadn't seen Matt's reply. I like his suggestion as well.
I don't think it's worth resubmitting - if you make both of those changes,
you can probably just go ahead and push the patch.
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] Removing unused opcodes (TGSI, Mesa IR)
On 19/11/14 19:32, Eric Anholt wrote: Eric Anholt e...@anholt.net writes: This series removes a bunch of unused opcodes, mostly from TGSI. It doesn't go as far as we could possibly go -- while I welcome discussion for future patch series deleting more, I hope that discussion doesn't derail the review process for these changes. I haven't messed with the subroutine stuff, since I don't know what people are planning with that. I also haven't messed with the pack/unpack opcodes in TGSI, since they might be useful for some of the GLSL packing stuff. Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe. Lots of looks good, no Reviewed-by (other than Ian on the Mesa side). I'm probably going to push soon anyway if somebody doesn't actually give Reviewed-by or NAK. By looks good is my shorthand for reviewed-by. But here it is: Reviewed-by: Jose Fonseca jfons...@vmware.com to patches 4-5,7-13 in the series, and v2 of patch 6. (That is, we're fine removing all except ARR.) Jose ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 3/3] wgl: Ensure PIXELFORMATDESCRIPTOR members are zeroed.
Some small nitpick below, but otherwise the series is:
Reviewed-by: Roland Scheidegger srol...@vmware.com
Am 19.11.2014 um 19:23 schrieb jfons...@vmware.com:
From: José Fonseca jfons...@vmware.com
I suddenly started seeing many simple GL apps, including wglinfo,
choosing Microsoft GDI OpenGL implementation, even though hardware
accelerated pixel formats were available.
It turned out that:
- the screen was in 16bpp mode (some WHCK tests have the nasty habit
of doing that)
- NVIDIA opengl driver only reports R5G6B5 pixel formats (ie no alpha
bits) in this case
- non-zero cAlphaBits was being passed to ChoosePixelformat (or in the
wglinfo case, garbage, as the structure wasn't being properly zeroed)
- ChoosePixelFormat will choose a SW pixel format, just to honour the
The sentence here is incomplete.
At least on the wglinfo and friends case the alpha bits are not needed,
so this change will make sure that HW accelerated formats will be chosen
before SW ones.
---
src/wgl/sharedtex_mt.c | 6 --
src/wgl/wglinfo.c | 1 +
src/wgl/wglthreads.c | 3 ++-
3 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/src/wgl/sharedtex_mt.c b/src/wgl/sharedtex_mt.c
index 161b2bb..073b100 100644
--- a/src/wgl/sharedtex_mt.c
+++ b/src/wgl/sharedtex_mt.c
@@ -118,7 +118,7 @@ initMainthread(void)
{
WNDCLASS wc = {0};
HWND win;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
wc.lpfnWndProc = WndProc;
@@ -147,6 +147,7 @@ initMainthread(void)
Error(Couldn't obtain HDC);
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
@@ -405,7 +406,7 @@ threadRunner (void *arg)
{
struct thread_init_arg *tia = (struct thread_init_arg *) arg;
struct window *win;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
win = Windows[tia-id];
@@ -419,6 +420,7 @@ threadRunner (void *arg)
if(tia-id 0)
WaitForSingleObject(Windows[tia-id - 1].hEventInitialised, INFINITE);
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
diff --git a/src/wgl/wglinfo.c b/src/wgl/wglinfo.c
index 30b1307..b6285ec 100644
--- a/src/wgl/wglinfo.c
+++ b/src/wgl/wglinfo.c
@@ -123,6 +123,7 @@ print_screen_info(HDC _hdc, GLboolean limits, GLboolean
singleLine,
return;
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 3;
pfd.cRedBits = 1;
pfd.cGreenBits = 1;
diff --git a/src/wgl/wglthreads.c b/src/wgl/wglthreads.c
index 27dca10..2ee42e2 100644
--- a/src/wgl/wglthreads.c
+++ b/src/wgl/wglthreads.c
@@ -430,7 +430,7 @@ create_window(struct winthread *wt, HGLRC shareCtx)
int ypos = (wt-Index / 8) * (width + 20);
HWND win;
HDC hdc;
- PIXELFORMATDESCRIPTOR pfd = {0};
+ PIXELFORMATDESCRIPTOR pfd;
int visinfo;
HGLRC ctx;
@@ -463,6 +463,7 @@ create_window(struct winthread *wt, HGLRC shareCtx)
Error(Couldn't obtain HDC);
}
+ memset(pfd, 0, sizeof(pfd));
pfd.cColorBits = 24;
pfd.cDepthBits = 24;
pfd.dwFlags = PFD_DOUBLEBUFFER | PFD_DRAW_TO_WINDOW | PFD_SUPPORT_OPENGL;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] rtasm,translate: Re-enable SSE on Mingw64.
Am 19.11.2014 um 18:21 schrieb Jon TURNEY: On 19/11/2014 15:25, Jose Fonseca wrote: No idea. But the impression I generally have is MinGW has come a long way since then (3 years ago.) I think there was at least one bug in mesa which prevented this from working, see commit cedfd79b Ah yes that looks like a pretty bad bug which would have prevented it from working before, though I don't know if there might have been other issues too. In any case though makes sense to reenable it now. Roland ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Removing unused opcodes (TGSI, Mesa IR)
On 19/11/14 19:45, Ilia Mirkin wrote:
On Wed, Nov 19, 2014 at 2:32 PM, Eric Anholt e...@anholt.net wrote:
Eric Anholt e...@anholt.net writes:
This series removes a bunch of unused opcodes, mostly from TGSI. It
doesn't go as far as we could possibly go -- while I welcome discussion
for future patch series deleting more, I hope that discussion doesn't
derail the review process for these changes.
I haven't messed with the subroutine stuff, since I don't know what people
are planning with that. I also haven't messed with the pack/unpack
opcodes in TGSI, since they might be useful for some of the GLSL packing
stuff.
Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe.
Lots of looks good, no Reviewed-by (other than Ian on the Mesa side).
I'm probably going to push soon anyway if somebody doesn't actually give
Reviewed-by or NAK.
st/nine got merged. It uses ARR. And at the very least references
TGSI_OPCODE_NRM even if it doesn't use it. It also has code that uses
CND and DP2A although it's presently disabled via #define's.
The patch attached should do it.
I'm pretty sure that your series will cause st/nine to fail compiling.
BTW, given it's not trivial to compile nine state-tracker, I think it
might be better to not obfuscate which opcodes or symbols it uses with
macros. It pays off to be upfront with these things, so that `git grep
foo` finds everything that should be found.
Jose
diff --git a/src/gallium/state_trackers/nine/nine_shader.c b/src/gallium/state_trackers/nine/nine_shader.c
index cc027b4..4c4d38e 100644
--- a/src/gallium/state_trackers/nine/nine_shader.c
+++ b/src/gallium/state_trackers/nine/nine_shader.c
@@ -38,7 +38,6 @@
#if 1
#define NINE_TGSI_LAZY_DEVS /* don't use TGSI_OPCODE_BREAKC */
#endif
-#define NINE_TGSI_LAZY_R600 /* don't use TGSI_OPCODE_DP2A */
#define DUMP(args...) _nine_debug_printf(DBG_CHANNEL, NULL, args)
@@ -1374,7 +1373,6 @@ DECL_SPECIAL(CND)
}
cnd = tx_src_param(tx, tx-insn.src[0]);
-#ifdef NINE_TGSI_LAZY_R600
cgt = tx_scratch(tx);
if (tx-version.major == 1 tx-version.minor 4) {
@@ -1387,13 +1385,6 @@ DECL_SPECIAL(CND)
ureg_CMP(tx-ureg, dst,
tx_src_param(tx, tx-insn.src[1]),
tx_src_param(tx, tx-insn.src[2]), ureg_negate(cnd));
-#else
-if (tx-version.major == 1 tx-version.minor 4)
-cnd = ureg_scalar(cnd, TGSI_SWIZZLE_W);
-ureg_CND(tx-ureg, dst,
- tx_src_param(tx, tx-insn.src[1]),
- tx_src_param(tx, tx-insn.src[2]), cnd);
-#endif
return D3D_OK;
}
@@ -1980,7 +1971,6 @@ DECL_SPECIAL(NRM)
DECL_SPECIAL(DP2ADD)
{
-#ifdef NINE_TGSI_LAZY_R600
struct ureg_dst tmp = tx_scratch_scalar(tx);
struct ureg_src dp2 = tx_src_scalar(tmp);
struct ureg_dst dst = tx_dst_param(tx, tx-insn.dst[0]);
@@ -1994,9 +1984,6 @@ DECL_SPECIAL(DP2ADD)
ureg_ADD(tx-ureg, dst, src[2], dp2);
return D3D_OK;
-#else
-return NineTranslateInstruction_Generic(tx);
-#endif
}
DECL_SPECIAL(TEXCOORD)
@@ -2316,7 +2303,7 @@ struct sm1_op_info inst_table[] =
_OPI(CRS, XPD, V(0,0), V(3,0), V(0,0), V(3,0), 1, 2, NULL), /* XXX: .w */
_OPI(SGN, SSG, V(2,0), V(3,0), V(0,0), V(0,0), 1, 3, SPECIAL(SGN)), /* ignore src1,2 */
_OPI(ABS, ABS, V(0,0), V(3,0), V(0,0), V(3,0), 1, 1, NULL),
-_OPI(NRM, NRM, V(0,0), V(3,0), V(0,0), V(3,0), 1, 1, SPECIAL(NRM)), /* NRM doesn't fit */
+_OPI(NRM, NOP, V(0,0), V(3,0), V(0,0), V(3,0), 1, 1, SPECIAL(NRM)), /* NRM doesn't fit */
_OPI(SINCOS, SCS, V(2,0), V(2,1), V(2,0), V(2,1), 1, 3, SPECIAL(SINCOS)),
_OPI(SINCOS, SCS, V(3,0), V(3,0), V(3,0), V(3,0), 1, 1, SPECIAL(SINCOS)),
@@ -2371,7 +2358,7 @@ struct sm1_op_info inst_table[] =
/* Misc */
_OPI(CMP,CMP, V(0,0), V(0,0), V(1,2), V(3,0), 1, 3, SPECIAL(CMP)), /* reversed */
_OPI(BEM,NOP, V(0,0), V(0,0), V(1,4), V(1,4), 0, 0, SPECIAL(BEM)),
-_OPI(DP2ADD, DP2A, V(0,0), V(0,0), V(2,0), V(3,0), 1, 3, SPECIAL(DP2ADD)), /* for radeons */
+_OPI(DP2ADD, NOP, V(0,0), V(0,0), V(2,0), V(3,0), 1, 3, SPECIAL(DP2ADD)), /* for radeons */
_OPI(DSX,DDX, V(0,0), V(0,0), V(2,1), V(3,0), 1, 1, NULL),
_OPI(DSY,DDY, V(0,0), V(0,0), V(2,1), V(3,0), 1, 1, NULL),
_OPI(TEXLDD, TXD, V(0,0), V(0,0), V(2,1), V(3,0), 1, 4, SPECIAL(TEXLDD)),
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/3] cmake: Don't use gcc specific warnings with g++.
On 11/19/2014 11:23 AM, jfons...@vmware.com wrote:
From: José Fonseca jfons...@vmware.com
Avoids warning: command line option ‘-W...’ is valid for Ada/C/ObjC but not for
C++.
---
CMakeLists.txt | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt
index c3e217f..57a46f8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -72,16 +72,13 @@ if (CMAKE_COMPILER_IS_GNUCC)
add_definitions(
-Wall
-Wpointer-arith
- -Wstrict-prototypes
- -Wmissing-prototypes
-Wmissing-declarations
- -Wnested-externs
-fno-strict-aliasing
- -Wbad-function-cast
#-Wold-style-definition
#-Wdeclaration-after-statement
)
-endif (CMAKE_COMPILER_IS_GNUCC)
+ set (CMAKE_C_FLAGS -Wstrict-prototypes -Wmissing-prototypes
-Wnested-externs -Wbad-function-cast ${CMAKE_CXX_FLAGS})
+endif ()
if (WIN32)
# Nobody likes to include windows.h:
I still haven't seen patch 2/3 come through yet, but 13 look ok (same
comment as Roland on 3/3).
Reviewed-by: Brian Paul bri...@vmware.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Removing unused opcodes (TGSI, Mesa IR)
On Wed, Nov 19, 2014 at 3:45 PM, Jose Fonseca jfons...@vmware.com wrote: On 19/11/14 19:45, Ilia Mirkin wrote: On Wed, Nov 19, 2014 at 2:32 PM, Eric Anholt e...@anholt.net wrote: Eric Anholt e...@anholt.net writes: This series removes a bunch of unused opcodes, mostly from TGSI. It doesn't go as far as we could possibly go -- while I welcome discussion for future patch series deleting more, I hope that discussion doesn't derail the review process for these changes. I haven't messed with the subroutine stuff, since I don't know what people are planning with that. I also haven't messed with the pack/unpack opcodes in TGSI, since they might be useful for some of the GLSL packing stuff. Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe. Lots of looks good, no Reviewed-by (other than Ian on the Mesa side). I'm probably going to push soon anyway if somebody doesn't actually give Reviewed-by or NAK. st/nine got merged. It uses ARR. And at the very least references TGSI_OPCODE_NRM even if it doesn't use it. It also has code that uses CND and DP2A although it's presently disabled via #define's. The patch attached should do it. I'm pretty sure that your series will cause st/nine to fail compiling. BTW, given it's not trivial to compile nine state-tracker, I think it might be better to not obfuscate which opcodes or symbols it uses with macros. It pays off to be upfront with these things, so that `git grep foo` finds everything that should be found. Hmmm... why is it not easy to build st/nine? I haven't looked at that in detail yet, but whatever's hard about it should get fixed. [Not Eric's problem, of course.] ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/4] i965: Remove spurious casts in copy_image_with_memcpy()
On Wed 19 Nov 2014, Matt Turner wrote:
On Wed, Nov 19, 2014 at 3:35 AM, Kristian Høgsberg k...@bitplanet.net wrote:
On Tue, Nov 18, 2014 at 9:02 PM, Chad Versace
chad.vers...@linux.intel.com wrote:
If a pointer points to raw, untyped memory and is never dereferenced,
then declare it as 'void*' instead of casting it to 'void*'.
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index 341220c..cb44474 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -144,7 +144,7 @@ copy_image_with_memcpy(struct brw_context *brw,
int src_width, int src_height)
{
bool same_slice;
- uint8_t *mapped, *src_mapped, *dst_mapped;
+ void *mapped, *src_mapped, *dst_mapped;
Making these void * means that this code below:
src_mapped = mapped + ((src_y - map_y1) / src_bh) * src_stride +
((src_x - map_x1) / src_bw) * cpp;
(same for dst_mapped) becomes arithmetic on void pointers. gcc
supports that and treats it as uint8_t pointer arithmetic [1], but I'm
not aware of any official C standard that allows it. I don't think we
rely on that elsewhere
i965 relies on it elsewhere. Look at the intel_miptree_map family of
functions.
Arithmetic on void* works on gcc and clang, and i965 has relied on that
gcc support for a long time.
We have in the past, and using gcc extensions are fine. But as you
say, it's probably not what he wanted anyway.
This patch makes the code a little bit cleaner by removing a weird cast
in a function call. And it relies on no new compiler features that i965
doesn't already required. Is anything wrong with that?
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Removing unused opcodes (TGSI, Mesa IR)
The patch attached should do it. just small nitpick, please drop /* for radeons */ comment from DP2ADD, thanks Reviewed-by: David Heidelberg da...@ixit.cz ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
On 11/18/2014 09:11 PM, Chad Versace wrote:
This patch should diminish the likelihood of pointer arithmetic overflow
bugs, like the one fixed by b69c7c5dac.
Change the type of parameter 'out_stride' from int to ptrdiff_t. The
logic is that if you call intel_miptree_map() and use the value of
'out_stride', then you must be doing pointer arithmetic on 'out_ptr'.
Using ptrdiff_t instead of int should make a little bit harder to hit
overflow bugs.
So... we talked about this a little bit on Monday, and I don't think we
ever had a conclusion. What happens if you have a 32-bit application
running on a platform with 48-bit GPU address space?
As a side-effect, some function-scope variables needed to be retyped to
avoid compilation errors.
Cc: Ian Romanick i...@freedesktop.org
Cc: Kenneth Graunke kenn...@whitecape.org
Signed-off-by: Chad Versace chad.vers...@linux.intel.com
---
src/mesa/drivers/dri/i965/intel_copy_image.c | 4 ++--
src/mesa/drivers/dri/i965/intel_fbo.c | 4 ++--
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 17 ++---
src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 2 +-
src/mesa/drivers/dri/i965/intel_tex.c | 7 +--
5 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/src/mesa/drivers/dri/i965/intel_copy_image.c
b/src/mesa/drivers/dri/i965/intel_copy_image.c
index cb44474..f4c7eff 100644
--- a/src/mesa/drivers/dri/i965/intel_copy_image.c
+++ b/src/mesa/drivers/dri/i965/intel_copy_image.c
@@ -145,7 +145,7 @@ copy_image_with_memcpy(struct brw_context *brw,
{
bool same_slice;
void *mapped, *src_mapped, *dst_mapped;
- int src_stride, dst_stride, i, cpp;
+ ptrdiff_t src_stride, dst_stride, cpp;
int map_x1, map_y1, map_x2, map_y2;
GLuint src_bw, src_bh;
@@ -197,7 +197,7 @@ copy_image_with_memcpy(struct brw_context *brw,
src_width /= (int)src_bw;
src_height /= (int)src_bh;
- for (i = 0; i src_height; ++i) {
+ for (int i = 0; i src_height; ++i) {
memcpy(dst_mapped, src_mapped, src_width * cpp);
src_mapped += src_stride;
dst_mapped += dst_stride;
diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c
b/src/mesa/drivers/dri/i965/intel_fbo.c
index 4a03b57..96e7b9e 100644
--- a/src/mesa/drivers/dri/i965/intel_fbo.c
+++ b/src/mesa/drivers/dri/i965/intel_fbo.c
@@ -127,7 +127,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
struct intel_renderbuffer *irb = intel_renderbuffer(rb);
struct intel_mipmap_tree *mt;
void *map;
- int stride;
+ ptrdiff_t stride;
if (srb-Buffer) {
/* this is a malloc'd renderbuffer (accum buffer), not an irb */
@@ -189,7 +189,7 @@ intel_map_renderbuffer(struct gl_context *ctx,
stride = -stride;
}
- DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%d\n,
+ DBG(%s: rb %d (%s) mt mapped: (%d, %d) (%dx%d) - %p/%PRIdPTR\n,
__FUNCTION__, rb-Name, _mesa_get_format_name(rb-Format),
x, y, w, h, map, stride);
diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
index 7081f1d..f815fbe 100644
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
@@ -,7 +,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
int height)
{
void *src, *dst;
- int src_stride, dst_stride;
+ ptrdiff_t src_stride, dst_stride;
int cpp = dst_mt-cpp;
intel_miptree_map(brw, src_mt,
@@ -1129,7 +1129,7 @@ intel_miptree_copy_slice_sw(struct brw_context *brw,
BRW_MAP_DIRECT_BIT,
dst, dst_stride);
- DBG(sw blit %s mt %p %p/%d - %s mt %p %p/%d (%dx%d)\n,
+ DBG(sw blit %s mt %p %p/%PRIdPTR - %s mt %p %p/%PRIdPTR (%dx%d)\n,
_mesa_get_format_name(src_mt-format),
src_mt, src, src_stride,
_mesa_get_format_name(dst_mt-format),
@@ -2259,6 +2259,17 @@ can_blit_slice(struct intel_mipmap_tree *mt,
return true;
}
+/**
+ * Parameter \a out_stride has type ptrdiff_t not because the buffer stride
may
+ * exceed 32 bits but to diminish the likelihood subtle bugs in pointer
+ * arithmetic overflow.
+ *
+ * If you call this function and use \a out_stride, then you're doing pointer
+ * arithmetic on \a out_ptr. The type of \a out_stride doesn't prevent all
+ * bugs. The caller must still take care to avoid 32-bit overflow errors in
+ * all arithmetic expressions that contain buffer offsets and pixel sizes,
+ * which usually have type uint32_t or GLuint.
+ */
void
intel_miptree_map(struct brw_context *brw,
struct intel_mipmap_tree *mt,
@@ -2270,7 +2281,7 @@ intel_miptree_map(struct brw_context *brw,
unsigned int h,
GLbitfield mode,
void **out_ptr,
- int *out_stride)
+ ptrdiff_t *out_stride)
{
struct
[Mesa-dev] [Bug 79039] [TRACKER] Mesa 10.2 release tracker
https://bugs.freedesktop.org/show_bug.cgi?id=79039 Bug 79039 depends on bug 78770, which changed state. Bug 78770 Summary: [SNB bisected]Webglc conformance/textures/texture-size-limit.html fails https://bugs.freedesktop.org/show_bug.cgi?id=78770 What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 86480] Mesa 10.4.0-devel implementation error: unexpected format GL_DEPTH24_STENCIL8 in _mesa_choose_tex_format()
https://bugs.freedesktop.org/show_bug.cgi?id=86480 Bug ID: 86480 Summary: Mesa 10.4.0-devel implementation error: unexpected format GL_DEPTH24_STENCIL8 in _mesa_choose_tex_format() Product: Mesa Version: unspecified Hardware: x86 (IA32) OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: Other Assignee: mesa-dev@lists.freedesktop.org Reporter: bigtall...@gmail.com Trying to get Starcraft 1 running on Lubuntu, using Wine 1.7.31, I get this error in the console: Mesa 10.4.0-devel implementation error: unexpected format GL_DEPTH24_STENCIL8 in _mesa_choose_tex_format() The screen goes black, sometimes with dark blue on a diagonal half. The sound does seem to be working fine though. Renderer: Mesa DRI Intel(R) 852GM/855GM x86/MMX/SSE2 Version: 1.3 Mesa 10.4.0-devel -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 86480] Mesa 10.4.0-devel implementation error: unexpected format GL_DEPTH24_STENCIL8 in _mesa_choose_tex_format()
https://bugs.freedesktop.org/show_bug.cgi?id=86480 bigtall...@gmail.com changed: What|Removed |Added See Also||https://bugs.freedesktop.or ||g/show_bug.cgi?id=79841 -- You are receiving this mail because: You are the assignee for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
On Wednesday, November 19, 2014 02:13:03 PM Ian Romanick wrote: On 11/18/2014 09:11 PM, Chad Versace wrote: This patch should diminish the likelihood of pointer arithmetic overflow bugs, like the one fixed by b69c7c5dac. Change the type of parameter 'out_stride' from int to ptrdiff_t. The logic is that if you call intel_miptree_map() and use the value of 'out_stride', then you must be doing pointer arithmetic on 'out_ptr'. Using ptrdiff_t instead of int should make a little bit harder to hit overflow bugs. So... we talked about this a little bit on Monday, and I don't think we ever had a conclusion. What happens if you have a 32-bit application running on a platform with 48-bit GPU address space? CC'ing Ben, who knows all the gory details. I don't really understand the problem - the GPU uses 48-bit addressing, and can access more than 4G...but we're talking about map, which makes a buffer available in an application's virtual address space...which is 32-bit in your example. It should always be placed at a 4GB virtual address and work out fine. That said, I don't think the kernel ever uses = 4GB address spaces today. Ben wrote 4GGGTT support and had both kernel and userspace patches to make it work, but I don't think it ever actually landed. I'm pretty sure shipping userspace is not quite 48-bit safe - there are a few buffers that have to be placed 4GB (some hardware packets only take 32-bit addresses still), and I don't think any software is in place to make that happen. --Ken 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] i965: Pushing UBO values, NIR, etc.
Hello, I've been wondering about the best approach to handle UBO values as push constants. Right now, we always treat them as pull constants, which means sending messages and hitting memory (or at least the sampler cache) - it's quite inefficient. 3DSTATE_CONSTANT_* can read a contiguous block of constant data from up to four arbitrary buffer objects. They do not have to be relative to dynamic state base address; you simply have to set a bit in INSTPM and use relocs. I have that working in a branch today: http://cgit.freedesktop.org/~kwg/mesa/commit/?h=ubopushid=1e2edd311846bfed6ebc1a775854c95c2efa4467 http://cgit.freedesktop.org/~kwg/mesa/commit/?h=ubopushid=65a2fb0f4b1065bc1f40a2e4b577bd61980ef829 Our current GLSL IR backend's approach to uniforms makes this really painful, though. Regular uniforms show up in the i965 backend as if they were push constants (UNIFORM registers), and are demoted to pull constants in some circumstances. In contrast, UBOs show up as pulls (ir_binop_ubo_load), and we'd like to turn them into push constants - the exact opposite! Converting both ways would be crazy, and the separate representations make it hard to decide how many (and which) regular uniforms and UBO values to push. Ideally, we could decide how many to push based on estimated register pressure, and decide which based on usage (if they're used near the end of the shader, pushing would create a huge live range), and taking into account the constraint that we can only push contiguous blocks (in the UBO case - regular uniforms are always packed already). My hope is that with the move to NIR, we can represent incoming uniform values in a consistent manner. My gut feeling is that we should treat all uniform values as memory loads. We'd do the above partitioning into push/pull constants, then lower the memory loads. For pull constants, the uniform load operation would become an LD message. For push constants, the uniform load would simply turn into a MOV from a payload register, which could be optimized away by copy propagation or register coalescing. Being able to push UBO values could dramatically improve Witcher 2 performance, which is currently completely unplayable even on Iris Pro. --Ken 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] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
On Wed, Nov 19, 2014 at 09:18:54PM -0800, Kenneth Graunke wrote: On Wednesday, November 19, 2014 02:13:03 PM Ian Romanick wrote: On 11/18/2014 09:11 PM, Chad Versace wrote: This patch should diminish the likelihood of pointer arithmetic overflow bugs, like the one fixed by b69c7c5dac. Change the type of parameter 'out_stride' from int to ptrdiff_t. The logic is that if you call intel_miptree_map() and use the value of 'out_stride', then you must be doing pointer arithmetic on 'out_ptr'. Using ptrdiff_t instead of int should make a little bit harder to hit overflow bugs. So... we talked about this a little bit on Monday, and I don't think we ever had a conclusion. What happens if you have a 32-bit application running on a platform with 48-bit GPU address space? CC'ing Ben, who knows all the gory details. I don't really understand the problem - the GPU uses 48-bit addressing, and can access more than 4G...but we're talking about map, which makes a buffer available in an application's virtual address space...which is 32-bit in your example. It should always be placed at a 4GB virtual address and work out fine. That's correct. Behind the scenes you very well may be getting 48b addresses, but like Ken said, that should present no issue assuming you can still reprsent a 64b data type. One would hope that whoever actually lands the patches provides a way on context create to only get a 4GB address space, since you'll save some memory, and make correctness a lot easier to prove. For the SVM case, there are two models. 1. Create a buffer on the CPU, and just use it at the same offset in the GPU - no problem. 2. Create a buffer with GEM on the GPU, and just use it on the CPU - This case, which doesn't make a whole lot of sense IMO, would take some work. That said, I don't think the kernel ever uses = 4GB address spaces today. Ben wrote 4GGGTT support and had both kernel and userspace patches to make it work, but I don't think it ever actually landed. I'm pretty sure shipping userspace is not quite 48-bit safe - there are a few buffers that have to be placed 4GB (some hardware packets only take 32-bit addresses still), and I don't think any software is in place to make that happen. 4GGGTT landed (I haven't checked if it's been reverted). WRT to unsafe userspace, it's probably mostly safe after your patch to fix libdrm. Unless you start getting into the prelocation stuff, where I'd bet there are at least a few unsafe assumptions (unless my patches or equivalent were merged, the kernel isn't even 48b safe). I also think we determined the 0 state base thing won't work, which means the odds of actually needing more than 32b are quite small. --Ken ___ 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
Re: [Mesa-dev] [PATCH 2/4] i965: Fix intel_miptree_map() signature to be more 64-bit safe
On Wed, Nov 19, 2014 at 10:40:56PM -0800, Ben Widawsky wrote: On Wed, Nov 19, 2014 at 09:18:54PM -0800, Kenneth Graunke wrote: On Wednesday, November 19, 2014 02:13:03 PM Ian Romanick wrote: On 11/18/2014 09:11 PM, Chad Versace wrote: This patch should diminish the likelihood of pointer arithmetic overflow bugs, like the one fixed by b69c7c5dac. Change the type of parameter 'out_stride' from int to ptrdiff_t. The logic is that if you call intel_miptree_map() and use the value of 'out_stride', then you must be doing pointer arithmetic on 'out_ptr'. Using ptrdiff_t instead of int should make a little bit harder to hit overflow bugs. So... we talked about this a little bit on Monday, and I don't think we ever had a conclusion. What happens if you have a 32-bit application running on a platform with 48-bit GPU address space? CC'ing Ben, who knows all the gory details. I don't really understand the problem - the GPU uses 48-bit addressing, and can access more than 4G...but we're talking about map, which makes a buffer available in an application's virtual address space...which is 32-bit in your example. It should always be placed at a 4GB virtual address and work out fine. That's correct. Behind the scenes you very well may be getting 48b addresses, but like Ken said, that should present no issue assuming you can still reprsent a 64b data type. One would hope that whoever actually lands the patches provides a way on context create to only get a 4GB address space, since you'll save some memory, and make correctness a lot easier to prove. For the SVM case, there are two models. 1. Create a buffer on the CPU, and just use it at the same offset in the GPU - no problem. 2. Create a buffer with GEM on the GPU, and just use it on the CPU - This case, which doesn't make a whole lot of sense IMO, would take some work. That said, I don't think the kernel ever uses = 4GB address spaces today. Ben wrote 4GGGTT support and had both kernel and userspace patches to make it work, but I don't think it ever actually landed. I'm pretty sure shipping userspace is not quite 48-bit safe - there are a few buffers that have to be placed 4GB (some hardware packets only take 32-bit addresses still), and I don't think any software is in place to make that happen. 4GGGTT landed (I haven't checked if it's been reverted). WRT to unsafe userspace, it's probably mostly safe after your patch to fix libdrm. Unless you start getting into the prelocation stuff, where I'd bet there are at least a few unsafe assumptions (unless my patches or equivalent were merged, the kernel isn't even 48b safe). I also think we determined the 0 state base thing won't work, which means the odds of actually needing more than 32b are quite small. --Ken My memory unfailed. 4GGGTT is disabled for 32b platforms because of aforementioned unreadiness in the kernel at that time. -- Ben Widawsky, Intel Open Source Technology Center ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] Using the 'f' suffix to create a float from an integer literal
On Wed, 2014-11-19 at 10:27 -0800, Ian Romanick wrote: On 11/19/2014 03:47 AM, Iago Toral Quiroga wrote: Hi, I came across a GLSL test that checks that doing something like this in a shader should fail: Is this one of the dEQP tests? Yes. float value = 1f; It seems like we have a test related to this in piglit somewhere... it looks like tests/shaders/glsl-floating-constant-120.shader_test uses that syntax, but it's not explicitly testing that feature. However, this works fine in Mesa. Checking the spec I see: Floating-point constants are defined as follows. floating-constant: fractional-constant exponent-part(opt) floating-suffix(opt) digit-sequence exponent-part floating-suffix(opt) fractional-constant: digit-sequence . digit-sequence digit-sequence . . digit-sequence exponent-part: e sign(opt) digit-sequence E sign(opt) digit-sequence sign: one of + - digit-sequence: digit digit-sequence digit floating-suffix: one of f F which suggests that the test is correct and Mesa has a bug. According to the above rules, however, something like this is fine: float f = 1e2f; which I find kind of weird if the other case is not valid, so I wonder if there is a bug in the spec or this is on purpose for some reason that I am missing. Then, to make matters worse, I read this in opengl.org wiki [1]: A numeric literal that uses a decimal is by default of type float. To create a float literal from an integer value, use the suffix f or F as in C/C++. which contradicts the spec and the test and is aligned with the current way Mesa works. So: does anyone know what version is right? Could this be a bug in the spec? and if it is not, do we want to change the behavior to follow the spec as it is now? The $64,000 question: What do other GLSL compilers (including, perhaps, glslang) do? This seems like one of the cases where nobody is likely to follow the spec, and some application will depend on that. If that's the case, I'll submit a spec bug. Good point. I'll try to check a few cases and reply here. Thanks! Iago Thanks, Iago [1] https://www.opengl.org/wiki/Data_Type_%28GLSL%29 ___ 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] Removing unused opcodes (TGSI, Mesa IR)
Does anyone remember if there was a reason that the TGSI_OPCODE_ tokens are #defines instead of an enumeration? I think it was because enums are not allowed in struct bit fields, furthermore C++ will complain about enum-int conversion without cast. (IIRC this is the reason many things in src/gallium/include/pipe/p_defines.h were defines.) That said, I think we should revisit this. At for TGSI opcodes, relatively few lines of code access packed tgsi tokens directly. And enums look nicer when debugging. Jose From: mesa-dev mesa-dev-boun...@lists.freedesktop.org on behalf of Brian Paul bri...@vmware.com Sent: 13 November 2014 16:27 To: Eric Anholt; mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] Removing unused opcodes (TGSI, Mesa IR) On 11/12/2014 06:18 PM, Eric Anholt wrote: This series removes a bunch of unused opcodes, mostly from TGSI. It doesn't go as far as we could possibly go -- while I welcome discussion for future patch series deleting more, I hope that discussion doesn't derail the review process for these changes. I haven't messed with the subroutine stuff, since I don't know what people are planning with that. I also haven't messed with the pack/unpack opcodes in TGSI, since they might be useful for some of the GLSL packing stuff. Testing status: compile-tested ilo/r600/softpipe, touch-tested softpipe. Except for what Jose said, this looks fine to me. Does anyone remember if there was a reason that the TGSI_OPCODE_ tokens are #defines instead of an enumeration? -Brian ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddevd=AAIGaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=MUbS190yOjHL36ha_AaaQFM1__dfKUWsBqH34fL_9GUs=c06VM0QMqrPh3u4ifx1opmH41ktcGp-6VAhIkN8gkz0e= ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 14/20] mesa: Add non-normalized formats support for ubyte packing functions
On Tue, 2014-11-18 at 11:08 -0800, Jason Ekstrand wrote:
On Tue, Nov 18, 2014 at 12:44 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
From: Samuel Iglesias Gonsalvez sigles...@igalia.com
Signed-off-by: Samuel Iglesias Gonsalvez
sigles...@igalia.com
---
src/mesa/main/format_pack.c.mako | 10 +-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/src/mesa/main/format_pack.c.mako
b/src/mesa/main/format_pack.c.mako
index b9f4656..97adf6e 100644
--- a/src/mesa/main/format_pack.c.mako
+++ b/src/mesa/main/format_pack.c.mako
@@ -84,7 +84,15 @@ pack_ubyte_${f.short_name()}(const GLubyte
src[4], void *dst)
%endif
${channel_datatype(c)} ${c.name} =
- %if c.type == parser.UNSIGNED:
+ %if not f.is_normalized():
+ %if c.type == parser.FLOAT and c.size == 32:
+UBYTE_TO_FLOAT(src[${i}]);
+ %elif c.type == parser.FLOAT and c.size == 16:
+_mesa_float_to_half(UBYTE_TO_FLOAT(src[${i}]));
Same question here as in the previous patch. Why are we using
UBYTE_TO_FLOAT?
This is what current format_pack.c is doing for those formats and some
piglit tests complain if it is not there.
Sam
+ %else:
+(${channel_datatype(c)}) src[${i}];
+ %endif
+ %elif c.type == parser.UNSIGNED:
%if f.colorspace == 'srgb' and c.name in 'rgb':
util_format_linear_to_srgb_8unorm(src[${i}]);
%else:
--
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
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] [PATCH 13/20] mesa: Add _mesa_pack_uint_rgba_row() format conversion function
On Tue, 2014-11-18 at 11:05 -0800, Jason Ekstrand wrote:
On Tue, Nov 18, 2014 at 12:44 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
From: Samuel Iglesias Gonsalvez sigles...@igalia.com
We will use this later on to handle uint conversion scenarios
in a master
convert function.
Signed-off-by: Samuel Iglesias Gonsalvez
sigles...@igalia.com
---
src/mesa/main/format_pack.c.mako | 88
src/mesa/main/format_pack.h | 3 ++
2 files changed, 91 insertions(+)
diff --git a/src/mesa/main/format_pack.c.mako
b/src/mesa/main/format_pack.c.mako
index 13a20c1..b9f4656 100644
--- a/src/mesa/main/format_pack.c.mako
+++ b/src/mesa/main/format_pack.c.mako
@@ -150,6 +150,62 @@ pack_ubyte_r11g11b10_float(const GLubyte
src[4], void *dst)
*d = float3_to_r11g11b10f(rgb);
}
+/* uint packing functions */
+
+%for f in rgb_formats:
+ %if not f.is_int():
+ % continue %
+ %elif f.is_normalized():
+ % continue %
+ %elif f.is_compressed():
+ % continue %
+ %endif
+
+static inline void
+pack_uint_${f.short_name()}(const GLuint src[4], void *dst)
+{
+ %for (i, c) in enumerate(f.channels):
+ % i = f.swizzle.inverse()[i] %
+ %if c.type == 'x':
+ % continue %
+ %endif
+
+ ${channel_datatype(c)} ${c.name} =
+ %if not f.is_normalized():
This if isn't needed. If you want the assertion, an assert not
f.is_normalized would be ok, but I don't see the need given the
continues above.
You are right, this conditional is not needed. I'm going to remove it
for the second version of the series.
+ %if c.type == parser.FLOAT and c.size == 32:
+UINT_TO_FLOAT(src[${i}]);
Why are we doing UINT_TO_FLOAT here? If these are for non-normalied
functions, shouldn't we just be clampping the floating-point value to
the maximum range for the integer?
This is a mistake. I'm going to remove it.
Thanks,
Sam
+ %elif c.type == parser.FLOAT and c.size == 16:
+_mesa_float_to_half(UINT_TO_FLOAT(src[${i}]));
+ %else:
+(${channel_datatype(c)}) src[${i}];
+ %endif
+ %else:
+ % assert False %
+ %endif
+ %endfor
+
+ %if f.layout == parser.ARRAY:
+ ${format_datatype(f)} *d = (${format_datatype(f)}
*)dst;
+ %for (i, c) in enumerate(f.channels):
+ %if c.type == 'x':
+% continue %
+ %endif
+ d[${i}] = ${c.name};
+ %endfor
+ %elif f.layout == parser.PACKED:
+ ${format_datatype(f)} d = 0;
+ %for (i, c) in enumerate(f.channels):
+ %if c.type == 'x':
+% continue %
+ %endif
+ d |= PACK(${c.name}, ${c.shift}, ${c.size});
+ %endfor
+ (*(${format_datatype(f)} *)dst) = d;
+ %else:
+ % assert False %
+ %endif
+}
+%endfor
/* float packing functions */
@@ -298,6 +354,38 @@ _mesa_pack_ubyte_rgba_row(mesa_format
format, GLuint n,
}
/**
+ * Pack a row of GLuint rgba[4] values to the destination.
+ */
+void
+_mesa_pack_uint_rgba_row(mesa_format format, GLuint n,
+ const GLuint src[][4], void *dst)
+{
+ GLuint i;
+ GLubyte *d = dst;
+
+ switch (format) {
+%for f in rgb_formats:
+ %if not f.is_int():
+ % continue %
+ %elif f.is_normalized():
+ % continue %
+ %elif f.is_compressed():
+ % continue %
+ %endif
+
+ case ${f.name}:
+ for (i = 0; i n; ++i) {
+ pack_uint_${f.short_name()}(src[i], d);
+ d += ${f.block_size() / 8};
+ }
+ break;
+%endfor
+ default:
+ assert(!Invalid format);
+ }
+}
+
+/**
* Pack a row of GLfloat rgba[4] values to the destination.
*/
void
diff --git a/src/mesa/main/format_pack.h
b/src/mesa/main/format_pack.h
index 2577def..1582ad1 100644
--- a/src/mesa/main/format_pack.h
+++
Re: [Mesa-dev] [PATCH 06/20] mesa: Fix _mesa_swizzle_and_convert integer conversions to clamp properly
On Tue, 2014-11-18 at 10:34 -0800, Jason Ekstrand wrote:
In general, I like this patch. However, if we are going to claim to
follow the GL rule of Colors are clampped to their representable
range, then there still seem to be quite a few cases missing. For
instance, when going from signed to unsigned 8-bit formats, we should
should take a min with 0 and when converting from unsigned to signed,
we should take a max with 0x7f. Maybe we need to add integer helper
functions like we have for the normalized ones and use those just to
make sure we don't miss anything.
OK, I'll do that for the second version.
Thanks!
Sam
--Jason
On Tue, Nov 18, 2014 at 12:43 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
From: Samuel Iglesias Gonsalvez sigles...@igalia.com
Fix various conversion paths that involved integer data types
of different
sizes (uint16_t to uint8_t, int16_t to uint8_t, etc) that were
not
being clamped properly.
Also, one of the paths was incorrectly assigning the value 12,
instead of 1,
to the constant one.
Signed-off-by: Samuel Iglesias Gonsalvez
sigles...@igalia.com
---
src/mesa/main/format_utils.c | 33
+
1 file changed, 17 insertions(+), 16 deletions(-)
diff --git a/src/mesa/main/format_utils.c
b/src/mesa/main/format_utils.c
index b6d0fbc..8040173 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -24,6 +24,7 @@
#include format_utils.h
#include glformats.h
+#include macros.h
static const uint8_t map_identity[7] = { 0, 1, 2, 3, 4, 5,
6 };
static const uint8_t map_3210[7] = { 3, 2, 1, 0, 4, 5, 6 };
@@ -593,28 +594,28 @@ convert_ubyte(void *void_dst, int
num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(uint8_t, uint16_t,
unorm_to_unorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, uint16_t, src);
+ SWIZZLE_CONVERT(uint8_t, uint16_t, MIN2(src, 0xff));
}
break;
case GL_SHORT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, int16_t,
snorm_to_unorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, int16_t, (src 0) ? 0 :
src);
+ SWIZZLE_CONVERT(uint8_t, int16_t, CLAMP(src, 0,
0xff));
}
break;
case GL_UNSIGNED_INT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, uint32_t,
unorm_to_unorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, uint32_t, src);
+ SWIZZLE_CONVERT(uint8_t, uint32_t, MIN2(src, 0xff));
}
break;
case GL_INT:
if (normalized) {
SWIZZLE_CONVERT(uint8_t, int32_t,
snorm_to_unorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(uint8_t, int32_t, (src 0) ? 0 :
src);
+ SWIZZLE_CONVERT(uint8_t, int32_t, CLAMP(src, 0,
0xff));
}
break;
default:
@@ -649,7 +650,7 @@ convert_byte(void *void_dst, int
num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint8_t, unorm_to_snorm(src,
8, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint8_t, src);
+ SWIZZLE_CONVERT(int8_t, uint8_t, MIN2(src, 0x7f));
}
break;
case GL_BYTE:
@@ -659,28 +660,28 @@ convert_byte(void *void_dst, int
num_dst_channels,
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint16_t,
unorm_to_snorm(src, 16, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint16_t, src);
+ SWIZZLE_CONVERT(int8_t, uint16_t, MIN2(src, 0x7f));
}
break;
case GL_SHORT:
if (normalized) {
SWIZZLE_CONVERT(int8_t, int16_t, snorm_to_snorm(src,
16, 8));
} else {
- SWIZZLE_CONVERT(int8_t, int16_t, src);
+ SWIZZLE_CONVERT(int8_t, int16_t, CLAMP(src, -0x80,
0x7f));
}
break;
case GL_UNSIGNED_INT:
if (normalized) {
SWIZZLE_CONVERT(int8_t, uint32_t,
unorm_to_snorm(src, 32, 8));
} else {
- SWIZZLE_CONVERT(int8_t, uint32_t, src);
+ SWIZZLE_CONVERT(int8_t, uint32_t,
Re: [Mesa-dev] [PATCH 00/20] Auto-generate pack/unpack functions
On Wed, 2014-11-19 at 01:15 +, Emil Velikov wrote: Hi Iago, On 18/11/14 08:43, Iago Toral Quiroga wrote: [snip] Summary of the patches: * Patches 1-7 are general fixes to the current code that were found while working on this. Have you noticed if any of those fixes resolves a piglit and/or a real world application ? If so it might be worth tagging them for the stable branches :) OK, we will review these fixes and tag them for the stable branch accordingly. I've noticed with this series we require mako in order to build mesa. Please update the documentation to reflect the new dependency. How gracefully do we handle the cases when it's missing ? Printing a message similar to mako vXX or later is required would be very helpful. Yeah, good idea. I am not an expert in autoconf tools but let's see if I can add some check like that. Thanks, Sam Thanks Emil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev 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] Using the 'f' suffix to create a float from an integer literal
The $64,000 question: What do other GLSL compilers (including, perhaps,glslang) do? :) Shouldn't this be the $1e6f question? Jose From: mesa-dev mesa-dev-boun...@lists.freedesktop.org on behalf of Iago Toral ito...@igalia.com Sent: 20 November 2014 07:08 To: Ian Romanick Cc: mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] Using the 'f' suffix to create a float from an integer literal On Wed, 2014-11-19 at 10:27 -0800, Ian Romanick wrote: On 11/19/2014 03:47 AM, Iago Toral Quiroga wrote: Hi, I came across a GLSL test that checks that doing something like this in a shader should fail: Is this one of the dEQP tests? Yes. float value = 1f; It seems like we have a test related to this in piglit somewhere... it looks like tests/shaders/glsl-floating-constant-120.shader_test uses that syntax, but it's not explicitly testing that feature. However, this works fine in Mesa. Checking the spec I see: Floating-point constants are defined as follows. floating-constant: fractional-constant exponent-part(opt) floating-suffix(opt) digit-sequence exponent-part floating-suffix(opt) fractional-constant: digit-sequence . digit-sequence digit-sequence . . digit-sequence exponent-part: e sign(opt) digit-sequence E sign(opt) digit-sequence sign: one of + - digit-sequence: digit digit-sequence digit floating-suffix: one of f F which suggests that the test is correct and Mesa has a bug. According to the above rules, however, something like this is fine: float f = 1e2f; which I find kind of weird if the other case is not valid, so I wonder if there is a bug in the spec or this is on purpose for some reason that I am missing. Then, to make matters worse, I read this in opengl.org wiki [1]: A numeric literal that uses a decimal is by default of type float. To create a float literal from an integer value, use the suffix f or F as in C/C++. which contradicts the spec and the test and is aligned with the current way Mesa works. So: does anyone know what version is right? Could this be a bug in the spec? and if it is not, do we want to change the behavior to follow the spec as it is now? The $64,000 question: What do other GLSL compilers (including, perhaps, glslang) do? This seems like one of the cases where nobody is likely to follow the spec, and some application will depend on that. If that's the case, I'll submit a spec bug. Good point. I'll try to check a few cases and reply here. Thanks! Iago Thanks, Iago [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.opengl.org_wiki_Data-5FType-5F-28GLSL-29d=AAIGaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=-vNxCjRApDMGfEN7OH0gdQY45tX4GCnJezazsrd2DNgs=v-iOTBhqyw5b2dul6DNiwG3s4jGuIujxKjaxWWGRqc8e= ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddevd=AAIGaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=-vNxCjRApDMGfEN7OH0gdQY45tX4GCnJezazsrd2DNgs=YtsceeKLkQZUT8ymwqfviOL4cuiZKwMSPLqvO44zptEe= ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddevd=AAIGaQc=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEsr=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzEm=-vNxCjRApDMGfEN7OH0gdQY45tX4GCnJezazsrd2DNgs=YtsceeKLkQZUT8ymwqfviOL4cuiZKwMSPLqvO44zptEe= ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 02/29] mesa: Set normalized=true for float array formats.
Hi Jason,
we discussed this some weeks ago actually, the detailed explanation is
here:
https://bugs.freedesktop.org/show_bug.cgi?id=84566#c5
the short answer is that this is necessary because there is a normalized
parameter to _mesa_swizzle_and_convert, and when we deal with float
types we want to set this to true.
Iago
On Wed, 2014-11-19 at 11:31 -0800, Jason Ekstrand wrote:
I'm not sure what I think about this. Does it make a functional
change other than consistancy?
--Jason
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
In order to check if a format is normalized Mesa does
something like this:
normalized = !_mesa_is_enum_format_integer(srcFormat);
So all float types will set normalized to true. Since our
mesa_array_format
includes a normalized flag for each type we want to make it
consistent with
this.
---
src/mesa/main/format_info.py | 3 ++-
src/mesa/main/format_utils.c | 2 +-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/src/mesa/main/format_info.py
b/src/mesa/main/format_info.py
index 315767d..d4bc276 100644
--- a/src/mesa/main/format_info.py
+++ b/src/mesa/main/format_info.py
@@ -220,9 +220,10 @@ for fmat in formats:
print ' {{ {0} }},'.format(', '.join(map(str,
fmat.swizzle)))
if fmat.is_array() and fmat.colorspace in ('rgb', 'srgb'):
chan = fmat.array_element()
+ norm = chan.norm or chan.type == parser.FLOAT
print ' {0} ,'.format(', '.join([
get_array_format_datatype(chan),
- str(int(chan.norm)),
+ str(int(norm)),
str(len(fmat.channels)),
str(fmat.swizzle[0]),
str(fmat.swizzle[1]),
diff --git a/src/mesa/main/format_utils.c
b/src/mesa/main/format_utils.c
index c3815cb..1d65f2b 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -30,7 +30,7 @@
mesa_array_format RGBA_FLOAT = {{
MESA_ARRAY_FORMAT_TYPE_FLOAT,
- 0,
+ 1,
4,
0, 1, 2, 3,
0, 1
--
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 05/29] mesa: Consider internal base format in _mesa_format_convert
On Wed, 2014-11-19 at 11:43 -0800, Jason Ekstrand wrote:
A couple of specific comments are below. More generally, why are you
only considering the base format on two cases? Do we never use it for
anything else?
I thought about that too but when I looked at the original code it
seemed that it only cared for the base format in these two scenarios, so
I thought that maybe the conversions cases that could be affected are
all handled in those two paths. I'll check again though, just in case I
missed something.
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
Add a dst_internal_format parameter to _mesa_format_convert,
that represents
the base internal format for texture/pixel uploads, so we can
do the right
thing when the driver has selected a different internal format
for the target
dst format.
---
src/mesa/main/format_utils.c | 65
+++-
src/mesa/main/format_utils.h | 2 +-
2 files changed, 65 insertions(+), 2 deletions(-)
diff --git a/src/mesa/main/format_utils.c
b/src/mesa/main/format_utils.c
index fc59e86..5964689 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -303,7 +303,7 @@ _mesa_compute_component_mapping(GLenum
inFormat, GLenum outFormat, GLubyte *map)
void
_mesa_format_convert(void *void_dst, uint32_t dst_format,
size_t dst_stride,
void *void_src, uint32_t src_format,
size_t src_stride,
- size_t width, size_t height)
+ size_t width, size_t height, GLenum
dst_internal_format)
{
uint8_t *dst = (uint8_t *)void_dst;
uint8_t *src = (uint8_t *)void_src;
@@ -422,6 +422,36 @@ _mesa_format_convert(void *void_dst,
uint32_t dst_format, size_t dst_stride,
if (src_array_format.as_uint dst_array_format.as_uint)
{
assert(src_array_format.normalized ==
dst_array_format.normalized);
+ /* If the base format of our dst is not the same as the
provided base
+ * format it means that we are converting to a
different format
+ * than the one originally requested by the client.
This can happen when
+ * the internal base format requested is not supported
by the driver.
+ * In this case we need to consider the requested
internal base format to
+ * compute the correct swizzle operation from src to
dst. We will do this
+ * by computing the swizzle transform
src-rgba-base-rgba-dst instead
+ * of src-rgba-dst.
+ */
+ mesa_format dst_mesa_format;
+ if (dst_format MESA_ARRAY_FORMAT_BIT)
+ dst_mesa_format =
_mesa_format_from_array_format(dst_format);
+ else
+ dst_mesa_format = dst_format;
Let's put an extra line here so it doesn't get confused with the below
if statement
+ if (dst_internal_format !=
_mesa_get_format_base_format(dst_mesa_format)) {
+ /* Compute src2rgba as src-rgba-base-rgba */
+ uint8_t rgba2base[4], base2rgba[4], swizzle[4];
+ _mesa_compute_component_mapping(GL_RGBA,
dst_internal_format, rgba2base);
+ _mesa_compute_component_mapping(dst_internal_format,
GL_RGBA, base2rgba);
+
+ for (i = 0; i 4; i++) {
+if (base2rgba[i] MESA_FORMAT_SWIZZLE_W)
+ swizzle[i] = base2rgba[i];
+else
+ swizzle[i] =
src2rgba[rgba2base[base2rgba[i]]];
This doesn't work for composing three swizzles. If you get a ZERO or
ONE in rgba2base, you'll read the wrong memory from src2rgba.
Actually, the problem is worse, because the mapping written by
_mesa_compute_component_mapping is a 6-component mapping and we are
passing a 4-component array. I'll fix this.
+ }
+ memcpy(src2rgba, swizzle, sizeof(src2rgba));
+ }
+
+ /* Compute src2dst from src2rgba */
for (i = 0; i 4; i++) {
if (rgba2dst[i] MESA_FORMAT_SWIZZLE_W) {
src2dst[i] = rgba2dst[i];
@@ -539,9 +569,42 @@ _mesa_format_convert(void *void_dst,
uint32_t dst_format, size_t dst_stride,
src += src_stride;
}
} else {
+ /* For some conversions, doing src-rgba-dst is not
enough and we
+ * need to
Re: [Mesa-dev] [PATCH 06/29] mesa: Avoid pack/unpack fast paths if base internal format != base format
On Wed, 2014-11-19 at 11:57 -0800, Jason Ekstrand wrote:
A couple of general comments on this patch:
1) The prerequisites should be moved to before the first patch in the
series and it should be squashed into the patch that introduces the
function. There are one or two more patches which also modify it and
those should also be squashed in.
Ok.
2) I wonder if giving _mesa_format_convert an internal swizzle
wouldn't be better than a destination internal format. There are a
couple of reasons for this:
a) It's more general. If it doesn't cost us anything extra to do
it that way, we might as well.
I think that would only work directly for conversions between array
formats, but what if we have, for example, a texture upload from RGB565
to a Luminance format (so that we get an RGBA base format)? that would
not go though _mesa_swizzle_and_convert and would require to unpack to
RGBA and then pack to the dst... and unless the client has provided the
dst format as an array format that won't use _mesa_swizzle_and_convert
either. That should not be a problem when the calls to
_mesa_format_convert come from things like glTexImage or glReadPixels,
since in these cases the compute the dst format from a GL type and if it
is an array format we should get that, but in other cases that might not
be the case...
b) It removes the GL enum dependency from the _mesa_format_convert
c) I think this implementation misses the case where we download a
texture that is storred in a different format than its base format.
For instance, if you are downloading a GL_RGB texture as GL_RGBA but
it's storred as GL_RGBA. I think with the current implementaion,
you'll get the junk in the alpha component of the texture's backing
storage instead of a constant alpha value of 1.
That's correct. In the implementation of readpixels and getteximage we
had to rebase the results in some cases to account for that.
Iago
On Tue, Nov 18, 2014 at 1:23 AM, Iago Toral Quiroga
ito...@igalia.com wrote:
In general, if the dst base internal format and the selected
dst format are
different we can't be sure that directly packing or unpacking
to the destination
format will produce correct results. In these cases we
probably want to fall
back to other paths (like mesa_swizzle_and_convert) that can
handle these
situations better.
An example of this includes a luminance internal format for
which the driver
decided to use something like BGRA. In these case, unpacking
to BGRA won't
match the requirements of the luminance internal format.
In the future we may want to allow these fast paths for
specific cases
where we know the direct pack/unpack functions will do the
right thing.
---
src/mesa/main/format_utils.c | 137
+++
1 file changed, 72 insertions(+), 65 deletions(-)
diff --git a/src/mesa/main/format_utils.c
b/src/mesa/main/format_utils.c
index 5964689..34c90d9 100644
--- a/src/mesa/main/format_utils.c
+++ b/src/mesa/main/format_utils.c
@@ -331,65 +331,82 @@ _mesa_format_convert(void *void_dst,
uint32_t dst_format, size_t dst_stride,
dst_array_format.as_uint =
_mesa_format_to_array_format(dst_format);
}
- /* Handle the cases where we can directly unpack */
- if (!(src_format MESA_ARRAY_FORMAT_BIT)) {
- if (dst_array_format.as_uint == RGBA_FLOAT.as_uint)
{
- for (row = 0; row height; ++row) {
-_mesa_unpack_rgba_row(src_format, width,
- src, (float (*)[4])dst);
-src += src_stride;
-dst += dst_stride;
- }
- return;
- } else if (dst_array_format.as_uint ==
RGBA_UBYTE.as_uint
- !_mesa_is_format_integer_color(src_format))
{
- for (row = 0; row height; ++row) {
-_mesa_unpack_ubyte_rgba_row(src_format, width,
-src, (uint8_t
(*)[4])dst);
-src += src_stride;
-dst += dst_stride;
- }
- return;
- } else if (dst_array_format.as_uint ==
RGBA_UINT.as_uint
- _mesa_is_format_integer_color(src_format)) {
- for (row = 0; row height; ++row) {
-_mesa_unpack_uint_rgba_row(src_format, width,
- src, (uint32_t
(*)[4])dst);
-src += src_stride;
-dst +=
