Re: [Mesa-dev] Bogus bounds checking in api_validate.c?
On 11/10/2014 08:12 PM, Timothy Arceri wrote: > On Mon, 2014-11-10 at 06:09 -0800, Ian Romanick wrote: >> On 11/06/2014 11:40 PM, Kenneth Graunke wrote: >>> On Thursday, November 06, 2014 08:09:18 PM Ian Romanick wrote: While working on some other things, I came across some bounds checking code in _mesa_validate_DrawElements (and related functions) in api_validate.c. /* use indices in the buffer object */ /* make sure count doesn't go outside buffer bounds */ if (index_bytes(type, count) > ctx->Array.VAO->IndexBufferObj->Size) { _mesa_warning(ctx, "glDrawElements index out of buffer bounds"); return GL_FALSE; } index_bytes calculates how many bytes of data "count" indices will occupy based on the type. The problem is that this doesn't consider the base pointer. As far as I can tell, if I had a 64 byte buffer object for my index data, and I did glDrawElements(GL_POINTS, 16, GL_UNSIGNED_INT, 60); _mesa_validate_DrawElements would say, "Ok!" Am I missing something, or is this just broken? >>> >>> It sure seems broken to me - but, thankfully, in a conservative fashion. >>> (It >>> will say some invalid things are OK, but won't say legal things are >>> invalid.) >>> >>> Software drivers may be relying on this working to avoid a crash. >>> >>> I checked the Ivybridge documentation, and found: >>> >>> "Software is responsible for ensuring that accesses outside the IB do not >>> occur. This is possible as software can compute the range of IB values >>> referenced by a 3DPRIMITIVE command (knowing the StartVertexLocation, >>> InstanceCount, and VerticesPerInstance values) and can then compare this >>> range to the IB extent." >>> >>> which makes it sound like an accurate computation is necessary. But, right >>> below that, it says: >>> >>> "this field contains the address of the last valid byte in the index buffer. >>> Any index buffer reads past this address returns an index value of 0 (as if >>> the index buffer was zero-extended)." >>> >>> So the earlier statement is false; i965 will draw the in-bounds elements >>> correctly, and then repeat element 0 over and over for any out-of-bounds >>> data, >>> resulting in one strange primitive and a lot of degenerate ones. >>> >>> It's proabbly worth fixing, but I doubt it's critical either. >> >> Hmm... I came across this while looking at cachegrind traces of GL >> applications. Time spent in _mesa_validate_Draw* was non-trivial. >> Since at least some hardware doesn't need this check, I think I want to >> move it down into drivers that actually need the check... which is kind >> of a bummer since I came up with a clever optimization for index_bytes. > > I'm not sure what applications you looked at in cachegrind but OpenArena > and Nexuiz both spend a lot of time in here. On my Ivybridge: > > OpenArena 5.4% out of a total 27.94% in i965_dri.so > Nexuiz 3.28% out of a total of 29.4% in i965_dri.so I was looking at an apitrace of Tesseract. It's not spending quite as much time in these functions as OpenArena and Nexuiz, but it's still quite a bit of time. > For those not up to speed are you able to give a one line explanation of > why some hardware can get away without this check? DX10 requires that the hardware bounds-check accesses to buffer objects. In most cases DX10 requires that out-of-bounds reads return zero, and out-of-bounds writes are always dropped. >>> A more interesting thing to fix, I think, would be enforcing alignment >>> restrictions (i.e. your offset has to be a multiple of the IB element size). >> >> That would probably be useful in debug builds, but I'm pretty sure the >> GL spec says the behavior is undefined specifically to avoid the check >> in the hot path. >> >>> --Ken >> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 84566] Unify the format conversion code
https://bugs.freedesktop.org/show_bug.cgi?id=84566 --- Comment #54 from Iago Toral --- Hi Jason, I think we are close to completing this, so here is a summary of the current state and how we intend to submit the patches for review, since we assume that you will be reviewing most of these: (In reply to Jason Ekstrand from comment #0) (...) > I would like to fix clean up this mess and unify a lot of the conversion > code. Here's what I've envisioned: > > 1) Autogenerate all of the color packing/unpacking functions Done. > 2) Convert all of the packing/unpacking functions to pack/unpack a > rectangle taking a width, height, and stride. We can use 1x1 for > single-pixel conversions. We decided not to do this yet because in the end format conversion will mostly happen through _mesa_format_convert now which already accepts a rect. Although making the pack/unpack functions accept a rect would slightly simplify _mesa_format_covert I think it won't really make a big difference anywhere else, so we have kept postponing this until now. We can still do this change if you think it is worth it. > 3) Make swrast use the unpacking functions instead of its own texture > sampling functions. > 4) Add an array format enum that allows us to enumerate all possible array > formats. Between mesa_format and this array format, we should also be able > to enumerate all of the GL datatypes. > 5) Make a masater conversion function that takes a void*, format, width, > height, and stride for both source and destination and just does the > conversion. If the above mentioned array format enum is distinct from the > mesa_format enum, the function could be written to take a uint32_t type and > accept either mesa_format or an array format in the same parameter. > 6) Use the above master conversion function for TexSubimage, TexImage, > GetTexImage, DrawPixels, and ReadPixels. We still have to deal with pixel > conversion, but it should vastly simplify all of them. All this is done. We have run piglit for i965, classic swrast, gallium swrast, llvmpipe, nouveau and gallium radeon without regressions so I guess it should be quite okay. Actually, there are now about 14 new piglit subtests that pass when compared to master. That said, I think there are probably many conversions that piglit won't cover, so I would not be surprised if we have a bunch of regressions anyway, but hopefully they won't be anything major. We are now preparing the patch set for review now. It is a large collection of patches (>50 patches), so we are thinking to split it in 3 series: one with general bugfixes, another for the auto-generation of pack/unpack functions and a final series with the master convert function. Git says that the patches add ~5K LOC and remove ~15K LOC, so it a huge cleanup. -- You are receiving this mail because: You are the QA Contact for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix segfault in WebGL Conformance on Ivybridge
On 11/10/2014 04:02 PM, Chad Versace wrote: > Fixes regression of WebGL Conformance test texture-size-limit [1] on > Ivybridge Mobile GT2 0x0166 with Google Chrome R38. > > Regression introduced by > > commit 6c044231535b93c5d16404528946cad618d96bd9 > Author: Kenneth Graunke > Date: Sun Feb 2 02:58:42 2014 -0800 > > i965: Bump GL_MAX_CUBE_MAP_TEXTURE_SIZE to 8192. > > The test regressed because the pointer offset arithmetic in > intel_miptree_map_gtt() overflows for large textures. The pointer > arithmetic is not 64-bit safe. > > This patch fixes the bugzilla ticket below on Ivybridge. This patch > doesn't close the ticket, though, because the bug report is against > Sandybridge, and QA cannot confirm the fix on that hardware. > > [1] > https://github.com/KhronosGroup/WebGL/blob/52f0dc240f04dce31b1b8e2b8107fe2b8332dc90/sdk/tests/conformance/textures/texture-size-limit.html > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=78770 > Fixes: Intel CHRMOS-1377 > Reported-by: Lu Hua > Signed-off-by: Chad Versace > --- > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > index 8fda25d..24e217c 100644 > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > @@ -1769,7 +1769,16 @@ intel_miptree_map_gtt(struct brw_context *brw, >y += image_y; > >map->stride = mt->pitch; > - map->ptr = base + y * map->stride + x * mt->cpp; > + > + /* The variables in below pointer arithmetic are 32-bit. The arithmetic > + * overflows for large textures. Therefore the cast to intptr_t is > + * needed. > + * > + * TODO(chadv): Fix this everywhere in i965 by fixing the signature of > + * intel_miptree_get_image_offset() to use intptr_t. > + */ > + map->ptr = base + (intptr_t) y * (intptr_t) map->stride > + + (intptr_t) x * (intptr_t) mt->cpp; I don't even want to know how long it took to track that down. :( Is it sufficient to just use unsigned? y and map->stride are the problematic values here. Isn't y bounded by (basically) 2*8192 and map->stride bounded by 16*8192? That sounds like 0x8000, which is negative. It seems like we could have similar problems in many places, and I'd like to figure out what the correct coding practices are to avoid it. > } > > DBG("%s: %d,%d %dx%d from mt %p (%s) %d,%d = %p/%d\n", __FUNCTION__, > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/cfg: Remove if_block/else_block.
Both patches are Reviewed-by: Jason Ekstrand On Nov 10, 2014 11:37 PM, "Matt Turner" wrote: > I used these in the SEL peephole, but they require extra tracking and > fix ups. The SEL peephole can pretty easily find the blocks it needs > without these. > --- > src/mesa/drivers/dri/i965/brw_cfg.cpp | 15 +-- > src/mesa/drivers/dri/i965/brw_cfg.h | 8 > src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 8 > 3 files changed, 1 insertion(+), 30 deletions(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_cfg.cpp > b/src/mesa/drivers/dri/i965/brw_cfg.cpp > index bb49a0a..02149e2 100644 > --- a/src/mesa/drivers/dri/i965/brw_cfg.cpp > +++ b/src/mesa/drivers/dri/i965/brw_cfg.cpp > @@ -51,8 +51,7 @@ link(void *mem_ctx, bblock_t *block) > } > > bblock_t::bblock_t(cfg_t *cfg) : > - cfg(cfg), start_ip(0), end_ip(0), num(0), > - if_block(NULL), else_block(NULL) > + cfg(cfg), start_ip(0), end_ip(0), num(0) > { > instructions.make_empty(); > parents.make_empty(); > @@ -136,7 +135,6 @@ bblock_t::combine_with(bblock_t *that) > } > > this->end_ip = that->end_ip; > - this->else_block = that->else_block; > this->instructions.append_list(&that->instructions); > > this->cfg->remove_block(that); > @@ -238,17 +236,6 @@ cfg_t::cfg_t(exec_list *instructions) > assert(cur_if->end()->opcode == BRW_OPCODE_IF); > assert(!cur_else || cur_else->end()->opcode == BRW_OPCODE_ELSE); > > - cur_if->if_block = cur_if; > - cur_if->else_block = cur_else; > - > -if (cur_else) { > -cur_else->if_block = cur_if; > -cur_else->else_block = cur_else; > - } > - > - cur->if_block = cur_if; > - cur->else_block = cur_else; > - > /* Pop the stack so we're in the previous if/else/endif */ > cur_if = pop_stack(&if_stack); > cur_else = pop_stack(&else_stack); > diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h > b/src/mesa/drivers/dri/i965/brw_cfg.h > index 388d29e..48bca9f 100644 > --- a/src/mesa/drivers/dri/i965/brw_cfg.h > +++ b/src/mesa/drivers/dri/i965/brw_cfg.h > @@ -89,14 +89,6 @@ struct bblock_t { > struct exec_list parents; > struct exec_list children; > int num; > - > - /* If the current basic block ends in an IF or ELSE instruction, these > will > -* point to the basic blocks containing the other associated > instruction. > -* > -* Otherwise they are NULL. > -*/ > - struct bblock_t *if_block; > - struct bblock_t *else_block; > }; > > static inline struct backend_instruction * > diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > index 4c9d7b9..03f838d 100644 > --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp > @@ -85,7 +85,6 @@ dead_control_flow_eliminate(backend_visitor *v) > } > > if (else_inst) { > -else_block->if_block->else_block = NULL; > else_inst->remove(else_block); > } > > @@ -102,13 +101,6 @@ dead_control_flow_eliminate(backend_visitor *v) > if (earlier_block && > earlier_block->can_combine_with(later_block)) { > earlier_block->combine_with(later_block); > > -foreach_block (block, v->cfg) { > - if (block->if_block == later_block) > - block->if_block = earlier_block; > - if (block->else_block == later_block) > - block->else_block = earlier_block; > -} > - > /* If ENDIF was in its own block, then we've now deleted it > and > * merged the two surrounding blocks, the latter of which the > * __next block pointer was pointing to. > -- > 2.0.4 > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/cfg: Remove if_block/else_block.
Two questions: Does it fix the bug? And did you ever figure out what was going on there? On Nov 11, 2014 6:58 AM, "Jason Ekstrand" wrote: > Both patches are Reviewed-by: Jason Ekstrand > On Nov 10, 2014 11:37 PM, "Matt Turner" wrote: > >> I used these in the SEL peephole, but they require extra tracking and >> fix ups. The SEL peephole can pretty easily find the blocks it needs >> without these. >> --- >> src/mesa/drivers/dri/i965/brw_cfg.cpp | 15 +-- >> src/mesa/drivers/dri/i965/brw_cfg.h | 8 >> src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 8 >> 3 files changed, 1 insertion(+), 30 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.cpp >> b/src/mesa/drivers/dri/i965/brw_cfg.cpp >> index bb49a0a..02149e2 100644 >> --- a/src/mesa/drivers/dri/i965/brw_cfg.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_cfg.cpp >> @@ -51,8 +51,7 @@ link(void *mem_ctx, bblock_t *block) >> } >> >> bblock_t::bblock_t(cfg_t *cfg) : >> - cfg(cfg), start_ip(0), end_ip(0), num(0), >> - if_block(NULL), else_block(NULL) >> + cfg(cfg), start_ip(0), end_ip(0), num(0) >> { >> instructions.make_empty(); >> parents.make_empty(); >> @@ -136,7 +135,6 @@ bblock_t::combine_with(bblock_t *that) >> } >> >> this->end_ip = that->end_ip; >> - this->else_block = that->else_block; >> this->instructions.append_list(&that->instructions); >> >> this->cfg->remove_block(that); >> @@ -238,17 +236,6 @@ cfg_t::cfg_t(exec_list *instructions) >> assert(cur_if->end()->opcode == BRW_OPCODE_IF); >> assert(!cur_else || cur_else->end()->opcode == BRW_OPCODE_ELSE); >> >> - cur_if->if_block = cur_if; >> - cur_if->else_block = cur_else; >> - >> -if (cur_else) { >> -cur_else->if_block = cur_if; >> -cur_else->else_block = cur_else; >> - } >> - >> - cur->if_block = cur_if; >> - cur->else_block = cur_else; >> - >> /* Pop the stack so we're in the previous if/else/endif */ >> cur_if = pop_stack(&if_stack); >> cur_else = pop_stack(&else_stack); >> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h >> b/src/mesa/drivers/dri/i965/brw_cfg.h >> index 388d29e..48bca9f 100644 >> --- a/src/mesa/drivers/dri/i965/brw_cfg.h >> +++ b/src/mesa/drivers/dri/i965/brw_cfg.h >> @@ -89,14 +89,6 @@ struct bblock_t { >> struct exec_list parents; >> struct exec_list children; >> int num; >> - >> - /* If the current basic block ends in an IF or ELSE instruction, >> these will >> -* point to the basic blocks containing the other associated >> instruction. >> -* >> -* Otherwise they are NULL. >> -*/ >> - struct bblock_t *if_block; >> - struct bblock_t *else_block; >> }; >> >> static inline struct backend_instruction * >> diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >> b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >> index 4c9d7b9..03f838d 100644 >> --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >> @@ -85,7 +85,6 @@ dead_control_flow_eliminate(backend_visitor *v) >> } >> >> if (else_inst) { >> -else_block->if_block->else_block = NULL; >> else_inst->remove(else_block); >> } >> >> @@ -102,13 +101,6 @@ dead_control_flow_eliminate(backend_visitor *v) >> if (earlier_block && >> earlier_block->can_combine_with(later_block)) { >> earlier_block->combine_with(later_block); >> >> -foreach_block (block, v->cfg) { >> - if (block->if_block == later_block) >> - block->if_block = earlier_block; >> - if (block->else_block == later_block) >> - block->else_block = earlier_block; >> -} >> - >> /* If ENDIF was in its own block, then we've now deleted it >> and >> * merged the two surrounding blocks, the latter of which the >> * __next block pointer was pointing to. >> -- >> 2.0.4 >> >> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 85419] [llvmpipe] Assertion fail with triangle strips
https://bugs.freedesktop.org/show_bug.cgi?id=85419 José Fonseca changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |INVALID --- Comment #6 from José Fonseca --- The trace crashes on NVIDIA GL driver. This is because you never call glEnable(PRIMITIVE_RESTART), so the driver interprets the index 0x literally, causing a buffer overflow. I also add to manually edit the trace and remove trailing \\ from one of the shaders, as NVIDIA GLSL compiler fails otherwise. AFAICS, you're getting undefined behavior because GL state is not being properly set. -- 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] [PATCH 0/2] clover: CL_MEM_HOST_* flags checks
This serie add CL_MEM_HOST_WRITE_ONLY, CL_MEM_HOST_READ_ONLY and CL_MEM_HOST_NO_ACCESS checks Those flags should be inherited when creting sub buffer. So I change clGetMemObjectInfo to report them and the otherd that should also be reported. EdB (2): clover: add CL_MEM_HOST_* flags checks clover: clGetMemObjectInfo report sub buffer inherited flags src/gallium/state_trackers/clover/api/memory.cpp | 32 +++--- src/gallium/state_trackers/clover/api/transfer.cpp | 32 -- 2 files changed, 57 insertions(+), 7 deletions(-) -- 1.9.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 1/2] clover: add CL_MEM_HOST_* flags checks
those flags have been introduced in OpenCL 1.2 --- src/gallium/state_trackers/clover/api/memory.cpp | 15 -- src/gallium/state_trackers/clover/api/transfer.cpp | 32 -- 2 files changed, 42 insertions(+), 5 deletions(-) diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp index a094e74..fe01d3f 100644 --- a/src/gallium/state_trackers/clover/api/memory.cpp +++ b/src/gallium/state_trackers/clover/api/memory.cpp @@ -44,13 +44,18 @@ clCreateBuffer(cl_context d_ctx, cl_mem_flags flags, size_t size, if (flags & ~(CL_MEM_READ_WRITE | CL_MEM_WRITE_ONLY | CL_MEM_READ_ONLY | CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | - CL_MEM_COPY_HOST_PTR)) + CL_MEM_COPY_HOST_PTR | CL_MEM_HOST_WRITE_ONLY | + CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS )) throw error(CL_INVALID_VALUE); if (util_bitcount(flags & (CL_MEM_READ_ONLY | CL_MEM_WRITE_ONLY | CL_MEM_READ_WRITE)) > 1) throw error(CL_INVALID_VALUE); + if (util_bitcount(flags & (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_READ_ONLY | + CL_MEM_HOST_NO_ACCESS)) > 1) + throw error(CL_INVALID_VALUE); + if ((flags & CL_MEM_USE_HOST_PTR) && (flags & (CL_MEM_COPY_HOST_PTR | CL_MEM_ALLOC_HOST_PTR))) throw error(CL_INVALID_VALUE); @@ -76,6 +81,11 @@ clCreateSubBuffer(cl_mem d_mem, cl_mem_flags flags, CL_MEM_WRITE_ONLY))) throw error(CL_INVALID_VALUE); + if (util_bitcount((flags | parent.flags()) & + (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_READ_ONLY | + CL_MEM_HOST_NO_ACCESS)) > 1) + throw error(CL_INVALID_VALUE); + if (op == CL_BUFFER_CREATE_TYPE_REGION) { auto reg = reinterpret_cast(op_info); @@ -182,7 +192,8 @@ clGetSupportedImageFormats(cl_context d_ctx, cl_mem_flags flags, if (flags & ~(CL_MEM_READ_WRITE | CL_MEM_WRITE_ONLY | CL_MEM_READ_ONLY | CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | - CL_MEM_COPY_HOST_PTR)) + CL_MEM_COPY_HOST_PTR | CL_MEM_HOST_WRITE_ONLY | + CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS )) throw error(CL_INVALID_VALUE); if (r_buf && !r_count) diff --git a/src/gallium/state_trackers/clover/api/transfer.cpp b/src/gallium/state_trackers/clover/api/transfer.cpp index b8d7771..32bd47a 100644 --- a/src/gallium/state_trackers/clover/api/transfer.cpp +++ b/src/gallium/state_trackers/clover/api/transfer.cpp @@ -171,10 +171,30 @@ namespace { /// Checks that the mapping flags are correct. /// void - validate_flags(const cl_map_flags flags) { + validate_map_flags(const cl_map_flags flags, const cl_mem_flags mem_flags) { if ((flags & (CL_MAP_WRITE | CL_MAP_READ)) && (flags & CL_MAP_WRITE_INVALIDATE_REGION)) throw error(CL_INVALID_VALUE); + + if ((flags & CL_MAP_READ) && + (mem_flags & (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_NO_ACCESS))) + throw error(CL_MEM_HOST_WRITE_ONLY); + + if ((flags & (CL_MAP_WRITE | CL_MAP_WRITE_INVALIDATE_REGION)) && + (mem_flags & (CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS))) + throw error(CL_MEM_HOST_WRITE_ONLY); + } + + void + validate_read_permission(const cl_mem_flags mem_flags) { + if (mem_flags & (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_NO_ACCESS)) + throw error(CL_INVALID_OPERATION); + } + + void + validate_write_permission(const cl_mem_flags mem_flags) { + if (mem_flags & (CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS)) + throw error(CL_INVALID_OPERATION); } /// @@ -269,6 +289,7 @@ clEnqueueReadBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking, validate_common(q, deps); validate_object(q, ptr, {}, obj_pitch, region); validate_object(q, mem, obj_origin, obj_pitch, region); + validate_read_permission(mem.flags()); auto hev = create( q, CL_COMMAND_READ_BUFFER, deps, @@ -298,6 +319,7 @@ clEnqueueWriteBuffer(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking, validate_common(q, deps); validate_object(q, mem, obj_origin, obj_pitch, region); validate_object(q, ptr, {}, obj_pitch, region); + validate_write_permission(mem.flags()); auto hev = create( q, CL_COMMAND_WRITE_BUFFER, deps, @@ -334,6 +356,7 @@ clEnqueueReadBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking, validate_common(q, deps); validate_object(q, ptr, host_origin, host_pitch, region); validate_object(q, mem, obj_origin, obj_pitch, region); + validate_read_permission(mem.flags()); auto hev = create( q, CL_COMMAND_READ_BUFFER_RECT, deps, @@ -370,6 +393,7 @@ clEnqueueWriteBufferRect(cl_command_queue d_q, cl_mem d_mem, cl_bool blocking, validate_common(q, deps); validate_object(q, mem
[Mesa-dev] [PATCH 2/2] clover: clGetMemObjectInfo report sub buffer inherited flags
when call cbCreateSubBuffer when followings memory flags are not set they should be inherited from parent buffer. Report them when calling clGetMemObjectInfo flags: CL_MEM_READ_WRITE, CL_MEM_READ_ONLY, CL_MEM_WRITE_ONLY, CL_MEM_USE_HOST_PTR, CL_MEM_ALLOC_HOST_PTR, CL_MEM_COPY_HOST_PTR, CL_MEM_HOST_WRITE_ONLY, CL_MEM_HOST_READ_ONLY, CL_MEM_HOST_NO_ACCESS --- src/gallium/state_trackers/clover/api/memory.cpp | 17 +++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp index fe01d3f..4843cff 100644 --- a/src/gallium/state_trackers/clover/api/memory.cpp +++ b/src/gallium/state_trackers/clover/api/memory.cpp @@ -226,9 +226,22 @@ clGetMemObjectInfo(cl_mem d_mem, cl_mem_info param, buf.as_scalar() = mem.type(); break; - case CL_MEM_FLAGS: - buf.as_scalar() = mem.flags(); + case CL_MEM_FLAGS: { + cl_mem_flags flags = mem.flags(); + + //inherited flags + sub_buffer *sub = dynamic_cast(&mem); + if (sub) { + flags = flags | (sub->parent().flags() & + (CL_MEM_READ_WRITE | CL_MEM_READ_ONLY | CL_MEM_WRITE_ONLY | + CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | + CL_MEM_COPY_HOST_PTR | + CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_READ_ONLY | + CL_MEM_HOST_NO_ACCESS)); + } + buf.as_scalar() = flags; break; + } case CL_MEM_SIZE: buf.as_scalar() = mem.size(); -- 1.9.3 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 85419] [llvmpipe] Assertion fail with triangle strips
https://bugs.freedesktop.org/show_bug.cgi?id=85419 --- Comment #7 from James Evans --- I am so sorry for bugging you with this. You are absolutely correct. I completely missed the glEnable call. Adding that fixed everything. Thank you. -- 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 84566] Unify the format conversion code
https://bugs.freedesktop.org/show_bug.cgi?id=84566 --- Comment #55 from Jason Ekstrand --- (In reply to Iago Toral from comment #54) > Hi Jason, > > I think we are close to completing this, so here is a summary of the current > state and how we intend to submit the patches for review, since we assume > that you will be reviewing most of these: > > (In reply to Jason Ekstrand from comment #0) > (...) > > I would like to fix clean up this mess and unify a lot of the conversion > > code. Here's what I've envisioned: > > > > 1) Autogenerate all of the color packing/unpacking functions > > Done. > > > 2) Convert all of the packing/unpacking functions to pack/unpack a > > rectangle taking a width, height, and stride. We can use 1x1 for > > single-pixel conversions. > > We decided not to do this yet because in the end format conversion will > mostly happen through _mesa_format_convert now which already accepts a rect. > Although making the pack/unpack functions accept a rect would slightly > simplify _mesa_format_covert I think it won't really make a big difference > anywhere else, so we have kept postponing this until now. We can still do > this change if you think it is worth it. That's fine. It's not that big of a change. Mostly, it's just a small performance boost for really narrow conversions. > > 3) Make swrast use the unpacking functions instead of its own texture > > sampling functions. > > 4) Add an array format enum that allows us to enumerate all possible array > > formats. Between mesa_format and this array format, we should also be able > > to enumerate all of the GL datatypes. > > 5) Make a masater conversion function that takes a void*, format, width, > > height, and stride for both source and destination and just does the > > conversion. If the above mentioned array format enum is distinct from the > > mesa_format enum, the function could be written to take a uint32_t type and > > accept either mesa_format or an array format in the same parameter. > > 6) Use the above master conversion function for TexSubimage, TexImage, > > GetTexImage, DrawPixels, and ReadPixels. We still have to deal with pixel > > conversion, but it should vastly simplify all of them. > > All this is done. > > We have run piglit for i965, classic swrast, gallium swrast, llvmpipe, > nouveau and gallium radeon without regressions so I guess it should be quite > okay. Actually, there are now about 14 new piglit subtests that pass when > compared to master. Good work! If it passes all of those, then I'm not too concerned about the kinds of hidden bugs we hit on my first format conversion series. > That said, I think there are probably many conversions that piglit won't > cover, so I would not be surprised if we have a bunch of regressions anyway, > but hopefully they won't be anything major. I'm sure there's things piglit covers. However, I'm going to give it a good look over and between doing what "looks right" and passing piglit, I think we've got it mostly covered. > We are now preparing the patch set for review now. It is a large collection > of patches (>50 patches), so we are thinking to split it in 3 series: one > with general bugfixes, another for the auto-generation of pack/unpack > functions and a final series with the master convert function. That seems perfectly reasonable. > Git says that the patches add ~5K LOC and remove ~15K LOC, so it a huge > cleanup. Nice! -- You are receiving this mail because: You are the QA Contact for the bug. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 1/2] clover: add CL_MEM_HOST_* flags checks
On Tuesday 11 November 2014 17:42:42 EdB wrote: > those flags have been introduced in OpenCL 1.2 > --- > src/gallium/state_trackers/clover/api/memory.cpp | 15 -- > src/gallium/state_trackers/clover/api/transfer.cpp | 32 > -- 2 files changed, 42 insertions(+), 5 deletions(-) > > diff --git a/src/gallium/state_trackers/clover/api/memory.cpp > b/src/gallium/state_trackers/clover/api/memory.cpp index a094e74..fe01d3f > 100644 > --- a/src/gallium/state_trackers/clover/api/memory.cpp > +++ b/src/gallium/state_trackers/clover/api/memory.cpp > @@ -44,13 +44,18 @@ clCreateBuffer(cl_context d_ctx, cl_mem_flags flags, > size_t size, > > if (flags & ~(CL_MEM_READ_WRITE | CL_MEM_WRITE_ONLY | CL_MEM_READ_ONLY | > CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | > - CL_MEM_COPY_HOST_PTR)) > + CL_MEM_COPY_HOST_PTR | CL_MEM_HOST_WRITE_ONLY | > + CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS )) >throw error(CL_INVALID_VALUE); > > if (util_bitcount(flags & (CL_MEM_READ_ONLY | CL_MEM_WRITE_ONLY | >CL_MEM_READ_WRITE)) > 1) >throw error(CL_INVALID_VALUE); > > + if (util_bitcount(flags & (CL_MEM_HOST_WRITE_ONLY | > CL_MEM_HOST_READ_ONLY | + > CL_MEM_HOST_NO_ACCESS)) > 1) > + throw error(CL_INVALID_VALUE); > + > if ((flags & CL_MEM_USE_HOST_PTR) && > (flags & (CL_MEM_COPY_HOST_PTR | CL_MEM_ALLOC_HOST_PTR))) >throw error(CL_INVALID_VALUE); > @@ -76,6 +81,11 @@ clCreateSubBuffer(cl_mem d_mem, cl_mem_flags flags, > CL_MEM_WRITE_ONLY))) >throw error(CL_INVALID_VALUE); > > + if (util_bitcount((flags | parent.flags()) & > + (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_READ_ONLY | > + CL_MEM_HOST_NO_ACCESS)) > 1) > + throw error(CL_INVALID_VALUE); > + > if (op == CL_BUFFER_CREATE_TYPE_REGION) { >auto reg = reinterpret_cast(op_info); > > @@ -182,7 +192,8 @@ clGetSupportedImageFormats(cl_context d_ctx, > cl_mem_flags flags, > > if (flags & ~(CL_MEM_READ_WRITE | CL_MEM_WRITE_ONLY | CL_MEM_READ_ONLY | > CL_MEM_USE_HOST_PTR | CL_MEM_ALLOC_HOST_PTR | > - CL_MEM_COPY_HOST_PTR)) > + CL_MEM_COPY_HOST_PTR | CL_MEM_HOST_WRITE_ONLY | > + CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS )) >throw error(CL_INVALID_VALUE); > > if (r_buf && !r_count) > diff --git a/src/gallium/state_trackers/clover/api/transfer.cpp > b/src/gallium/state_trackers/clover/api/transfer.cpp index b8d7771..32bd47a > 100644 > --- a/src/gallium/state_trackers/clover/api/transfer.cpp > +++ b/src/gallium/state_trackers/clover/api/transfer.cpp > @@ -171,10 +171,30 @@ namespace { > /// Checks that the mapping flags are correct. > /// > void > - validate_flags(const cl_map_flags flags) { > + validate_map_flags(const cl_map_flags flags, const cl_mem_flags > mem_flags) { if ((flags & (CL_MAP_WRITE | CL_MAP_READ)) && >(flags & CL_MAP_WRITE_INVALIDATE_REGION)) > throw error(CL_INVALID_VALUE); > + > + if ((flags & CL_MAP_READ) && > + (mem_flags & (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_NO_ACCESS))) > + throw error(CL_MEM_HOST_WRITE_ONLY); sorry, I recreate the patches series before sending it and forgot one change. This should be : throw error(CL_INVALID_OPERATION); > + > + if ((flags & (CL_MAP_WRITE | CL_MAP_WRITE_INVALIDATE_REGION)) && > + (mem_flags & (CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS))) > + throw error(CL_MEM_HOST_WRITE_ONLY); same here : throw error(CL_INVALID_OPERATION); > + } > + > + void > + validate_read_permission(const cl_mem_flags mem_flags) { > + if (mem_flags & (CL_MEM_HOST_WRITE_ONLY | CL_MEM_HOST_NO_ACCESS)) > + throw error(CL_INVALID_OPERATION); > + } > + > + void > + validate_write_permission(const cl_mem_flags mem_flags) { > + if (mem_flags & (CL_MEM_HOST_READ_ONLY | CL_MEM_HOST_NO_ACCESS)) > + throw error(CL_INVALID_OPERATION); > } > > /// > @@ -269,6 +289,7 @@ clEnqueueReadBuffer(cl_command_queue d_q, cl_mem d_mem, > cl_bool blocking, validate_common(q, deps); > validate_object(q, ptr, {}, obj_pitch, region); > validate_object(q, mem, obj_origin, obj_pitch, region); > + validate_read_permission(mem.flags()); > > auto hev = create( >q, CL_COMMAND_READ_BUFFER, deps, > @@ -298,6 +319,7 @@ clEnqueueWriteBuffer(cl_command_queue d_q, cl_mem d_mem, > cl_bool blocking, validate_common(q, deps); > validate_object(q, mem, obj_origin, obj_pitch, region); > validate_object(q, ptr, {}, obj_pitch, region); > + validate_write_permission(mem.flags()); > > auto hev = create( >q, CL_COMMAND_WRITE_BUFFER, deps, > @@ -334,6 +356,7 @@ clEnqueueReadBufferRect(cl_command_queue d_q, cl_mem > d_mem, cl_bool blocking, validate_common(q, deps); > validate_o
[Mesa-dev] [Bug 72572] no error on draw call without vbo set
https://bugs.freedesktop.org/show_bug.cgi?id=72572 José Fonseca changed: What|Removed |Added Priority|medium |low Summary|[llvmpipe] no error on draw |no error on draw call |call without vbo set|without vbo set Severity|normal |enhancement -- 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 79706] [TRACKER] Mesa regression tracker
https://bugs.freedesktop.org/show_bug.cgi?id=79706 Bug 79706 depends on bug 80835, which changed state. Bug 80835 Summary: [llvmpipe] piglit fs-struct-pad regression https://bugs.freedesktop.org/show_bug.cgi?id=80835 What|Removed |Added Status|NEW |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 80835] [llvmpipe] piglit fs-struct-pad regression
https://bugs.freedesktop.org/show_bug.cgi?id=80835 José Fonseca changed: What|Removed |Added Status|NEW |RESOLVED Resolution|--- |FIXED --- Comment #1 from José Fonseca --- It passes now. It must have been fixed since. -- 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/2] i965/cfg: Remove if_block/else_block.
One more comment: These and the other two CFG fixes should probably be promoted to stable. On Tue, Nov 11, 2014 at 7:37 AM, Jason Ekstrand wrote: > Two questions: Does it fix the bug? And did you ever figure out what was > going on there? > On Nov 11, 2014 6:58 AM, "Jason Ekstrand" wrote: > >> Both patches are Reviewed-by: Jason Ekstrand >> On Nov 10, 2014 11:37 PM, "Matt Turner" wrote: >> >>> I used these in the SEL peephole, but they require extra tracking and >>> fix ups. The SEL peephole can pretty easily find the blocks it needs >>> without these. >>> --- >>> src/mesa/drivers/dri/i965/brw_cfg.cpp | 15 +-- >>> src/mesa/drivers/dri/i965/brw_cfg.h | 8 >>> src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp | 8 >>> 3 files changed, 1 insertion(+), 30 deletions(-) >>> >>> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.cpp >>> b/src/mesa/drivers/dri/i965/brw_cfg.cpp >>> index bb49a0a..02149e2 100644 >>> --- a/src/mesa/drivers/dri/i965/brw_cfg.cpp >>> +++ b/src/mesa/drivers/dri/i965/brw_cfg.cpp >>> @@ -51,8 +51,7 @@ link(void *mem_ctx, bblock_t *block) >>> } >>> >>> bblock_t::bblock_t(cfg_t *cfg) : >>> - cfg(cfg), start_ip(0), end_ip(0), num(0), >>> - if_block(NULL), else_block(NULL) >>> + cfg(cfg), start_ip(0), end_ip(0), num(0) >>> { >>> instructions.make_empty(); >>> parents.make_empty(); >>> @@ -136,7 +135,6 @@ bblock_t::combine_with(bblock_t *that) >>> } >>> >>> this->end_ip = that->end_ip; >>> - this->else_block = that->else_block; >>> this->instructions.append_list(&that->instructions); >>> >>> this->cfg->remove_block(that); >>> @@ -238,17 +236,6 @@ cfg_t::cfg_t(exec_list *instructions) >>> assert(cur_if->end()->opcode == BRW_OPCODE_IF); >>> assert(!cur_else || cur_else->end()->opcode == >>> BRW_OPCODE_ELSE); >>> >>> - cur_if->if_block = cur_if; >>> - cur_if->else_block = cur_else; >>> - >>> -if (cur_else) { >>> -cur_else->if_block = cur_if; >>> -cur_else->else_block = cur_else; >>> - } >>> - >>> - cur->if_block = cur_if; >>> - cur->else_block = cur_else; >>> - >>> /* Pop the stack so we're in the previous if/else/endif */ >>> cur_if = pop_stack(&if_stack); >>> cur_else = pop_stack(&else_stack); >>> diff --git a/src/mesa/drivers/dri/i965/brw_cfg.h >>> b/src/mesa/drivers/dri/i965/brw_cfg.h >>> index 388d29e..48bca9f 100644 >>> --- a/src/mesa/drivers/dri/i965/brw_cfg.h >>> +++ b/src/mesa/drivers/dri/i965/brw_cfg.h >>> @@ -89,14 +89,6 @@ struct bblock_t { >>> struct exec_list parents; >>> struct exec_list children; >>> int num; >>> - >>> - /* If the current basic block ends in an IF or ELSE instruction, >>> these will >>> -* point to the basic blocks containing the other associated >>> instruction. >>> -* >>> -* Otherwise they are NULL. >>> -*/ >>> - struct bblock_t *if_block; >>> - struct bblock_t *else_block; >>> }; >>> >>> static inline struct backend_instruction * >>> diff --git a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >>> b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >>> index 4c9d7b9..03f838d 100644 >>> --- a/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >>> +++ b/src/mesa/drivers/dri/i965/brw_dead_control_flow.cpp >>> @@ -85,7 +85,6 @@ dead_control_flow_eliminate(backend_visitor *v) >>> } >>> >>> if (else_inst) { >>> -else_block->if_block->else_block = NULL; >>> else_inst->remove(else_block); >>> } >>> >>> @@ -102,13 +101,6 @@ dead_control_flow_eliminate(backend_visitor *v) >>> if (earlier_block && >>> earlier_block->can_combine_with(later_block)) { >>> earlier_block->combine_with(later_block); >>> >>> -foreach_block (block, v->cfg) { >>> - if (block->if_block == later_block) >>> - block->if_block = earlier_block; >>> - if (block->else_block == later_block) >>> - block->else_block = earlier_block; >>> -} >>> - >>> /* If ENDIF was in its own block, then we've now deleted it >>> and >>> * merged the two surrounding blocks, the latter of which >>> the >>> * __next block pointer was pointing to. >>> -- >>> 2.0.4 >>> >>> ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 70359] [llvmpipe] piglit arb_shader_texture_lod-texgrad fails
https://bugs.freedesktop.org/show_bug.cgi?id=70359 José Fonseca changed: What|Removed |Added Summary|[llvmpipe] piglit |[llvmpipe] piglit |arb_shader_texture_lod-texg |arb_shader_texture_lod-texg |rad regression |rad fails --- Comment #4 from José Fonseca --- With no_brilinear it goes a bit further. But it still fails: $ GALLIVM_DEBUG=no_quad_lod,no_rho_approx,no_brilinear ./bin/arb_shader_texture_lod-texgrad -auto Left: texture2D, Right: texture2DGradARB Probe color at (107,91) Left: 0.00 0.258824 0.737255 0.00 Right: 0.00 0.235294 0.760784 0.00 PIGLIT: {"result": "fail" } Anyway, this is not a proper regression, as Roland said, but just one case where two wrongs were making a right. -- 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/2] i965/cfg: Remove if_block/else_block.
On Tue, Nov 11, 2014 at 7:37 AM, Jason Ekstrand wrote: > Two questions: Does it fix the bug? And did you ever figure out what was > going on there? It does fix the bug. I checked out your nir-v0.7 branch and did the work from there. I spent probably an hour in gdb trying to figure out what was going wrong and narrowed it down to the 13th time the dead_control_flow_eliminate pass made progress, but I couldn't figure out what was going wrong. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/cfg: Remove if_block/else_block.
On Tue, Nov 11, 2014 at 9:27 AM, Jason Ekstrand wrote: > One more comment: These and the other two CFG fixes should probably be > promoted to stable. Seems like a good plan. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH] i965/fs: Remove try_replace_with_sel().
The rest of our backend optimizations have replaced the need for this since it was written. instructions in affected programs: 30626 -> 30564 (-0.20%) Hurts a small number of CSGO shaders by one instruction, but helps even more. Hurts two by a larger number because of something I noticed when I first wrote the SEL peephole: try_replace_with_sel() operates on instructions before we've demoted uniforms to pull constants. So code like var.x = ( -abs(r6.w) >= 0.0 ) ? pc[82].x : r9.x; var.y = ( -abs(r6.w) >= 0.0 ) ? pc[82].y : r9.y; var.z = ( -abs(r6.w) >= 0.0 ) ? pc[82].z : r9.z; var.w = ( -abs(r6.w) >= 0.0 ) ? pc[82].w : r9.w; where pc[82] gets demoted to a pull constant, we end up emitting a send(4) instruction to load pc[82] each time, and since they're in different basic blocks because we mishandle the ternary operator in this case we can't combine them. Once we handle this common ternary pattern better the problem will go away. --- src/mesa/drivers/dri/i965/brw_fs.h | 1 - src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 87 2 files changed, 88 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h index d9150c3..1c14d13 100644 --- a/src/mesa/drivers/dri/i965/brw_fs.h +++ b/src/mesa/drivers/dri/i965/brw_fs.h @@ -510,7 +510,6 @@ public: const fs_reg &src0, const fs_reg &src1); bool try_emit_saturate(ir_expression *ir); bool try_emit_mad(ir_expression *ir); - void try_replace_with_sel(); bool opt_peephole_sel(); bool opt_peephole_predicated_break(); bool opt_saturate_propagation(); diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp index 4e1badd..d4f08aa 100644 --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp @@ -2565,91 +2565,6 @@ fs_visitor::emit_if_gen6(ir_if *ir) emit(IF(this->result, fs_reg(0), BRW_CONDITIONAL_NZ)); } -/** - * Try to replace IF/MOV/ELSE/MOV/ENDIF with SEL. - * - * Many GLSL shaders contain the following pattern: - * - *x = condition ? foo : bar - * - * The compiler emits an ir_if tree for this, since each subexpression might be - * a complex tree that could have side-effects or short-circuit logic. - * - * However, the common case is to simply select one of two constants or - * variable values---which is exactly what SEL is for. In this case, the - * assembly looks like: - * - *(+f0) IF - *MOV dst src0 - *ELSE - *MOV dst src1 - *ENDIF - * - * which can be easily translated into: - * - *(+f0) SEL dst src0 src1 - * - * If src0 is an immediate value, we promote it to a temporary GRF. - */ -void -fs_visitor::try_replace_with_sel() -{ - fs_inst *endif_inst = (fs_inst *) instructions.get_tail(); - assert(endif_inst->opcode == BRW_OPCODE_ENDIF); - - /* Pattern match in reverse: IF, MOV, ELSE, MOV, ENDIF. */ - int opcodes[] = { - BRW_OPCODE_IF, BRW_OPCODE_MOV, BRW_OPCODE_ELSE, BRW_OPCODE_MOV, - }; - - fs_inst *match = (fs_inst *) endif_inst->prev; - for (int i = 0; i < 4; i++) { - if (match->is_head_sentinel() || match->opcode != opcodes[4-i-1]) - return; - match = (fs_inst *) match->prev; - } - - /* The opcodes match; it looks like the right sequence of instructions. */ - fs_inst *else_mov = (fs_inst *) endif_inst->prev; - fs_inst *then_mov = (fs_inst *) else_mov->prev->prev; - fs_inst *if_inst = (fs_inst *) then_mov->prev; - - /* Check that the MOVs are the right form. */ - if (then_mov->dst.equals(else_mov->dst) && - !then_mov->is_partial_write() && - !else_mov->is_partial_write()) { - - /* Remove the matched instructions; we'll emit a SEL to replace them. */ - while (!if_inst->next->is_tail_sentinel()) - if_inst->next->exec_node::remove(); - if_inst->exec_node::remove(); - - /* Only the last source register can be a constant, so if the MOV in - * the "then" clause uses a constant, we need to put it in a temporary. - */ - fs_reg src0(then_mov->src[0]); - if (src0.file == IMM) { - src0 = fs_reg(this, glsl_type::float_type); - src0.type = then_mov->src[0].type; - emit(MOV(src0, then_mov->src[0])); - } - - fs_inst *sel; - if (if_inst->conditional_mod) { - /* Sandybridge-specific IF with embedded comparison */ - emit(CMP(reg_null_d, if_inst->src[0], if_inst->src[1], - if_inst->conditional_mod)); - sel = emit(BRW_OPCODE_SEL, then_mov->dst, src0, else_mov->src[0]); - sel->predicate = BRW_PREDICATE_NORMAL; - } else { - /* Separate CMP and IF instructions */ - sel = emit(BRW_OPCODE_SEL, then_mov->dst, src0, else_mov->src[0]); - sel->predicate = if_inst->predicate; - sel->predicate_inverse = if_inst->predicate_inverse; - } - } -} - void fs_visitor::visit(ir_if *ir) { @@
Re: [Mesa-dev] [PATCH] i965/vec4: Allow CSE on uniform-vec4 expansion MOVs.
On Friday, October 24, 2014 03:09:45 PM Matt Turner wrote: > Three source instructions cannot directly source a packed vec4 (<0,4,1> > regioning) like vec4 uniforms, so we emit a MOV that expands the vec4 to > both halves of a register. > > If these uniform values are used by multiple three-source instructions, > we'll emit multiple expansion moves, which we cannot combine in CSE > (because CSE emits moves itself). > > So emit a virtual instruction that we can CSE. > > Sometimes we demote a uniform to to a pull constant after emitting an > expansion move for it. In that case, recognize in opt_algebraic that if > the .file of the new instruction is GRF then it's just a real move that > we can copy propagate and such. > > total instructions in shared programs: 5509805 -> 5499940 (-0.18%) > instructions in affected programs: 348923 -> 339058 (-2.83%) > --- > src/mesa/drivers/dri/i965/brw_defines.h | 1 + > src/mesa/drivers/dri/i965/brw_shader.cpp | 2 ++ > src/mesa/drivers/dri/i965/brw_vec4.cpp | 7 +++ > src/mesa/drivers/dri/i965/brw_vec4_cse.cpp | 1 + > src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 1 + > src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +- > 6 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h > index a68d607..a6807ae 100644 > --- a/src/mesa/drivers/dri/i965/brw_defines.h > +++ b/src/mesa/drivers/dri/i965/brw_defines.h > @@ -904,6 +904,7 @@ enum opcode { > SHADER_OPCODE_GEN7_SCRATCH_READ, > > VEC4_OPCODE_PACK_BYTES, > + VEC4_OPCODE_UNPACK_UNIFORM, > > FS_OPCODE_DDX, > FS_OPCODE_DDY, > diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp b/src/mesa/drivers/dri/i965/brw_shader.cpp > index 87f2681..ea73dd7 100644 > --- a/src/mesa/drivers/dri/i965/brw_shader.cpp > +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp > @@ -457,6 +457,8 @@ brw_instruction_name(enum opcode op) > > case VEC4_OPCODE_PACK_BYTES: >return "pack_bytes"; > + case VEC$_OPCODE_UNPACK_UNIFORM: MONEY! ^^^ I'm really not crazy about this patch...it's just another MOV operation, which can be CSE'd, but isn't considered everywhere else we handle MOV. Such as copy propagation, for example. I suppose this is OK, because you're only using it for "the following instruction can't handle " MOVs, where you're not likely to be able to do things like copy propagation anyway. Speaking of, you could probably use it for other workarounds too: - resolve_ud_negate - fix_math_operand - emit_math1_gen6 - emit_math2_gen6 But I don't know if that would be helpful. Although I'm not a fan, I don't have anything better to suggest, and your statistics clearly show a benefit. It's probably fine. Reviewed-by: Kenneth Graunke > + return "unpack_uniform"; > > case FS_OPCODE_DDX: >return "ddx"; > diff --git a/src/mesa/drivers/dri/i965/brw_vec4.cpp b/src/mesa/drivers/dri/i965/brw_vec4.cpp > index 5029495..cb8658d 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4.cpp > @@ -717,6 +717,13 @@ vec4_visitor::opt_algebraic() > > foreach_block_and_inst(block, vec4_instruction, inst, cfg) { >switch (inst->opcode) { > + case VEC4_OPCODE_UNPACK_UNIFORM: > + if (inst->src[0].file != UNIFORM) { > +inst->opcode = BRW_OPCODE_MOV; > +progress = true; > + } > + break; > + >case BRW_OPCODE_ADD: >if (inst->src[1].is_zero()) { > inst->opcode = BRW_OPCODE_MOV; > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp > index 28c69ca..6ff6902 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4_cse.cpp > @@ -69,6 +69,7 @@ is_expression(const vec4_instruction *const inst) > case BRW_OPCODE_PLN: > case BRW_OPCODE_MAD: > case BRW_OPCODE_LRP: > + case VEC4_OPCODE_UNPACK_UNIFORM: >return true; > case SHADER_OPCODE_RCP: > case SHADER_OPCODE_RSQ: > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > index 37c0806..6150d1d 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4_generator.cpp > @@ -1183,6 +1183,7 @@ vec4_generator::generate_code(const cfg_t *cfg) >} > >switch (inst->opcode) { > + case VEC4_OPCODE_UNPACK_UNIFORM: >case BRW_OPCODE_MOV: > brw_MOV(p, dst, src[0]); > break; > diff --git a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > index 3b279dc..e10972b 100644 > --- a/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > +++ b/src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp > @@ -302,7 +302,7 @@ vec4_visitor::fix_3src_operand(src_reg src) > > dst_reg expanded = dst_reg(
Re: [Mesa-dev] [PATCH] i965/vec4: Allow CSE on uniform-vec4 expansion MOVs.
On Tue, Nov 11, 2014 at 9:48 AM, Kenneth Graunke wrote: > On Friday, October 24, 2014 03:09:45 PM Matt Turner wrote: >> Three source instructions cannot directly source a packed vec4 (<0,4,1> >> regioning) like vec4 uniforms, so we emit a MOV that expands the vec4 to >> both halves of a register. >> >> If these uniform values are used by multiple three-source instructions, >> we'll emit multiple expansion moves, which we cannot combine in CSE >> (because CSE emits moves itself). >> >> So emit a virtual instruction that we can CSE. >> >> Sometimes we demote a uniform to to a pull constant after emitting an >> expansion move for it. In that case, recognize in opt_algebraic that if >> the .file of the new instruction is GRF then it's just a real move that >> we can copy propagate and such. >> >> total instructions in shared programs: 5509805 -> 5499940 (-0.18%) >> instructions in affected programs: 348923 -> 339058 (-2.83%) >> --- >> src/mesa/drivers/dri/i965/brw_defines.h | 1 + >> src/mesa/drivers/dri/i965/brw_shader.cpp | 2 ++ >> src/mesa/drivers/dri/i965/brw_vec4.cpp | 7 +++ >> src/mesa/drivers/dri/i965/brw_vec4_cse.cpp | 1 + >> src/mesa/drivers/dri/i965/brw_vec4_generator.cpp | 1 + >> src/mesa/drivers/dri/i965/brw_vec4_visitor.cpp | 2 +- >> 6 files changed, 13 insertions(+), 1 deletion(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h > b/src/mesa/drivers/dri/i965/brw_defines.h >> index a68d607..a6807ae 100644 >> --- a/src/mesa/drivers/dri/i965/brw_defines.h >> +++ b/src/mesa/drivers/dri/i965/brw_defines.h >> @@ -904,6 +904,7 @@ enum opcode { >> SHADER_OPCODE_GEN7_SCRATCH_READ, >> >> VEC4_OPCODE_PACK_BYTES, >> + VEC4_OPCODE_UNPACK_UNIFORM, >> >> FS_OPCODE_DDX, >> FS_OPCODE_DDY, >> diff --git a/src/mesa/drivers/dri/i965/brw_shader.cpp > b/src/mesa/drivers/dri/i965/brw_shader.cpp >> index 87f2681..ea73dd7 100644 >> --- a/src/mesa/drivers/dri/i965/brw_shader.cpp >> +++ b/src/mesa/drivers/dri/i965/brw_shader.cpp >> @@ -457,6 +457,8 @@ brw_instruction_name(enum opcode op) >> >> case VEC4_OPCODE_PACK_BYTES: >>return "pack_bytes"; >> + case VEC$_OPCODE_UNPACK_UNIFORM: > > MONEY! ^^^ > > I'm really not crazy about this patch...it's just another MOV operation, which > can be CSE'd, but isn't considered everywhere else we handle MOV. Such as > copy propagation, for example. > > I suppose this is OK, because you're only using it for "the following > instruction can't handle " MOVs, where you're not likely to be able > to do things like copy propagation anyway. That's exactly right. The only place it's emitted is from is fix_3src_operand, which we know can't take these arguments directly so copy propagation can't help us. opt_algebraic changes the opcode to MOV if the source isn't a uniform, like for an immediate. From there we can constant propagate it or whatever. > Speaking of, you could probably use it for other workarounds too: > - resolve_ud_negate > - fix_math_operand > - emit_math1_gen6 > - emit_math2_gen6 > > But I don't know if that would be helpful. > > Although I'm not a fan, I don't have anything better to suggest, and your > statistics clearly show a benefit. It's probably fine. I really don't have any idea why. Short of adding a bunch of code to CSE to rewrite operands rather than emitting another MOV (I don't know how feasible that is, given the complexity of register coalescing) I really don't know of a more elegant solution. > Reviewed-by: Kenneth Graunke Thanks! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix segfault in WebGL Conformance on Ivybridge
On Mon 10 Nov 2014, kalyan kondapally wrote: The patch seems to fix the crash I used to see while running this WebGL conformance test on my machine (Intel(R) Core(TM) i7-4600U CPU @ 2.10GHz). and now I can see all the tests pass. I believe 78770 is a different issue (Added my comments to 78770) and not actually dealing with fixing the crash we saw on 64 bit machines. Based on Lua's comments on bug 78770, I'm coming to the same conclusion. It's probably a different issue. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Fix segfault in WebGL Conformance on Ivybridge
On Tue 11 Nov 2014, Ian Romanick wrote: On 11/10/2014 04:02 PM, Chad Versace wrote: map->stride = mt->pitch; - map->ptr = base + y * map->stride + x * mt->cpp; + + /* The variables in below pointer arithmetic are 32-bit. The arithmetic + * overflows for large textures. Therefore the cast to intptr_t is + * needed. + * + * TODO(chadv): Fix this everywhere in i965 by fixing the signature of + * intel_miptree_get_image_offset() to use intptr_t. + */ + map->ptr = base + (intptr_t) y * (intptr_t) map->stride + + (intptr_t) x * (intptr_t) mt->cpp; I don't even want to know how long it took to track that down. :( It took way too long... :( I spent a lot of time on the wrong trail, trying to find errors in Mesa Core's pack/unpack routines. The bisect gave me the clue to where the problem was. Is it sufficient to just use unsigned? No. This expression is doing pointer arithmetic. Let's move into the 21st century and just use intptr_t and uintptr_t for pointer arithmetic. Just using a 32-bit unsigned here may fix this particular bug, but still leaves the door open for future buckets-of-fun debugging sessions. Let's do the right thing and use a C99 pointer-width type. y and map->stride are the problematic values here. Isn't y bounded by (basically) 2*8192 and map->stride bounded by 16*8192? That sounds like 0x8000, which is negative. It seems like we could have similar problems in many places, and I'd like to figure out what the correct coding practices are to avoid it. I believe the correct coding practice is to always use C99 pointer-width types (inptr_t, uintptr_t, ptrdiff_t) when doing pointer arithmetic. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Use the predicate enable bit for conditional rendering without stalling
Kenneth Graunke writes: > drm-intel-next must have the new software checker turned on, which > disallows non-whitelisted register writes (along with libva, so it > can't really be enabled upstream yet). For what it's worth, I get the EINVAL error even on the stock Fedora 20 kernel on Haswell (and presumably IvyBridge) so I can only assume the software checker is already upstream, unless I'm misunderstanding something. $ uname -r 3.16.7-200.fc20.x86_64 $ modinfo i915 | grep cmd_parser parm: enable_cmd_parser:Enable command parsing [...] (1=enabled [default], 0=disabled) (int) $ sudo cat /sys/module/i915/parameters/enable_cmd_parser 1 If I cat 0 to /sys/module/i915/parameters/enable_cmd_parser then I no longer get the EINVAL error. - Neil ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] i965: Use the predicate enable bit for conditional rendering without stalling
On Tuesday, November 11, 2014 06:59:51 PM Neil Roberts wrote: > Kenneth Graunke writes: > > > drm-intel-next must have the new software checker turned on, which > > disallows non-whitelisted register writes (along with libva, so it > > can't really be enabled upstream yet). > > For what it's worth, I get the EINVAL error even on the stock Fedora 20 > kernel on Haswell (and presumably IvyBridge) so I can only assume the > software checker is already upstream, unless I'm misunderstanding > something. > > $ uname -r > 3.16.7-200.fc20.x86_64 > $ modinfo i915 | grep cmd_parser > parm: enable_cmd_parser:Enable command parsing [...] > (1=enabled [default], 0=disabled) (int) > $ sudo cat /sys/module/i915/parameters/enable_cmd_parser > 1 > > If I cat 0 to /sys/module/i915/parameters/enable_cmd_parser then I no > longer get the EINVAL error. > > - Neil Huh. Yeah, I thought they turned it on by default in 3.16, which I don't understand at all. AFAIK the libva issue isn't fixed (or wasn't by then), so it sure seems like it would've broken userspace. Which would be a pretty clear kernel policy violation... I must have the facts wrong, I guess... --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/2] i965/vec4: Rewrite dead code elimination to use live in/out.
On Sat, Nov 8, 2014 at 10:18 PM, Kenneth Graunke wrote: > On Monday, November 03, 2014 01:34:49 PM Matt Turner wrote: >> +static bool >> +can_do_writemask(const struct brw_context *brw, >> + const vec4_instruction *inst) >> +{ >> + switch (inst->opcode) { >> + case SHADER_OPCODE_GEN4_SCRATCH_READ: >> + case VS_OPCODE_PULL_CONSTANT_LOAD: >> + case VS_OPCODE_PULL_CONSTANT_LOAD_GEN7: >> + return false; >> + default: >> + /* The MATH instruction on Gen6 only executes in align1 mode, which >> does >> + * not support writemasking. >> + */ >> + if (brw->gen == 6 && inst->is_math()) >> + return false; >> + >> + if (inst->is_tex()) >> + return false; > > I'd feel a lot more confident in this function if it were: > > { >/* The MATH instruction on Gen6 only executes in align1 mode, which does > * not support writemasking. > */ >if (brw->gen == 6 && inst->is_math()) > return false; > >return inst->mlen == 0; > } I like that too, but I noticed that VS_OPCODE_PULL_CONSTANT_LOAD_GEN7 doesn't set mlen. I guess that's just a bug? ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 2/2] i965/vec4: Rewrite dead code elimination to use live in/out.
On Tue, Nov 11, 2014 at 2:17 PM, Matt Turner wrote: > On Sat, Nov 8, 2014 at 10:18 PM, Kenneth Graunke > wrote: >> On Monday, November 03, 2014 01:34:49 PM Matt Turner wrote: >>> +static bool >>> +can_do_writemask(const struct brw_context *brw, >>> + const vec4_instruction *inst) >>> +{ >>> + switch (inst->opcode) { >>> + case SHADER_OPCODE_GEN4_SCRATCH_READ: >>> + case VS_OPCODE_PULL_CONSTANT_LOAD: >>> + case VS_OPCODE_PULL_CONSTANT_LOAD_GEN7: >>> + return false; >>> + default: >>> + /* The MATH instruction on Gen6 only executes in align1 mode, which >>> does >>> + * not support writemasking. >>> + */ >>> + if (brw->gen == 6 && inst->is_math()) >>> + return false; >>> + >>> + if (inst->is_tex()) >>> + return false; >> >> I'd feel a lot more confident in this function if it were: >> >> { >>/* The MATH instruction on Gen6 only executes in align1 mode, which does >> * not support writemasking. >> */ >>if (brw->gen == 6 && inst->is_math()) >> return false; >> >>return inst->mlen == 0; >> } > > I like that too, but I noticed that VS_OPCODE_PULL_CONSTANT_LOAD_GEN7 > doesn't set mlen. I guess that's just a bug? Looks like it's not. mlen seems to imply some MRF writes, according to ::implied_mrf_writes(). I think I'll settle for adding if (inst->opcode == VS_OPCODE_PULL_CONSTANT_LOAD_GEN7) return false; to the function. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 01/14] i965/hiz: Start to separate miptree out from hiz buffers
On 2014-11-10 10:42:16, Ben Widawsky wrote: > On Mon, Jul 21, 2014 at 11:00:50PM -0700, Jordan Justen wrote: > > Today we allocate a miptree's for the hiz buffer. We needed this in > > the past because we would point the hardware at offsets of the hiz > > buffer. Since the hiz format is not documented, this is not a good > > idea. > > > > Since moving to support layered rendering on Gen7+, we no longer point > > at an offset into the buffer on Gen7+. > > > > Therefore, to support hiz on Gen7+, we don't need a full miptree > > structure allocated. > > > > This patch starts to create a new auxiliary buffer structure > > (intel_miptree_aux_buffer) that can be a more simplistic miptree > > side-band buffer associated with a miptree. (For example, to serve the > > needs of the hiz buffer.) > > > > Signed-off-by: Jordan Justen > > Reviewed-by: Topi Pohjolainen > > --- > > src/mesa/drivers/dri/i965/brw_misc_state.c| 4 +- > > src/mesa/drivers/dri/i965/gen6_blorp.cpp | 2 +- > > src/mesa/drivers/dri/i965/gen7_blorp.cpp | 2 +- > > src/mesa/drivers/dri/i965/gen7_misc_state.c | 2 +- > > src/mesa/drivers/dri/i965/gen8_depth_state.c | 6 +-- > > src/mesa/drivers/dri/i965/intel_fbo.c | 4 +- > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 55 > > +++ > > src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 29 -- > > 8 files changed, 76 insertions(+), 28 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c > > b/src/mesa/drivers/dri/i965/brw_misc_state.c > > index 76e22bd..af900cd 100644 > > --- a/src/mesa/drivers/dri/i965/brw_misc_state.c > > +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c > > @@ -182,7 +182,7 @@ brw_get_depthstencil_tile_masks(struct > > intel_mipmap_tree *depth_mt, > > > >if (intel_miptree_level_has_hiz(depth_mt, depth_level)) { > > uint32_t hiz_tile_mask_x, hiz_tile_mask_y; > > - intel_miptree_get_tile_masks(depth_mt->hiz_mt, > > + intel_miptree_get_tile_masks(depth_mt->hiz_buf->mt, > >&hiz_tile_mask_x, &hiz_tile_mask_y, > >false); > > > > @@ -643,7 +643,7 @@ brw_emit_depth_stencil_hiz(struct brw_context *brw, > > > >/* Emit hiz buffer. */ > >if (hiz) { > > - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt; > > + struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt; > >BEGIN_BATCH(3); > >OUT_BATCH((_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2)); > >OUT_BATCH(hiz_mt->pitch - 1); > > diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp > > b/src/mesa/drivers/dri/i965/gen6_blorp.cpp > > index eb865b9..282c7b2 100644 > > --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp > > +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp > > @@ -855,7 +855,7 @@ gen6_blorp_emit_depth_stencil_config(struct brw_context > > *brw, > > > > /* 3DSTATE_HIER_DEPTH_BUFFER */ > > { > > - struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_mt; > > + struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt; > >uint32_t hiz_offset = > > intel_miptree_get_aligned_offset(hiz_mt, > >draw_x & ~tile_mask_x, > > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > index 0ad570b..d0d623d 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > @@ -752,7 +752,7 @@ gen7_blorp_emit_depth_stencil_config(struct brw_context > > *brw, > > > > /* 3DSTATE_HIER_DEPTH_BUFFER */ > > { > > - struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_mt; > > + struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt; > > > >BEGIN_BATCH(3); > >OUT_BATCH((GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2)); > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > index 22911bf..6c6a79b 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > @@ -145,7 +145,7 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw, > >OUT_BATCH(0); > >ADVANCE_BATCH(); > > } else { > > - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_mt; > > + struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt; > >BEGIN_BATCH(3); > >OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2)); > >OUT_BATCH((mocs << 25) | > > diff --git a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > index 7c3bfe0..b6f373d 100644 > > --- a/src/mesa/drivers/dri/i965/gen8_depth_state.c > > +++ b/src/mesa/drivers/dri/i965/gen8_depth_state.c > > @@ -89,10 +89,10 @@ emit_depth_packets(struct brw_context *brw, > > } else { > >BEGIN_BATCH(5); > >
Re: [Mesa-dev] [PATCH v2 02/14] i965/gen7: Don't rely directly on the hiz miptree structure
On 2014-11-10 10:51:17, Ben Widawsky wrote: > On Mon, Jul 21, 2014 at 11:00:51PM -0700, Jordan Justen wrote: > > We are still allocating a miptree for hiz, but we only use fields from > > intel_miptree_aux_buffer. This will allow us to switch over to not > > allocating a miptree. > > > > Signed-off-by: Jordan Justen > > Reviewed-by: Topi Pohjolainen > > --- > > src/mesa/drivers/dri/i965/gen7_blorp.cpp| 6 +++--- > > src/mesa/drivers/dri/i965/gen7_misc_state.c | 7 --- > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > index d0d623d..ebfe6d1 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > @@ -752,13 +752,13 @@ gen7_blorp_emit_depth_stencil_config(struct > > brw_context *brw, > > > > /* 3DSTATE_HIER_DEPTH_BUFFER */ > > { > > - struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt; > > + struct intel_miptree_aux_buffer *hiz_buf = params->depth.mt->hiz_buf; > > > >BEGIN_BATCH(3); > >OUT_BATCH((GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2)); > >OUT_BATCH((mocs << 25) | > > -(hiz_mt->pitch - 1)); > > - OUT_RELOC(hiz_mt->bo, > > +(hiz_buf->pitch - 1)); > > + OUT_RELOC(hiz_buf->bo, > > I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, > > 0); > >ADVANCE_BATCH(); > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > index 6c6a79b..8f8c33e 100644 > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > @@ -145,12 +145,13 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw, > >OUT_BATCH(0); > >ADVANCE_BATCH(); > > } else { > > - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt; > > + struct intel_miptree_aux_buffer *hiz_buf = depth_mt->hiz_buf; > > + > >BEGIN_BATCH(3); > >OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2)); > >OUT_BATCH((mocs << 25) | > > -(hiz_mt->pitch - 1)); > > - OUT_RELOC(hiz_mt->bo, > > +(hiz_buf->pitch - 1)); > > + OUT_RELOC(hiz_buf->bo, > > I915_GEM_DOMAIN_RENDER, > > I915_GEM_DOMAIN_RENDER, > > 0); > > -- > > Just a simple grep turns up a few places that weren't modified. I am > not sure if you intentionally left out the <= gen6 state, but just for > the pedant: > > brw_emit_depth_stencil_hiz() > gen6_blorp_emit_depth_stencil_config() > gen6_emit_depth_stencil_hiz() Yeah, this is intentional. Gen6 will have to keep using the miptree structure because of limitations of hiz on gen6. To support layered rendering (and thus GL 3.2) we'll need to stick with this. For gen < 6, I'm not sure. We might be able to rework some of how the texturing works there because I don't think they will have to support layered rendering. (Except, I may be veering off track from hiz now...) > If it was intentional, maybe adjust the commit message? The subject does say gen7. :) Any suggestions on what to add? Maybe just mention that gen <= 6 will not be updated similarly? -Jordan > I see nothing that would break as a result here though. > Reviewed-by: Ben Widawsky > > -- > 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] How difficult would it be to have debugging information for Jitted code show up?
I'm hoping that it would be possible to have symbol names for Jitted code to show up for profiling tools. For example, with software rendering and examining my code using perf utils 70% of my code is just a mysterious callstack inside the i915 shared library that has no symbols and no source information as the JITted code has no debug information. Thank you, Steven Stewart-Gallus ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2 02/14] i965/gen7: Don't rely directly on the hiz miptree structure
On Tue, Nov 11, 2014 at 03:06:03PM -0800, Jordan Justen wrote: > On 2014-11-10 10:51:17, Ben Widawsky wrote: > > On Mon, Jul 21, 2014 at 11:00:51PM -0700, Jordan Justen wrote: > > > We are still allocating a miptree for hiz, but we only use fields from > > > intel_miptree_aux_buffer. This will allow us to switch over to not > > > allocating a miptree. > > > > > > Signed-off-by: Jordan Justen > > > Reviewed-by: Topi Pohjolainen > > > --- > > > src/mesa/drivers/dri/i965/gen7_blorp.cpp| 6 +++--- > > > src/mesa/drivers/dri/i965/gen7_misc_state.c | 7 --- > > > 2 files changed, 7 insertions(+), 6 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > > b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > > index d0d623d..ebfe6d1 100644 > > > --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > > +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp > > > @@ -752,13 +752,13 @@ gen7_blorp_emit_depth_stencil_config(struct > > > brw_context *brw, > > > > > > /* 3DSTATE_HIER_DEPTH_BUFFER */ > > > { > > > - struct intel_mipmap_tree *hiz_mt = params->depth.mt->hiz_buf->mt; > > > + struct intel_miptree_aux_buffer *hiz_buf = > > > params->depth.mt->hiz_buf; > > > > > >BEGIN_BATCH(3); > > >OUT_BATCH((GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16) | (3 - 2)); > > >OUT_BATCH((mocs << 25) | > > > -(hiz_mt->pitch - 1)); > > > - OUT_RELOC(hiz_mt->bo, > > > +(hiz_buf->pitch - 1)); > > > + OUT_RELOC(hiz_buf->bo, > > > I915_GEM_DOMAIN_RENDER, I915_GEM_DOMAIN_RENDER, > > > 0); > > >ADVANCE_BATCH(); > > > diff --git a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > > b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > > index 6c6a79b..8f8c33e 100644 > > > --- a/src/mesa/drivers/dri/i965/gen7_misc_state.c > > > +++ b/src/mesa/drivers/dri/i965/gen7_misc_state.c > > > @@ -145,12 +145,13 @@ gen7_emit_depth_stencil_hiz(struct brw_context *brw, > > >OUT_BATCH(0); > > >ADVANCE_BATCH(); > > > } else { > > > - struct intel_mipmap_tree *hiz_mt = depth_mt->hiz_buf->mt; > > > + struct intel_miptree_aux_buffer *hiz_buf = depth_mt->hiz_buf; > > > + > > >BEGIN_BATCH(3); > > >OUT_BATCH(GEN7_3DSTATE_HIER_DEPTH_BUFFER << 16 | (3 - 2)); > > >OUT_BATCH((mocs << 25) | > > > -(hiz_mt->pitch - 1)); > > > - OUT_RELOC(hiz_mt->bo, > > > +(hiz_buf->pitch - 1)); > > > + OUT_RELOC(hiz_buf->bo, > > > I915_GEM_DOMAIN_RENDER, > > > I915_GEM_DOMAIN_RENDER, > > > 0); > > > -- > > > > Just a simple grep turns up a few places that weren't modified. I am > > not sure if you intentionally left out the <= gen6 state, but just for > > the pedant: > > > > brw_emit_depth_stencil_hiz() > > gen6_blorp_emit_depth_stencil_config() > > gen6_emit_depth_stencil_hiz() > > Yeah, this is intentional. Gen6 will have to keep using the miptree > structure because of limitations of hiz on gen6. To support layered > rendering (and thus GL 3.2) we'll need to stick with this. > As we discussed in my cube yesterday, I was sort of hoping there would be one data structure for both gen6 and > gen6, but I think we agreed it doesn't really work out well. So it seems okay to me. > For gen < 6, I'm not sure. We might be able to rework some of how the > texturing works there because I don't think they will have to support > layered rendering. (Except, I may be veering off track from hiz > now...) > > > If it was intentional, maybe adjust the commit message? > > The subject does say gen7. :) > > Any suggestions on what to add? Maybe just mention that gen <= 6 will > not be updated similarly? > > -Jordan You're right. The subject says enough. You did already have my r-b... > > > I see nothing that would break as a result here though. > > Reviewed-by: Ben Widawsky > > > > -- > > Ben Widawsky, Intel Open Source Technology Center -- 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/2] i965/vec4: Rewrite dead code elimination to use live in/out.
On Tuesday, November 11, 2014 02:39:13 PM Matt Turner wrote: > On Tue, Nov 11, 2014 at 2:17 PM, Matt Turner wrote: > > On Sat, Nov 8, 2014 at 10:18 PM, Kenneth Graunke wrote: > >> On Monday, November 03, 2014 01:34:49 PM Matt Turner wrote: > >>> +static bool > >>> +can_do_writemask(const struct brw_context *brw, > >>> + const vec4_instruction *inst) > >>> +{ > >>> + switch (inst->opcode) { > >>> + case SHADER_OPCODE_GEN4_SCRATCH_READ: > >>> + case VS_OPCODE_PULL_CONSTANT_LOAD: > >>> + case VS_OPCODE_PULL_CONSTANT_LOAD_GEN7: > >>> + return false; > >>> + default: > >>> + /* The MATH instruction on Gen6 only executes in align1 mode, which does > >>> + * not support writemasking. > >>> + */ > >>> + if (brw->gen == 6 && inst->is_math()) > >>> + return false; > >>> + > >>> + if (inst->is_tex()) > >>> + return false; > >> > >> I'd feel a lot more confident in this function if it were: > >> > >> { > >>/* The MATH instruction on Gen6 only executes in align1 mode, which does > >> * not support writemasking. > >> */ > >>if (brw->gen == 6 && inst->is_math()) > >> return false; > >> > >>return inst->mlen == 0; > >> } > > > > I like that too, but I noticed that VS_OPCODE_PULL_CONSTANT_LOAD_GEN7 > > doesn't set mlen. I guess that's just a bug? > > Looks like it's not. mlen seems to imply some MRF writes, according to > ::implied_mrf_writes(). > > I think I'll settle for adding > >if (inst->opcode == VS_OPCODE_PULL_CONSTANT_LOAD_GEN7) > return false; > > to the function. This would do the trick: return inst->mlen == 0 && !inst->is_send_from_grf(); and has the benefit of fewer places to update if we add more send-from-GRF opcodes. 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] How difficult would it be to have debugging information for Jitted code show up?
Is this the i915 gallium or classic driver? What does glxinfo show? Jose From: mesa-dev on behalf of Steven Stewart-Gallus Sent: 11 November 2014 23:45 To: Kenneth Graunke Cc: mesa-dev@lists.freedesktop.org Subject: Re: [Mesa-dev] How difficult would it be to have debugging information for Jitted code show up? I'm hoping that it would be possible to have symbol names for Jitted code to show up for profiling tools. For example, with software rendering and examining my code using perf utils 70% of my code is just a mysterious callstack inside the i915 shared library that has no symbols and no source information as the JITted code has no debug information. Thank you, Steven Stewart-Gallus ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.freedesktop.org_mailman_listinfo_mesa-2Ddev&d=AAIGaQ&c=Sqcl0Ez6M0X8aeM67LKIiDJAXVeAw-YihVMNtXt-uEs&r=zfmBZnnVGHeYde45pMKNnVyzeaZbdIqVLprmZCM2zzE&m=5-qyNgNTfbll239LRSjQLKJuiUBT0u_c9rBpLMXBFgA&s=xDSlgkTlh6J3y4qx789edpfnRxZPgS7OSx3twMfza0o&e= ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [Bug 86070] Host application crash on vmware fusion 7 in vmw_swc_flush
https://bugs.freedesktop.org/show_bug.cgi?id=86070 Sinclair Yeh changed: What|Removed |Added Status|NEW |ASSIGNED -- 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 86070] Host application crash on vmware fusion 7 in vmw_swc_flush
https://bugs.freedesktop.org/show_bug.cgi?id=86070 --- Comment #1 from Sinclair Yeh --- >From the description I assume the submitter is using mplayer. I've done the following steps and could not reproduce this issue: 1. download and build mplayer from http://www.mplayerhq.hu 2. download an mpeg2 clip from http://support.apple.com/en-us/HT1425 3. ran: mplayer sample_mpeg2.m2v The clip seems to run fine. My system configuration: Fedora 20 64-bit MESA 10.4 Please provide more details in terms of steps to reproduce and the system configuration. -- 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 86070] Host application crash on vmware fusion 7 in vmw_swc_flush
https://bugs.freedesktop.org/show_bug.cgi?id=86070 --- Comment #2 from Nicholas Yue --- Hi Sinclair, The application mplay (not mplayer) is from the Houdini suite of software by Side Effects http://www.sidefx.com/index.php?option=com_download&task=apprentice&Itemid=208 You can use the Apprentice (free) version to verify the crash when launching mplay, gplay or houdini itself Mesa version was already included in the original report OpenGL version string: 2.1 Mesa 8.0.4 My OS in question is Ubuntu 12.04.5 x64 Kernel is : Linux ubuntu 3.13.0-32-generic #57~precise1-Ubuntu SMP Tue Jul 15 03:51:20 UTC 2014 x86_64 x86_64 x86_64 GNU/Linux Cheers -- 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 86070] Host application crash on vmware fusion 7 in vmw_swc_flush
https://bugs.freedesktop.org/show_bug.cgi?id=86070 --- Comment #3 from Nicholas Yue --- I am using the most recent version of VMWare Fusion 7 on OS X Yosemite to host Ubuntu 12.04.5 x64 and it only provides Mesa3D at version 8.0.4 not the 10.4 you are testing with. Is there an update in VMWare Fusion 7 I should get and retest before debugging further ? -- 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] nvc0: remove unused nvc0_screen::mm_VRAM_fe0
LG. I had this same patch locally I think... I came up with it after I went looking at the various VRAM usage after you were asking questions about it. On Tue, Nov 11, 2014 at 11:59 PM, Alexandre Courbot wrote: > Ping, how about this guy? > > On Mon, Oct 27, 2014 at 7:36 PM, Alexandre Courbot > wrote: > > This member is declared, allocated and destroyed, but doesn't seem to be > > used or referenced anywhere in the code. > > > > Signed-off-by: Alexandre Courbot > > --- > > Resending after fixing typo in email address - apologies for the > inconvenience. > > > > src/gallium/drivers/nouveau/nvc0/nvc0_screen.c | 3 --- > > src/gallium/drivers/nouveau/nvc0/nvc0_screen.h | 2 -- > > 2 files changed, 5 deletions(-) > > > > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > > index a7581f286cfc..61b381693224 100644 > > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c > > @@ -407,8 +407,6 @@ nvc0_screen_destroy(struct pipe_screen *pscreen) > > > > FREE(screen->tic.entries); > > > > - nouveau_mm_destroy(screen->mm_VRAM_fe0); > > - > > nouveau_object_del(&screen->eng3d); > > nouveau_object_del(&screen->eng2d); > > nouveau_object_del(&screen->m2mf); > > @@ -1027,7 +1025,6 @@ nvc0_screen_create(struct nouveau_device *dev) > > > > mm_config.nvc0.tile_mode = 0; > > mm_config.nvc0.memtype = 0xfe0; > > - screen->mm_VRAM_fe0 = nouveau_mm_create(dev, NOUVEAU_BO_VRAM, > &mm_config); > > > > if (!nvc0_blitter_create(screen)) > >goto fail; > > diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.h > b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.h > > index 4802057f70ee..8a1991f52eb4 100644 > > --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.h > > +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.h > > @@ -73,8 +73,6 @@ struct nvc0_screen { > >boolean mp_counters_enabled; > > } pm; > > > > - struct nouveau_mman *mm_VRAM_fe0; > > - > > struct nouveau_object *eng3d; /* sqrt(1/2)|kepler> + > sqrt(1/2)|fermi> */ > > struct nouveau_object *eng2d; > > struct nouveau_object *m2mf; > > -- > > 2.1.2 > > > ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH] nvc0: remove unused nvc0_screen::mm_VRAM_fe0
On 11/12/2014 03:07 PM, Ilia Mirkin wrote: LG. I had this same patch locally I think... I came up with it after I went looking at the various VRAM usage after you were asking questions about it. Good, I'm dropping this version then, and assume yours will get merged soon. Thanks! ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev