[Mesa-dev] [PATCH] meson: drop vulkan if no drivers are built

2017-10-27 Thread Erik Faye-Lund 
This avoids the following build-error when building with emtpy
vulkan-drivers and without glx=dri:

Meson encountered an error in file src/vulkan/wsi/meson.build, line 30,
column 2:
Unknown variable "dep_xcb".

Signed-off-by: Erik Faye-Lund 
---
 src/meson.build | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/meson.build b/src/meson.build
index 9b1b0ae594..4b00ab910c 100644
--- a/src/meson.build
+++ b/src/meson.build
@@ -47,7 +47,9 @@ subdir('mapi')
 # TODO: osmesa
 subdir('compiler')
 subdir('egl/wayland/wayland-drm')
-subdir('vulkan')
+if with_any_vk
+  subdir('vulkan')
+endif
 subdir('amd')
 if with_gallium_vc4
   subdir('broadcom')
-- 
2.11.0

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

Re: [Mesa-dev] [PATCH v3 38/48] intel/fs: Don't use automatic exec size

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> The automatic exec size inference can accidentally mess things up if
> we're not careful.  For instance, if we have
> 
> add(4)g38.2<4>Dg38.1<8,2,4>Dg38.2<8,2,4>D
> 
> then the destination register will end up having a width of 2 with a
> horizontal stride of 4 and a vertical stride of 8.  The EU emit code
> sees the width of 2 and decides that we really wanted an exec size of
> 2
> which doesn't do what we wanted.

Right :-/

I have to say that this change makes me a little nervous, mostly
because it doesn't look easy to identify all the cases where we were
relying on automatic execsizes to fix things up for us... since this is
not as easy as to look for places where we use 'vec1' or something like
that. How did you get the list of things that needed explicit sizes?

Also, both commits before this address cases of exec_size = 1, but we
rely on automatic exec sizes for exec_size = 2 as well, I guess we have
none of these?

Anyway, I guess Jenkins would have caught at least most omissions so
maybe I am being too paranoid.

> ---
>  src/intel/compiler/brw_fs_generator.cpp | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_generator.cpp
> b/src/intel/compiler/brw_fs_generator.cpp
> index 8322be1..46f9a33 100644
> --- a/src/intel/compiler/brw_fs_generator.cpp
> +++ b/src/intel/compiler/brw_fs_generator.cpp
> @@ -190,6 +190,12 @@ fs_generator::fs_generator(const struct
> brw_compiler *compiler, void *log_data,
>  {
> p = rzalloc(mem_ctx, struct brw_codegen);
> brw_init_codegen(devinfo, p, mem_ctx);
> +
> +   /* In the FS code generator, we are very careful to ensure that
> we always
> +* set the right execution size so we don't need the EU code to
> "help" us
> +* by trying to infer it.  Sometimes, it infers the wrong thing.
> +*/
> +   p->automatic_exec_sizes = false;
>  }
>  
>  fs_generator::~fs_generator()
> @@ -395,17 +401,17 @@ fs_generator::generate_fb_write(fs_inst *inst,
> struct brw_reg payload)
>    struct brw_reg v1_null_ud = vec1(retype(brw_null_reg(),
> BRW_REGISTER_TYPE_UD));
>  
>    /* Check runtime bit to detect if we have to send AA data or
> not */
> -  brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
>    brw_push_insn_state(p);
> -  brw_inst_set_exec_size(p->devinfo, brw_last_inst,
> BRW_EXECUTE_1);
> +  brw_set_default_compression_control(p, BRW_COMPRESSION_NONE);
> +  brw_set_default_exec_size(p, BRW_EXECUTE_1);
>    brw_AND(p,
>    v1_null_ud,
>    retype(brw_vec1_grf(1, 6), BRW_REGISTER_TYPE_UD),
>    brw_imm_ud(1<<26));
>    brw_inst_set_cond_modifier(p->devinfo, brw_last_inst,
> BRW_CONDITIONAL_NZ);
> -  brw_pop_insn_state(p);
>  
>    int jmp = brw_JMPI(p, brw_imm_ud(0), BRW_PREDICATE_NORMAL) -
> p->store;
> +  brw_pop_insn_state(p);
>    {
>   /* Don't send AA data */
>   fire_fb_write(inst, offset(payload, 1), implied_header,
> inst->mlen-1);
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Android: move drivers' symlinks to /vendor

2017-10-27 Thread Emil Velikov 
On 26 October 2017 at 23:48, Mauro Rossi  wrote:
> Having moved gallium_dri.so library to /vendor/lib/dri
> also symlinks need to be coherently created using TARGET_OUT_VENDOR insted of 
> TARGET_OUT
> or all non Intel drivers will not be loaded with Android N and earlier,
> thus causing SurfaceFlinger SIGABRT
>
> Fixes: c3f75d483c ("Android: move libraries to /vendor")
>
> Cc: 17.3 
> ---
>  src/gallium/targets/dri/Android.mk | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/targets/dri/Android.mk 
> b/src/gallium/targets/dri/Android.mk
> index 61b65769ff..3fa86a2d56 100644
> --- a/src/gallium/targets/dri/Android.mk
> +++ b/src/gallium/targets/dri/Android.mk
> @@ -70,8 +70,8 @@ LOCAL_SHARED_LIBRARIES += $(sort $(GALLIUM_SHARED_LIBS))
>  ifneq ($(filter 5 6 7, $(MESA_ANDROID_MAJOR_VERSION)),)
>  LOCAL_POST_INSTALL_CMD := \
> $(foreach l, lib $(if $(filter true,$(TARGET_IS_64_BIT)),lib64), \
> - mkdir -p $(TARGET_OUT)/$(l)/$(MESA_DRI_MODULE_REL_PATH); \
> - $(foreach d, $(GALLIUM_TARGET_DRIVERS), ln -sf gallium_dri.so 
> $(TARGET_OUT)/$(l)/$(MESA_DRI_MODULE_REL_PATH)/$(d)_dri.so;) \
> + mkdir -p $(TARGET_OUT_VENDOR)/$(l)/$(MESA_DRI_MODULE_REL_PATH); \
> + $(foreach d, $(GALLIUM_TARGET_DRIVERS), ln -sf gallium_dri.so 
> $(TARGET_OUT_VENDOR)/$(l)/$(MESA_DRI_MODULE_REL_PATH)/$(d)_dri.so;) \
Can we fold the long path into a variable and then reuse it?
This code will be around for a bit, so it might be worth it.

foo=$(TARGET_OUT_VENDOR)/$(l)/$(MESA_DRI_MODULE_REL_PATH)
mkdir -p $(foo)
$(foreach d, $(GALLIUM_TARGET_DRIVERS), ln -sf gallium_dri.so
$(foo)/$(d)_dri.so;)

-Emil
*Please use better variable name than foo
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/compiler/gen9: Pixel shader header only workaround

2017-10-27 Thread Kenneth Graunke 
On Friday, October 27, 2017 4:26:32 AM PDT Pohjolainen, Topi wrote:
> On Fri, Oct 27, 2017 at 03:02:59AM -0700, Kenneth Graunke wrote:
> > On Wednesday, October 25, 2017 10:37:37 AM PDT Topi Pohjolainen wrote:
> > > Fixes intermittent GPU hangs on Broxton with an Intel internal
> > > test case.
> > > 
> > > There are plenty of similar fragment shaders in piglit that do
> > > not use any varyings and any uniforms. According to the
> > > documentation special timing is needed between pipeline stages.
> > > Apparently we just don't hit that with piglit. Even with the
> > > failing test case one doesn't always get the hang.
> > > 
> > > Moreover, according to the error states the hang happens
> > > significantly later than the execution of the problematic shader.
> > > There are multiple render cycles (primitive submissions) in between.
> > > I've also seen error states where the ACTHD points outside the
> > > batch. Almost as if the hardware writes somewhere that gets used
> > > later on. That would also explain why piglit doesn't suffer from
> > > this - most tests kick off one render cycle and any corruption
> > > is left unseen.
> > > 
> > > v2 (Ken): Instead of enabling push constants, enable one of the
> > >   inputs (PSIZ).
> > > v3 (Ken, Jason): Use LAYER instead making vulkan emit_3dstate_sbe()
> > >  happy.
> > > 
> > > CC: Kenneth Graunke 
> > > CC: Jason Ekstrand 
> > > CC: Eero Tamminen 
> > > Signed-off-by: Topi Pohjolainen 
> > > ---
> > >  src/intel/compiler/brw_fs.cpp | 29 +
> > >  1 file changed, 29 insertions(+)
> > 
> > This looks great, thanks a ton for fixing this, Topi!
> > 
> > Cc: "17.3 17.2" 
> 
> We just need to make sure 17.2/3 contain also Iago's:
> 
> 
> commit 566a0c43f0b9fbf5106161471dd5061c7275f761
> Author: Iago Toral Quiroga 
> Date:   Thu Jan 5 13:17:53 2017 +0100
> 
> anv: don't skip the VUE header if we are reading gl_Layer in a fragment 
> shader
> 
> This is the same we do in the GL driver: the hardware provides gl_Layer
> in the VUE header, so when the fragment shader reads it we can't skip it.
> 
> 
> otherwise it'll assert.

Fortunately, both the 17.2 and 17.3 branches already contain that
commit.  I thought you might also need this one:

commit 70cd05d6ac533977f96aa832bbb2886172019f35
Author: Kenneth Graunke 
Date:   Wed Oct 25 09:37:09 2017 -0700

anv: Fix assert about source attrs.

Asserting slot >= 2 made sense when the URB read offset was always 1
(pair of slots).  Commit 566a0c43f0b9fbf5106161471dd5061c7275f761 made
it possible to read from the VUE header in slot 0, by adjusting the
offset to be 0.  So, this assert is now bogus.  Use the one from GL.

Reviewed-by: Jason Ekstrand 

But it looks like you technically don't, since with VARYING_SLOT_LAYER
it'll "continue" and skip over the slot >= 2 assert.

So I think we're fine.


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/compiler/gen9: Pixel shader header only workaround

2017-10-27 Thread Pohjolainen, Topi 
On Fri, Oct 27, 2017 at 03:02:59AM -0700, Kenneth Graunke wrote:
> On Wednesday, October 25, 2017 10:37:37 AM PDT Topi Pohjolainen wrote:
> > Fixes intermittent GPU hangs on Broxton with an Intel internal
> > test case.
> > 
> > There are plenty of similar fragment shaders in piglit that do
> > not use any varyings and any uniforms. According to the
> > documentation special timing is needed between pipeline stages.
> > Apparently we just don't hit that with piglit. Even with the
> > failing test case one doesn't always get the hang.
> > 
> > Moreover, according to the error states the hang happens
> > significantly later than the execution of the problematic shader.
> > There are multiple render cycles (primitive submissions) in between.
> > I've also seen error states where the ACTHD points outside the
> > batch. Almost as if the hardware writes somewhere that gets used
> > later on. That would also explain why piglit doesn't suffer from
> > this - most tests kick off one render cycle and any corruption
> > is left unseen.
> > 
> > v2 (Ken): Instead of enabling push constants, enable one of the
> >   inputs (PSIZ).
> > v3 (Ken, Jason): Use LAYER instead making vulkan emit_3dstate_sbe()
> >  happy.
> > 
> > CC: Kenneth Graunke 
> > CC: Jason Ekstrand 
> > CC: Eero Tamminen 
> > Signed-off-by: Topi Pohjolainen 
> > ---
> >  src/intel/compiler/brw_fs.cpp | 29 +
> >  1 file changed, 29 insertions(+)
> 
> This looks great, thanks a ton for fixing this, Topi!
> 
> Cc: "17.3 17.2" 

We just need to make sure 17.2/3 contain also Iago's:


commit 566a0c43f0b9fbf5106161471dd5061c7275f761
Author: Iago Toral Quiroga 
Date:   Thu Jan 5 13:17:53 2017 +0100

anv: don't skip the VUE header if we are reading gl_Layer in a fragment 
shader

This is the same we do in the GL driver: the hardware provides gl_Layer
in the VUE header, so when the fragment shader reads it we can't skip it.


otherwise it'll assert.

> Reviewed-by: Kenneth Graunke 

Thanks!
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/7] svga: Use __asm__ instead of asm

2017-10-27 Thread Emil Velikov 
On 27 October 2017 at 00:57, Dylan Baker  wrote:
> Which allows the code to be compiled with c99 instead of gnu99.
>
> A little history. This code is guarded by #ifdef __GNUC__, so it's only
> compiled with autotools on *nix, SCons with MSVC wont hit that code.
> However, meson is going to build both MSVC and GCC/Clang paths. As such
> it makes sense to not have to override the std for gcc/clang, but ensure
> that it's not set to gnu99 when building with MSVC when there's a
> straightforward code change that allows removing the need for gnu99.
>
I'm afraid that most of the buildsystem details are off. Patch makes
sense regardless :-)

With a more generic commit message (one example below), the commit is
Reviewed-by: Emil Velikov 

Replace the GNU specific keyword asm with __asm_.
This allows us to remove the explicit request for GNU extensions aka -std=gnu99

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

Re: [Mesa-dev] [PATCH 9/9] meson: build gallium based osmesa

2017-10-27 Thread Eric Engestrom 
On Thursday, 2017-10-26 13:55:35 -0700, Dylan Baker wrote:
> Quoting Eric Engestrom (2017-10-26 02:40:20)
> > On Wednesday, 2017-10-25 15:58:23 -0700, Dylan Baker wrote:
> > > This has been tested with the osdemo from mesa-demos
> > > 
> > > Signed-off-by: Dylan Baker 
> > > ---
> > >  meson.build   |  3 ++
> > >  meson_options.txt |  2 +-
> > >  src/gallium/meson.build   |  7 ++-
> > >  src/gallium/state_trackers/osmesa/meson.build | 28 +++
> > >  src/gallium/targets/osmesa/meson.build| 68 
> > > +++
> > >  5 files changed, 106 insertions(+), 2 deletions(-)
> > >  create mode 100644 src/gallium/state_trackers/osmesa/meson.build
> > >  create mode 100644 src/gallium/targets/osmesa/meson.build
> > > 
> > > diff --git a/meson.build b/meson.build
> > > index 79ce59c6b27..0bbe330042b 100644
> > > --- a/meson.build
> > > +++ b/meson.build
> > > @@ -704,6 +704,9 @@ if with_osmesa != 'none'
> > >if with_osmesa == 'classic' and not with_dri_swrast
> > >  error('OSMesa classic requires dri (classic) swrast.')
> > >endif
> > > +  if with_osmesa == 'gallium' and not with_gallium_softpipe
> > > +error('OSMesa gallium requires gallium softpipe or llvmpipe.')
> > > +  endif
> > >osmesa_lib_name = 'OSMesa'
> > >osmesa_bits = get_option('osmesa-bits')
> > >if osmesa_bits != '8'
> > > diff --git a/meson_options.txt b/meson_options.txt
> > > index 97aca571a48..a0b8044e4bb 100644
> > > --- a/meson_options.txt
> > > +++ b/meson_options.txt
> > > @@ -164,7 +164,7 @@ option(
> > >'osmesa',
> > >type : 'combo',
> > >value : 'none',
> > > -  choices : ['none', 'classic'],
> > > +  choices : ['none', 'classic', 'gallium'],
> > >description : 'Build OSmesa.'
> > >  )
> > >  option(
> > > diff --git a/src/gallium/meson.build b/src/gallium/meson.build
> > > index e0941103b93..6edfe80321d 100644
> > > --- a/src/gallium/meson.build
> > > +++ b/src/gallium/meson.build
> > > @@ -66,6 +66,9 @@ if with_gallium_imx
> > >subdir('winsys/imx/drm')
> > >  endif
> > >  subdir('state_trackers/dri')
> > > +if with_osmesa == 'gallium'
> > > +  subdir('state_trackers/osmesa')
> > > +endif
> > >  # TODO: i915
> > >  # TODO: SVGA
> > >  # TODO: r300
> > > @@ -77,9 +80,11 @@ subdir('state_trackers/dri')
> > >  if with_dri and with_gallium
> > >subdir('targets/dri')
> > >  endif
> > > +if with_osmesa == 'gallium'
> > > +  subdir('targets/osmesa')
> > > +endif
> > >  # TODO: xlib-glx
> > >  # TODO: OMX
> > > -# TODO: osmesa
> > >  # TODO: VA
> > >  # TODO: vdpau
> > >  # TODO: xa
> > > diff --git a/src/gallium/state_trackers/osmesa/meson.build 
> > > b/src/gallium/state_trackers/osmesa/meson.build
> > > new file mode 100644
> > > index 000..dacf10512d6
> > > --- /dev/null
> > > +++ b/src/gallium/state_trackers/osmesa/meson.build
> > > @@ -0,0 +1,28 @@
> > > +# Copyright © 2017 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 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.
> > > +
> > > +libosmesa_st = static_library(
> > > +  'osmesa_st',
> > > +  'osmesa.c',
> > > +  c_args : ['-DGALLIUM_SOFTPIPE', '-DGALLIUM_TRACE'],
> > > +  include_directories : [
> > > +inc_include, inc_src, inc_gallium, inc_gallium_aux, inc_mapi, 
> > > inc_mesa,
> > > +  ],
> > > +)
> > > diff --git a/src/gallium/targets/osmesa/meson.build 
> > > b/src/gallium/targets/osmesa/meson.build
> > > new file mode 100644
> > > index 000..af81c5adbbe
> > > --- /dev/null
> > > +++ b/src/gallium/targets/osmesa/meson.build
> > > @@ -0,0 +1,68 @@
> > > +# Copyright © 2017 Intel Corporation
> > > +
> > > +# Permission is hereby granted, free of charge, to any person obtaining 
> > > a copy
> > > +# of this software and associated

Re: [Mesa-dev] [PATCH 1/2] i965: remove if conditions from scratch_bo unref

2017-10-27 Thread Emil Velikov 
For the series
Reviewed-by: Emil Velikov 

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

Re: [Mesa-dev] [PATCH v3 34/48] intel/fs: Rework zero-length URB write handling

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> Originally we tried to handle this case based on
> slots_valid.  However,
> there are a number of ways that this can go wrong.  For one, we throw
> away any trailing slots which either aren't written or are set to
> VARYING_SLOT_PAD. 

I don't get this... is slots_valid is 0 it means that we don't have any
outputs to write, so why would it be a problem to emit a minimal URB
write and return early in that case?

>  Second, even if PSIZ is a valid slot, we may not
> actually write anything there.

Yes, I see this can happen.

>   Between the lot of these, it was
> possible to end up in a case where we tried to do a regular URB write
> but ended up with a length of 1 which is invalid.  This commit moves
> it
> to the end and makes it based on a new boolean flag urb_written.

This looks good to me, in the end we need to cover the case where we
don't write PSIZ so moving the code to the end of the function when we
know if we have actually written anything or not makes sense.

> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/intel/compiler/brw_fs_visitor.cpp | 60 ++---
> --
>  1 file changed, 31 insertions(+), 29 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_visitor.cpp
> b/src/intel/compiler/brw_fs_visitor.cpp
> index 9fd4c20..9a19dc2 100644
> --- a/src/intel/compiler/brw_fs_visitor.cpp
> +++ b/src/intel/compiler/brw_fs_visitor.cpp
> @@ -566,34 +566,6 @@ fs_visitor::emit_urb_writes(const fs_reg
> _vertex_count)
> else
>    urb_handle = fs_reg(retype(brw_vec8_grf(1, 0),
> BRW_REGISTER_TYPE_UD));
>  
> -   /* If we don't have any valid slots to write, just do a minimal
> urb write
> -* send to terminate the shader.  This includes 1 slot of
> undefined data,
> -* because it's invalid to write 0 data:
> -*
> -* From the Broadwell PRM, Volume 7: 3D Media GPGPU, Shared
> Functions -
> -* Unified Return Buffer (URB) > URB_SIMD8_Write and
> URB_SIMD8_Read >
> -* Write Data Payload:
> -*
> -*"The write data payload can be between 1 and 8 message
> phases long."
> -*/
> -   if (vue_map->slots_valid == 0) {
> -  /* For GS, just turn EmitVertex() into a no-op.  We don't want
> it to
> -   * end the thread, and emit_gs_thread_end() already emits a
> SEND with
> -   * EOT at the end of the program for us.
> -   */
> -  if (stage == MESA_SHADER_GEOMETRY)
> - return;
> -
> -  fs_reg payload = fs_reg(VGRF, alloc.allocate(2),
> BRW_REGISTER_TYPE_UD);
> -  bld.exec_all().MOV(payload, urb_handle);
> -
> -  fs_inst *inst = bld.emit(SHADER_OPCODE_URB_WRITE_SIMD8,
> reg_undef, payload);
> -  inst->eot = true;
> -  inst->mlen = 2;
> -  inst->offset = 1;
> -  return;
> -   }
> -
> opcode opcode = SHADER_OPCODE_URB_WRITE_SIMD8;
> int header_size = 1;
> fs_reg per_slot_offsets;
> @@ -645,6 +617,7 @@ fs_visitor::emit_urb_writes(const fs_reg
> _vertex_count)
>    last_slot--;
> }
>  
> +   bool urb_written = false;
> for (slot = 0; slot < vue_map->num_slots; slot++) {
>    int varying = vue_map->slot_to_varying[slot];
>    switch (varying) {
> @@ -730,7 +703,7 @@ fs_visitor::emit_urb_writes(const fs_reg
> _vertex_count)
> * the last slot or if we need to flush (see BAD_FILE varying
> case
> * above), emit a URB write send now to flush out the data.
> */
> -  if (length == 8 || slot == last_slot)
> +  if (length == 8 || (length > 0 && slot == last_slot))
>   flush = true;
>    if (flush) {
>   fs_reg *payload_sources =
> @@ -755,8 +728,37 @@ fs_visitor::emit_urb_writes(const fs_reg
> _vertex_count)
>   urb_offset = starting_urb_offset + slot + 1;
>   length = 0;
>   flush = false;
> + urb_written = true;
>    }
> }
> +
> +   /* If we don't have any valid slots to write, just do a minimal
> urb write
> +* send to terminate the shader.  This includes 1 slot of
> undefined data,
> +* because it's invalid to write 0 data:
> +*
> +* From the Broadwell PRM, Volume 7: 3D Media GPGPU, Shared
> Functions -
> +* Unified Return Buffer (URB) > URB_SIMD8_Write and
> URB_SIMD8_Read >
> +* Write Data Payload:
> +*
> +*"The write data payload can be between 1 and 8 message
> phases long."
> +*/
> +   if (!urb_written) {
> +  /* For GS, just turn EmitVertex() into a no-op.  We don't want
> it to
> +   * end the thread, and emit_gs_thread_end() already emits a
> SEND with
> +   * EOT at the end of the program for us.
> +   */
> +  if (stage == MESA_SHADER_GEOMETRY)
> + return;
> +
> +  fs_reg payload = fs_reg(VGRF, alloc.allocate(2),
> BRW_REGISTER_TYPE_UD);
> +  bld.exec_all().MOV(payload, urb_handle);
> +
> +  fs_inst *inst = bld.emit(SHADER_OPCODE_URB_WRITE_SIMD8,
> reg_undef, payload);
> +  inst->eot = true;
> +  inst->mlen = 2;
> +  

Re: [Mesa-dev] [PATCH v4 2/2] glsl: fix interpolateAtXxx(some_vec[idx], ...) with dynamic idx

2017-10-27 Thread Timothy Arceri 

Reviewed-by: Timothy Arceri 

On 10/10/17 23:09, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

The dynamic index of a vector (not array!) is lowered to a sequence of
conditional assignments. However, the interpolate_at_* expressions
require that the interpolant is an l-value of a shader input.

So instead of doing conditional assignments of parts of the shader input
and then interpolating that (which is nonsensical), we interpolate the
entire shader input and then do conditional assignments of the interpolated
result.
---
  .../glsl/lower_vec_index_to_cond_assign.cpp| 31 +-
  1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/compiler/glsl/lower_vec_index_to_cond_assign.cpp 
b/src/compiler/glsl/lower_vec_index_to_cond_assign.cpp
index a26253998e0..89244266602 100644
--- a/src/compiler/glsl/lower_vec_index_to_cond_assign.cpp
+++ b/src/compiler/glsl/lower_vec_index_to_cond_assign.cpp
@@ -121,21 +121,50 @@ 
ir_vec_index_to_cond_assign_visitor::convert_vec_index_to_cond_assign(void *mem_
  
 this->progress = true;

 return deref(var).val;
  }
  
  ir_rvalue *

  
ir_vec_index_to_cond_assign_visitor::convert_vector_extract_to_cond_assign(ir_rvalue
 *ir)
  {
 ir_expression *const expr = ir->as_expression();
  
-   if (expr == NULL || expr->operation != ir_binop_vector_extract)

+   if (expr == NULL)
+  return ir;
+
+   if (expr->operation == ir_unop_interpolate_at_centroid ||
+   expr->operation == ir_binop_interpolate_at_offset ||
+   expr->operation == ir_binop_interpolate_at_sample) {
+  /* Lower interpolateAtXxx(some_vec[idx], ...) to
+   * interpolateAtXxx(some_vec, ...)[idx] before lowering to conditional
+   * assignments, to maintain the rule that the interpolant is an l-value
+   * referring to a (part of a) shader input.
+   *
+   * This is required when idx is dynamic (otherwise it gets lowered to
+   * a swizzle).
+   */
+  ir_expression *const interpolant = expr->operands[0]->as_expression();
+  if (!interpolant || interpolant->operation != ir_binop_vector_extract)
+ return ir;
+
+  ir_rvalue *vec_input = interpolant->operands[0];
+  ir_expression *const vec_interpolate =
+ new(base_ir) ir_expression(expr->operation, vec_input->type,
+vec_input, expr->operands[1]);
+
+  return convert_vec_index_to_cond_assign(ralloc_parent(ir),
+  vec_interpolate,
+  interpolant->operands[1],
+  ir->type);
+   }
+
+   if (expr->operation != ir_binop_vector_extract)
return ir;
  
 return convert_vec_index_to_cond_assign(ralloc_parent(ir),

 expr->operands[0],
 expr->operands[1],
 ir->type);
  }
  
  ir_visitor_status

  ir_vec_index_to_cond_assign_visitor::visit_enter(ir_expression *ir)


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

Re: [Mesa-dev] [PATCH v4 1/2] glsl: allow any l-value of an input variable as interpolant in interpolateAt*

2017-10-27 Thread Timothy Arceri 

I meant to review this a while ago. Sorry for the delay.

Reviewed-by: Timothy Arceri 

On 10/10/17 23:09, Nicolai Hähnle wrote:

From: Nicolai Hähnle 

The intended rule has been clarified in GLSL 4.60, Section 8.13.2
(Interpolation Functions):

"For all of the interpolation functions, interpolant must be an l-value
 from an in declaration; this can include a variable, a block or
 structure member, an array element, or some combination of these.
 Component selection operators (e.g., .xy) may be used when specifying
 interpolant."

For members of interface blocks, var->data.must_be_shader_input must be
determined on-the-fly after lowering interface blocks, since we don't want
to disable varying packing for an entire block just because one input in it
is used in interpolateAt*.

v2: keep setting must_be_shader_input in ast_function (Ian)
v3: follow the relaxed rule of GLSL 4.60
v4: only apply the relaxed rules to desktop GL
 (the ES WG decided that the relaxed rules may apply in a future version
  but not retroactively; see also
  
dEQP-GLES31.functional.shaders.multisample_interpolation.interpolate_at_centroid.negative.*)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101378
Reviewed-by: Ian Romanick  (v1)
---
  src/compiler/glsl/ast_function.cpp | 19 ++-
  src/compiler/glsl/lower_named_interface_blocks.cpp | 18 ++
  2 files changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/compiler/glsl/ast_function.cpp 
b/src/compiler/glsl/ast_function.cpp
index 46a61e46fd5..d1596c272e6 100644
--- a/src/compiler/glsl/ast_function.cpp
+++ b/src/compiler/glsl/ast_function.cpp
@@ -220,33 +220,42 @@ verify_parameter_modes(_mesa_glsl_parse_state *state,
   if (val->ir_type == ir_type_swizzle) {
  if (!state->is_version(440, 0)) {
 _mesa_glsl_error(, state,
  "parameter `%s` must not be swizzled",
  formal->name);
 return false;
  }
  val = ((ir_swizzle *)val)->val;
   }
  
- while (val->ir_type == ir_type_dereference_array) {

-val = ((ir_dereference_array *)val)->array;
+ for (;;) {
+if (val->ir_type == ir_type_dereference_array) {
+   val = ((ir_dereference_array *)val)->array;
+} else if (val->ir_type == ir_type_dereference_record &&
+   !state->es_shader) {
+   val = ((ir_dereference_record *)val)->record;
+} else
+   break;
   }
  
- if (!val->as_dereference_variable() ||

- val->variable_referenced()->data.mode != ir_var_shader_in) {
+ ir_variable *var = NULL;
+ if (const ir_dereference_variable *deref_var = 
val->as_dereference_variable())
+var = deref_var->variable_referenced();
+
+ if (!var || var->data.mode != ir_var_shader_in) {
  _mesa_glsl_error(, state,
   "parameter `%s` must be a shader input",
   formal->name);
  return false;
   }
  
- val->variable_referenced()->data.must_be_shader_input = 1;

+ var->data.must_be_shader_input = 1;
}
  
/* Verify that 'out' and 'inout' actual parameters are lvalues. */

if (formal->data.mode == ir_var_function_out
|| formal->data.mode == ir_var_function_inout) {
   const char *mode = NULL;
   switch (formal->data.mode) {
   case ir_var_function_out:   mode = "out";   break;
   case ir_var_function_inout: mode = "inout"; break;
   default:assert(false);  break;
diff --git a/src/compiler/glsl/lower_named_interface_blocks.cpp 
b/src/compiler/glsl/lower_named_interface_blocks.cpp
index 064694128bf..136352a131b 100644
--- a/src/compiler/glsl/lower_named_interface_blocks.cpp
+++ b/src/compiler/glsl/lower_named_interface_blocks.cpp
@@ -108,20 +108,21 @@ public:
  
 flatten_named_interface_blocks_declarations(void *mem_ctx)

: mem_ctx(mem_ctx),
  interface_namespace(NULL)
 {
 }
  
 void run(exec_list *instructions);
  
 virtual ir_visitor_status visit_leave(ir_assignment *);

+   virtual ir_visitor_status visit_leave(ir_expression *);
 virtual void handle_rvalue(ir_rvalue **rvalue);
  };
  
  } /* anonymous namespace */
  
  void

  flatten_named_interface_blocks_declarations::run(exec_list *instructions)
  {
 interface_namespace = _mesa_hash_table_create(NULL, _mesa_key_hash_string,
   _mesa_key_string_equal);
@@ -231,20 +232,37 @@ 
flatten_named_interface_blocks_declarations::visit_leave(ir_assignment *ir)
}
  
ir_variable *lhs_var =  lhs_rec_tmp->variable_referenced();

if

Re: [Mesa-dev] [PATCH 1/2] i965: remove if conditions from scratch_bo unref

2017-10-27 Thread Kenneth Graunke 
On Friday, October 27, 2017 2:56:45 AM PDT Tapani Pälli wrote:
> brw_bo_unreference handles NULL case
> 
> Signed-off-by: Tapani Pälli 
> ---
>  src/mesa/drivers/dri/i965/brw_context.c | 16 ++--
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
> b/src/mesa/drivers/dri/i965/brw_context.c
> index c8de074638..39b2a938f6 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1061,16 +1061,12 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> brw_draw_destroy(brw);
>  
> brw_bo_unreference(brw->curbe.curbe_bo);
> -   if (brw->vs.base.scratch_bo)
> -  brw_bo_unreference(brw->vs.base.scratch_bo);
> -   if (brw->tcs.base.scratch_bo)
> -  brw_bo_unreference(brw->tcs.base.scratch_bo);
> -   if (brw->tes.base.scratch_bo)
> -  brw_bo_unreference(brw->tes.base.scratch_bo);
> -   if (brw->gs.base.scratch_bo)
> -  brw_bo_unreference(brw->gs.base.scratch_bo);
> -   if (brw->wm.base.scratch_bo)
> -  brw_bo_unreference(brw->wm.base.scratch_bo);
> +
> +   brw_bo_unreference(brw->vs.base.scratch_bo);
> +   brw_bo_unreference(brw->tcs.base.scratch_bo);
> +   brw_bo_unreference(brw->tes.base.scratch_bo);
> +   brw_bo_unreference(brw->gs.base.scratch_bo);
> +   brw_bo_unreference(brw->wm.base.scratch_bo);
>  
> brw_destroy_hw_context(brw->bufmgr, brw->hw_ctx);
>  
> 

Series is:
Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 31/48] intel/cs: Re-run final NIR optimizations for each SIMD size

2017-10-27 Thread Iago Toral 
This should be squashed into the previous commit

On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> With the advent of SPIR-V subgroup operations, compute shaders will
> have
> to be slightly different depending on the SIMD size at which they
> execute.  In order to allow us to do dispatch-width specific things
> in
> NIR, we re-run the final NIR stages for each sIMD width.
> 
> As a side-effect of this change, we start using ralloc on fs_visitor
> so
> we need to add DECLARE_RALLOC_OPERATORS to fs_visitor.
> ---
>  src/intel/compiler/brw_fs.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/intel/compiler/brw_fs.h
> b/src/intel/compiler/brw_fs.h
> index d3ab385..9ff06b6 100644
> --- a/src/intel/compiler/brw_fs.h
> +++ b/src/intel/compiler/brw_fs.h
> @@ -60,7 +60,7 @@ offset(const fs_reg , const brw::fs_builder
> , unsigned delta)
>  class fs_visitor : public backend_shader
>  {
>  public:
> -   DECLARE_RALLOC_CXX_OPERATORS(fs_reg)
> +   DECLARE_RALLOC_CXX_OPERATORS(fs_visitor)
>  
> fs_visitor(const struct brw_compiler *compiler, void *log_data,
>    void *mem_ctx,
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel/compiler/gen9: Pixel shader header only workaround

2017-10-27 Thread Kenneth Graunke 
On Wednesday, October 25, 2017 10:37:37 AM PDT Topi Pohjolainen wrote:
> Fixes intermittent GPU hangs on Broxton with an Intel internal
> test case.
> 
> There are plenty of similar fragment shaders in piglit that do
> not use any varyings and any uniforms. According to the
> documentation special timing is needed between pipeline stages.
> Apparently we just don't hit that with piglit. Even with the
> failing test case one doesn't always get the hang.
> 
> Moreover, according to the error states the hang happens
> significantly later than the execution of the problematic shader.
> There are multiple render cycles (primitive submissions) in between.
> I've also seen error states where the ACTHD points outside the
> batch. Almost as if the hardware writes somewhere that gets used
> later on. That would also explain why piglit doesn't suffer from
> this - most tests kick off one render cycle and any corruption
> is left unseen.
> 
> v2 (Ken): Instead of enabling push constants, enable one of the
>   inputs (PSIZ).
> v3 (Ken, Jason): Use LAYER instead making vulkan emit_3dstate_sbe()
>  happy.
> 
> CC: Kenneth Graunke 
> CC: Jason Ekstrand 
> CC: Eero Tamminen 
> Signed-off-by: Topi Pohjolainen 
> ---
>  src/intel/compiler/brw_fs.cpp | 29 +
>  1 file changed, 29 insertions(+)

This looks great, thanks a ton for fixing this, Topi!

Cc: "17.3 17.2" 
Reviewed-by: Kenneth Graunke 


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: unref push_const_bo in intelDestroyContext

2017-10-27 Thread Tapani Pälli 



On 10/27/2017 12:57 PM, Kenneth Graunke wrote:

On Friday, October 27, 2017 2:08:36 AM PDT Emil Velikov wrote:

On 27 October 2017 at 07:52, Tapani Pälli  wrote:

Valgrind shows that leak is caused by gen6_upload_push_constant, add
unref push_const_bo per stage to destructor to fix this (like done for
scratch_bo).

==10952== 144 bytes in 1 blocks are definitely lost in loss record 44 of 66
==10952==at 0x4C30A1E: calloc (vg_replace_malloc.c:711)
==10952==by 0x8C02847: bo_alloc_internal.constprop.10 (brw_bufmgr.c:344)
==10952==by 0x8C425C4: intel_upload_space (intel_upload.c:101)
==10952==by 0x8C22ED0: gen6_upload_push_constants 
(gen6_constant_state.c:154)

Fixes: 24891d7c05 ("i965: Store per-stage push constant BO pointers.")
Signed-off-by: Tapani Pälli 
Cc: mesa-sta...@lists.freedesktop.org
---
  src/mesa/drivers/dri/i965/brw_context.c | 11 +++
  1 file changed, 11 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index c8de074638..61088e2f1f 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1072,6 +1072,17 @@ intelDestroyContext(__DRIcontext * driContextPriv)
 if (brw->wm.base.scratch_bo)
brw_bo_unreference(brw->wm.base.scratch_bo);

+   if (brw->vs.base.push_const_bo)

I'd drop the if checks - brw_bo_unreference works fine when the bo
pointer is NULL.

With that the patch is
Reviewed-by: Emil Velikov 

-Emil


Likewise, with the ifs gone,
Reviewed-by: Kenneth Graunke 

Thanks for fixing my mistake...sorry for the leaks!


No problem, I sent separate patch also to remove the if's from 
scratch_bo unrefs.


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

Re: [Mesa-dev] [PATCH v3 30/48] intel/cs: Re-run final NIR optimizations for each SIMD size

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> With the advent of SPIR-V subgroup operations, compute shaders will
> have
> to be slightly different depending on the SIMD size at which they
> execute.  In order to allow us to do dispatch-width specific things
> in
> NIR, we re-run the final NIR stages for each sIMD width.
> 
> One side-effect of this change is that we start rallocing fs_visitors
> which means we need DECLARE_RALLOC_CXX_OPERATORS.
> ---
>  src/intel/compiler/brw_fs.cpp | 103 ++
> 
>  src/intel/compiler/brw_fs.h   |   2 +
>  2 files changed, 66 insertions(+), 39 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index c0d4c05..c054537 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -6770,6 +6770,20 @@ cs_set_simd_size(struct brw_cs_prog_data
> *cs_prog_data, unsigned size)
> cs_prog_data->threads = (group_size + size - 1) / size;
>  }
>  
> +static nir_shader *
> +compile_cs_to_nir(const struct brw_compiler *compiler,
> +  void *mem_ctx,
> +  const struct brw_cs_prog_key *key,
> +  struct brw_cs_prog_data *prog_data,
> +  const nir_shader *src_shader,
> +  unsigned dispatch_width)
> +{
> +   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> +   shader = brw_nir_apply_sampler_key(shader, compiler, >tex,
> true);
> +   brw_nir_lower_cs_intrinsics(shader);
> +   return brw_postprocess_nir(shader, compiler, true);
> +}
> +
>  const unsigned *
>  brw_compile_cs(const struct brw_compiler *compiler, void *log_data,
> void *mem_ctx,
> @@ -6780,17 +6794,12 @@ brw_compile_cs(const struct brw_compiler
> *compiler, void *log_data,
> unsigned *final_assembly_size,
> char **error_str)
>  {
> -   nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> -   shader = brw_nir_apply_sampler_key(shader, compiler, >tex,
> true);
> -   brw_nir_lower_cs_intrinsics(shader);
> -   shader = brw_postprocess_nir(shader, compiler, true);
> -
> -   prog_data->local_size[0] = shader->info.cs.local_size[0];
> -   prog_data->local_size[1] = shader->info.cs.local_size[1];
> -   prog_data->local_size[2] = shader->info.cs.local_size[2];
> +   prog_data->local_size[0] = src_shader->info.cs.local_size[0];
> +   prog_data->local_size[1] = src_shader->info.cs.local_size[1];
> +   prog_data->local_size[2] = src_shader->info.cs.local_size[2];
> unsigned local_workgroup_size =
> -  shader->info.cs.local_size[0] * shader->info.cs.local_size[1]
> *
> -  shader->info.cs.local_size[2];
> +  src_shader->info.cs.local_size[0] * src_shader-
> >info.cs.local_size[1] *
> +  src_shader->info.cs.local_size[2];
>  
> unsigned min_dispatch_width =
>    DIV_ROUND_UP(local_workgroup_size, compiler->devinfo-
> >max_cs_threads);
> @@ -6798,71 +6807,87 @@ brw_compile_cs(const struct brw_compiler
> *compiler, void *log_data,
> min_dispatch_width = util_next_power_of_two(min_dispatch_width);
> assert(min_dispatch_width <= 32);
>  
> +

Extra blank line

> +   fs_visitor *v8 = NULL, *v16 = NULL, *v32 = NULL;
> cfg_t *cfg = NULL;
> const char *fail_msg = NULL;
> +   unsigned promoted_constants;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] i965: unref push_const_bo in intelDestroyContext

2017-10-27 Thread Kenneth Graunke 
On Friday, October 27, 2017 2:08:36 AM PDT Emil Velikov wrote:
> On 27 October 2017 at 07:52, Tapani Pälli  wrote:
> > Valgrind shows that leak is caused by gen6_upload_push_constant, add
> > unref push_const_bo per stage to destructor to fix this (like done for
> > scratch_bo).
> >
> >==10952== 144 bytes in 1 blocks are definitely lost in loss record 44 of 
> > 66
> >==10952==at 0x4C30A1E: calloc (vg_replace_malloc.c:711)
> >==10952==by 0x8C02847: bo_alloc_internal.constprop.10 
> > (brw_bufmgr.c:344)
> >==10952==by 0x8C425C4: intel_upload_space (intel_upload.c:101)
> >==10952==by 0x8C22ED0: gen6_upload_push_constants 
> > (gen6_constant_state.c:154)
> >
> > Fixes: 24891d7c05 ("i965: Store per-stage push constant BO pointers.")
> > Signed-off-by: Tapani Pälli 
> > Cc: mesa-sta...@lists.freedesktop.org
> > ---
> >  src/mesa/drivers/dri/i965/brw_context.c | 11 +++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
> > b/src/mesa/drivers/dri/i965/brw_context.c
> > index c8de074638..61088e2f1f 100644
> > --- a/src/mesa/drivers/dri/i965/brw_context.c
> > +++ b/src/mesa/drivers/dri/i965/brw_context.c
> > @@ -1072,6 +1072,17 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> > if (brw->wm.base.scratch_bo)
> >brw_bo_unreference(brw->wm.base.scratch_bo);
> >
> > +   if (brw->vs.base.push_const_bo)
> I'd drop the if checks - brw_bo_unreference works fine when the bo
> pointer is NULL.
> 
> With that the patch is
> Reviewed-by: Emil Velikov 
> 
> -Emil

Likewise, with the ifs gone,
Reviewed-by: Kenneth Graunke 

Thanks for fixing my mistake...sorry for the leaks!


signature.asc
Description: This is a digitally signed message part.
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 2/2] i965: unref push_const_bo in intelDestroyContext

2017-10-27 Thread Tapani Pälli 
Valgrind shows that leak is caused by gen6_upload_push_constant, add
unref push_const_bo per stage to destructor to fix this (like done for
scratch_bo).

   ==10952== 144 bytes in 1 blocks are definitely lost in loss record 44 of 66
   ==10952==at 0x4C30A1E: calloc (vg_replace_malloc.c:711)
   ==10952==by 0x8C02847: bo_alloc_internal.constprop.10 (brw_bufmgr.c:344)
   ==10952==by 0x8C425C4: intel_upload_space (intel_upload.c:101)
   ==10952==by 0x8C22ED0: gen6_upload_push_constants 
(gen6_constant_state.c:154)

v2: remove if conditions, brw_bo_unreference handles NULL (Ken, Emil)

Fixes: 24891d7c05 ("i965: Store per-stage push constant BO pointers.")
Signed-off-by: Tapani Pälli 
Cc: mesa-sta...@lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/brw_context.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index 39b2a938f6..eed42468b1 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1068,6 +1068,12 @@ intelDestroyContext(__DRIcontext * driContextPriv)
brw_bo_unreference(brw->gs.base.scratch_bo);
brw_bo_unreference(brw->wm.base.scratch_bo);
 
+   brw_bo_unreference(brw->vs.base.push_const_bo);
+   brw_bo_unreference(brw->tcs.base.push_const_bo);
+   brw_bo_unreference(brw->tes.base.push_const_bo);
+   brw_bo_unreference(brw->gs.base.push_const_bo);
+   brw_bo_unreference(brw->wm.base.push_const_bo);
+
brw_destroy_hw_context(brw->bufmgr, brw->hw_ctx);
 
if (ctx->swrast_context) {
-- 
2.13.6

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

[Mesa-dev] [PATCH 1/2] i965: remove if conditions from scratch_bo unref

2017-10-27 Thread Tapani Pälli 
brw_bo_unreference handles NULL case

Signed-off-by: Tapani Pälli 
---
 src/mesa/drivers/dri/i965/brw_context.c | 16 ++--
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index c8de074638..39b2a938f6 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1061,16 +1061,12 @@ intelDestroyContext(__DRIcontext * driContextPriv)
brw_draw_destroy(brw);
 
brw_bo_unreference(brw->curbe.curbe_bo);
-   if (brw->vs.base.scratch_bo)
-  brw_bo_unreference(brw->vs.base.scratch_bo);
-   if (brw->tcs.base.scratch_bo)
-  brw_bo_unreference(brw->tcs.base.scratch_bo);
-   if (brw->tes.base.scratch_bo)
-  brw_bo_unreference(brw->tes.base.scratch_bo);
-   if (brw->gs.base.scratch_bo)
-  brw_bo_unreference(brw->gs.base.scratch_bo);
-   if (brw->wm.base.scratch_bo)
-  brw_bo_unreference(brw->wm.base.scratch_bo);
+
+   brw_bo_unreference(brw->vs.base.scratch_bo);
+   brw_bo_unreference(brw->tcs.base.scratch_bo);
+   brw_bo_unreference(brw->tes.base.scratch_bo);
+   brw_bo_unreference(brw->gs.base.scratch_bo);
+   brw_bo_unreference(brw->wm.base.scratch_bo);
 
brw_destroy_hw_context(brw->bufmgr, brw->hw_ctx);
 
-- 
2.13.6

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

Re: [Mesa-dev] [PATCH] formatquery: use correct target check for IMAGE_FORMAT_COMPATIBILITY_TYPE

2017-10-27 Thread Antia Puentes 

Thanks for fixing this.

Reviewed-by: Antia Puentes 


On 27/10/17 11:18, Alejandro Piñeiro wrote:

 From the spec:
"IMAGE_FORMAT_COMPATIBILITY_TYPE: The matching criteria use for the
 resource when used as an image textures is returned in
 . This is equivalent to calling GetTexParameter"

So we would need to return None for any target not supported by
GetTexParameter. By mistake, we were using the target check for
GetTexLevelParameter.
---
  src/mesa/main/formatquery.c | 8 +++-
  1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
index 77c7faa2251..39c628039b8 100644
--- a/src/mesa/main/formatquery.c
+++ b/src/mesa/main/formatquery.c
@@ -1430,7 +1430,13 @@ _mesa_GetInternalformativ(GLenum target, GLenum 
internalformat, GLenum pname,
if (!_mesa_has_ARB_shader_image_load_store(ctx))
   goto end;
  
-  if (!_mesa_legal_get_tex_level_parameter_target(ctx, target, true))

+  /* As pointed by the spec quote below, this pname query should return
+   * the same value that GetTexParameter. So if the target is not valid
+   * for GetTexParameter we return the unsupported value. The check below
+   * is the same target check used by GetTextParameter.
+   */
+  int targetIndex = _mesa_tex_target_to_index(ctx, target);
+  if (targetIndex < 0 || targetIndex == TEXTURE_BUFFER_INDEX)
   goto end;
  
/* From spec: "Equivalent to calling GetTexParameter with  set


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

[Mesa-dev] [PATCH] formatquery: use correct target check for IMAGE_FORMAT_COMPATIBILITY_TYPE

2017-10-27 Thread Alejandro Piñeiro 
From the spec:
   "IMAGE_FORMAT_COMPATIBILITY_TYPE: The matching criteria use for the
resource when used as an image textures is returned in
. This is equivalent to calling GetTexParameter"

So we would need to return None for any target not supported by
GetTexParameter. By mistake, we were using the target check for
GetTexLevelParameter.
---
 src/mesa/main/formatquery.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/mesa/main/formatquery.c b/src/mesa/main/formatquery.c
index 77c7faa2251..39c628039b8 100644
--- a/src/mesa/main/formatquery.c
+++ b/src/mesa/main/formatquery.c
@@ -1430,7 +1430,13 @@ _mesa_GetInternalformativ(GLenum target, GLenum 
internalformat, GLenum pname,
   if (!_mesa_has_ARB_shader_image_load_store(ctx))
  goto end;
 
-  if (!_mesa_legal_get_tex_level_parameter_target(ctx, target, true))
+  /* As pointed by the spec quote below, this pname query should return
+   * the same value that GetTexParameter. So if the target is not valid
+   * for GetTexParameter we return the unsupported value. The check below
+   * is the same target check used by GetTextParameter.
+   */
+  int targetIndex = _mesa_tex_target_to_index(ctx, target);
+  if (targetIndex < 0 || targetIndex == TEXTURE_BUFFER_INDEX)
  goto end;
 
   /* From spec: "Equivalent to calling GetTexParameter with  set
-- 
2.11.0

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

Re: [Mesa-dev] [PATCH v3 29/48] intel/cs: Rework the way thread local ID is handled

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:26 -0700, Jason Ekstrand wrote:
> Previously, brw_nir_lower_intrinsics added the param and then emitted
> a
> load_uniform intrinsic to load it directly.  This commit switches
> things
> over to use a specific NIR intrinsic for the thread id.  The one
> thing I
> don't like about this approach is that we have to copy
> thread_local_id
> over to the new visitor in import_uniforms.

It is not clear to me why you are doing this... why do you like this
better?

> ---
>  src/compiler/nir/nir_intrinsics.h|  3 ++
>  src/intel/compiler/brw_fs.cpp|  4 +-
>  src/intel/compiler/brw_fs.h  |  1 +
>  src/intel/compiler/brw_fs_nir.cpp| 14 +++
>  src/intel/compiler/brw_nir.h |  3 +-
>  src/intel/compiler/brw_nir_lower_cs_intrinsics.c | 53 +-
> --
>  6 files changed, 32 insertions(+), 46 deletions(-)
> 
> diff --git a/src/compiler/nir/nir_intrinsics.h
> b/src/compiler/nir/nir_intrinsics.h
> index cefd18b..47022dd 100644
> --- a/src/compiler/nir/nir_intrinsics.h
> +++ b/src/compiler/nir/nir_intrinsics.h
> @@ -364,6 +364,9 @@ SYSTEM_VALUE(blend_const_color_a_float, 1, 0, xx,
> xx, xx)
>  SYSTEM_VALUE(blend_const_color_rgba_unorm, 1, 0, xx, xx, xx)
>  SYSTEM_VALUE(blend_const_color__unorm, 1, 0, xx, xx, xx)
>  
> +/* Intel specific system values */
> +SYSTEM_VALUE(intel_thread_local_id, 1, 0, xx, xx, xx)
> +
>  /**
>   * Barycentric coordinate intrinsics.
>   *
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 2acd838..c0d4c05 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -996,6 +996,7 @@ fs_visitor::import_uniforms(fs_visitor *v)
> this->push_constant_loc = v->push_constant_loc;
> this->pull_constant_loc = v->pull_constant_loc;
> this->uniforms = v->uniforms;
> +   this->thread_local_id = v->thread_local_id;
>  }
>  
>  void
> @@ -6781,8 +6782,7 @@ brw_compile_cs(const struct brw_compiler
> *compiler, void *log_data,
>  {
> nir_shader *shader = nir_shader_clone(mem_ctx, src_shader);
> shader = brw_nir_apply_sampler_key(shader, compiler, >tex,
> true);
> -
> -   brw_nir_lower_cs_intrinsics(shader, prog_data);
> +   brw_nir_lower_cs_intrinsics(shader);
> shader = brw_postprocess_nir(shader, compiler, true);
>  
> prog_data->local_size[0] = shader->info.cs.local_size[0];
> diff --git a/src/intel/compiler/brw_fs.h
> b/src/intel/compiler/brw_fs.h
> index da32593..f51a4d8 100644
> --- a/src/intel/compiler/brw_fs.h
> +++ b/src/intel/compiler/brw_fs.h
> @@ -315,6 +315,7 @@ public:
>  */
> int *push_constant_loc;
>  
> +   fs_reg thread_local_id;
> fs_reg frag_depth;
> fs_reg frag_stencil;
> fs_reg sample_mask;
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 05efee3..fdc6fc6 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -88,6 +88,16 @@ fs_visitor::nir_setup_uniforms()
> }
>  
> uniforms = nir->num_uniforms / 4;
> +
> +   if (stage == MESA_SHADER_COMPUTE) {
> +  /* Add a uniform for the thread local id.  It must be the last
> uniform
> +   * on the list.
> +   */
> +  assert(uniforms == prog_data->nr_params);
> +  uint32_t *param = brw_stage_prog_data_add_params(prog_data,
> 1);
> +  *param = BRW_PARAM_BUILTIN_THREAD_LOCAL_ID;
> +  thread_local_id = fs_reg(UNIFORM, uniforms++,
> BRW_REGISTER_TYPE_UD);
> +   }
>  }
>  
>  static bool
> @@ -3409,6 +3419,10 @@ fs_visitor::nir_emit_cs_intrinsic(const
> fs_builder ,
>    cs_prog_data->uses_barrier = true;
>    break;
>  
> +   case nir_intrinsic_load_intel_thread_local_id:
> +  bld.MOV(retype(dest, BRW_REGISTER_TYPE_UD), thread_local_id);
> +  break;
> +
> case nir_intrinsic_load_local_invocation_id:
> case nir_intrinsic_load_work_group_id: {
>    gl_system_value sv = nir_system_value_from_intrinsic(instr-
> >intrinsic);
> diff --git a/src/intel/compiler/brw_nir.h
> b/src/intel/compiler/brw_nir.h
> index 1493b74..3e40712 100644
> --- a/src/intel/compiler/brw_nir.h
> +++ b/src/intel/compiler/brw_nir.h
> @@ -95,8 +95,7 @@ void brw_nir_analyze_boolean_resolves(nir_shader
> *nir);
>  nir_shader *brw_preprocess_nir(const struct brw_compiler *compiler,
> nir_shader *nir);
>  
> -bool brw_nir_lower_cs_intrinsics(nir_shader *nir,
> - struct brw_cs_prog_data
> *prog_data);
> +bool brw_nir_lower_cs_intrinsics(nir_shader *nir);
>  void brw_nir_lower_vs_inputs(nir_shader *nir,
>   bool use_legacy_snorm_formula,
>   const uint8_t *vs_attrib_wa_flags);
> diff --git a/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> b/src/intel/compiler/brw_nir_lower_cs_intrinsics.c
> index d277276..07d2dcc 100644
> ---

Re: [Mesa-dev] [PATCH] i965: unref push_const_bo in intelDestroyContext

2017-10-27 Thread Emil Velikov 
On 27 October 2017 at 07:52, Tapani Pälli  wrote:
> Valgrind shows that leak is caused by gen6_upload_push_constant, add
> unref push_const_bo per stage to destructor to fix this (like done for
> scratch_bo).
>
>==10952== 144 bytes in 1 blocks are definitely lost in loss record 44 of 66
>==10952==at 0x4C30A1E: calloc (vg_replace_malloc.c:711)
>==10952==by 0x8C02847: bo_alloc_internal.constprop.10 
> (brw_bufmgr.c:344)
>==10952==by 0x8C425C4: intel_upload_space (intel_upload.c:101)
>==10952==by 0x8C22ED0: gen6_upload_push_constants 
> (gen6_constant_state.c:154)
>
> Fixes: 24891d7c05 ("i965: Store per-stage push constant BO pointers.")
> Signed-off-by: Tapani Pälli 
> Cc: mesa-sta...@lists.freedesktop.org
> ---
>  src/mesa/drivers/dri/i965/brw_context.c | 11 +++
>  1 file changed, 11 insertions(+)
>
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
> b/src/mesa/drivers/dri/i965/brw_context.c
> index c8de074638..61088e2f1f 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -1072,6 +1072,17 @@ intelDestroyContext(__DRIcontext * driContextPriv)
> if (brw->wm.base.scratch_bo)
>brw_bo_unreference(brw->wm.base.scratch_bo);
>
> +   if (brw->vs.base.push_const_bo)
I'd drop the if checks - brw_bo_unreference works fine when the bo
pointer is NULL.

With that the patch is
Reviewed-by: Emil Velikov 

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

Re: [Mesa-dev] [PATCH v2] radeonsi: update hack for HTILE corruption in ARK: Survival Evolved

2017-10-27 Thread Samuel Pitoiset 

Pushed with "clear_db_cache_before_clear", Thanks Marek.

On 10/26/2017 10:42 PM, Marek Olšák wrote:

Please "clear_db_cache_before_clear" and the option too. With that,
the patch is:

Reviewed-by: Marek Olšák 

Thanks,
Marek

On Thu, Oct 26, 2017 at 6:08 PM, Samuel Pitoiset
 wrote:

It appears that flushing the DB metadata is actually not sufficient
since the driver uses the new VS blit shaders. This looks quite
strange though, but it seems like we need to flush DB for fixing
the corruption.

v2: rename the drirc option

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102955
Fixes: 69ccb9dae7 (radeonsi: use new VS blit shaders (VS inputs in SGPRs)
Signed-off-by: Samuel Pitoiset 
---
  src/gallium/drivers/radeonsi/driinfo_radeonsi.h |  2 +-
  src/gallium/drivers/radeonsi/si_blit.c  | 10 +-
  src/gallium/drivers/radeonsi/si_pipe.c  |  4 ++--
  src/gallium/drivers/radeonsi/si_pipe.h  |  2 +-
  src/util/drirc  |  2 +-
  src/util/xmlpool/t_options.h|  6 +++---
  6 files changed, 13 insertions(+), 13 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h 
b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
index 402d3406d4..ef264b7d5e 100644
--- a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
+++ b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
@@ -6,5 +6,5 @@ DRI_CONF_SECTION_PERFORMANCE
  DRI_CONF_SECTION_END

  DRI_CONF_SECTION_DEBUG
-   DRI_CONF_RADEONSI_CLEAR_DB_META_BEFORE_CLEAR("false")
+   DRI_CONF_RADEONSI_CLEAR_DB_BEFORE_CLEAR("false")
  DRI_CONF_SECTION_END
diff --git a/src/gallium/drivers/radeonsi/si_blit.c 
b/src/gallium/drivers/radeonsi/si_blit.c
index fd8559ac98..ce1b5a3e1a 100644
--- a/src/gallium/drivers/radeonsi/si_blit.c
+++ b/src/gallium/drivers/radeonsi/si_blit.c
@@ -901,16 +901,16 @@ static void si_clear(struct pipe_context *ctx, unsigned 
buffers,
  * corruption in ARK: Survival Evolved, but that may just be
  * a coincidence and the root cause is elsewhere.
  *
-* The corruption can be fixed by putting the DB metadata flush
-* before or after the depth clear. (suprisingly)
+* The corruption can be fixed by putting the DB flush before
+* or after the depth clear. (surprisingly)
  *
  * https://bugs.freedesktop.org/show_bug.cgi?id=102955 
(apitrace)
  *
  * This hack decreases back-to-back ClearDepth performance.
  */
-   if (sctx->screen->clear_db_meta_before_clear)
-   sctx->b.flags |= SI_CONTEXT_FLUSH_AND_INV_DB_META |
-SI_CONTEXT_PS_PARTIAL_FLUSH;
+   if (sctx->screen->clear_db_before_clear) {
+   sctx->b.flags |= SI_CONTEXT_FLUSH_AND_INV_DB;
+   }
 }

 si_blitter_begin(ctx, SI_CLEAR);
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index 759d539471..21266611c7 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -1072,8 +1072,8 @@ struct pipe_screen *radeonsi_screen_create(struct 
radeon_winsys *ws,
 driQueryOptionb(config->options, 
"radeonsi_assume_no_z_fights");
 sscreen->commutative_blend_add =
 driQueryOptionb(config->options, 
"radeonsi_commutative_blend_add");
-   sscreen->clear_db_meta_before_clear =
-   driQueryOptionb(config->options, 
"radeonsi_clear_db_meta_before_clear");
+   sscreen->clear_db_before_clear =
+   driQueryOptionb(config->options, 
"radeonsi_clear_db_before_clear");
 sscreen->has_msaa_sample_loc_bug = (sscreen->b.family >= CHIP_POLARIS10 
&&
 sscreen->b.family <= 
CHIP_POLARIS12) ||
sscreen->b.family == CHIP_VEGA10 ||
diff --git a/src/gallium/drivers/radeonsi/si_pipe.h 
b/src/gallium/drivers/radeonsi/si_pipe.h
index c162a0fcd6..8138d4234a 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.h
+++ b/src/gallium/drivers/radeonsi/si_pipe.h
@@ -98,7 +98,7 @@ struct si_screen {
 boolhas_out_of_order_rast;
 boolassume_no_z_fights;
 boolcommutative_blend_add;
-   boolclear_db_meta_before_clear;
+   boolclear_db_before_clear;
 boolhas_msaa_sample_loc_bug;
 booldpbb_allowed;
 booldfsm_allowed;
diff --git a/src/util/drirc b/src/util/drirc
index 39ac3c858c..2d1f53ccbc 100644
--- a/src/util/drirc
+++ b/src/util/drirc
@@ -264,7 +264,7 @@

Re: [Mesa-dev] [PATCH v3 25/48] intel/cs: Drop max_dispatch_width checks from compile_cs

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> The only things that adjust fs_visitor::max_dispatch_width are render
> target writes which don't happen in compute shaders so they're
> pointless.
> ---
>  src/intel/compiler/brw_fs.cpp | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index a23366b..4c362ba 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp

Maybe add an assert before this to check that max_dispatch_width is >=
32 as expected here?

> @@ -6818,8 +6818,7 @@ brw_compile_cs(const struct brw_compiler
> *compiler, void *log_data,
>   NULL, /* Never used in core profile */
>   shader, 16, shader_time_index);
> if (likely(!(INTEL_DEBUG & DEBUG_NO16)) &&
> -   !fail_msg && v8.max_dispatch_width >= 16 &&
> -   min_dispatch_width <= 16) {
> +   !fail_msg && min_dispatch_width <= 16) {
>    /* Try a SIMD16 compile */
>    if (min_dispatch_width <= 8)
>   v16.import_uniforms();
> @@ -6843,8 +6842,7 @@ brw_compile_cs(const struct brw_compiler
> *compiler, void *log_data,
> fs_visitor v32(compiler, log_data, mem_ctx, key, _data-
> >base,
>   NULL, /* Never used in core profile */
>   shader, 32, shader_time_index);
> -   if (!fail_msg && v8.max_dispatch_width >= 32 &&
> -   (min_dispatch_width > 16 || (INTEL_DEBUG & DEBUG_DO32))) {
> +   if (!fail_msg && (min_dispatch_width > 16 || (

Maybe use unlikely() with (INTEL_DEBUG & DEBUG_DO32)?


>    /* Try a SIMD32 compile */
>    if (min_dispatch_width <= 8)
>   v32.import_uniforms();
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] Android: move drivers' symlinks to /vendor

2017-10-27 Thread Tapani Pälli 

Reviewed-by: Tapani Pälli 

On 10/27/2017 01:48 AM, Mauro Rossi wrote:

Having moved gallium_dri.so library to /vendor/lib/dri
also symlinks need to be coherently created using TARGET_OUT_VENDOR insted of 
TARGET_OUT
or all non Intel drivers will not be loaded with Android N and earlier,
thus causing SurfaceFlinger SIGABRT

Fixes: c3f75d483c ("Android: move libraries to /vendor")

Cc: 17.3 
---
  src/gallium/targets/dri/Android.mk | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/gallium/targets/dri/Android.mk 
b/src/gallium/targets/dri/Android.mk
index 61b65769ff..3fa86a2d56 100644
--- a/src/gallium/targets/dri/Android.mk
+++ b/src/gallium/targets/dri/Android.mk
@@ -70,8 +70,8 @@ LOCAL_SHARED_LIBRARIES += $(sort $(GALLIUM_SHARED_LIBS))
  ifneq ($(filter 5 6 7, $(MESA_ANDROID_MAJOR_VERSION)),)
  LOCAL_POST_INSTALL_CMD := \
$(foreach l, lib $(if $(filter true,$(TARGET_IS_64_BIT)),lib64), \
- mkdir -p $(TARGET_OUT)/$(l)/$(MESA_DRI_MODULE_REL_PATH); \
- $(foreach d, $(GALLIUM_TARGET_DRIVERS), ln -sf gallium_dri.so 
$(TARGET_OUT)/$(l)/$(MESA_DRI_MODULE_REL_PATH)/$(d)_dri.so;) \
+ mkdir -p $(TARGET_OUT_VENDOR)/$(l)/$(MESA_DRI_MODULE_REL_PATH); \
+ $(foreach d, $(GALLIUM_TARGET_DRIVERS), ln -sf gallium_dri.so 
$(TARGET_OUT_VENDOR)/$(l)/$(MESA_DRI_MODULE_REL_PATH)/$(d)_dri.so;) \
)
  else
  LOCAL_MODULE_SYMLINKS := $(foreach d, $(GALLIUM_TARGET_DRIVERS), $(d)_dri.so)


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

Re: [Mesa-dev] [PATCH v3 00/10] glsl_to_tgsi: Further improvement of lifetime tracking for register merge

2017-10-27 Thread Gert Wollny 
Am Donnerstag, den 26.10.2017, 17:28 +0100 schrieb Emil Velikov:
> 
> >  .../tests/test_glsl_to_tgsi_lifetime.cpp   | 1278
> > +++-
> 
> JFYI you'd want to explicitly undef NDEBUG in the test.
> git grep -10 "#undef NDEBUG" -  for examples
> 
> Otherwise the asserts will not trigger since they're not around ;-)
> 

Well, these asserts are not testing library code, they just check the
sanity of the test setup, i.e. whether the mock shaders use the right
number of source and destination registers with respect to the opcodes.
With that in mind I don't think that they really need to be around in a
release check build. 

Nevertheless, I will contemplate whether it makes sence to replace them
with the Google test ASSERT_EQ. 

thanks for the pointer anyway, 
Gert 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 23/48] intel/fs: Assign constant locations if they haven't been assigned

2017-10-27 Thread Iago Toral 
This sounds good to me, but I guess it is not really fixing anything,
right? I ask because the subject claims that this patch does something
that the original code was already supposed to be doing.

On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> Before, we bailing in assign_constant_locations based on the minimum
> dispatch size.  The more direct thing to do is simply to check for
> whether or not we have constant locations and bail if we do.  For
> nir_setup_uniforms, it's completely safe to do it multiple times
> because
> we just copy a value from the NIR shader.
> ---
>  src/intel/compiler/brw_fs.cpp | 4 +++-
>  src/intel/compiler/brw_fs_nir.cpp | 5 -
>  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 52079d3..75139fd 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -1956,8 +1956,10 @@ void
>  fs_visitor::assign_constant_locations()
>  {
> /* Only the first compile gets to decide on locations. */
> -   if (dispatch_width != min_dispatch_width)
> +   if (push_constant_loc) {
> +  assert(pull_constant_loc);
>    return;
> +   }
>  
> bool is_live[uniforms];
> memset(is_live, 0, sizeof(is_live));
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 7556576..05efee3 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -81,8 +81,11 @@ fs_visitor::nir_setup_outputs()
>  void
>  fs_visitor::nir_setup_uniforms()
>  {
> -   if (dispatch_width != min_dispatch_width)
> +   /* Only the first compile gets to set up uniforms. */
> +   if (push_constant_loc) {
> +  assert(pull_constant_loc);
>    return;
> +   }
>  
> uniforms = nir->num_uniforms / 4;
>  }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 20/48] intel/fs: Protect opt_algebraic from OOB BROADCAST indices

2017-10-27 Thread Iago Toral 

On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> ---
>  src/intel/compiler/brw_fs.cpp | 10 --
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs.cpp
> b/src/intel/compiler/brw_fs.cpp
> index 1c4351b..52079d3 100644
> --- a/src/intel/compiler/brw_fs.cpp
> +++ b/src/intel/compiler/brw_fs.cpp
> @@ -2416,8 +2416,14 @@ fs_visitor::opt_algebraic()
>  progress = true;
>   } else if (inst->src[1].file == IMM) {
>  inst->opcode = BRW_OPCODE_MOV;
> -inst->src[0] = component(inst->src[0],
> - inst->src[1].ud);
> +/* It's possible that the selected component will be too
> large and
> + * overflow the register.  If this happens and we some
> how manage
> + * to constant fold it in and get here, it would cause
> an assert
> + * in component() below.  Instead, just let it wrap
> around if it
> + * goes over exec_size.
> + */

component() is really a horiz_offset() call which is in turn a
byte_offset() call, which doesn't assert on anything other than invalid
register files. I guess you mean that the byte offset computed by the
component() call below can later lead to hitting assertions as we
attempt to read outside the allocated space for the vgrf, right?

My question is whether this is supposed to happen at all, it seems like
we should not be emitting broadcast operations like this at all since
they are invalid and here we are instead papering over that bug.

> +const unsigned comp = inst->src[1].ud & (inst->exec_size 
> - 1);
> +inst->src[0] = component(inst->src[0], comp);
>  inst->sources = 1;
>  inst->force_writemask_all = true;
>  progress = true;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3 19/48] i965/fs/nir: Don't stomp 64-bit values to D in get_nir_src

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 33 +--
> --
>  1 file changed, 21 insertions(+), 12 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index e008e2e..a441f57 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -1441,11 +1441,19 @@ fs_visitor::get_nir_src(const nir_src )
> src.reg.base_offset * src.reg.reg-
> >num_components);
> }
>  
> -   /* to avoid floating-point denorm flushing problems, set the type
> by
> -* default to D - instructions that need floating point semantics
> will set
> -* this to F if they need to
> -*/
> -   return retype(reg, BRW_REGISTER_TYPE_D);
> +   if (nir_src_bit_size(src) == 64 && devinfo->gen == 7) {
> +  /* The only 64-bit type available on gen7 is DF, so use that.
> */
> +  reg.type = BRW_REGISTER_TYPE_DF;
> +   } else {
> +  /* To avoid floating-point denorm flushing problems, set the
> type by
> +   * default to an integer type - instructions that need
> floating point
> +   * semantics will set this to F if they need to
> +   */
> +  reg.type = brw_reg_type_from_bit_size(nir_src_bit_size(src),
> +BRW_REGISTER_TYPE_D);
> +   }
> +
> +   return reg;
>  }
>  
>  /**
> @@ -1455,6 +1463,10 @@ fs_reg
>  fs_visitor::get_nir_src_imm(const nir_src )
>  {
> nir_const_value *val = nir_src_as_const_value(src);
> +   /* This function shouldn't be called on anything which can even
> +* possibly be 64 bits as it can't do what it claims.
> +*/

What would be wrong with something like this?

if (nir_src_bit_size(src) == 32)
   return val ? fs_reg(brw_imm_d(val->i32[0])) : get_nir_src(src);
else
   return val ? fs_reg(brw_imm_df(val->f64[0])) : get_nir_src(src);


> +   assert(nir_src_bit_size(src) == 32);
> return val ? fs_reg(brw_imm_d(val->i32[0])) : get_nir_src(src);
>  }
>  
> @@ -2648,8 +2660,7 @@ fs_visitor::nir_emit_tcs_intrinsic(const
> fs_builder ,
>  */
> unsigned channel = iter * 2 + i;
> fs_reg dest = shuffle_64bit_data_for_32bit_write(bld,
> -  retype(offset(value, bld, 2 * channel),
> BRW_REGISTER_TYPE_DF),
> -  1);
> +  offset(value, bld, channel), 1);
>  
> srcs[header_regs + (i + first_component) * 2] = dest;
> srcs[header_regs + (i + first_component) * 2 + 1] =
> @@ -3505,8 +3516,7 @@ fs_visitor::nir_emit_cs_intrinsic(const
> fs_builder ,
>    if (nir_src_bit_size(instr->src[0]) == 64) {
>   type_size = 8;
>   val_reg = shuffle_64bit_data_for_32bit_write(bld,
> -retype(val_reg, BRW_REGISTER_TYPE_DF),
> -instr->num_components);
> +val_reg, instr->num_components);
>    }
>  
>    unsigned type_slots = type_size / 4;
> @@ -4005,8 +4015,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder
> , nir_intrinsic_instr *instr
>    if (nir_src_bit_size(instr->src[0]) == 64) {
>   type_size = 8;
>   val_reg = shuffle_64bit_data_for_32bit_write(bld,
> -retype(val_reg, BRW_REGISTER_TYPE_DF),
> -instr->num_components);
> +val_reg, instr->num_components);
>    }
>  
>    unsigned type_slots = type_size / 4;
> @@ -4063,7 +4072,7 @@ fs_visitor::nir_emit_intrinsic(const fs_builder
> , nir_intrinsic_instr *instr
>    unsigned first_component = nir_intrinsic_component(instr);
>    if (nir_src_bit_size(instr->src[0]) == 64) {
>   fs_reg tmp = shuffle_64bit_data_for_32bit_write(bld,
> -retype(src, BRW_REGISTER_TYPE_DF), num_components);
> +src, num_components);
>   src = tmp;
>   num_components *= 2;
>    }
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: unref push_const_bo in intelDestroyContext

2017-10-27 Thread Tapani Pälli 
Valgrind shows that leak is caused by gen6_upload_push_constant, add
unref push_const_bo per stage to destructor to fix this (like done for
scratch_bo).

   ==10952== 144 bytes in 1 blocks are definitely lost in loss record 44 of 66
   ==10952==at 0x4C30A1E: calloc (vg_replace_malloc.c:711)
   ==10952==by 0x8C02847: bo_alloc_internal.constprop.10 (brw_bufmgr.c:344)
   ==10952==by 0x8C425C4: intel_upload_space (intel_upload.c:101)
   ==10952==by 0x8C22ED0: gen6_upload_push_constants 
(gen6_constant_state.c:154)

Fixes: 24891d7c05 ("i965: Store per-stage push constant BO pointers.")
Signed-off-by: Tapani Pälli 
Cc: mesa-sta...@lists.freedesktop.org
---
 src/mesa/drivers/dri/i965/brw_context.c | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index c8de074638..61088e2f1f 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1072,6 +1072,17 @@ intelDestroyContext(__DRIcontext * driContextPriv)
if (brw->wm.base.scratch_bo)
   brw_bo_unreference(brw->wm.base.scratch_bo);
 
+   if (brw->vs.base.push_const_bo)
+  brw_bo_unreference(brw->vs.base.push_const_bo);
+   if (brw->tcs.base.push_const_bo)
+  brw_bo_unreference(brw->tcs.base.push_const_bo);
+   if (brw->tes.base.push_const_bo)
+  brw_bo_unreference(brw->tes.base.push_const_bo);
+   if (brw->gs.base.push_const_bo)
+  brw_bo_unreference(brw->gs.base.push_const_bo);
+   if (brw->wm.base.push_const_bo)
+  brw_bo_unreference(brw->wm.base.push_const_bo);
+
brw_destroy_hw_context(brw->bufmgr, brw->hw_ctx);
 
if (ctx->swrast_context) {
-- 
2.13.6

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

Re: [Mesa-dev] [PATCH v3 18/48] i965/fs/nir: Minor refactor of store_output

2017-10-27 Thread Iago Toral 
On Wed, 2017-10-25 at 16:25 -0700, Jason Ekstrand wrote:
> Stop retyping the output of shuffle_64bit_data_for_32bit_write.  It's
> always BRW_REGISTER_TYPE_D which is perfectly fine for writing out.
> Also, when we change get_nir_src to return something with a 64-bit
> type
> for 64-bit values, the retyping will not be at all what we
> want.  Also,
> retyping the output based on src.type before we whack it back to 32
> bits
> is a problem because the output is always 32 bits.
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 5bcdb1a..e008e2e 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -4058,18 +4058,18 @@ fs_visitor::nir_emit_intrinsic(const
> fs_builder , nir_intrinsic_instr *instr
>  
>    nir_const_value *const_offset = nir_src_as_const_value(instr-
> >src[1]);
>    assert(const_offset && "Indirect output stores not allowed");
> -  fs_reg new_dest = retype(offset(outputs[instr-
> >const_index[0]], bld,
> -  4 * const_offset->u32[0]),
> src.type);
>  
>    unsigned num_components = instr->num_components;
>    unsigned first_component = nir_intrinsic_component(instr);
>    if (nir_src_bit_size(instr->src[0]) == 64) {
>   fs_reg tmp = shuffle_64bit_data_for_32bit_write(bld,
>  retype(src, BRW_REGISTER_TYPE_DF), num_components);
> - src = retype(tmp, src.type);
> + src = tmp;

Maybe just make this:

src = suffle_64bit_data_for_32bit_write(...) ?

>   num_components *= 2;
>    }
>  
> +  fs_reg new_dest = retype(offset(outputs[instr-
> >const_index[0]], bld,
> +  4 * const_offset->u32[0]),
> src.type);
>    for (unsigned j = 0; j < num_components; j++) {
>   bld.MOV(offset(new_dest, bld, j + first_component),
>   offset(src, bld, j));
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH v3.5] intel/compiler: Add union types for prog_data and prog_key stages

2017-10-27 Thread Jordan Justen 
Signed-off-by: Jordan Justen 
Reviewed-by: Jason Ekstrand 
Cc: Jason Ekstrand 
Cc: Kenneth Graunke 
---

 * Add comment (Ken)
 * No typedef (Jason)

 src/intel/compiler/brw_compiler.h | 20 
 1 file changed, 20 insertions(+)

diff --git a/src/intel/compiler/brw_compiler.h 
b/src/intel/compiler/brw_compiler.h
index 701b4a80bf1..6ad89171ce4 100644
--- a/src/intel/compiler/brw_compiler.h
+++ b/src/intel/compiler/brw_compiler.h
@@ -403,6 +403,16 @@ struct brw_cs_prog_key {
struct brw_sampler_prog_key_data tex;
 };
 
+/* brw_any_prog_key is any of the keys that map to an API stage */
+union brw_any_prog_key {
+   struct brw_vs_prog_key vs;
+   struct brw_tcs_prog_key tcs;
+   struct brw_tes_prog_key tes;
+   struct brw_gs_prog_key gs;
+   struct brw_wm_prog_key wm;
+   struct brw_cs_prog_key cs;
+};
+
 /*
  * Image metadata structure as laid out in the shader parameter
  * buffer.  Entries have to be 16B-aligned for the vec4 back-end to be
@@ -1066,6 +1076,16 @@ struct brw_clip_prog_data {
uint32_t total_grf;
 };
 
+/* brw_any_prog_data is prog_data for any stage that maps to an API stage */
+union brw_any_prog_data {
+   struct brw_vs_prog_data vs;
+   struct brw_tcs_prog_data tcs;
+   struct brw_tes_prog_data tes;
+   struct brw_gs_prog_data gs;
+   struct brw_wm_prog_data wm;
+   struct brw_cs_prog_data cs;
+};
+
 #define DEFINE_PROG_DATA_DOWNCAST(stage)   \
 static inline struct brw_##stage##_prog_data * \
 brw_##stage##_prog_data(struct brw_stage_prog_data *prog_data) \
-- 
2.15.0.rc2

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

<    1   2