date:20180903

Re: [Mesa-dev] [PATCH] intel/compiler: Remove redundant nir_remove_dead_variables call

2018-09-03 Thread Tapani Pälli


Reviewed-by: Tapani Pälli 

On 09/03/2018 09:22 PM, Jason Ekstrand wrote:

As of 07a2098a708a2, brw_nir_optimize calls nir_remove_dead_variables as
the last optimization.  Doing it again is just pointless.
---
  src/intel/compiler/brw_nir.c | 2 --
  1 file changed, 2 deletions(-)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index a906a026042..ce865e2ce71 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -715,8 +715,6 @@ brw_preprocess_nir(const struct brw_compiler *compiler, 
nir_shader *nir)
 /* Get rid of split copies */
 nir = brw_nir_optimize(nir, compiler, is_scalar, false);
  
-   OPT(nir_remove_dead_variables, nir_var_local);

-
 return nir;
  }
  


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

Re: [Mesa-dev] [PATCH 2/4] anv: Implement a VF cache invalidate workaround

2018-09-03 Thread Kenneth Graunke

On Saturday, September 1, 2018 9:24:53 AM PDT Jason Ekstrand wrote:
> This appears to hang broadwell; we should probably think twice before
> enabling it so broadly.  I'll adjust it to be gen9 only or we can just can
> the patch entirely.

There a bunch of workarounds and they're tricky to get right.

You might look at iris_emit_raw_pipe_control here:

https://gitlab.freedesktop.org/kwg/mesa/blob/iris/src/gallium/drivers/iris/iris_state.c#L3878

Maybe we should make src/intel/common/gen_pipe_control.c...

--Ken

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] anv: Apply pipeline lushes a bit earlier and more often

2018-09-03 Thread Jason Ekstrand

Previously, all needed pipe controls were flushed out right before
either 3DPRIMITIVE or GPGPU_WALKER.  This commit moves pipeline flushes
to the beginning of state emit for a given draw or dispatch call rather
than the end.  The result is that any previously pending state is now
flushed before we even start setting up new state.  Since nothing in the
pipeline state emit code dirties any PIPE_CONTROL bits, it's safe to
move it higher up and it won't even lead to any additional flushing.
One additional flush is added at the end of begin_subpass right before
emitting depth and stencil buffer packets.  This may lead to some extra
flushing but that's likely to happen on a subpass boundary anyway.

The idea behind moving things earlier is so that PIPE_CONTROLs that are
inserted due to a hardware workaround will happen closer to where the
workaround is applied.  For example, the pipe control that happens after
doing a HiZ clear now occurs *before* the depth/stencil or any 3D
pipeline setup packets for the next draw occur.  This appears to maybe
help DiRT3 on Sky Lake (though the hang is very hard to reproduce).

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107760
---
 src/intel/vulkan/genX_cmd_buffer.c | 14 --
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/src/intel/vulkan/genX_cmd_buffer.c 
b/src/intel/vulkan/genX_cmd_buffer.c
index f95e106f923..a03c0ff8a27 100644
--- a/src/intel/vulkan/genX_cmd_buffer.c
+++ b/src/intel/vulkan/genX_cmd_buffer.c
@@ -2525,6 +2525,8 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
 
genX(flush_pipeline_select_3d)(cmd_buffer);
 
+   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
+
if (vb_emit) {
   const uint32_t num_buffers = __builtin_popcount(vb_emit);
   const uint32_t num_dwords = 1 + num_buffers * 4;
@@ -2642,8 +2644,6 @@ genX(cmd_buffer_flush_state)(struct anv_cmd_buffer 
*cmd_buffer)
   gen7_cmd_buffer_emit_scissor(cmd_buffer);
 
genX(cmd_buffer_flush_dynamic_state)(cmd_buffer);
-
-   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
 }
 
 static void
@@ -3043,10 +3043,12 @@ genX(cmd_buffer_flush_compute_state)(struct 
anv_cmd_buffer *cmd_buffer)
*sufficient."
*/
   cmd_buffer->state.pending_pipe_bits |= ANV_PIPE_CS_STALL_BIT;
-  genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
+   }
+
+   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
 
+   if (cmd_buffer->state.compute.pipeline_dirty)
   anv_batch_emit_batch(_buffer->batch, >batch);
-   }
 
if ((cmd_buffer->state.descriptors_dirty & VK_SHADER_STAGE_COMPUTE_BIT) ||
cmd_buffer->state.compute.pipeline_dirty) {
@@ -3073,8 +3075,6 @@ genX(cmd_buffer_flush_compute_state)(struct 
anv_cmd_buffer *cmd_buffer)
}
 
cmd_buffer->state.compute.pipeline_dirty = false;
-
-   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
 }
 
 #if GEN_GEN == 7
@@ -3825,6 +3825,8 @@ cmd_buffer_begin_subpass(struct anv_cmd_buffer 
*cmd_buffer,
   att_state->pending_load_aspects = 0;
}
 
+   genX(cmd_buffer_apply_pipe_flushes)(cmd_buffer);
+
cmd_buffer_emit_depth_stencil(cmd_buffer);
 }
 
-- 
2.17.1

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

Re: [Mesa-dev] [PATCH] i965: Deny persistent mappings of incoherent GTT mmaps

2018-09-03 Thread Lionel Landwerlin

Talking with Ken about this, it seems we might not actually coherent use 
GTT maps because those are just for buffer (which are allocated linear).

GTT maps are only used with tiled buffers.

So we most likely don't even need this patch.
The code is confusing though.

-
Lionel

On 31/08/2018 12:16, Lionel Landwerlin wrote:

We would need a fairly recent kernel (drm-tip?) to test this in CI.

I can't see any issue with this because we always have the meta path 
as a fallback for tiled buffers.


Reviewed-by: Lionel Landwerlin 

On 30/08/2018 16:22, Chris Wilson wrote:

On more recent HW, the indirect writes via the GGTT are internally
buffered presenting a nuisance to trying to use them for persistent
client maps. (We cannot guarantee that any write by the client will
then be visible to either the GPU or third parties in a timely manner,
leading to corruption.) Fortunately, we try very hard not to even use
the GGTT for anything and even less so for persistent mmaps, so their
loss is unlikely to be noticed.

No piglits harmed.

Cc: Kenneth Graunke 
Cc: Lionel Landwerlin 
Cc: Joonas Lahtinen 
---
  include/drm-uapi/i915_drm.h  | 22 +++
  src/mesa/drivers/dri/i965/brw_bufmgr.c   | 36 
  src/mesa/drivers/dri/i965/intel_screen.c |  3 ++
  src/mesa/drivers/dri/i965/intel_screen.h |  1 +
  4 files changed, 62 insertions(+)

diff --git a/include/drm-uapi/i915_drm.h b/include/drm-uapi/i915_drm.h
index 16e452aa12d..268b585f8a4 100644
--- a/include/drm-uapi/i915_drm.h
+++ b/include/drm-uapi/i915_drm.h
@@ -529,6 +529,28 @@ typedef struct drm_i915_irq_wait {
   */
  #define I915_PARAM_CS_TIMESTAMP_FREQUENCY 51
  +/*
+ * Once upon a time we supposed that writes through the GGTT would be
+ * immediately in physical memory (once flushed out of the CPU 
path). However,
+ * on a few different processors and chipsets, this is not 
necessarily the case
+ * as the writes appear to be buffered internally. Thus a read of 
the backing
+ * storage (physical memory) via a different path (with different 
physical tags
+ * to the indirect write via the GGTT) will see stale values from 
before
+ * the GGTT write. Inside the kernel, we can for the most part keep 
track of
+ * the different read/write domains in use (e.g. set-domain), but 
the assumption
+ * of coherency is baked into the ABI, hence reporting its true 
state in this

+ * parameter.
+ *
+ * Reports true when writes via mmap_gtt are immediately visible 
following an

+ * lfence to flush the WCB.
+ *
+ * Reports false when writes via mmap_gtt are indeterminately 
delayed in an in
+ * internal buffer and are _not_ immediately visible to third 
parties accessing

+ * directly via mmap_cpu/mmap_wc. Use of mmap_gtt as part of an IPC
+ * communications channel when reporting false is strongly disadvised.
+ */
+#define I915_PARAM_MMAP_GTT_COHERENT    52
+
  typedef struct drm_i915_getparam {
  __s32 param;
  /*
diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c 
b/src/mesa/drivers/dri/i965/brw_bufmgr.c

index f1675b191c1..6955c5c890c 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -1068,6 +1068,19 @@ brw_bo_map_wc(struct brw_context *brw, struct 
brw_bo *bo, unsigned flags)

 return bo->map_wc;
  }
  +static void
+bo_set_domain(struct brw_bo *bo, unsigned int read, unsigned int write)
+{
+   struct brw_bufmgr *bufmgr = bo->bufmgr;
+
+   struct drm_i915_gem_set_domain arg = {
+  .handle = bo->gem_handle,
+  .read_domains = read,
+  .write_domain = write,
+   };
+   drmIoctl(bufmgr->fd, DRM_IOCTL_I915_GEM_SET_DOMAIN, );
+}
+
  /**
   * Perform an uncached mapping via the GTT.
   *
@@ -1095,6 +1108,25 @@ brw_bo_map_gtt(struct brw_context *brw, struct 
brw_bo *bo, unsigned flags)

  {
 struct brw_bufmgr *bufmgr = bo->bufmgr;
  +   /* Once upon a time we believed that there was no internal 
buffering of

+    * the indirect writes via the Global GTT; that is once flushed from
+    * the processor the write would be immediately visible to any one
+    * else reading that memory location be they the GPU, kernel or 
another
+    * client. As it turns out, on modern hardware there is an 
internal buffer
+    * that cannot be directly flushed (e.g. using the sfence one 
would normally
+    * use to flush the WCB) and so far the w/a requires us to do an 
uncached
+    * mmio read, quite expensive and requires user cooperation. That 
is we
+    * cannot simply support persistent user access to the GTT mmap 
buffers

+    * as we have no means of flushing their writes in a timely manner.
+    */
+   if (flags & MAP_PERSISTENT &&
+   flags & MAP_COHERENT &&
+   flags & MAP_WRITE &&
+   !(brw->screen->kernel_features & 
KERNEL_ALLOWS_COHERENT_MMAP_GTT)) {
+  DBG("bo_map_gtt: rejected attempt to make a coherent, 
persistent and writable GGTT mmap, %d (%s)\n", bo->gem_handle, 
bo->name);

+  return NULL;
+   }
+
 /* Get a mapping of the buffer

Re: [Mesa-dev] [PATCH] radeonsi: clear VRAM buffers on creation if zerovram is set

2018-09-03 Thread Timothy Arceri


This patch causes the corruptions to return in No Mans Sky.

On 04/09/18 04:36, Marek Olšák wrote:

From: Marek Olšák 

This includes reused buffers. Prefer DCC clear if DCC is enabled.
---
  src/gallium/drivers/radeonsi/si_buffer.c  | 8 
  src/gallium/drivers/radeonsi/si_pipe.c| 3 ++-
  src/gallium/drivers/radeonsi/si_texture.c | 5 -
  src/gallium/winsys/amdgpu/drm/amdgpu_bo.c | 3 ---
  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c | 2 --
  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h | 1 -
  6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_buffer.c 
b/src/gallium/drivers/radeonsi/si_buffer.c
index a03a944..f31767c 100644
--- a/src/gallium/drivers/radeonsi/si_buffer.c
+++ b/src/gallium/drivers/radeonsi/si_buffer.c
@@ -235,20 +235,28 @@ bool si_alloc_resource(struct si_screen *sscreen,
  
  	util_range_set_empty(>valid_buffer_range);

res->TC_L2_dirty = false;
  
  	/* Print debug information. */

if (sscreen->debug_flags & DBG(VM) && res->b.b.target == PIPE_BUFFER) {
fprintf(stderr, "VM start=0x%"PRIX64"  end=0x%"PRIX64" | Buffer 
%"PRIu64" bytes\n",
res->gpu_address, res->gpu_address + res->buf->size,
res->buf->size);
}
+
+   /* Only clear if DCC is disabled, because DCC clears are more 
efficient. */
+   if (sscreen->debug_flags & DBG_ZERO_VRAM &&
+   res->domains & RADEON_DOMAIN_VRAM &&
+   (res->b.b.target == PIPE_BUFFER ||
+!((struct si_texture*)res)->dcc_offset))
+   si_screen_clear_buffer(sscreen, >b.b, 0, res->bo_size, 0);
+
return true;
  }
  
  static void si_buffer_destroy(struct pipe_screen *screen,

  struct pipe_resource *buf)
  {
struct r600_resource *rbuffer = r600_resource(buf);
  
  	threaded_resource_deinit(buf);

util_range_destroy(>valid_buffer_range);
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index 9c8ed36..6e14066 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -850,21 +850,22 @@ struct pipe_screen *radeonsi_screen_create(struct 
radeon_winsys *ws,
si_init_screen_query_functions(sscreen);
  
  	/* Set these flags in debug_flags early, so that the shader cache takes

 * them into account.
 */
if (driQueryOptionb(config->options,
"glsl_correct_derivatives_after_discard"))
sscreen->debug_flags |= DBG(FS_CORRECT_DERIVS_AFTER_KILL);
if (driQueryOptionb(config->options, "radeonsi_enable_sisched"))
sscreen->debug_flags |= DBG(SI_SCHED);
-
+   if (driQueryOptionb(config->options, "radeonsi_zerovram"))
+   sscreen->debug_flags |= DBG(ZERO_VRAM);
  
  	if (sscreen->debug_flags & DBG(INFO))

ac_print_gpu_info(>info);
  
  	slab_create_parent(>pool_transfers,

   sizeof(struct si_transfer), 64);
  
  	sscreen->force_aniso = MIN2(16, debug_get_num_option("R600_TEX_ANISO", -1));

if (sscreen->force_aniso >= 0) {
printf("radeonsi: Forcing anisotropy filter to %ix\n",
diff --git a/src/gallium/drivers/radeonsi/si_texture.c 
b/src/gallium/drivers/radeonsi/si_texture.c
index e55fd81..2faeef2 100644
--- a/src/gallium/drivers/radeonsi/si_texture.c
+++ b/src/gallium/drivers/radeonsi/si_texture.c
@@ -1250,24 +1250,27 @@ si_texture_create_object(struct pipe_screen *screen,
clear_value = 0x030F;
  
  		si_screen_clear_buffer(sscreen, >buffer.b.b,

 tex->htile_offset,
 tex->surface.htile_size,
 clear_value);
}
  
  	/* Initialize DCC only if the texture is not being imported. */

if (!buf && tex->dcc_offset) {
+   unsigned clear_value =
+   sscreen->debug_flags & DBG_ZERO_VRAM ? 0 : 0x;
+
si_screen_clear_buffer(sscreen, >buffer.b.b,
 tex->dcc_offset,
 tex->surface.dcc_size,
-0x);
+clear_value);
}
  
  	/* Initialize the CMASK base register value. */

tex->cmask_base_address_reg =
(tex->buffer.gpu_address + tex->cmask_offset) >> 8;
  
  	if (sscreen->debug_flags & DBG(VM)) {

fprintf(stderr, "VM start=0x%"PRIX64"  end=0x%"PRIX64" | Texture 
%ix%ix%i, %i levels, %i samples, %s\n",
tex->buffer.gpu_address,
tex->buffer.gpu_address + tex->buffer.buf->size,
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c 
b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
index 68f0562..2842936 100644
---

Re: [Mesa-dev] [PATCH 1/3] android: broadcom/genxml: fix collision with intel/genxml header-gen macro

2018-09-03 Thread Eric Anholt

Mauro Rossi  writes:

> Fixes the following building error, happening when building both intel and 
> broadcom:

I wish someone maintaining android Mesa would work on making the meson
build work for them instead of just continuing to maintain the
Android.mk mess.  However, whatever it takes to make it build for now
gets:

Acked-by: Eric Anholt 

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

Re: [Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation (v2)

2018-09-03 Thread Gert Wollny

Am Montag, den 03.09.2018, 14:42 +0100 schrieb Emil Velikov:
> 
> struct virgl_hw_res **new_bo =
> 
> > +new_nres * sizeof(struct
> > virgl_hw_buf*));
> > +  if (!new_ptr) {
> > +  fprintf(stderr,"failure to add relocation %d, %d\n",
> > cbuf->cres, new_nres);
> >return;
> >}
> > +  cbuf->res_bo = new_ptr;
> > +
> > +  new_ptr = REALLOC(cbuf->res_hlist, cbuf->nres *
> > sizeof(uint32_t),
> 
> uint32_t *new_hlist =
> 
> > +new_nres * sizeof(uint32_t));
> > +  if (!new_ptr) {
> > +  fprintf(stderr,"failure to add hlist relocation %d,
> > %d\n", cbuf->cres, cbuf->nres);
> 
> FREE(new_bo);
I don't think this is correct: as far as I  understand how ralloc
works, then if the realloc succeeds, then the old cbuf->res_bo is
either invalidated, or points to the same area like new_bo. In both
cases free will free the memory where resource data is stored, and
later at cleanup this will lead to problems.  


Best, 
Gert

> > +  return;
> > +  }
> > +  cbuf->res_hlist = new_ptr;
> > +  /* This is not perfect, because when the first re-allocation 
> > succeeds but
> > +   * the second failes the first old_size will be wrong, but
> > currently it
> > +   * isn't used anyway. */
> 
> Now you can drop the comment.
> 
> Aside: the old_size of REALLOC could/should go - debug_realloc can
> use
> old_hdr->size.
> Using sizeof(instance) should be clearer and more consistent with the
> rest of winsys/virgl.
> 
> -Emil
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

Re: [Mesa-dev] [PATCH] gallium/swr: Enable support bptc format.

2018-09-03 Thread Denis Pauk

Hi,

Could you please merge patch to master?

On Wed, Aug 1, 2018 at 10:33 PM Denis Pauk  wrote:

> Hi Bruce,
>
> Thank you. Best wishes to Alok.
>
> Could someone also update docs/features.txt with bptc/astc support? Look
> like we can mark bptc as done for all gallium software renders and astc_ldr
> for all gallium drivers.
>
>
> Best regards,
>   Denis.
>
> On Wed, Aug 1, 2018, 6:14 PM Cherniak, Bruce 
> wrote:
>
>>
>> On Aug 1, 2018, at 8:10 AM, Denis Pauk  wrote:
>>
>> Hi Marek,
>>
>> Could you merge this changes to master? Or someone else with push rights?
>>
>> Do mesa have some formal rule what amount of commits/contribution
>> required for recieve "merge" rights?
>>
>>
>> Hi Denis,
>>
>> We will push this for you on our next commit.  Alok has just gotten
>> married and is out on honeymoon,
>> so it may be a little while.  If Marek (or anyone else) would like to get
>> to it sooner, that would be
>> great.
>>
>> Thanks,
>> Bruce
>>
>>
>> On Sat, Jul 28, 2018 at 11:03 PM Denis Pauk  wrote:
>>
>>> Hi Bruce,
>>>
>>> Thank you, could you please merge commits? (I don't have commit rights.)
>>>
>>> On Fri, Jul 27, 2018 at 11:02 PM Cherniak, Bruce <
>>> bruce.chern...@intel.com> wrote:
>>>
 Reviewed-by: Bruce Cherniak 

 > On Jul 27, 2018, at 1:45 PM, Denis Pauk  wrote:
 >
 > Reuse Code from:
 > f69bc797e1 gallium/auxiliary: Add helper support for bptc format
 compress/decompress
 >
 > Signed-off-by: Denis Pauk 
 > CC: Marek Olšák 
 > CC: Bruce Cherniak 
 > CC: Tim Rowley 
 > ---
 > src/gallium/drivers/swr/swr_screen.cpp | 3 +--
 > 1 file changed, 1 insertion(+), 2 deletions(-)
 >
 > diff --git a/src/gallium/drivers/swr/swr_screen.cpp
 b/src/gallium/drivers/swr/swr_screen.cpp
 > index 65fa1bc50e..1cc01aa47d 100644
 > --- a/src/gallium/drivers/swr/swr_screen.cpp
 > +++ b/src/gallium/drivers/swr/swr_screen.cpp
 > @@ -137,8 +137,7 @@ swr_is_format_supported(struct pipe_screen
 *_screen,
 >  return FALSE;
 >}
 >
 > -   if (format_desc->layout == UTIL_FORMAT_LAYOUT_BPTC ||
 > -   format_desc->layout == UTIL_FORMAT_LAYOUT_ASTC) {
 > +   if (format_desc->layout == UTIL_FORMAT_LAYOUT_ASTC) {
 >   return FALSE;
 >}
 >
 > --
 > 2.18.0
 >


>>>
>>> --
>>> Best regards,
>>>   Denis.
>>>
>>
>>
>> --
>> Best regards,
>>   Denis.
>>
>>
>>

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

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add radeonsi_zerovram driconfig option

2018-09-03 Thread Marek Olšák

On Mon, Sep 3, 2018 at 6:29 AM, Timothy Arceri  wrote:
>
>
> On 28/08/18 04:26, Marek Olšák wrote:
>>
>> On Fri, Aug 24, 2018 at 10:33 AM, Michel Dänzer 
>> wrote:
>>>
>>> On 2018-08-24 1:06 p.m., Timothy Arceri wrote:

 More and more games seem to require this so lets make it a config
 option.
 ---
   src/gallium/drivers/radeonsi/driinfo_radeonsi.h |  1 +
   src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c   | 10 +++---
   src/util/xmlpool/t_options.h|  5 +
   3 files changed, 13 insertions(+), 3 deletions(-)

 diff --git a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
 b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
 index 7f57b4ea892..8c5078c13f3 100644
 --- a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
 +++ b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
 @@ -3,6 +3,7 @@ DRI_CONF_SECTION_PERFORMANCE
   DRI_CONF_RADEONSI_ENABLE_SISCHED("false")
   DRI_CONF_RADEONSI_ASSUME_NO_Z_FIGHTS("false")
   DRI_CONF_RADEONSI_COMMUTATIVE_BLEND_ADD("false")
 +DRI_CONF_RADEONSI_ZERO_ALL_VRAM_ALLOCS("false")
   DRI_CONF_SECTION_END

   [...]

 @@ -414,3 +414,8 @@ DRI_CONF_OPT_END
   DRI_CONF_OPT_BEGIN_B(radeonsi_clear_db_cache_before_clear, def) \
   DRI_CONF_DESC(en,"Clear DB cache before fast depth clear") \
   DRI_CONF_OPT_END
 +
 +#define DRI_CONF_RADEONSI_ZERO_ALL_VRAM_ALLOCS(def) \
 +DRI_CONF_OPT_BEGIN_B(radeonsi_zerovram, def) \
 +DRI_CONF_DESC(en,"Zero all vram allocations") \
 +DRI_CONF_OPT_END

>>>
>>> I'd name the option simply "zerovram", so it could be used by other
>>> drivers as well.
>>>
>>>
>>> BTW, AFAICT, currently this only affects BOs allocated from the kernel,
>>> not those re-used from the BO cache. I wonder if that couldn't still
>>> cause trouble with some apps.
>>
>>
>> It could.
>
>
> Maybe related to this?
>
> https://bugs.freedesktop.org/show_bug.cgi?id=65968#c12

I don't know. BTW, I've sent a patch that clears VRAM differently,
including reused buffers.

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

[Mesa-dev] [PATCH] radeonsi: clear VRAM buffers on creation if zerovram is set

2018-09-03 Thread Marek Olšák

From: Marek Olšák 

This includes reused buffers. Prefer DCC clear if DCC is enabled.
---
 src/gallium/drivers/radeonsi/si_buffer.c  | 8 
 src/gallium/drivers/radeonsi/si_pipe.c| 3 ++-
 src/gallium/drivers/radeonsi/si_texture.c | 5 -
 src/gallium/winsys/amdgpu/drm/amdgpu_bo.c | 3 ---
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c | 2 --
 src/gallium/winsys/amdgpu/drm/amdgpu_winsys.h | 1 -
 6 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/si_buffer.c 
b/src/gallium/drivers/radeonsi/si_buffer.c
index a03a944..f31767c 100644
--- a/src/gallium/drivers/radeonsi/si_buffer.c
+++ b/src/gallium/drivers/radeonsi/si_buffer.c
@@ -235,20 +235,28 @@ bool si_alloc_resource(struct si_screen *sscreen,
 
util_range_set_empty(>valid_buffer_range);
res->TC_L2_dirty = false;
 
/* Print debug information. */
if (sscreen->debug_flags & DBG(VM) && res->b.b.target == PIPE_BUFFER) {
fprintf(stderr, "VM start=0x%"PRIX64"  end=0x%"PRIX64" | Buffer 
%"PRIu64" bytes\n",
res->gpu_address, res->gpu_address + res->buf->size,
res->buf->size);
}
+
+   /* Only clear if DCC is disabled, because DCC clears are more 
efficient. */
+   if (sscreen->debug_flags & DBG_ZERO_VRAM &&
+   res->domains & RADEON_DOMAIN_VRAM &&
+   (res->b.b.target == PIPE_BUFFER ||
+!((struct si_texture*)res)->dcc_offset))
+   si_screen_clear_buffer(sscreen, >b.b, 0, res->bo_size, 0);
+
return true;
 }
 
 static void si_buffer_destroy(struct pipe_screen *screen,
  struct pipe_resource *buf)
 {
struct r600_resource *rbuffer = r600_resource(buf);
 
threaded_resource_deinit(buf);
util_range_destroy(>valid_buffer_range);
diff --git a/src/gallium/drivers/radeonsi/si_pipe.c 
b/src/gallium/drivers/radeonsi/si_pipe.c
index 9c8ed36..6e14066 100644
--- a/src/gallium/drivers/radeonsi/si_pipe.c
+++ b/src/gallium/drivers/radeonsi/si_pipe.c
@@ -850,21 +850,22 @@ struct pipe_screen *radeonsi_screen_create(struct 
radeon_winsys *ws,
si_init_screen_query_functions(sscreen);
 
/* Set these flags in debug_flags early, so that the shader cache takes
 * them into account.
 */
if (driQueryOptionb(config->options,
"glsl_correct_derivatives_after_discard"))
sscreen->debug_flags |= DBG(FS_CORRECT_DERIVS_AFTER_KILL);
if (driQueryOptionb(config->options, "radeonsi_enable_sisched"))
sscreen->debug_flags |= DBG(SI_SCHED);
-
+   if (driQueryOptionb(config->options, "radeonsi_zerovram"))
+   sscreen->debug_flags |= DBG(ZERO_VRAM);
 
if (sscreen->debug_flags & DBG(INFO))
ac_print_gpu_info(>info);
 
slab_create_parent(>pool_transfers,
   sizeof(struct si_transfer), 64);
 
sscreen->force_aniso = MIN2(16, debug_get_num_option("R600_TEX_ANISO", 
-1));
if (sscreen->force_aniso >= 0) {
printf("radeonsi: Forcing anisotropy filter to %ix\n",
diff --git a/src/gallium/drivers/radeonsi/si_texture.c 
b/src/gallium/drivers/radeonsi/si_texture.c
index e55fd81..2faeef2 100644
--- a/src/gallium/drivers/radeonsi/si_texture.c
+++ b/src/gallium/drivers/radeonsi/si_texture.c
@@ -1250,24 +1250,27 @@ si_texture_create_object(struct pipe_screen *screen,
clear_value = 0x030F;
 
si_screen_clear_buffer(sscreen, >buffer.b.b,
 tex->htile_offset,
 tex->surface.htile_size,
 clear_value);
}
 
/* Initialize DCC only if the texture is not being imported. */
if (!buf && tex->dcc_offset) {
+   unsigned clear_value =
+   sscreen->debug_flags & DBG_ZERO_VRAM ? 0 : 0x;
+
si_screen_clear_buffer(sscreen, >buffer.b.b,
 tex->dcc_offset,
 tex->surface.dcc_size,
-0x);
+clear_value);
}
 
/* Initialize the CMASK base register value. */
tex->cmask_base_address_reg =
(tex->buffer.gpu_address + tex->cmask_offset) >> 8;
 
if (sscreen->debug_flags & DBG(VM)) {
fprintf(stderr, "VM start=0x%"PRIX64"  end=0x%"PRIX64" | 
Texture %ix%ix%i, %i levels, %i samples, %s\n",
tex->buffer.gpu_address,
tex->buffer.gpu_address + tex->buffer.buf->size,
diff --git a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c 
b/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
index 68f0562..2842936 100644
--- a/src/gallium/winsys/amdgpu/drm/amdgpu_bo.c
+++

[Mesa-dev] [PATCH] intel/compiler: Remove redundant nir_remove_dead_variables call

2018-09-03 Thread Jason Ekstrand

As of 07a2098a708a2, brw_nir_optimize calls nir_remove_dead_variables as
the last optimization.  Doing it again is just pointless.
---
 src/intel/compiler/brw_nir.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index a906a026042..ce865e2ce71 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -715,8 +715,6 @@ brw_preprocess_nir(const struct brw_compiler *compiler, 
nir_shader *nir)
/* Get rid of split copies */
nir = brw_nir_optimize(nir, compiler, is_scalar, false);
 
-   OPT(nir_remove_dead_variables, nir_var_local);
-
return nir;
 }
 
-- 
2.17.1

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

Re: [Mesa-dev] [PATCH mesa] intel/compiler: remove unused get_image_base_type()

2018-09-03 Thread Jason Ekstrand

Acked-by: Jason Ekstrand 

On Mon, Sep 3, 2018 at 12:08 PM Eric Engestrom  wrote:

> Unused since 09f1de97a76a4990fd7c "anv,i965: Lower away image derefs in
> the driver".
>
> Cc: Jason Ekstrand 
> Signed-off-by: Eric Engestrom 
> ---
>  src/intel/compiler/brw_fs_nir.cpp | 19 ---
>  1 file changed, 19 deletions(-)
>
> diff --git a/src/intel/compiler/brw_fs_nir.cpp
> b/src/intel/compiler/brw_fs_nir.cpp
> index 2fef050f81a233491126..d5a5f60f942940e02904 100644
> --- a/src/intel/compiler/brw_fs_nir.cpp
> +++ b/src/intel/compiler/brw_fs_nir.cpp
> @@ -1712,25 +1712,6 @@ fs_visitor::emit_percomp(const fs_builder ,
> const fs_inst ,
> }
>  }
>
> -/**
> - * Get the matching channel register datatype for an image intrinsic of
> the
> - * specified GLSL image type.
> - */
> -static brw_reg_type
> -get_image_base_type(const glsl_type *type)
> -{
> -   switch ((glsl_base_type)type->sampled_type) {
> -   case GLSL_TYPE_UINT:
> -  return BRW_REGISTER_TYPE_UD;
> -   case GLSL_TYPE_INT:
> -  return BRW_REGISTER_TYPE_D;
> -   case GLSL_TYPE_FLOAT:
> -  return BRW_REGISTER_TYPE_F;
> -   default:
> -  unreachable("Not reached.");
> -   }
> -}
> -
>  static fs_inst *
>  emit_pixel_interpolater_send(const fs_builder ,
>   enum opcode opcode,
> --
> Cheers,
>   Eric
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/6] egl: make eglSwapInterval a no-op for !window surfaces

2018-09-03 Thread Eric Engestrom

On Monday, 2018-09-03 13:05:22 +0100, Emil Velikov wrote:
> From: Emil Velikov 
> 
> As the spec says, the function is a no-op when the surface is not a
> window one.
> 
> That spec implies that EGL_TRUE should be returned in that case, yet
> the ARM driver seems to return EGL_FALSE + EGL_BAD_SURFACE.
> 
> The Nvidia driver returns EGL_TRUE. We follow that behaviour until a
> decision is made.
> 
> https://gitlab.khronos.org/egl/API/merge_requests/17
> 
> Cc: samiuddi 
> Cc: Eric Engestrom 
> Cc: Erik Faye-Lund 
> Cc: Tomasz Figa 
> Cc: 
> Signed-off-by: Emil Velikov 
> ---
> Since this is a high-level API decision I've moved it to eglapi.c
> This will allow us to avoid duplicating the check across each platform
> codebase ... or more crashes because we forgot to update one.

Very good point! Series is:
Reviewed-by: Eric Engestrom 

Could you add the references to the bugs and tests fixed from the other
patch, and add Cc: stable to this patch and the eglSwapBuffers() one
(the rest of the series is just cleanup of stuff that these two patches
makes unnecessary, so let's just not touch that on stable branches)?

Cheers :)

> ---
>  src/egl/main/eglapi.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
> index c8c6a50f6ad..0af31a3f774 100644
> --- a/src/egl/main/eglapi.c
> +++ b/src/egl/main/eglapi.c
> @@ -1222,6 +1222,9 @@ eglSwapInterval(EGLDisplay dpy, EGLint interval)
> if (_eglGetSurfaceHandle(surf) == EGL_NO_SURFACE)
>RETURN_EGL_ERROR(disp, EGL_BAD_SURFACE, EGL_FALSE);
>  
> +   if (surf->Type != EGL_WINDOW_BIT)
> +  RETURN_EGL_EVAL(disp, EGL_TRUE);
> +
> interval = CLAMP(interval,
>  surf->Config->MinSwapInterval,
>  surf->Config->MaxSwapInterval);
> -- 
> 2.18.0
> 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH mesa] intel/compiler: remove unused get_image_base_type()

2018-09-03 Thread Eric Engestrom

Unused since 09f1de97a76a4990fd7c "anv,i965: Lower away image derefs in
the driver".

Cc: Jason Ekstrand 
Signed-off-by: Eric Engestrom 
---
 src/intel/compiler/brw_fs_nir.cpp | 19 ---
 1 file changed, 19 deletions(-)

diff --git a/src/intel/compiler/brw_fs_nir.cpp 
b/src/intel/compiler/brw_fs_nir.cpp
index 2fef050f81a233491126..d5a5f60f942940e02904 100644
--- a/src/intel/compiler/brw_fs_nir.cpp
+++ b/src/intel/compiler/brw_fs_nir.cpp
@@ -1712,25 +1712,6 @@ fs_visitor::emit_percomp(const fs_builder , const 
fs_inst ,
}
 }
 
-/**
- * Get the matching channel register datatype for an image intrinsic of the
- * specified GLSL image type.
- */
-static brw_reg_type
-get_image_base_type(const glsl_type *type)
-{
-   switch ((glsl_base_type)type->sampled_type) {
-   case GLSL_TYPE_UINT:
-  return BRW_REGISTER_TYPE_UD;
-   case GLSL_TYPE_INT:
-  return BRW_REGISTER_TYPE_D;
-   case GLSL_TYPE_FLOAT:
-  return BRW_REGISTER_TYPE_F;
-   default:
-  unreachable("Not reached.");
-   }
-}
-
 static fs_inst *
 emit_pixel_interpolater_send(const fs_builder ,
  enum opcode opcode,
-- 
Cheers,
  Eric

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

Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones

2018-09-03 Thread Erik Faye-Lund




On ma., sep. 3, 2018 at 11:50 AM, andrey simiklit 
 wrote:

Hi,

One more small think here:
> +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx,
> +   unsigned start_slot, 
unsigned count,
> +   const struct 
pipe_shader_buffer *buffers)

> +{
> +   int i;
I believe that it will be better to use 'unsigned' type for 'i' here
because the 'count' variable has 'unsigned' type.

Please lat me know if I am incorrect.



While I agree with you in principle, I'd rather keep it this way, and 
be consistent with what the surrounding code does (like 
virgl_encode_set_shader_buffers and virgl_encode_set_shader_images). 
These are already guaranteed never be big enough to require unsigned 
anyway.



Regars,
Andrii.

On Fri, Aug 31, 2018 at 8:35 PM Gurchetan Singh 
 wrote:

On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund
 wrote:
>
> From: Tomeu Vizoso 
>
> Emulating atomics on top of ssbos can lead to too small max SSBO 
count,
> so let's use the hw-atomics mechanism to expose atomic buffers 
instead.

>
> Signed-off-by: Erik Faye-Lund 
> ---
>  src/gallium/drivers/virgl/virgl_context.c  | 37 
++

>  src/gallium/drivers/virgl/virgl_context.h  |  2 ++
>  src/gallium/drivers/virgl/virgl_encode.c   | 23 ++
>  src/gallium/drivers/virgl/virgl_encode.h   |  3 ++
>  src/gallium/drivers/virgl/virgl_hw.h   |  5 +++
>  src/gallium/drivers/virgl/virgl_protocol.h |  9 ++
>  src/gallium/drivers/virgl/virgl_screen.c   | 14 +---
>  7 files changed, 88 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/drivers/virgl/virgl_context.c 
b/src/gallium/drivers/virgl/virgl_context.c

> index edc03f5dcf..30cd0e4331 100644
> --- a/src/gallium/drivers/virgl/virgl_context.c
> +++ b/src/gallium/drivers/virgl/virgl_context.c
> @@ -196,6 +196,19 @@ static void 
virgl_attach_res_shader_images(struct virgl_context *vctx,

> }
>  }
>
> +static void virgl_attach_res_atomic_buffers(struct virgl_context 
*vctx)

> +{
> +   struct virgl_winsys *vws = 
virgl_screen(vctx->base.screen)->vws;

> +   struct virgl_resource *res;
> +   unsigned i;
> +   for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) {

Why not PIPE_MAX_HW_ATOMIC_BUFFERS?

> +  res = virgl_resource(vctx->atomic_buffers[i]);
> +  if (res) {
> + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE);
> +  }
> +   }
> +}
> +
>  /*
>   * after flushing, the hw context still has a bunch of
>   * resources bound, so we need to rebind those here.
> @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct 
virgl_context *vctx)

>virgl_attach_res_shader_buffers(vctx, shader_type);
>virgl_attach_res_shader_images(vctx, shader_type);
> }
> +   virgl_attach_res_atomic_buffers(vctx);
> virgl_attach_res_vertex_buffers(vctx);
> virgl_attach_res_so_targets(vctx);
>  }
> @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context 
*ctx,

>  blit);
>  }
>
> +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx,
> +   unsigned start_slot,
> +   unsigned count,
> +   const struct 
pipe_shader_buffer *buffers)


nit: mixing tabs and spaces

> +{
> +   struct virgl_context *vctx = virgl_context(ctx);
> +
> +   for (unsigned i = 0; i < count; i++) {
> +  unsigned idx = start_slot + i;
> +
> +  if (buffers) {
> + if (buffers[i].buffer) {
> +pipe_resource_reference(>atomic_buffers[idx],
> +buffers[i].buffer);
> +continue;
> + }
> +  }
> +  pipe_resource_reference(>atomic_buffers[idx], NULL);
> +   }
> +   virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, 
buffers);

> +}
> +
>  static void virgl_set_shader_buffers(sunsignedtruct pipe_context 
*ctx,

>   enum pipe_shader_type shader,
>   unsigned start_slot, 
unsigned count,
> @@ -1209,6 +1245,7 @@ struct pipe_context 
*virgl_context_create(struct pipe_screen *pscreen,

> vctx->base.blit =  virgl_blit;
>
> vctx->base.set_shader_buffers = virgl_set_shader_buffers;
> +   vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers;
> vctx->base.set_shader_images = virgl_set_shader_images;
> vctx->base.memory_barrier = virgl_memory_barrier;
>
> diff --git a/src/gallium/drivers/virgl/virgl_context.h 
b/src/gallium/drivers/virgl/virgl_context.h

> index 38d1f450e1..20988baa3c 100644
> --- a/src/gallium/drivers/virgl/virgl_context.h
> +++ b/src/gallium/drivers/virgl/virgl_context.h
> @@ -75,6 +75,8 @@ struct virgl_context {
> int num_draws;
> struct list_head to_flush_bufs;
>
> +   struct pipe_resource 
*atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS];

> +
> struct primconvert_context *primconvert;
> uint32_t hw_sub_ctx_id;
>  };
> diff

Re: [Mesa-dev] [PATCH] intel: compiler: remove dead local variables at optimization pass

2018-09-03 Thread Lionel Landwerlin


On 03/09/2018 17:15, Jason Ekstrand wrote:
On September 3, 2018 10:47:11 Lionel Landwerlin 
 wrote:



We're hitting an assert in gfxbench because one of the local variable
is a sampler (according to Jason this isn't valid) :

testfw_app: ../src/compiler/nir_types.cpp:551: void 
glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, 
unsigned int*): Assertion `!"type does not have a natural size"' failed.


Since this particular variable isn't used and it can be eliminated by
removing unused local variables in the optimization pass. This makes
sense also for valid local variables.

v2: Move additional local variable removal out of optimization loop,
   but before large constant removal (Jason/Lionel)


Oh, I meant to make it the last thing brw_optimize_nir does because 
the other call to this pass is also right after brw_optimize_nir.  
This is also fine.


Reviewed-by: Jason Ekstrand 



Oh right, I'll update.





Signed-off-by: Lionel Landwerlin 
---
src/intel/compiler/brw_nir.c | 5 +
1 file changed, 5 insertions(+)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index ef5034d1e1e..8c1bcb99f8c 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler 
*compiler, nir_shader *nir)


   nir = brw_nir_optimize(nir, compiler, is_scalar, true);

+   /* Workaround Gfxbench unused local sampler variable which will 
trigger an

+    * assert in the opt_large_constants pass.
+    */
+   OPT(nir_remove_dead_variables, nir_var_local);
+
   /* This needs to be run after the first optimization pass but 
before we

    * lower indirect derefs away
    */
--
2.19.0.rc1




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



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

Re: [Mesa-dev] [PATCH 5/7] gallium: addPIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER{S, _BUFFERS}

2018-09-03 Thread Erik Faye-Lund




On fr., aug. 31, 2018 at 7:44 PM, Gurchetan Singh 
 wrote:

On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund
 wrote:


 This moves the evergreen-specific max-sizes out as a driver-cap, so
 other drivers with less strict requirements also can use hw-atomics.

 Remove ssbo_atomic as it's no longer needed.

 We should now be able to use hw-atomics for some stages and not for
 other, if needed.

 Signed-off-by: Erik Faye-Lund 
 ---
  src/gallium/drivers/etnaviv/etnaviv_screen.c  |  5 +
  .../drivers/freedreno/freedreno_screen.c  |  5 +
  .../drivers/nouveau/nv30/nv30_screen.c|  2 ++
  .../drivers/nouveau/nv50/nv50_screen.c|  2 ++
  .../drivers/nouveau/nvc0/nvc0_screen.c|  2 ++
  src/gallium/drivers/r300/r300_screen.c|  2 ++
  src/gallium/drivers/r600/r600_pipe.c  |  9 +
  src/gallium/drivers/radeonsi/si_get.c |  2 ++
  src/gallium/drivers/svga/svga_screen.c|  2 ++
  src/gallium/drivers/v3d/v3d_screen.c  |  2 ++
  src/gallium/drivers/vc4/vc4_screen.c  |  2 ++
  src/gallium/drivers/virgl/virgl_screen.c  |  2 ++
  src/gallium/include/pipe/p_defines.h  |  2 ++
  src/mesa/state_tracker/st_extensions.c| 20 
+--

  14 files changed, 49 insertions(+), 10 deletions(-)

 diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.c 
b/src/gallium/drivers/etnaviv/etnaviv_screen.c

 index 108b97d35c..95166a2db1 100644
 --- a/src/gallium/drivers/etnaviv/etnaviv_screen.c
 +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.c
 @@ -372,6 +372,11 @@ etna_screen_get_param(struct pipe_screen 
*pscreen, enum pipe_cap param)

return 0;
 case PIPE_CAP_UMA:
return 1;
 +
 +   /* hw atomic counters */
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
 +  return 0;
 }

 debug_printf("unknown param %d", param);
 diff --git a/src/gallium/drivers/freedreno/freedreno_screen.c 
b/src/gallium/drivers/freedreno/freedreno_screen.c

 index af44ab698e..e6dfc2e48e 100644
 --- a/src/gallium/drivers/freedreno/freedreno_screen.c
 +++ b/src/gallium/drivers/freedreno/freedreno_screen.c
 @@ -491,6 +491,11 @@ fd_screen_get_param(struct pipe_screen 
*pscreen, enum pipe_cap param)

 return 1;
 case PIPE_CAP_NATIVE_FENCE_FD:
 return fd_device_version(screen->dev) >= 
FD_VERSION_FENCE_FD;

 +
 +   /* hw atomic counters */
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
 +   return 0;
 }
 debug_printf("unknown param %d\n", param);
 return 0;
 diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
b/src/gallium/drivers/nouveau/nv30/nv30_screen.c

 index d52d8f3988..3fdbadb6c6 100644
 --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
 +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
 @@ -239,6 +239,8 @@ nv30_screen_get_param(struct pipe_screen 
*pscreen, enum pipe_cap param)

 case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
 case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
 case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
return 0;

 case PIPE_CAP_MAX_GS_INVOCATIONS:
 diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
b/src/gallium/drivers/nouveau/nv50/nv50_screen.c

 index cd36795e56..a6dda5dfa0 100644
 --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
 +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
 @@ -293,6 +293,8 @@ nv50_screen_get_param(struct pipe_screen 
*pscreen, enum pipe_cap param)

 case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
 case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
 case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
return 0;

 case PIPE_CAP_MAX_GS_INVOCATIONS:
 diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c 
b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c

 index 446e30dcc8..b628b24d76 100644
 --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
 +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
 @@ -322,6 +322,8 @@ nvc0_screen_get_param(struct pipe_screen 
*pscreen, enum pipe_cap param)

 case PIPE_CAP_CONSTBUF0_FLAGS:
 case PIPE_CAP_PACKED_UNIFORMS:
 case PIPE_CAP_CONSERVATIVE_RASTER_PRE_SNAP_POINTS_LINES:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
 +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
return 0;

 case PIPE_CAP_VENDOR_ID:
 diff --git a/src/gallium/drivers/r300/r300_screen.c 
b/src/gallium/drivers/r300/r300_screen.c

 index d27c2b8f1d..f29d5244ff 100644
 --- a/src/gallium/drivers/r300/r300_screen.c
 +++ b/src/gallium/drivers/r300/r300_screen.c
 @@ -261,6 +261,8 @@ static int

Re: [Mesa-dev] [PATCH] intel: compiler: remove dead local variables at optimization pass

2018-09-03 Thread Jason Ekstrand

On September 3, 2018 10:47:11 Lionel Landwerlin 
 wrote:



We're hitting an assert in gfxbench because one of the local variable
is a sampler (according to Jason this isn't valid) :

testfw_app: ../src/compiler/nir_types.cpp:551: void 
glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned 
int*): Assertion `!"type does not have a natural size"' failed.


Since this particular variable isn't used and it can be eliminated by
removing unused local variables in the optimization pass. This makes
sense also for valid local variables.

v2: Move additional local variable removal out of optimization loop,
   but before large constant removal (Jason/Lionel)


Oh, I meant to make it the last thing brw_optimize_nir does because the 
other call to this pass is also right after brw_optimize_nir.  This is also 
fine.


Reviewed-by: Jason Ekstrand 


Signed-off-by: Lionel Landwerlin 
---
src/intel/compiler/brw_nir.c | 5 +
1 file changed, 5 insertions(+)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index ef5034d1e1e..8c1bcb99f8c 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler 
*compiler, nir_shader *nir)


   nir = brw_nir_optimize(nir, compiler, is_scalar, true);

+   /* Workaround Gfxbench unused local sampler variable which will trigger an
+* assert in the opt_large_constants pass.
+*/
+   OPT(nir_remove_dead_variables, nir_var_local);
+
   /* This needs to be run after the first optimization pass but before we
* lower indirect derefs away
*/
--
2.19.0.rc1




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

Re: [Mesa-dev] [PATCH 7/7] virgl: use hw-atomics instead of in-ssbo ones

2018-09-03 Thread Erik Faye-Lund




On fr., aug. 31, 2018 at 7:35 PM, Gurchetan Singh 
 wrote:

On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund
 wrote:


 From: Tomeu Vizoso 

 Emulating atomics on top of ssbos can lead to too small max SSBO 
count,
 so let's use the hw-atomics mechanism to expose atomic buffers 
instead.


 Signed-off-by: Erik Faye-Lund 
 ---
  src/gallium/drivers/virgl/virgl_context.c  | 37 
++

  src/gallium/drivers/virgl/virgl_context.h  |  2 ++
  src/gallium/drivers/virgl/virgl_encode.c   | 23 ++
  src/gallium/drivers/virgl/virgl_encode.h   |  3 ++
  src/gallium/drivers/virgl/virgl_hw.h   |  5 +++
  src/gallium/drivers/virgl/virgl_protocol.h |  9 ++
  src/gallium/drivers/virgl/virgl_screen.c   | 14 +---
  7 files changed, 88 insertions(+), 5 deletions(-)

 diff --git a/src/gallium/drivers/virgl/virgl_context.c 
b/src/gallium/drivers/virgl/virgl_context.c

 index edc03f5dcf..30cd0e4331 100644
 --- a/src/gallium/drivers/virgl/virgl_context.c
 +++ b/src/gallium/drivers/virgl/virgl_context.c
 @@ -196,6 +196,19 @@ static void 
virgl_attach_res_shader_images(struct virgl_context *vctx,

 }
  }

 +static void virgl_attach_res_atomic_buffers(struct virgl_context 
*vctx)

 +{
 +   struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws;
 +   struct virgl_resource *res;
 +   unsigned i;
 +   for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) {


Why not PIPE_MAX_HW_ATOMIC_BUFFERS?



Yeah, that seems better. Fixed locally.


 +  res = virgl_resource(vctx->atomic_buffers[i]);
 +  if (res) {
 + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE);
 +  }
 +   }
 +}
 +
  /*
   * after flushing, the hw context still has a bunch of
   * resources bound, so we need to rebind those here.
 @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct 
virgl_context *vctx)

virgl_attach_res_shader_buffers(vctx, shader_type);
virgl_attach_res_shader_images(vctx, shader_type);
 }
 +   virgl_attach_res_atomic_buffers(vctx);
 virgl_attach_res_vertex_buffers(vctx);
 virgl_attach_res_so_targets(vctx);
  }
 @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context 
*ctx,

  blit);
  }

 +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx,
 +   unsigned start_slot,
 +   unsigned count,
 +   const struct 
pipe_shader_buffer *buffers)


nit: mixing tabs and spaces



Thanks, fixed locally.


 +{
 +   struct virgl_context *vctx = virgl_context(ctx);
 +
 +   for (unsigned i = 0; i < count; i++) {
 +  unsigned idx = start_slot + i;
 +
 +  if (buffers) {
 + if (buffers[i].buffer) {
 +pipe_resource_reference(>atomic_buffers[idx],
 +buffers[i].buffer);
 +continue;
 + }
 +  }
 +  pipe_resource_reference(>atomic_buffers[idx], NULL);
 +   }
 +   virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, 
buffers);

 +}
 +
  static void virgl_set_shader_buffers(struct pipe_context *ctx,
   enum pipe_shader_type shader,
   unsigned start_slot, unsigned 
count,
 @@ -1209,6 +1245,7 @@ struct pipe_context 
*virgl_context_create(struct pipe_screen *pscreen,

 vctx->base.blit =  virgl_blit;

 vctx->base.set_shader_buffers = virgl_set_shader_buffers;
 +   vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers;
 vctx->base.set_shader_images = virgl_set_shader_images;
 vctx->base.memory_barrier = virgl_memory_barrier;

 diff --git a/src/gallium/drivers/virgl/virgl_context.h 
b/src/gallium/drivers/virgl/virgl_context.h

 index 38d1f450e1..20988baa3c 100644
 --- a/src/gallium/drivers/virgl/virgl_context.h
 +++ b/src/gallium/drivers/virgl/virgl_context.h
 @@ -75,6 +75,8 @@ struct virgl_context {
 int num_draws;
 struct list_head to_flush_bufs;

 +   struct pipe_resource 
*atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS];

 +
 struct primconvert_context *primconvert;
 uint32_t hw_sub_ctx_id;
  };
 diff --git a/src/gallium/drivers/virgl/virgl_encode.c 
b/src/gallium/drivers/virgl/virgl_encode.c

 index bcb14d8939..c9cc099061 100644
 --- a/src/gallium/drivers/virgl/virgl_encode.c
 +++ b/src/gallium/drivers/virgl/virgl_encode.c
 @@ -958,6 +958,29 @@ int virgl_encode_set_shader_buffers(struct 
virgl_context *ctx,

 return 0;
  }

 +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx,
 +   unsigned start_slot, 
unsigned count,
 +   const struct 
pipe_shader_buffer *buffers)

 +{
 +   int i;
 +   virgl_encoder_write_cmd_dword(ctx, 
VIRGL_CMD0(VIRGL_CCMD_SET_ATOMIC_BUFFERS, 0, 
VIRGL_SET_ATOMIC_BUFFER_SIZE(count)));

 +
 +   virgl_encoder_write_dword(ctx->cbuf, start_slot);
 +   for (i = 0; i < count; i++) {
 +  if (buffers) {
 +struct

Re: [Mesa-dev] [PATCH] intel: compiler: remove dead local variables at optimization pass

2018-09-03 Thread Lionel Landwerlin


On 03/09/2018 16:47, Lionel Landwerlin wrote:

We're hitting an assert in gfxbench because one of the local variable
is a sampler (according to Jason this isn't valid) :

testfw_app: ../src/compiler/nir_types.cpp:551: void 
glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned int*): 
Assertion `!"type does not have a natural size"' failed.

Since this particular variable isn't used and it can be eliminated by
removing unused local variables in the optimization pass. This makes
sense also for valid local variables.

v2: Move additional local variable removal out of optimization loop,
 but before large constant removal (Jason/Lionel)

Signed-off-by: Lionel Landwerlin 



Forgot to add Eero's bug : 
https://bugs.freedesktop.org/show_bug.cgi?id=107806




---
  src/intel/compiler/brw_nir.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index ef5034d1e1e..8c1bcb99f8c 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler *compiler, 
nir_shader *nir)
  
 nir = brw_nir_optimize(nir, compiler, is_scalar, true);
  
+   /* Workaround Gfxbench unused local sampler variable which will trigger an

+* assert in the opt_large_constants pass.
+*/
+   OPT(nir_remove_dead_variables, nir_var_local);
+
 /* This needs to be run after the first optimization pass but before we
  * lower indirect derefs away
  */



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

[Mesa-dev] [PATCH] intel: compiler: remove dead local variables at optimization pass

2018-09-03 Thread Lionel Landwerlin

We're hitting an assert in gfxbench because one of the local variable
is a sampler (according to Jason this isn't valid) :

testfw_app: ../src/compiler/nir_types.cpp:551: void 
glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned 
int*): Assertion `!"type does not have a natural size"' failed.

Since this particular variable isn't used and it can be eliminated by
removing unused local variables in the optimization pass. This makes
sense also for valid local variables.

v2: Move additional local variable removal out of optimization loop,
but before large constant removal (Jason/Lionel)

Signed-off-by: Lionel Landwerlin 
---
 src/intel/compiler/brw_nir.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
index ef5034d1e1e..8c1bcb99f8c 100644
--- a/src/intel/compiler/brw_nir.c
+++ b/src/intel/compiler/brw_nir.c
@@ -673,6 +673,11 @@ brw_preprocess_nir(const struct brw_compiler *compiler, 
nir_shader *nir)
 
nir = brw_nir_optimize(nir, compiler, is_scalar, true);
 
+   /* Workaround Gfxbench unused local sampler variable which will trigger an
+* assert in the opt_large_constants pass.
+*/
+   OPT(nir_remove_dead_variables, nir_var_local);
+
/* This needs to be run after the first optimization pass but before we
 * lower indirect derefs away
 */
-- 
2.19.0.rc1

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

Re: [Mesa-dev] [PATCH 4/5] util/gen_xmlpool: Add a --meson switch

2018-09-03 Thread Dylan Baker

Quoting Emil Velikov (2018-09-03 07:18:31)
> On 24 August 2018 at 19:45, Dylan Baker  wrote:
> > Quoting Emil Velikov (2018-08-24 08:57:29)
> >> On Fri, 24 Aug 2018 at 15:16, Dylan Baker  wrote:
> >> >
> >> > Meson won't put the .gmo files in the layout that python's
> >> > gettext.translation() expects, so we need to handle them differently,
> >> > this switch allows the script to load the files as meson lays them out
> >>
> >> No obvious reason comes to mind why we want divergence here.
> >> Can you elaborate more what's happening here - what are the .gmo
> >> files, I though we're using .mo ones?
> >
> > Meson uses .gmo to distinguish that they are specifics GNU mo files, as 
> > opposed
> > to one of the other flaovrs like the solaris mo files, which use a 
> > completely
> > different syntax. The real difference is that autotools generates a folder
> > hierarchy to place the .mo (or .gmo) files in, but meson doesn't guarantee
> > folder structures in the build directory, so mimicking the autotools 
> > behavior
> > isn't guaranteed to work or continue working.
> >
> >>
> >> If the only difference is a) extension and b) .mo file location - we
> >> could update the autoconf/others to follow the same pattern.
> >
> > We certainly could change autotools to follow the same pattern if that's 
> > what
> > you wanted to do. I just don't have the expertise with autotools to do it
> > quickly, and I wanted to get something wired for meson.
> >
> Off the top of my head, the following should work pretty much
> everywhere - android/autoconf/scons(?)

I know for certain scons doesn't use translations, so that's fine. I'm fairly
doubtful android does (and if they do they probably shouldn't) since this is
only for drirc description translations.

I'm just checking email today (It's a US holiday, but my dog woke me up at 6 and
I'm bored). But I'll apply this patch tomorrow and see about dropping the
--meson option.

Dylan

> 
> s/\/LC_MESSAGES\/options// -- change $lang/LC_MESSAGES/options.mo -> $lang.mo
> s/\/gmo/;s/\/GMOS/ -- change .mo -> .gmo
> s/$$lang\/LC_MESSAGES/$(@D)/ -- change $lang/LD_MESSAGES -> current_builddir
> 
> Autoconf patch - formatting may be off:
> 
>  LANGS=$(POS:%.po=%)
> -MOS=$(POS:%.po=%/LC_MESSAGES/options.mo)
> +GMOS=$(POS:%.po=%.gmo)
>  POT=xmlpool.pot
> 
> -.PHONY: all clean pot po mo
> +.PHONY: all clean pot po gmo
> 
>  EXTRA_DIST = \
>   gen_xmlpool.py \
>   options.h \
>   t_options.h \
>   $(POS) \
> - $(MOS) \
> + $(GMOS) \
>   SConscript \
>   meson.build
> 
> @@ -65,19 +65,19 @@ BUILT_SOURCES = options.h
>  CLEANFILES = \
>   options.h
>   $(POS) \
> - $(MOS)
> + $(GMOS)
> 
>  # Default target options.h
>  LOCALEDIR := .
>  options.h: t_options.h $(GMOS)
>   $(AM_V_GEN) $(PYTHON2) $(PYTHON_FLAGS) $(srcdir)/gen_xmlpool.py
> $(srcdir)/t_options.h $(LOCALEDIR) $(LANGS) > options.h
> 
> -# Update .mo files from the corresponding .po files.
> -%/LC_MESSAGES/options.mo: %.po
> - @mo="$@"; \
> - lang=$${mo%%/*}; \
> +# Update .gmo files from the corresponding .po files.
> +%.gmo: %.po
> + @gmo="$@"; \
> + lang=$${gmo%%/*}; \
>   echo "Updating ($$lang) $@ from $?."; \
> - $(MKDIR_P) $$lang/LC_MESSAGES; \
> + $(MKDIR_P) $(@D); \
>   msgfmt -o $@ $?
> 
>  # Use this target to create or update .po files with new messages in


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

[Mesa-dev] [Bug 107670] Massive slowdown under specific memcpy implementations (32bit, no-SIMD, backward copy).

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107670

--- Comment #12 from Emil Velikov  ---
Why are we even discussing a potential optimisation where the user is
_unknown_?
It contradicts with the principles that we've been using in Mesa for years.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()

2018-09-03 Thread Emil Velikov

On 3 September 2018 at 15:49,   wrote:
> From: Andrii Simiklit 
>
> MSDN:
> "va_end must be called on each argument list that's initialized
>  with va_start or va_copy before the function returns."
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
> Signed-off-by: Andrii Simiklit 
>
> ---
>  src/util/u_string.h | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/util/u_string.h b/src/util/u_string.h
> index ce45430..883fa64 100644
> --- a/src/util/u_string.h
> +++ b/src/util/u_string.h
> @@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char *format, 
> va_list ap)
> if (ret < 0) {
>ret = _vscprintf(format, ap_copy);
> }
> +   va_end(ap_copy);
> return ret;

These WIN32 compat functions seem iffy.
Namely - often we'll issue a va_copy only to use the original va.

Looking through the rest of the code base:
There are plenty of va_start + util_vsnprintf + va_free cases that
could be replaced with util_snprintf.

There are also some util_vsnprintf users which use va_copy and some
who do not :-\

I think those should be looked first, otherwise adding this va_end
could easily break things.

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

[Mesa-dev] [Bug 107670] Massive slowdown under specific memcpy implementations (32bit, no-SIMD, backward copy).

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107670

--- Comment #11 from Eero Tamminen  ---
Libc memcpy() obviously won't be optimized for PCI bus transfers, it's way too
rare use-case for it.

E.g. libpciaccess would seem more suitable place for PCI bus transfer optimized
memory copy function, but unfortunately it doesn't (currently) provide an API
for that.


(In reply to Emil Velikov from comment #10)
> If memcpy shows so prominently in perf, we should look why we're using it so
> often. Polishing the memcpy implementation is putting a band-aid instead of
> fixing the actual problem.

I.e. are the uploads triggered by something in driver, rather than application
itself directly doing it?

"valgrind --tool=callgrind " would output callgraph info with call
counts etc, which can be viewed in kcachegrind.

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 107810] The 'va_end' call is missed after 'va_copy' in 'util_vsnprintf' function under windows

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107810

--- Comment #1 from asimiklit  ---
Solution is suggested:
https://patchwork.freedesktop.org/patch/246968/

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] mesa/util: add missing va_end() after va_copy()

2018-09-03 Thread asimiklit . work

From: Andrii Simiklit 

MSDN:
"va_end must be called on each argument list that's initialized
 with va_start or va_copy before the function returns."

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107810
Signed-off-by: Andrii Simiklit 

---
 src/util/u_string.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/util/u_string.h b/src/util/u_string.h
index ce45430..883fa64 100644
--- a/src/util/u_string.h
+++ b/src/util/u_string.h
@@ -81,6 +81,7 @@ util_vsnprintf(char *str, size_t size, const char *format, 
va_list ap)
if (ret < 0) {
   ret = _vscprintf(format, ap_copy);
}
+   va_end(ap_copy);
return ret;
 }
 
-- 
2.7.4

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

[Mesa-dev] [Bug 107670] Massive slowdown under specific memcpy implementations (32bit, no-SIMD, backward copy).

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107670

--- Comment #10 from Emil Velikov  ---
My personal train of though:

Details such as WC are left to the kernel module. Even on the case where
userspace can provide hints, it's ultimately up-to the kernel to manage it.

Optimising w/o saying the benchmark/game name is _seriously_ moot.
Furthermore, doing benchmarks on a i586 build is also fairly moot.

You are correct though - _if_ glibc decides to change things perf. _may_ drop.

If memcpy shows so prominently in perf, we should look why we're using it so
often. Polishing the memcpy implementation is putting a band-aid instead of
fixing the actual problem.

Again, that's my personal take. Feel free to ignore.

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [Bug 107810] The 'va_end' call is missed after 'va_copy' in 'util_vsnprintf' function under windows

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107810

Bug ID: 107810
   Summary: The 'va_end' call is missed after 'va_copy' in
'util_vsnprintf' function under windows
   Product: Mesa
   Version: git
  Hardware: Other
OS: All
Status: NEW
  Severity: normal
  Priority: medium
 Component: Other
  Assignee: mesa-dev@lists.freedesktop.org
  Reporter: andrey.simik...@gmail.com
QA Contact: mesa-dev@lists.freedesktop.org

MSDN:
"va_end must be called on each argument list that's initialized
 with va_start or va_copy before the function returns."

-- 
You are receiving this mail because:
You are the assignee for the bug.
You are the QA Contact for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 4/5] util/gen_xmlpool: Add a --meson switch

2018-09-03 Thread Emil Velikov

On 24 August 2018 at 19:45, Dylan Baker  wrote:
> Quoting Emil Velikov (2018-08-24 08:57:29)
>> On Fri, 24 Aug 2018 at 15:16, Dylan Baker  wrote:
>> >
>> > Meson won't put the .gmo files in the layout that python's
>> > gettext.translation() expects, so we need to handle them differently,
>> > this switch allows the script to load the files as meson lays them out
>>
>> No obvious reason comes to mind why we want divergence here.
>> Can you elaborate more what's happening here - what are the .gmo
>> files, I though we're using .mo ones?
>
> Meson uses .gmo to distinguish that they are specifics GNU mo files, as 
> opposed
> to one of the other flaovrs like the solaris mo files, which use a completely
> different syntax. The real difference is that autotools generates a folder
> hierarchy to place the .mo (or .gmo) files in, but meson doesn't guarantee
> folder structures in the build directory, so mimicking the autotools behavior
> isn't guaranteed to work or continue working.
>
>>
>> If the only difference is a) extension and b) .mo file location - we
>> could update the autoconf/others to follow the same pattern.
>
> We certainly could change autotools to follow the same pattern if that's what
> you wanted to do. I just don't have the expertise with autotools to do it
> quickly, and I wanted to get something wired for meson.
>
Off the top of my head, the following should work pretty much
everywhere - android/autoconf/scons(?)

s/\/LC_MESSAGES\/options// -- change $lang/LC_MESSAGES/options.mo -> $lang.mo
s/\/gmo/;s/\/GMOS/ -- change .mo -> .gmo
s/$$lang\/LC_MESSAGES/$(@D)/ -- change $lang/LD_MESSAGES -> current_builddir

Autoconf patch - formatting may be off:

 LANGS=$(POS:%.po=%)
-MOS=$(POS:%.po=%/LC_MESSAGES/options.mo)
+GMOS=$(POS:%.po=%.gmo)
 POT=xmlpool.pot

-.PHONY: all clean pot po mo
+.PHONY: all clean pot po gmo

 EXTRA_DIST = \
  gen_xmlpool.py \
  options.h \
  t_options.h \
  $(POS) \
- $(MOS) \
+ $(GMOS) \
  SConscript \
  meson.build

@@ -65,19 +65,19 @@ BUILT_SOURCES = options.h
 CLEANFILES = \
  options.h
  $(POS) \
- $(MOS)
+ $(GMOS)

 # Default target options.h
 LOCALEDIR := .
 options.h: t_options.h $(GMOS)
  $(AM_V_GEN) $(PYTHON2) $(PYTHON_FLAGS) $(srcdir)/gen_xmlpool.py
$(srcdir)/t_options.h $(LOCALEDIR) $(LANGS) > options.h

-# Update .mo files from the corresponding .po files.
-%/LC_MESSAGES/options.mo: %.po
- @mo="$@"; \
- lang=$${mo%%/*}; \
+# Update .gmo files from the corresponding .po files.
+%.gmo: %.po
+ @gmo="$@"; \
+ lang=$${gmo%%/*}; \
  echo "Updating ($$lang) $@ from $?."; \
- $(MKDIR_P) $$lang/LC_MESSAGES; \
+ $(MKDIR_P) $(@D); \
  msgfmt -o $@ $?

 # Use this target to create or update .po files with new messages in
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] intel: compiler: remove dead local variables at optimization pass

2018-09-03 Thread Jason Ekstrand

On Thu, Aug 23, 2018 at 8:38 AM Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:

> We're hitting an assert in gfxbench because one of the local variable
> is a sampler (according to Jason this isn't valid) :
>
> testfw_app: ../src/compiler/nir_types.cpp:551: void
> glsl_get_natural_size_align_bytes(const glsl_type*, unsigned int*, unsigned
> int*): Assertion `!"type does not have a natural size"' failed.
>
> Since this particular variable isn't used and it can be eliminated by
> removing unused local variables in the optimization pass. This makes
> sense also for valid local variables.
>
> Signed-off-by: Lionel Landwerlin 
> ---
>  src/intel/compiler/brw_nir.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/src/intel/compiler/brw_nir.c b/src/intel/compiler/brw_nir.c
> index 29ad68fdb2a..ede341c8f3a 100644
> --- a/src/intel/compiler/brw_nir.c
> +++ b/src/intel/compiler/brw_nir.c
> @@ -586,6 +586,7 @@ brw_nir_optimize(nir_shader *nir, const struct
> brw_compiler *compiler,
>   nir_lower_dround_even |
>   nir_lower_dmod);
>OPT(nir_lower_pack);
> +  OPT(nir_remove_dead_variables, nir_var_local);
>

I'm not sure how much it actually helps to have this in the optimization
loop and it is an extra (though fairly cheap) pass over the IR.  Maybe we
should just put it right before opt_large_constants instead?  Or, better
yet, just put it the end of brw_nir_optimize after the loop and then we can
remove the other call to it that happens at the end of brw_preprocess_nir.

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

Re: [Mesa-dev] [PATCH 2/3] configure: allow building with python3

2018-09-03 Thread Emil Velikov

On 24 August 2018 at 19:51, Dylan Baker  wrote:
> Can we just change the script to write a file instead of sending it's output
> through the shell? That should fix any encoding problems since the shell wont
> touch it and the LANG settings (no matter what they are) shouldn't matter.
>
Seems like I forgot to reply to this. Yes, please - that would be
highly preferred.

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

Re: [Mesa-dev] [PATCH] st/dri: implement the __DRI_DRIVER_VTABLE extension

2018-09-03 Thread Emil Velikov

Hi Eric,

On 24 August 2018 at 14:11, Emil Velikov  wrote:
> From: Emil Velikov 
>
> As the comment above globalDriverAPI (in dri_util.c) says, if the loader
> is unaware of createNewScreen2 there is a race condition.
>
> In which globalDriverAPI, will be set in the driver driDriverGetExtensions*
> function and used in createNewScreen(). If we call another drivers'
> driDriverGetExtensions, the createNewScreen will use the latter's API
> instead of the former.
>
> To make it more convoluting, the driver _must_ also expose
> __DRI_DRIVER_VTABLE, as that one exposes the correct API.
>
> The race also occurs, for loaders which use the pre megadrivers
> driDriverGetExtensions entrypoint.
>
> Cc: mesa-sta...@lists.freedesktop.org
> Signed-off-by: Emil Velikov 
> ---
>  src/gallium/state_trackers/dri/dri2.c   | 21 +
>  src/gallium/state_trackers/dri/dri_screen.h |  1 +
>  src/gallium/state_trackers/dri/drisw.c  |  6 ++
>  src/gallium/targets/dri/target.c|  2 +-
>  4 files changed, 29 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/state_trackers/dri/dri2.c 
> b/src/gallium/state_trackers/dri/dri2.c
> index 3cbca4e5dc3..b21e6815796 100644
> --- a/src/gallium/state_trackers/dri/dri2.c
> +++ b/src/gallium/state_trackers/dri/dri2.c
> @@ -2318,11 +2318,32 @@ const struct __DriverAPIRec dri_kms_driver_api = {
> .ReleaseBuffer  = dri2_release_buffer,
>  };
>
> +static const struct __DRIDriverVtableExtensionRec gallium_drm_vtable = {
> +   .base = { __DRI_DRIVER_VTABLE, 1 },
> +   .vtable = _driver_api,
> +};
> +
> +static const struct __DRIDriverVtableExtensionRec dri_kms_vtable = {
> +   .base = { __DRI_DRIVER_VTABLE, 1 },
> +   .vtable = _kms_driver_api,
> +};
> +
>  /* This is the table of extensions that the loader will dlsym() for. */
>  const __DRIextension *galliumdrm_driver_extensions[] = {
>  ,
>  ,
>  ,
> +_drm_vtable.base,
> +_config_options.base,
> +NULL
> +};
> +
> +/* This is the table of extensions that the loader will dlsym() for. */
> +const __DRIextension *dri_kms_driver_extensions[] = {
> +,
> +,
> +,
> +_kms_vtable.base,
>  _config_options.base,
>  NULL
>  };
> diff --git a/src/gallium/state_trackers/dri/dri_screen.h 
> b/src/gallium/state_trackers/dri/dri_screen.h
> index 8d2d9c02892..fde3b4088a7 100644
> --- a/src/gallium/state_trackers/dri/dri_screen.h
> +++ b/src/gallium/state_trackers/dri/dri_screen.h
> @@ -147,6 +147,7 @@ void
>  dri_destroy_screen(__DRIscreen * sPriv);
>
>  extern const struct __DriverAPIRec dri_kms_driver_api;
> +extern const __DRIextension *dri_kms_driver_extensions[];
>
>  extern const struct __DriverAPIRec galliumdrm_driver_api;
>  extern const __DRIextension *galliumdrm_driver_extensions[];
> diff --git a/src/gallium/state_trackers/dri/drisw.c 
> b/src/gallium/state_trackers/dri/drisw.c
> index 1fba71bdd97..76a06b36664 100644
> --- a/src/gallium/state_trackers/dri/drisw.c
> +++ b/src/gallium/state_trackers/dri/drisw.c
> @@ -513,11 +513,17 @@ const struct __DriverAPIRec galliumsw_driver_api = {
> .CopySubBuffer = drisw_copy_sub_buffer,
>  };
>
> +static const struct __DRIDriverVtableExtensionRec galliumsw_vtable = {
> +   .base = { __DRI_DRIVER_VTABLE, 1 },
> +   .vtable = _driver_api,
> +};
> +
>  /* This is the table of extensions that the loader will dlsym() for. */
>  const __DRIextension *galliumsw_driver_extensions[] = {
>  ,
>  ,
>  ,
> +_vtable.base,
>  _config_options.base,
>  NULL
>  };
> diff --git a/src/gallium/targets/dri/target.c 
> b/src/gallium/targets/dri/target.c
> index 835d125f21e..e943cae6a16 100644
> --- a/src/gallium/targets/dri/target.c
> +++ b/src/gallium/targets/dri/target.c
> @@ -28,7 +28,7 @@ const __DRIextension 
> **__driDriverGetExtensions_kms_swrast(void);
>  PUBLIC const __DRIextension **__driDriverGetExtensions_kms_swrast(void)
>  {
> globalDriverAPI = _kms_driver_api;
> -   return galliumdrm_driver_extensions;
> +   return dri_kms_driver_extensions;
>  }
>
Can you please skim through the above?

Seems like I forgot to implement this while making the gallium mega-drivers.
I did not notice any issues in practise, but with EGLDevice this will
become more likely to hit.

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

Re: [Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation (v2)

2018-09-03 Thread Emil Velikov

On 3 September 2018 at 13:57, Gert Wollny  wrote:
> Fixes crash with
>   piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
>increment-offset -auto -fbo
>
> * Resize the resource storage already when the count is equal to the
>   allocated size, fixes:
>
>   Invalid write of size 8
>   at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
>   by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
>   by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
>   by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
>   by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
>   by 0x109A1E: upload (invalidate.c:169)
>   by 0x109C2F: piglit_display (invalidate.c:215)
>   by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
>   by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
>   by 0x10949D: main (invalidate.c:47)
>   Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
>   at 0x4C31B25: calloc (in
>/usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
>   by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)
>
> * Also resize the space allocated for the handles, fixes:
>
>   Invalid write of size 4
>   at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
>   by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
>   by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
>   by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
>   by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
>   by 0x109A1E: upload (invalidate.c:169)
>   by 0x109C2F: piglit_display (invalidate.c:215)
>   by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
>   by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
>   by 0x10949D: main (invalidate.c:47)
>   Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
>   at 0x4C2FB0F: malloc (
> in /usr/lib/valgrind/vgpreload_memcheck-amd64- linux.so)
>   by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)
>
> Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need to.")
>
> v2: - Use REALLOC macro and avoid memory leak when re-allocation fails
> - add Fixes tag (both Emil Velikov)
> - reorder commit message
>
> Signed-off-by: Gert Wollny 
> Reviewed-by: Emil Velikov  (v1)
>
> ---
> I resend it because I didn't apply all the changes Emil suggested, 
> specifically
> I still change to compare for equality.  The new commit message should make it
> clearer what this fixes.
>
Ack. I've throws some nitpicks below, please don't read too much into them.
Earlier R-B still stands.


>  .../winsys/virgl/drm/virgl_drm_winsys.c   | 24 +++
>  1 file changed, 19 insertions(+), 5 deletions(-)
>
> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> index aad6430c41..e780a5e514 100644
> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> @@ -617,13 +617,27 @@ static void virgl_drm_add_res(struct virgl_drm_winsys 
> *qdws,
>  {
> unsigned hash = res->res_handle & (sizeof(cbuf->is_handle_added)-1);
>
> -   if (cbuf->cres > cbuf->nres) {
> -  cbuf->nres += 256;
> -  cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres * sizeof(struct 
> virgl_hw_buf*));
> -  if (!cbuf->res_bo) {
> -  fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
> cbuf->nres);
> +   if (cbuf->cres >= cbuf->nres) {
> +  unsigned new_nres = cbuf->nres + 256;
> +  void *new_ptr = REALLOC(cbuf->res_bo, cbuf->nres * sizeof(struct 
> virgl_hw_buf*),
struct virgl_hw_res **new_bo =

> +new_nres * sizeof(struct virgl_hw_buf*));
> +  if (!new_ptr) {
> +  fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
> new_nres);
>return;
>}
> +  cbuf->res_bo = new_ptr;
> +
> +  new_ptr = REALLOC(cbuf->res_hlist, cbuf->nres * sizeof(uint32_t),
uint32_t *new_hlist =

> +new_nres * sizeof(uint32_t));
> +  if (!new_ptr) {
> +  fprintf(stderr,"failure to add hlist relocation %d, %d\n", 
> cbuf->cres, cbuf->nres);
FREE(new_bo);

> +  return;
> +  }
> +  cbuf->res_hlist = new_ptr;
> +  /* This is not perfect, because when the first re-allocation succeeds 
> but
> +   * the second failes the first old_size will be wrong, but currently it
> +   * isn't used anyway. */
Now you can drop the comment.

Aside: the old_size of REALLOC could/should go - debug_realloc can use
old_hdr->size.
Using sizeof(instance) should be clearer and more consistent with the
rest of winsys/virgl.

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

Re: [Mesa-dev] [ANNOUNCE] mesa 18.2.0-rc5

2018-09-03 Thread Andres Gomez

On Fri, 2018-08-31 at 13:24 -0700, Mark Janes wrote:
> Hi Andres,
> 
> The final blockers have been resolved.  You should be able to make an RC
> that passes all Intel validation, if you pick up:
> 
> 904c2a617d8 * i965/gen7_urb: Re-emit PUSH_CONSTANT_ALLOC on some gen9
> d9cf4308cee * i965/screen: Allow modifiers on sRGB formats
> 
> Looking forward to the release,

Awesome!

Thanks for the heads up! ☺

-- 
Br,

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

Re: [Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation

2018-09-03 Thread Gert Wollny


> That sounds reasonable - I would squash the off-by-one in both drm
> and vtest at once. Or keep it them separate if you prefer.
I guess I was a bit lost there, was searching in virglrender. I'll send
out the vtest patch shortly as a separate patch.

best, 
Gert

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

[Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation (v2)

2018-09-03 Thread Gert Wollny

Fixes crash with
  piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
   increment-offset -auto -fbo

* Resize the resource storage already when the count is equal to the
  allocated size, fixes:

  Invalid write of size 8
  at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
  by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
  by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
  by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
  by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
  by 0x109A1E: upload (invalidate.c:169)
  by 0x109C2F: piglit_display (invalidate.c:215)
  by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
  by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
  by 0x10949D: main (invalidate.c:47)
  Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
  at 0x4C31B25: calloc (in
   /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
  by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)

* Also resize the space allocated for the handles, fixes:

  Invalid write of size 4
  at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
  by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
  by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
  by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
  by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
  by 0x109A1E: upload (invalidate.c:169)
  by 0x109C2F: piglit_display (invalidate.c:215)
  by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
  by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
  by 0x10949D: main (invalidate.c:47)
  Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
  at 0x4C2FB0F: malloc (
in /usr/lib/valgrind/vgpreload_memcheck-amd64- linux.so)
  by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)

Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need to.")

v2: - Use REALLOC macro and avoid memory leak when re-allocation fails
- add Fixes tag (both Emil Velikov)
- reorder commit message

Signed-off-by: Gert Wollny 
Reviewed-by: Emil Velikov  (v1)

---
I resend it because I didn't apply all the changes Emil suggested, specifically 
I still change to compare for equality.  The new commit message should make it 
clearer what this fixes. 

 .../winsys/virgl/drm/virgl_drm_winsys.c   | 24 +++
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index aad6430c41..e780a5e514 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -617,13 +617,27 @@ static void virgl_drm_add_res(struct virgl_drm_winsys 
*qdws,
 {
unsigned hash = res->res_handle & (sizeof(cbuf->is_handle_added)-1);
 
-   if (cbuf->cres > cbuf->nres) {
-  cbuf->nres += 256;
-  cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres * sizeof(struct 
virgl_hw_buf*));
-  if (!cbuf->res_bo) {
-  fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
cbuf->nres);
+   if (cbuf->cres >= cbuf->nres) {
+  unsigned new_nres = cbuf->nres + 256;
+  void *new_ptr = REALLOC(cbuf->res_bo, cbuf->nres * sizeof(struct 
virgl_hw_buf*),
+new_nres * sizeof(struct virgl_hw_buf*));
+  if (!new_ptr) {
+  fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
new_nres);
   return;
   }
+  cbuf->res_bo = new_ptr;
+
+  new_ptr = REALLOC(cbuf->res_hlist, cbuf->nres * sizeof(uint32_t),
+new_nres * sizeof(uint32_t));
+  if (!new_ptr) {
+  fprintf(stderr,"failure to add hlist relocation %d, %d\n", 
cbuf->cres, cbuf->nres);
+  return;
+  }
+  cbuf->res_hlist = new_ptr;
+  /* This is not perfect, because when the first re-allocation succeeds but
+   * the second failes the first old_size will be wrong, but currently it
+   * isn't used anyway. */
+  cbuf->nres = new_nres;
}
 
cbuf->res_bo[cbuf->cres] = NULL;
-- 
2.17.1

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

[Mesa-dev] [PATCH v3] egl/android: rework device probing

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

Unlike the other platforms, here we aim do guess if the device that we
somewhat arbitrarily picked, is supported or not.

In particular: when a vendor is _not_ requested we loop through all
devices, picking the first one which can create a DRI screen.

When a vendor is requested - we use that and do _not_ fall-back to any
other device.

The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
EGL_MESA_query_renderer are MIA, this is the best we can do for the
moment.

With those (proposed) extensions userspace will be able to create a
separate EGL display for each device, query device details and make the
conscious decision which one to use.

v2:
 - update droid_open_device_drm_gralloc()
 - set the dri2_dpy->fd before using it
 - return a EGLBoolean for droid_{probe,open}_device*
 - do not warn on droid_load_driver failure (Tomasz)
 - plug mem leak on dri2_create_screen failure (Tomasz)
 - fixup function name typo (Tomasz, Rob)

v3:
 - add forward declaration for droid_load_driver()
Fixes the HAVE_DRM_GRALLOC build (Mauro)
 - split dup() assignment and check in separate lines (Tomasz, Eric)
 - make droid_load_driver() static (Tomasz)
 - drop unused prop_set variable (Tomasz)

Cc: Robert Foss 
Cc: Tomasz Figa 
Cc: Mauro Rossi 
Signed-off-by: Emil Velikov 
Tested-by: Tomasz Figa 
Tested-by: Tapani Pälli 
---
Thanks for the feedback everyone.

Mauro, this includes a forward declaration which should fix things for
you.
---
 src/egl/drivers/dri2/platform_android.c | 124 +++-
 1 file changed, 79 insertions(+), 45 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index ecc0245c9a2..5a73d9e7ea9 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1202,10 +1202,14 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
return (config_count != 0);
 }
 
+static EGLBoolean
+droid_load_driver(_EGLDisplay *disp);
+
 #ifdef HAVE_DRM_GRALLOC
-static int
-droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
+static EGLBoolean
+droid_open_device_drm_gralloc(_EGLDisplay *disp)
 {
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
int fd = -1, err = -EINVAL;
 
if (dri2_dpy->gralloc->perform)
@@ -1214,10 +1218,14 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
*dri2_dpy)
   );
if (err || fd < 0) {
   _eglLog(_EGL_WARNING, "fail to get drm fd");
-  fd = -1;
+  return EGL_FALSE;
}
 
-   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
+   dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
+   if (dri2_dpy->fd < 0)
+ return EGL_FALSE;
+
+   return droid_probe_device(disp);
 }
 #endif /* HAVE_DRM_GRALLOC */
 
@@ -1362,10 +1370,10 @@ static const __DRIextension 
*droid_image_loader_extensions[] = {
NULL,
 };
 
-EGLBoolean
+static EGLBoolean
 droid_load_driver(_EGLDisplay *disp)
 {
-   struct dri2_egl_display *dri2_dpy = disp->DriverData;
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
const char *err;
 
dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
@@ -1404,6 +1412,17 @@ error:
return false;
 }
 
+static void
+droid_unload_driver(_EGLDisplay *disp)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+
+   dlclose(dri2_dpy->driver);
+   dri2_dpy->driver = NULL;
+   free(dri2_dpy->driver_name);
+   dri2_dpy->driver_name = NULL;
+}
+
 static int
 droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
 {
@@ -1420,13 +1439,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
char *vendor)
return 0;
 }
 
-static int
+static EGLBoolean
+droid_probe_device(_EGLDisplay *disp)
+{
+  /* Check that the device is supported, by attempting to:
+   * - load the dri module
+   * - and, create a screen
+   */
+   if (!droid_load_driver(disp))
+  return EGL_FALSE;
+
+   if (!dri2_create_screen(disp)) {
+  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
+  droid_unload_driver(disp);
+  return EGL_FALSE;
+   }
+   return EGL_TRUE;
+}
+
+static EGLBoolean
 droid_open_device(_EGLDisplay *disp)
 {
 #define MAX_DRM_DEVICES 32
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
-   int prop_set, num_devices;
-   int fd = -1, fallback_fd = -1;
+   int num_devices;
 
char *vendor_name = NULL;
char vendor_buf[PROPERTY_VALUE_MAX];
@@ -1436,7 +1473,7 @@ droid_open_device(_EGLDisplay *disp)
 
num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
if (num_devices < 0)
-  return num_devices;
+  return EGL_FALSE;
 
for (int i = 0; i < num_devices; i++) {
   device = devices[i];
@@ -1444,41 +1481,49 @@ droid_open_device(_EGLDisplay *disp)
   if (!(device->available_nodes & (1 << DRM_NODE_RENDER)))
  continue;
 
-  fd =

Re: [Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation

2018-09-03 Thread Emil Velikov

On 3 September 2018 at 12:39, Gert Wollny  wrote:
> Am Montag, den 03.09.2018, 10:52 +0100 schrieb Emil Velikov:
>> Hi Gert,
>>
>> On 3 September 2018 at 09:17, Gert Wollny 
>> wrote:
>> > The ressource storage must already be resized when the count is
>> > equal to
>> > the allocated size and the space for the handles must be resized
>> > accordingly.
>> >
>>
>> s/ressource/resource/
>>
>> > Fixes:
>> >   Crashes with
>> >   piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
>> >increment-offset -auto -fbo
>> >
>> > Invalid write of size 8
>> > at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
>> > by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
>> > by 0xB72A44A: virgl_encode_resource_copy_region
>> > (virgl_encode.c:776)
>> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
>> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
>> > by 0x109A1E: upload (invalidate.c:169)
>> > by 0x109C2F: piglit_display (invalidate.c:215)
>> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
>> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
>> > by 0x10949D: main (invalidate.c:47)
>> > Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
>> > at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> > amd64-
>> > linux.so)
>> > by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)
>> >
>> > and
>> >
>> > Invalid write of size 4
>> > at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
>> > by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
>> > by 0xB72A44A: virgl_encode_resource_copy_region
>> > (virgl_encode.c:776)
>> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
>> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
>> > by 0x109A1E: upload (invalidate.c:169)
>> > by 0x109C2F: piglit_display (invalidate.c:215)
>> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
>> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
>> > by 0x10949D: main (invalidate.c:47)
>> > Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
>> > at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
>> > amd64-
>> > linux.so)
>> > by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)
>> >
>> > Signed-off-by: Gert Wollny 
>> > ---
>> >  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 7 ++-
>> >  1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> > b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> > index aad6430c41..2976b46484 100644
>> > --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> > +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
>> > @@ -617,13 +617,18 @@ static void virgl_drm_add_res(struct
>> > virgl_drm_winsys *qdws,
>> >  {
>> > unsigned hash = res->res_handle & (sizeof(cbuf-
>> > >is_handle_added)-1);
>> >
>> > -   if (cbuf->cres > cbuf->nres) {
>> > +   if (cbuf->cres >= cbuf->nres) {
>>
>> I'd leave the check as-is.
> But it's not correct,  the access cbuf->res_bo[cbuf->cres] is past the
> end when (cbuf->cres == cbuf->nres). Maybe I should put it in a
> separate patch to make this clearer (i.e. the first valgrind trace is
> because od the ">" and the second because res_hlist is not re-
> allocated.
>
That sounds reasonable - I would squash the off-by-one in both drm and
vtest at once.
Or keep it them separate if you prefer.

Either way, thanks for the correction.
Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] glsl: fixer lexer for unreachable defines

2018-09-03 Thread Timothy Arceri


On 03/09/18 22:00, Eero Tamminen wrote:

Hi,

Seems a lot of replicated code to deal with regression from few line 
line patch,


That's because the previous patch didn't cause the regression. It fixed 
another bug which happened to be hiding this problem we are seeing now.



but at least it fixed the failing GfxBench ALU2 test. :-)

Tested-By: Eero Tamminen 


 - Eero

On 01.09.2018 16:57, Timothy Arceri wrote:

If we have something like:

    #ifdef NOT_DEFINED
    #define A_MACRO(x) \
if (x)
    #endif

The # on the #define is not skipped but the define itself is so
this then gets recognised as #if.

Until 28a3731e3f this didn't happen because we ended up in
{NONSPACE} where BEGIN INITIAL was called stopping the
problem from happening.

This change makes sure we never call RETURN_TOKEN_NEVER_SKIP for
if/else/endif when processing a define.

Cc: Ian Romanick 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107772
---
  src/compiler/glsl/glcpp/glcpp-lex.l | 60 ++---
  src/compiler/glsl/glcpp/glcpp.h |  1 +
  2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/src/compiler/glsl/glcpp/glcpp-lex.l 
b/src/compiler/glsl/glcpp/glcpp-lex.l

index fe5845acd4e..f7003da0cc8 100644
--- a/src/compiler/glsl/glcpp/glcpp-lex.l
+++ b/src/compiler/glsl/glcpp/glcpp-lex.l
@@ -289,6 +289,7 @@ HEXADECIMAL_INTEGER    0[xX][0-9a-fA-F]+[uU]?
   * token. */
  if (parser->first_non_space_token_this_line) {
  BEGIN HASH;
+    yyextra->in_define = false;
  }
  RETURN_TOKEN_NEVER_SKIP (HASH_TOKEN);
@@ -336,43 +337,55 @@ HEXADECIMAL_INTEGER    0[xX][0-9a-fA-F]+[uU]?
  /* For the pre-processor directives, we return these tokens
   * even when we are otherwise skipping. */
  ifdef {
-    BEGIN INITIAL;
-    yyextra->lexing_directive = 1;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (IFDEF);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->lexing_directive = 1;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (IFDEF);
+    }
  }
  ifndef {
-    BEGIN INITIAL;
-    yyextra->lexing_directive = 1;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (IFNDEF);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->lexing_directive = 1;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (IFNDEF);
+    }
  }
  if/[^_a-zA-Z0-9] {
-    BEGIN INITIAL;
-    yyextra->lexing_directive = 1;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (IF);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->lexing_directive = 1;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (IF);
+    }
  }
  elif/[^_a-zA-Z0-9] {
-    BEGIN INITIAL;
-    yyextra->lexing_directive = 1;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (ELIF);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->lexing_directive = 1;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (ELIF);
+    }
  }
  else {
-    BEGIN INITIAL;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (ELSE);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (ELSE);
+    }
  }
  endif {
-    BEGIN INITIAL;
-    yyextra->space_tokens = 0;
-    RETURN_TOKEN_NEVER_SKIP (ENDIF);
+    if (!yyextra->in_define) {
+    BEGIN INITIAL;
+    yyextra->space_tokens = 0;
+    RETURN_TOKEN_NEVER_SKIP (ENDIF);
+    }
  }
  error[^\r\n]* {
@@ -399,7 +412,8 @@ HEXADECIMAL_INTEGER    0[xX][0-9a-fA-F]+[uU]?
   *  and not whitespace). This will generate an error.
   */
  define{HSPACE}* {
-    if (! parser->skipping) {
+    yyextra->in_define = true;
+    if (!parser->skipping) {
  BEGIN DEFINE;
  yyextra->space_tokens = 0;
  RETURN_TOKEN (DEFINE_TOKEN);
diff --git a/src/compiler/glsl/glcpp/glcpp.h 
b/src/compiler/glsl/glcpp/glcpp.h

index c7e382ed30c..e786b24b132 100644
--- a/src/compiler/glsl/glcpp/glcpp.h
+++ b/src/compiler/glsl/glcpp/glcpp.h
@@ -197,6 +197,7 @@ struct glcpp_parser {
  int first_non_space_token_this_line;
  int newline_as_space;
  int in_control_line;
+    bool in_define;
  int paren_count;
  int commented_newlines;
  skip_node_t *skip_stack;



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

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

[Mesa-dev] [PATCH 4/6] egl/drm: remove eglSwap* surface check

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

Already handled further up in eglapi.c

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Signed-off-by: Emil Velikov 
---
 src/egl/drivers/dri2/platform_drm.c | 30 ++---
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_drm.c 
b/src/egl/drivers/dri2/platform_drm.c
index 7538b3c7a45..da56064cbde 100644
--- a/src/egl/drivers/dri2/platform_drm.c
+++ b/src/egl/drivers/dri2/platform_drm.c
@@ -434,22 +434,20 @@ dri2_drm_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *draw)
   return EGL_TRUE;
}
 
-   if (dri2_surf->base.Type == EGL_WINDOW_BIT) {
-  if (dri2_surf->current)
- _eglError(EGL_BAD_SURFACE, "dri2_swap_buffers");
-  for (unsigned i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++)
- if (dri2_surf->color_buffers[i].age > 0)
-dri2_surf->color_buffers[i].age++;
-
-  /* Make sure we have a back buffer in case we're swapping without
-   * ever rendering. */
-  if (get_back_bo(dri2_surf) < 0)
- return _eglError(EGL_BAD_ALLOC, "dri2_swap_buffers");
-
-  dri2_surf->current = dri2_surf->back;
-  dri2_surf->current->age = 1;
-  dri2_surf->back = NULL;
-   }
+   if (dri2_surf->current)
+  _eglError(EGL_BAD_SURFACE, "dri2_swap_buffers");
+   for (unsigned i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++)
+  if (dri2_surf->color_buffers[i].age > 0)
+ dri2_surf->color_buffers[i].age++;
+
+   /* Make sure we have a back buffer in case we're swapping without
+* ever rendering. */
+   if (get_back_bo(dri2_surf) < 0)
+  return _eglError(EGL_BAD_ALLOC, "dri2_swap_buffers");
+
+   dri2_surf->current = dri2_surf->back;
+   dri2_surf->current->age = 1;
+   dri2_surf->back = NULL;
 
dri2_flush_drawable_for_swapbuffers(disp, draw);
dri2_dpy->flush->invalidate(dri2_surf->dri_drawable);
-- 
2.18.0

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

[Mesa-dev] [PATCH 6/6] egl/x11: remove eglSwap* surface check

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

Already handled further up in eglapi.c.

To make things a tiny bit strange, X11+DRI3 was doing the wrong thing by
returning EGL_FALSE (+ no error), while X11+DRI2 was returning EGL_TRUE.

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Signed-off-by: Emil Velikov 
---
 src/egl/drivers/dri2/platform_x11.c  | 4 
 src/egl/drivers/dri2/platform_x11_dri3.c | 4 
 2 files changed, 8 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_x11.c 
b/src/egl/drivers/dri2/platform_x11.c
index c525b583411..88040bda6e6 100644
--- a/src/egl/drivers/dri2/platform_x11.c
+++ b/src/egl/drivers/dri2/platform_x11.c
@@ -898,10 +898,6 @@ dri2_x11_swap_buffers_msc(_EGLDriver *drv, _EGLDisplay 
*disp, _EGLSurface *draw,
xcb_dri2_swap_buffers_reply_t *reply;
int64_t swap_count = -1;
 
-   /* No-op for a pixmap or pbuffer surface */
-   if (draw->Type == EGL_PIXMAP_BIT || draw->Type == EGL_PBUFFER_BIT)
-  return 0;
-
if (draw->SwapBehavior == EGL_BUFFER_PRESERVED || 
!dri2_dpy->swap_available) {
   swap_count = dri2_copy_region(drv, disp, draw, dri2_surf->region) ? 0 : 
-1;
} else {
diff --git a/src/egl/drivers/dri2/platform_x11_dri3.c 
b/src/egl/drivers/dri2/platform_x11_dri3.c
index e1967422f0a..1206cd39ae6 100644
--- a/src/egl/drivers/dri2/platform_x11_dri3.c
+++ b/src/egl/drivers/dri2/platform_x11_dri3.c
@@ -423,10 +423,6 @@ dri3_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *draw)
 {
struct dri3_egl_surface *dri3_surf = dri3_egl_surface(draw);
 
-   /* No-op for a pixmap or pbuffer surface */
-   if (draw->Type == EGL_PIXMAP_BIT || draw->Type == EGL_PBUFFER_BIT)
-  return EGL_FALSE;
-
return loader_dri3_swap_buffers_msc(_surf->loader_drawable,
0, 0, 0, 0,
draw->SwapBehavior == 
EGL_BUFFER_PRESERVED) != -1;
-- 
2.18.0

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

[Mesa-dev] [PATCH 1/6] egl: make eglSwapInterval a no-op for !window surfaces

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

As the spec says, the function is a no-op when the surface is not a
window one.

That spec implies that EGL_TRUE should be returned in that case, yet
the ARM driver seems to return EGL_FALSE + EGL_BAD_SURFACE.

The Nvidia driver returns EGL_TRUE. We follow that behaviour until a
decision is made.

https://gitlab.khronos.org/egl/API/merge_requests/17

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Cc: 
Signed-off-by: Emil Velikov 
---
Since this is a high-level API decision I've moved it to eglapi.c
This will allow us to avoid duplicating the check across each platform
codebase ... or more crashes because we forgot to update one.
---
 src/egl/main/eglapi.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index c8c6a50f6ad..0af31a3f774 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -1222,6 +1222,9 @@ eglSwapInterval(EGLDisplay dpy, EGLint interval)
if (_eglGetSurfaceHandle(surf) == EGL_NO_SURFACE)
   RETURN_EGL_ERROR(disp, EGL_BAD_SURFACE, EGL_FALSE);
 
+   if (surf->Type != EGL_WINDOW_BIT)
+  RETURN_EGL_EVAL(disp, EGL_TRUE);
+
interval = CLAMP(interval,
 surf->Config->MinSwapInterval,
 surf->Config->MaxSwapInterval);
-- 
2.18.0

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

[Mesa-dev] [PATCH 2/6] egl: make eglSwapBuffers* a no-op for !window surfaces

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

Analogous to the previous commit - the spec says the function is a
no-op when a pbuffer or pixmap surface is used.

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Cc: 
Signed-off-by: Emil Velikov 
---
 src/egl/main/eglapi.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/src/egl/main/eglapi.c b/src/egl/main/eglapi.c
index 0af31a3f774..0227197284f 100644
--- a/src/egl/main/eglapi.c
+++ b/src/egl/main/eglapi.c
@@ -1260,6 +1260,9 @@ eglSwapBuffers(EGLDisplay dpy, EGLSurface surface)
   RETURN_EGL_ERROR(disp, EGL_BAD_SURFACE, EGL_FALSE);
#endif
 
+   if (surf->Type != EGL_WINDOW_BIT)
+  RETURN_EGL_EVAL(disp, EGL_TRUE);
+
/* From the EGL 1.5 spec:
 *
 *If eglSwapBuffers is called and the native window associated with
@@ -1299,6 +1302,9 @@ _eglSwapBuffersWithDamageCommon(_EGLDisplay *disp, 
_EGLSurface *surf,
surf != ctx->DrawSurface)
   RETURN_EGL_ERROR(disp, EGL_BAD_SURFACE, EGL_FALSE);
 
+   if (surf->Type != EGL_WINDOW_BIT)
+  RETURN_EGL_EVAL(disp, EGL_TRUE);
+
if ((n_rects > 0 && rects == NULL) || n_rects < 0)
   RETURN_EGL_ERROR(disp, EGL_BAD_PARAMETER, EGL_FALSE);
 
-- 
2.18.0

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

[Mesa-dev] [PATCH 3/6] egl/android: remove eglSwap* surface check

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

Already handled further up in eglapi.c

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Signed-off-by: Emil Velikov 
---
 src/egl/drivers/dri2/platform_android.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index ecc0245c9a2..e9d5eafad0f 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -676,10 +676,6 @@ droid_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, 
_EGLSurface *draw)
 {
struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
struct dri2_egl_surface *dri2_surf = dri2_egl_surface(draw);
-
-   if (dri2_surf->base.Type != EGL_WINDOW_BIT)
-  return EGL_TRUE;
-
const bool has_mutable_rb = _eglSurfaceHasMutableRenderBuffer(draw);
 
/* From the EGL_KHR_mutable_render_buffer spec (v12):
-- 
2.18.0

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

[Mesa-dev] [PATCH 5/6] egl/surfaceless: remove eglSwap* stubs

2018-09-03 Thread Emil Velikov

From: Emil Velikov 

The API validation in eglapi.c already returns if the surface type is
!window.

Cc: samiuddi 
Cc: Eric Engestrom 
Cc: Erik Faye-Lund 
Cc: Tomasz Figa 
Cc: Gurchetan Singh 
Cc: Chad Versace 
Signed-off-by: Emil Velikov 
---
 src/egl/drivers/dri2/platform_surfaceless.c | 13 -
 1 file changed, 13 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_surfaceless.c 
b/src/egl/drivers/dri2/platform_surfaceless.c
index bfc8fb99eab..1fd5925ae3c 100644
--- a/src/egl/drivers/dri2/platform_surfaceless.c
+++ b/src/egl/drivers/dri2/platform_surfaceless.c
@@ -180,17 +180,6 @@ dri2_surfaceless_create_pbuffer_surface(_EGLDriver *drv, 
_EGLDisplay *disp,
   attrib_list);
 }
 
-static EGLBoolean
-surfaceless_swap_buffers(_EGLDriver *drv, _EGLDisplay *disp, _EGLSurface *surf)
-{
-   assert(!surf || surf->Type == EGL_PBUFFER_BIT);
-
-   /* From the EGL 1.5 spec:
-*If surface is a [...] pbuffer surface, eglSwapBuffers has no effect.
-*/
-   return EGL_TRUE;
-}
-
 static EGLBoolean
 surfaceless_add_configs_for_visuals(_EGLDriver *drv, _EGLDisplay *dpy)
 {
@@ -237,8 +226,6 @@ static const struct dri2_egl_display_vtbl 
dri2_surfaceless_display_vtbl = {
.create_pbuffer_surface = dri2_surfaceless_create_pbuffer_surface,
.destroy_surface = surfaceless_destroy_surface,
.create_image = dri2_create_image_khr,
-   .swap_buffers = surfaceless_swap_buffers,
-   .swap_buffers_with_damage = dri2_fallback_swap_buffers_with_damage,
.swap_buffers_region = dri2_fallback_swap_buffers_region,
.set_damage_region = dri2_fallback_set_damage_region,
.post_sub_buffer = dri2_fallback_post_sub_buffer,
-- 
2.18.0

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

Re: [Mesa-dev] [PATCH] glsl: fixer lexer for unreachable defines

2018-09-03 Thread Eero Tamminen


Hi,

Seems a lot of replicated code to deal with regression from few line 
line patch, but at least it fixed the failing GfxBench ALU2 test. :-)


Tested-By: Eero Tamminen 


- Eero

On 01.09.2018 16:57, Timothy Arceri wrote:

If we have something like:

#ifdef NOT_DEFINED
#define A_MACRO(x) \
if (x)
#endif

The # on the #define is not skipped but the define itself is so
this then gets recognised as #if.

Until 28a3731e3f this didn't happen because we ended up in
{NONSPACE} where BEGIN INITIAL was called stopping the
problem from happening.

This change makes sure we never call RETURN_TOKEN_NEVER_SKIP for
if/else/endif when processing a define.

Cc: Ian Romanick 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=107772
---
  src/compiler/glsl/glcpp/glcpp-lex.l | 60 ++---
  src/compiler/glsl/glcpp/glcpp.h |  1 +
  2 files changed, 38 insertions(+), 23 deletions(-)

diff --git a/src/compiler/glsl/glcpp/glcpp-lex.l 
b/src/compiler/glsl/glcpp/glcpp-lex.l
index fe5845acd4e..f7003da0cc8 100644
--- a/src/compiler/glsl/glcpp/glcpp-lex.l
+++ b/src/compiler/glsl/glcpp/glcpp-lex.l
@@ -289,6 +289,7 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
   * token. */
if (parser->first_non_space_token_this_line) {
BEGIN HASH;
+   yyextra->in_define = false;
}
  
  	RETURN_TOKEN_NEVER_SKIP (HASH_TOKEN);

@@ -336,43 +337,55 @@ HEXADECIMAL_INTEGER   0[xX][0-9a-fA-F]+[uU]?
/* For the pre-processor directives, we return these tokens
 * even when we are otherwise skipping. */
  ifdef {
-   BEGIN INITIAL;
-   yyextra->lexing_directive = 1;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (IFDEF);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->lexing_directive = 1;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (IFDEF);
+   }
  }
  
  ifndef {

-   BEGIN INITIAL;
-   yyextra->lexing_directive = 1;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (IFNDEF);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->lexing_directive = 1;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (IFNDEF);
+   }
  }
  
  if/[^_a-zA-Z0-9] {

-   BEGIN INITIAL;
-   yyextra->lexing_directive = 1;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (IF);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->lexing_directive = 1;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (IF);
+   }
  }
  
  elif/[^_a-zA-Z0-9] {

-   BEGIN INITIAL;
-   yyextra->lexing_directive = 1;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (ELIF);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->lexing_directive = 1;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (ELIF);
+   }
  }
  
  else {

-   BEGIN INITIAL;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (ELSE);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (ELSE);
+   }
  }
  
  endif {

-   BEGIN INITIAL;
-   yyextra->space_tokens = 0;
-   RETURN_TOKEN_NEVER_SKIP (ENDIF);
+   if (!yyextra->in_define) {
+   BEGIN INITIAL;
+   yyextra->space_tokens = 0;
+   RETURN_TOKEN_NEVER_SKIP (ENDIF);
+   }
  }
  
  error[^\r\n]* {

@@ -399,7 +412,8 @@ HEXADECIMAL_INTEGER 0[xX][0-9a-fA-F]+[uU]?
 *and not whitespace). This will generate an error.
 */
  define{HSPACE}* {
-   if (! parser->skipping) {
+   yyextra->in_define = true;
+   if (!parser->skipping) {
BEGIN DEFINE;
yyextra->space_tokens = 0;
RETURN_TOKEN (DEFINE_TOKEN);
diff --git a/src/compiler/glsl/glcpp/glcpp.h b/src/compiler/glsl/glcpp/glcpp.h
index c7e382ed30c..e786b24b132 100644
--- a/src/compiler/glsl/glcpp/glcpp.h
+++ b/src/compiler/glsl/glcpp/glcpp.h
@@ -197,6 +197,7 @@ struct glcpp_parser {
int first_non_space_token_this_line;
int newline_as_space;
int in_control_line;
+   bool in_define;
int paren_count;
int commented_newlines;
skip_node_t *skip_stack;



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

Re: [Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation

2018-09-03 Thread Gert Wollny

Am Montag, den 03.09.2018, 10:52 +0100 schrieb Emil Velikov:
> Hi Gert,
> 
> On 3 September 2018 at 09:17, Gert Wollny 
> wrote:
> > The ressource storage must already be resized when the count is
> > equal to
> > the allocated size and the space for the handles must be resized
> > accordingly.
> > 
> 
> s/ressource/resource/
> 
> > Fixes:
> >   Crashes with
> >   piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
> >increment-offset -auto -fbo
> > 
> > Invalid write of size 8
> > at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
> > by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> > by 0xB72A44A: virgl_encode_resource_copy_region
> > (virgl_encode.c:776)
> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> > by 0x109A1E: upload (invalidate.c:169)
> > by 0x109C2F: piglit_display (invalidate.c:215)
> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> > by 0x10949D: main (invalidate.c:47)
> > Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
> > at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-
> > amd64-
> > linux.so)
> > by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)
> > 
> > and
> > 
> > Invalid write of size 4
> > at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
> > by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> > by 0xB72A44A: virgl_encode_resource_copy_region
> > (virgl_encode.c:776)
> > by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> > by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> > by 0x109A1E: upload (invalidate.c:169)
> > by 0x109C2F: piglit_display (invalidate.c:215)
> > by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> > by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> > by 0x10949D: main (invalidate.c:47)
> > Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
> > at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-
> > amd64-
> > linux.so)
> > by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)
> > 
> > Signed-off-by: Gert Wollny 
> > ---
> >  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 7 ++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > index aad6430c41..2976b46484 100644
> > --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> > @@ -617,13 +617,18 @@ static void virgl_drm_add_res(struct
> > virgl_drm_winsys *qdws,
> >  {
> > unsigned hash = res->res_handle & (sizeof(cbuf-
> > >is_handle_added)-1);
> > 
> > -   if (cbuf->cres > cbuf->nres) {
> > +   if (cbuf->cres >= cbuf->nres) {
> 
> I'd leave the check as-is.
But it's not correct,  the access cbuf->res_bo[cbuf->cres] is past the
end when (cbuf->cres == cbuf->nres). Maybe I should put it in a
separate patch to make this clearer (i.e. the first valgrind trace is
because od the ">" and the second because res_hlist is not re-
allocated. 

> 
> >cbuf->nres += 256;
> >cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres *
> > sizeof(struct virgl_hw_buf*));
> >if (!cbuf->res_bo) {
> >fprintf(stderr,"failure to add relocation %d, %d\n",
> > cbuf->cres, cbuf->nres);
> >return;
> >}
> > +  cbuf->res_hlist = realloc(cbuf->res_hlist, cbuf->nres *
> > sizeof(uint32_t));
> > +  if (!cbuf->res_hlist) {
> > +  fprintf(stderr,"failure to add hlist relocation %d,
> > %d\n", cbuf->cres, cbuf->nres);
> > +  return;
> > +  }
> 
> Please use the uppercase REALLOC wrapper and do not leak memory of
> the call fails. 
That's for copy-pasting, will update the patch. 


> As a follow-up worth applying the same nitpicks to vtest?
I'll have a look, Tomeu already run into problems with the handles
there, it might be implemented differently. 

best, 
Gert


> 
> With the conditional and REALLOC (feel free to keep the leak fix as
> follow-up)
> 
> Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need
> to.")
> 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] Check if the window is non-NULL before setting swap interval.

2018-09-03 Thread Emil Velikov

On 3 September 2018 at 08:14, Eric Engestrom  wrote:
> On Monday, 2018-09-03 14:59:49 +0900, Tomasz Figa wrote:
>> On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov  
>> wrote:
>> >
>> > On 30 August 2018 at 11:41, Eric Engestrom  
>> > wrote:
>> > > On Thursday, 2018-08-30 17:55:14 +0900, Tomasz Figa wrote:
>> > >> Hi Erik, Emil, Eric,
>> > >>
>> > >> On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund  
>> > >> wrote:
>> > >> >
>> > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov 
>> > >> >  wrote:
>> > >> > >
>> > >> > > On 5 July 2018 at 17:17, Eric Engestrom  
>> > >> > > wrote:
>> > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote:
>> > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom 
>> > >> > > >>  wrote:
>> > >> > > >> > On Monday, 2018-07-02 14:12:44 +0530, samiuddi wrote:
>> > >> > > >> >> This fixes crash due to NULL window when swap interval is set
>> > >> > > >> >> for pbuffer surface.
>> > >> > > >> >>
>> > >> > > >> >> Test: CtsDisplayTestCases pass
>> > >> > > >> >>
>> > >> > > >> >> Signed-off-by: samiuddi 
>> > >> > > >> >> ---
>> > >> > > >> >>
>> > >> > > >> >> Kindly ignore this patch
>> > >> > > >> >> https://lists.freedesktop.org/archives/mesa-dev/2018-July/199098.html
>> > >> > > >> >>
>> > >> > > >> >>  src/egl/drivers/dri2/platform_android.c | 2 +-
>> > >> > > >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
>> > >> > > >> >>
>> > >> > > >> >> diff --git a/src/egl/drivers/dri2/platform_android.c 
>> > >> > > >> >> b/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> index ca8708a..b5b960a 100644
>> > >> > > >> >> --- a/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> +++ b/src/egl/drivers/dri2/platform_android.c
>> > >> > > >> >> @@ -485,7 +485,7 @@ droid_swap_interval(_EGLDriver *drv, 
>> > >> > > >> >> _EGLDisplay *dpy,
>> > >> > > >> >> struct dri2_egl_surface *dri2_surf = 
>> > >> > > >> >> dri2_egl_surface(surf);
>> > >> > > >> >> struct ANativeWindow *window = dri2_surf->window;
>> > >> > > >> >>
>> > >> > > >> >> -   if (window->setSwapInterval(window, interval))
>> > >> > > >> >> +   if (window && window->setSwapInterval(window, interval))
>> > >> > > >> >>return EGL_FALSE;
>> > >> > > >> >
>> > >> > > >> > Shouldn't we return false if we couldn't set the swap interval?
>> > >> > > >> >
>> > >> > > >> > I think this should be:
>> > >> > > >> >if (!window || window->setSwapInterval(window, interval))
>> > >> > > >> >   return EGL_FALSE;
>> > >> > > >> >
>> > >> > > >> Changing the patch as above will lead to eglSwapInterval 
>> > >> > > >> consistently
>> > >> > > >> failing for pbuffer surfaces.
>> > >> > > >
>> > >> > > > I'm not that familiar with pbuffers, but does SwapInterval really 
>> > >> > > > make
>> > >> > > > sense for them? I thought you couldn't swap a pbuffer.
>> > >> > > >
>> > >> > > > If so, failing an invalid op seems like the right thing to do.
>> > >> > > > Otherwise, I guess sure, no-op returning success is fine.
>> > >> > > >
>> > >> > > Looking at eglSwapInterval manpage [1] (with my annotation):
>> > >> > > "eglSwapInterval — specifies the minimum number of video frame 
>> > >> > > periods
>> > >> > > per buffer swap for the _window_ associated with the current 
>> > >> > > context."
>> > >> > > While the spec does not mention window/pbuffer/pixmap at all - it
>> > >> > > extensively refers to eglSwapBuffers.
>> > >> > >
>> > >> > > Wrt eglSwapBuffers manpage [2] and spec are consistent:
>> > >> > >
>> > >> > > "If surface is a pixel buffer or a pixmap, eglSwapBuffers has no
>> > >> > > effect, and no error is generated."
>> > >> > >
>> > >> > > Perhaps it's wrong to set eglSwapInterval for !window surfaces, but
>> > >> > > its sibling (eglSwapBuffers) does not error out.
>> > >> > > Hence I doubt many users make a distinction between window and 
>> > >> > > pbuffer
>> > >> > > surfaces for eglSwap*.
>> > >> > >
>> > >> > > Worth bringing to the WG meeting - it' planned for 1st August.
>> > >> > >
>> > >> >
>> > >> > As I pointed out when I proposed this variant here:
>> > >> > https://patchwork.freedesktop.org/patch/219313/
>> > >> >
>> > >> > "Also, I don't think EGL_FALSE is the right return-value, as it 
>> > >> > doesn't
>> > >> > seem the EGL 1.5 spec defines any such error. Also, for instance
>> > >> > dri2_swap_interval returns EGL_TRUE when there's no driver-function,
>> > >> > which further backs the "silent failure" in this case IMO."
>> > >> >
>> > >> > So I think EGL_TRUE is the correct return-value for now. If the spec
>> > >> > gets changed, we can of course update our implementation.
>> > >>
>> > >> What happens to this patch in the end? It looks like we're observing a
>> > >> similar problem in Chrome OS.
>> > >>
>> > >> Emil, was there any follow-up on the WG meeting?
>> > >
>> > > Meeting was cancelled, but I raised the issue here:
>> > > https://gitlab.khronos.org/egl/API/merge_requests/17
>> > >
>> > > Right now we have ARM saying they return false + error and NVIDIA

Re: [Mesa-dev] [PATCH 3/4] radeonsi: add radeonsi_zerovram driconfig option

2018-09-03 Thread Timothy Arceri




On 28/08/18 04:26, Marek Olšák wrote:

On Fri, Aug 24, 2018 at 10:33 AM, Michel Dänzer  wrote:

On 2018-08-24 1:06 p.m., Timothy Arceri wrote:

More and more games seem to require this so lets make it a config
option.
---
  src/gallium/drivers/radeonsi/driinfo_radeonsi.h |  1 +
  src/gallium/winsys/amdgpu/drm/amdgpu_winsys.c   | 10 +++---
  src/util/xmlpool/t_options.h|  5 +
  3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h 
b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
index 7f57b4ea892..8c5078c13f3 100644
--- a/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
+++ b/src/gallium/drivers/radeonsi/driinfo_radeonsi.h
@@ -3,6 +3,7 @@ DRI_CONF_SECTION_PERFORMANCE
  DRI_CONF_RADEONSI_ENABLE_SISCHED("false")
  DRI_CONF_RADEONSI_ASSUME_NO_Z_FIGHTS("false")
  DRI_CONF_RADEONSI_COMMUTATIVE_BLEND_ADD("false")
+DRI_CONF_RADEONSI_ZERO_ALL_VRAM_ALLOCS("false")
  DRI_CONF_SECTION_END

  [...]

@@ -414,3 +414,8 @@ DRI_CONF_OPT_END
  DRI_CONF_OPT_BEGIN_B(radeonsi_clear_db_cache_before_clear, def) \
  DRI_CONF_DESC(en,"Clear DB cache before fast depth clear") \
  DRI_CONF_OPT_END
+
+#define DRI_CONF_RADEONSI_ZERO_ALL_VRAM_ALLOCS(def) \
+DRI_CONF_OPT_BEGIN_B(radeonsi_zerovram, def) \
+DRI_CONF_DESC(en,"Zero all vram allocations") \
+DRI_CONF_OPT_END



I'd name the option simply "zerovram", so it could be used by other
drivers as well.


BTW, AFAICT, currently this only affects BOs allocated from the kernel,
not those re-used from the BO cache. I wonder if that couldn't still
cause trouble with some apps.


It could.


Maybe related to this?

https://bugs.freedesktop.org/show_bug.cgi?id=65968#c12


Anyway:

Reviewed-by: Marek Olšák 

Marek




--
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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


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

Re: [Mesa-dev] [PATCH v3] intel/decoder: fix the possible out of bounds group_iter

2018-09-03 Thread andrey simiklit

Hi,

Thanks a lot.

Regards,
Andrii.

On Mon, Sep 3, 2018 at 1:16 PM Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:

> Done.
>
> -
> Lionel
>
> On 03/09/2018 08:55, andrey simiklit wrote:
>
> Hi all,
>
> Could somebody push this small patch to mesa?
>
> Regards,
> Andrii.
> On Mon, Aug 20, 2018 at 9:13 PM Lionel Landwerlin <
> lionel.g.landwer...@intel.com> wrote:
>
>> On 20/08/2018 17:20, asimiklit.w...@gmail.com wrote:
>> > From: Andrii Simiklit 
>> >
>> > The "gen_group_get_length" function can return a negative value
>> > and it can lead to the out of bounds group_iter.
>> >
>> > v2: printing of "unknown command type" was added
>> > v3: just the asserts are added
>> >
>> > Signed-off-by: Andrii Simiklit 
>>
>> Reviewed-by: Lionel Landwerlin 
>>
>> Somebody should take a look at the other patches I sent out ;)
>> Thanks!
>>
>> -
>> Lionel
>> > ---
>> >   src/intel/common/gen_decoder.c | 5 -
>> >   1 file changed, 4 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/intel/common/gen_decoder.c
>> b/src/intel/common/gen_decoder.c
>> > index ec0a486..2d9609a 100644
>> > --- a/src/intel/common/gen_decoder.c
>> > +++ b/src/intel/common/gen_decoder.c
>> > @@ -803,8 +803,10 @@ static bool
>> >   iter_more_groups(const struct gen_field_iterator *iter)
>> >   {
>> >  if (iter->group->variable) {
>> > +  int length = gen_group_get_length(iter->group, iter->p);
>> > +  assert(length >= 0 && "error the length is unknown!");
>> > return iter_group_offset_bits(iter, iter->group_iter + 1) <
>> > -  (gen_group_get_length(iter->group, iter->p) * 32);
>> > +  (length * 32);
>> >  } else {
>> > return (iter->group_iter + 1) < iter->group->group_count ||
>> >iter->group->next != NULL;
>> > @@ -991,6 +993,7 @@ gen_field_iterator_init(struct gen_field_iterator
>> *iter,
>> >  iter->p_bit = p_bit;
>> >
>> >  int length = gen_group_get_length(iter->group, iter->p);
>> > +   assert(length >= 0 && "error the length is unknown!");
>> >  iter->p_end = length > 0 ? [length] : NULL;
>> >  iter->print_colors = print_colors;
>> >   }
>>
>>
>>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 8/9 v2] nir: Add partial redundancy elimination for compares

2018-09-03 Thread andrey simiklit

Hi,

I guess it is just a small mistake in description that:
'z' was before:
   z = y - x;
and became after:
   z = x - y;

I think you inadvertently misspelled in the description and you mean:

>
> This pass attempts to dectect code sequences like
>
> if (x < y) {
> z = y - x;
> ...
> }
>
> and replace them with sequences like
>
> t = *y - x*;
> if (t < 0) {
> z = t;
> ...
> }
>

Because the subtraction is not commutative operation)
Please correct me if I am incorrect.

Regards,
Andrii.

On Thu, Aug 30, 2018 at 8:36 AM Ian Romanick  wrote:

> From: Ian Romanick 
>
> This pass attempts to dectect code sequences like
>
> if (x < y) {
> z = y - x;
> ...
> }
>
> and replace them with sequences like
>
> t = x - y;
> if (t < 0) {
> z = t;
> ...
> }
>
> On architectures where the subtract can generate the flags used by the
> if-statement, this saves an instruction.  It's also possible that moving
> an instruction out of the if-statement will allow
> nir_opt_peephole_select to convert the whole thing to a bcsel.
>
> Currently only floating point compares and adds are supported.  Adding
> support for integer will be a challenge due to integer overflow.  There
> are a couple possible solutions, but they may not apply to all
> architectures.
>
> v2: Fix a typo in the commit message and a couple typos in comments.
> Fix possible NULL pointer deref from result of push_block().  Add
> missing (-A + B) case.  Suggested by Caio.
>
> Signed-off-by: Ian Romanick 
> ---
>  src/compiler/Makefile.sources |   1 +
>  src/compiler/nir/meson.build  |   1 +
>  src/compiler/nir/nir.h|   2 +
>  src/compiler/nir/nir_opt_comparison_pre.c | 360
> ++
>  src/compiler/nir/nir_search_helpers.h |  29 +++
>  5 files changed, 393 insertions(+)
>  create mode 100644 src/compiler/nir/nir_opt_comparison_pre.c
>
> diff --git a/src/compiler/Makefile.sources b/src/compiler/Makefile.sources
> index d3b06564832..9fe8d5b8904 100644
> --- a/src/compiler/Makefile.sources
> +++ b/src/compiler/Makefile.sources
> @@ -267,6 +267,7 @@ NIR_FILES = \
> nir/nir_move_load_const.c \
> nir/nir_move_vec_src_uses_to_dest.c \
> nir/nir_normalize_cubemap_coords.c \
> +   nir/nir_opt_comparison_pre.c \
> nir/nir_opt_conditional_discard.c \
> nir/nir_opt_constant_folding.c \
> nir/nir_opt_copy_prop_vars.c \
> diff --git a/src/compiler/nir/meson.build b/src/compiler/nir/meson.build
> index 5438c17a8f8..2bcc854829e 100644
> --- a/src/compiler/nir/meson.build
> +++ b/src/compiler/nir/meson.build
> @@ -151,6 +151,7 @@ files_libnir = files(
>'nir_move_load_const.c',
>'nir_move_vec_src_uses_to_dest.c',
>'nir_normalize_cubemap_coords.c',
> +  'nir_opt_comparison_pre.c',
>'nir_opt_conditional_discard.c',
>'nir_opt_constant_folding.c',
>'nir_opt_copy_prop_vars.c',
> diff --git a/src/compiler/nir/nir.h b/src/compiler/nir/nir.h
> index ddbcb3c647e..c78387d0acf 100644
> --- a/src/compiler/nir/nir.h
> +++ b/src/compiler/nir/nir.h
> @@ -3000,6 +3000,8 @@ bool nir_convert_from_ssa(nir_shader *shader, bool
> phi_webs_only);
>  bool nir_lower_phis_to_regs_block(nir_block *block);
>  bool nir_lower_ssa_defs_to_regs_block(nir_block *block);
>
> +bool nir_opt_comparison_pre(nir_shader *shader);
> +
>  bool nir_opt_algebraic(nir_shader *shader);
>  bool nir_opt_algebraic_before_ffma(nir_shader *shader);
>  bool nir_opt_algebraic_late(nir_shader *shader);
> diff --git a/src/compiler/nir/nir_opt_comparison_pre.c
> b/src/compiler/nir/nir_opt_comparison_pre.c
> new file mode 100644
> index 000..b2827c21816
> --- /dev/null
> +++ b/src/compiler/nir/nir_opt_comparison_pre.c
> @@ -0,0 +1,360 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the
> "Software"),
> + * to deal in the Software without restriction, including without
> limitation
> + * the rights to use, copy, modify, merge, publish, distribute,
> sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the
> next
> + * paragraph) shall be included in all copies or substantial portions of
> the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
> EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
> MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT
> SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
> OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH

Re: [Mesa-dev] [PATCH v3] intel/decoder: fix the possible out of bounds group_iter

2018-09-03 Thread Lionel Landwerlin


Done.

-
Lionel

On 03/09/2018 08:55, andrey simiklit wrote:

Hi all,

Could somebody push this small patch to mesa?

Regards,
Andrii.
On Mon, Aug 20, 2018 at 9:13 PM Lionel Landwerlin 
mailto:lionel.g.landwer...@intel.com>> 
wrote:


On 20/08/2018 17:20, asimiklit.w...@gmail.com
 wrote:
> From: Andrii Simiklit mailto:andrii.simik...@globallogic.com>>
>
> The "gen_group_get_length" function can return a negative value
> and it can lead to the out of bounds group_iter.
>
> v2: printing of "unknown command type" was added
> v3: just the asserts are added
>
> Signed-off-by: Andrii Simiklit mailto:andrii.simik...@globallogic.com>>

Reviewed-by: Lionel Landwerlin mailto:lionel.g.landwer...@intel.com>>

Somebody should take a look at the other patches I sent out ;)
Thanks!

-
Lionel
> ---
>   src/intel/common/gen_decoder.c | 5 -
>   1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/src/intel/common/gen_decoder.c
b/src/intel/common/gen_decoder.c
> index ec0a486..2d9609a 100644
> --- a/src/intel/common/gen_decoder.c
> +++ b/src/intel/common/gen_decoder.c
> @@ -803,8 +803,10 @@ static bool
>   iter_more_groups(const struct gen_field_iterator *iter)
>   {
>      if (iter->group->variable) {
> +      int length = gen_group_get_length(iter->group, iter->p);
> +      assert(length >= 0 && "error the length is unknown!");
>         return iter_group_offset_bits(iter, iter->group_iter + 1) <
> - (gen_group_get_length(iter->group, iter->p) * 32);
> +              (length * 32);
>      } else {
>         return (iter->group_iter + 1) < iter->group->group_count ||
>            iter->group->next != NULL;
> @@ -991,6 +993,7 @@ gen_field_iterator_init(struct
gen_field_iterator *iter,
>      iter->p_bit = p_bit;
>
>      int length = gen_group_get_length(iter->group, iter->p);
> +   assert(length >= 0 && "error the length is unknown!");
>      iter->p_end = length > 0 ? [length] : NULL;
>      iter->print_colors = print_colors;
>   }




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

Re: [Mesa-dev] [PATCH 3/4] gallium/u_inlines: improve pipe_reference_described perf for debug builds

2018-09-03 Thread Michel Dänzer

On 2018-09-01 8:54 a.m., Marek Olšák wrote:
> From: Marek Olšák 
> 
> +41% performance in debug builds
> (testing piglit/drawoverhead + u_threaded_context)

Nice, but please include ministat output.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] gallium/u_inlines: normalize naming, use dst & src, style fixes

2018-09-03 Thread Michel Dänzer

On 2018-09-01 8:54 a.m., Marek Olšák wrote:
> From: Marek Olšák 

Nice cleanup. This patch and patch 4 are

Reviewed-by: Michel Dänzer 


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/4] gallium/auxiliary: don't dereference counters twice needlessly

2018-09-03 Thread Michel Dänzer

On 2018-09-01 8:54 a.m., Marek Olšák wrote:
> From: Marek Olšák 
> 
> +1.2% performance

Please either state what you tested with, and include ministat output,
or don't make any statement about performance impact.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 2/2] winsys/virgl: Initialize value to silence valgrind

2018-09-03 Thread Emil Velikov

On 3 September 2018 at 09:17, Gert Wollny  wrote:
> Silences:
>
>   Conditional jump or move depends on uninitialised value(s)
>   at 0xB72F2C0: virgl_drm_winsys_create (virgl_drm_winsys.c:854)
>   by 0xB72F2C0: virgl_drm_screen_create (virgl_drm_winsys.c:926)
>   by 0xB21C885: pipe_virgl_create_screen (drm_helper.h:275)
>   by 0xB7201F0: pipe_loader_create_screen (pipe_loader.c:137)
>   by 0xB639C91: dri2_init_screen (dri2.c:2112)
>   by 0xB634F68: driCreateNewScreen2 (dri_util.c:153)
>   by 0x63023E6: dri3_create_screen (dri3_glx.c:893)
>   by 0x62D35BD: AllocAndFetchScreenConfigs (glxext.c:820)
>   by 0x62D35BD: __glXInitialize (glxext.c:946)
>   by 0x62CECB3: GetGLXPrivScreenConfig (glxcmds.c:174)
>   by 0x62CF69C: glXQueryExtensionsString (glxcmds.c:1304)
>   by 0x60AA7D9: ??? (in /usr/lib/x86_64-linux-gnu/libwaffle-1.so.0.5.2)
>   by 0x4F81450: wfl_checked_display_connect (piglit-util-waffle.h:74)
>   by 0x4F829E0: piglit_wfl_framework_init (piglit_wfl_framework.c:627)
>
> Signed-off-by: Gert Wollny 
> ---
>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> index 2976b46484..a53b015598 100644
> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> @@ -846,7 +846,7 @@ virgl_drm_winsys_create(int drmFD)
>
> qdws->base.get_caps = virgl_drm_get_caps;
>
> -   uint32_t value;
> +   uint32_t value = 0;
> getparam.param = VIRTGPU_PARAM_CAPSET_QUERY_FIX;
> getparam.value = (uint64_t)(uintptr_t)
> ret = drmIoctl(qdws->fd, DRM_IOCTL_VIRTGPU_GETPARAM, );


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 1/2] winsys/virgl: correct resource and handle allocation

2018-09-03 Thread Emil Velikov

Hi Gert,

On 3 September 2018 at 09:17, Gert Wollny  wrote:
> The ressource storage must already be resized when the count is equal to
> the allocated size and the space for the handles must be resized
> accordingly.
>
s/ressource/resource/

> Fixes:
>   Crashes with
>   piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
>increment-offset -auto -fbo
>
> Invalid write of size 8
> at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
> by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
> by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> by 0x109A1E: upload (invalidate.c:169)
> by 0x109C2F: piglit_display (invalidate.c:215)
> by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> by 0x10949D: main (invalidate.c:47)
> Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
> at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-
> linux.so)
> by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)
>
> and
>
> Invalid write of size 4
> at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
> by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
> by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
> by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
> by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
> by 0x109A1E: upload (invalidate.c:169)
> by 0x109C2F: piglit_display (invalidate.c:215)
> by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
> by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
> by 0x10949D: main (invalidate.c:47)
> Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
> at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-
> linux.so)
> by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)
>
> Signed-off-by: Gert Wollny 
> ---
>  src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
> b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> index aad6430c41..2976b46484 100644
> --- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> +++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
> @@ -617,13 +617,18 @@ static void virgl_drm_add_res(struct virgl_drm_winsys 
> *qdws,
>  {
> unsigned hash = res->res_handle & (sizeof(cbuf->is_handle_added)-1);
>
> -   if (cbuf->cres > cbuf->nres) {
> +   if (cbuf->cres >= cbuf->nres) {
I'd leave the check as-is.

>cbuf->nres += 256;
>cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres * sizeof(struct 
> virgl_hw_buf*));
>if (!cbuf->res_bo) {
>fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
> cbuf->nres);
>return;
>}
> +  cbuf->res_hlist = realloc(cbuf->res_hlist, cbuf->nres * 
> sizeof(uint32_t));
> +  if (!cbuf->res_hlist) {
> +  fprintf(stderr,"failure to add hlist relocation %d, %d\n", 
> cbuf->cres, cbuf->nres);
> +  return;
> +  }
Please use the uppercase REALLOC wrapper and do not leak memory of the
call fails.
As a follow-up worth applying the same nitpicks to vtest?

With the conditional and REALLOC (feel free to keep the leak fix as follow-up)

Fixes: 4b15b5e803e ("virgl: resize resource bo allocation if we need to.")
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 7/7] virgl: use hw-atomics instead of in-ssbo ones

2018-09-03 Thread andrey simiklit

Hi,

One more small think here:
> +int virgl_encode_set_hw_atomic_buffers(struct virgl_context *ctx,
> +   unsigned start_slot, unsigned
count,
> +   const struct pipe_shader_buffer
*buffers)
> +{
> +   int i;
I believe that it will be better to use 'unsigned' type for 'i' here
because the 'count' variable has 'unsigned' type.

Please lat me know if I am incorrect.

Regars,
Andrii.

On Fri, Aug 31, 2018 at 8:35 PM Gurchetan Singh 
wrote:

> On Thu, Aug 30, 2018 at 6:41 AM Erik Faye-Lund
>  wrote:
> >
> > From: Tomeu Vizoso 
> >
> > Emulating atomics on top of ssbos can lead to too small max SSBO count,
> > so let's use the hw-atomics mechanism to expose atomic buffers instead.
> >
> > Signed-off-by: Erik Faye-Lund 
> > ---
> >  src/gallium/drivers/virgl/virgl_context.c  | 37 ++
> >  src/gallium/drivers/virgl/virgl_context.h  |  2 ++
> >  src/gallium/drivers/virgl/virgl_encode.c   | 23 ++
> >  src/gallium/drivers/virgl/virgl_encode.h   |  3 ++
> >  src/gallium/drivers/virgl/virgl_hw.h   |  5 +++
> >  src/gallium/drivers/virgl/virgl_protocol.h |  9 ++
> >  src/gallium/drivers/virgl/virgl_screen.c   | 14 +---
> >  7 files changed, 88 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/gallium/drivers/virgl/virgl_context.c
> b/src/gallium/drivers/virgl/virgl_context.c
> > index edc03f5dcf..30cd0e4331 100644
> > --- a/src/gallium/drivers/virgl/virgl_context.c
> > +++ b/src/gallium/drivers/virgl/virgl_context.c
> > @@ -196,6 +196,19 @@ static void virgl_attach_res_shader_images(struct
> virgl_context *vctx,
> > }
> >  }
> >
> > +static void virgl_attach_res_atomic_buffers(struct virgl_context *vctx)
> > +{
> > +   struct virgl_winsys *vws = virgl_screen(vctx->base.screen)->vws;
> > +   struct virgl_resource *res;
> > +   unsigned i;
> > +   for (i = 0; i < PIPE_MAX_SHADER_BUFFERS; i++) {
>
> Why not PIPE_MAX_HW_ATOMIC_BUFFERS?
>
> > +  res = virgl_resource(vctx->atomic_buffers[i]);
> > +  if (res) {
> > + vws->emit_res(vws, vctx->cbuf, res->hw_res, FALSE);
> > +  }
> > +   }
> > +}
> > +
> >  /*
> >   * after flushing, the hw context still has a bunch of
> >   * resources bound, so we need to rebind those here.
> > @@ -214,6 +227,7 @@ static void virgl_reemit_res(struct virgl_context
> *vctx)
> >virgl_attach_res_shader_buffers(vctx, shader_type);
> >virgl_attach_res_shader_images(vctx, shader_type);
> > }
> > +   virgl_attach_res_atomic_buffers(vctx);
> > virgl_attach_res_vertex_buffers(vctx);
> > virgl_attach_res_so_targets(vctx);
> >  }
> > @@ -952,6 +966,28 @@ static void virgl_blit(struct pipe_context *ctx,
> >  blit);
> >  }
> >
> > +static void virgl_set_hw_atomic_buffers(struct pipe_context *ctx,
> > +   unsigned start_slot,
> > +   unsigned count,
> > +   const struct
> pipe_shader_buffer *buffers)
>
> nit: mixing tabs and spaces
>
> > +{
> > +   struct virgl_context *vctx = virgl_context(ctx);
> > +
> > +   for (unsigned i = 0; i < count; i++) {
> > +  unsigned idx = start_slot + i;
> > +
> > +  if (buffers) {
> > + if (buffers[i].buffer) {
> > +pipe_resource_reference(>atomic_buffers[idx],
> > +buffers[i].buffer);
> > +continue;
> > + }
> > +  }
> > +  pipe_resource_reference(>atomic_buffers[idx], NULL);
> > +   }
> > +   virgl_encode_set_hw_atomic_buffers(vctx, start_slot, count, buffers);
> > +}
> > +
> >  static void virgl_set_shader_buffers(sunsignedtruct pipe_context *ctx,
> >   enum pipe_shader_type shader,
> >   unsigned start_slot, unsigned
> count,
> > @@ -1209,6 +1245,7 @@ struct pipe_context *virgl_context_create(struct
> pipe_screen *pscreen,
> > vctx->base.blit =  virgl_blit;
> >
> > vctx->base.set_shader_buffers = virgl_set_shader_buffers;
> > +   vctx->base.set_hw_atomic_buffers = virgl_set_hw_atomic_buffers;
> > vctx->base.set_shader_images = virgl_set_shader_images;
> > vctx->base.memory_barrier = virgl_memory_barrier;
> >
> > diff --git a/src/gallium/drivers/virgl/virgl_context.h
> b/src/gallium/drivers/virgl/virgl_context.h
> > index 38d1f450e1..20988baa3c 100644
> > --- a/src/gallium/drivers/virgl/virgl_context.h
> > +++ b/src/gallium/drivers/virgl/virgl_context.h
> > @@ -75,6 +75,8 @@ struct virgl_context {
> > int num_draws;
> > struct list_head to_flush_bufs;
> >
> > +   struct pipe_resource *atomic_buffers[PIPE_MAX_HW_ATOMIC_BUFFERS];
> > +
> > struct primconvert_context *primconvert;
> > uint32_t hw_sub_ctx_id;
> >  };
> > diff --git a/src/gallium/drivers/virgl/virgl_encode.c
> b/src/gallium/drivers/virgl/virgl_encode.c
> > index bcb14d8939..c9cc099061 100644
> > ---

Re: [Mesa-dev] [PATCH] mesa: ignore VAO IDs equal to 0 in glDeleteVertexArrays

2018-09-03 Thread andrey simiklit

Hello,

I believe that this check already added inside _mesa_lookup_vao:

>_mesa_lookup_vao(struct gl_context *ctx, GLuint id)
>{
>   if (id == 0) {
>  return NULL;
>   } else {
>  ...
>}

I guess that the '_mesa_lookup_vao' function returns NULL for zero indexes.
So we will never pass into the 'if(obj)' for 0 index case.
So I think that this additional check which was added by this patch is
unnecessary here
because this functionality works fine for zero indexes as is.
Please correct me if am wrong.

Regards,
Andrii.

On Fri, Aug 31, 2018 at 10:24 AM Ian Romanick  wrote:

> On 08/30/2018 07:42 PM, Marek Olšák wrote:
> > Sadly, there are no tests for this.
>
> Ok... I just sent one to the piglit list.  It should be easy enough to
> create a few more for other glDeleteFoo functions.  I'll save that for
> during boring meetings. ;)
>
> > Marek
> >
> > On Thu, Aug 30, 2018 at 6:24 PM, Ian Romanick 
> wrote:
> >> This patch is
> >>
> >> Reviewed-by: Ian Romanick 
> >>
> >> Is there a piglit test?  I wonder how many other glDeleteFoo functions
> >> mishandle the id=0 case. :(
> >>
> >> On 08/30/2018 12:16 PM, Marek Olšák wrote:
> >>> From: Marek Olšák 
> >>>
> >>> This fixes a firefox crash.
> >>>
> >>> Fixes: 781a78914c798dc64005b37c6ca1224ce06803fc
> >>> ---
> >>>  src/mesa/main/arrayobj.c | 4 
> >>>  1 file changed, 4 insertions(+)
> >>>
> >>> diff --git a/src/mesa/main/arrayobj.c b/src/mesa/main/arrayobj.c
> >>> index a23031fe182..6e0665c0e5d 100644
> >>> --- a/src/mesa/main/arrayobj.c
> >>> +++ b/src/mesa/main/arrayobj.c
> >>> @@ -1007,20 +1007,24 @@ _mesa_BindVertexArray(GLuint id)
> >>>   *
> >>>   * \param n  Number of array objects to delete.
> >>>   * \param idsArray of \c n array object IDs.
> >>>   */
> >>>  static void
> >>>  delete_vertex_arrays(struct gl_context *ctx, GLsizei n, const GLuint
> *ids)
> >>>  {
> >>> GLsizei i;
> >>>
> >>> for (i = 0; i < n; i++) {
> >>> +  /* IDs equal to 0 should be silently ignored. */
> >>> +  if (!ids[i])
> >>> + continue;
> >>> +
> >>>struct gl_vertex_array_object *obj = _mesa_lookup_vao(ctx,
> ids[i]);
> >>>
> >>>if (obj) {
> >>>   assert(obj->Name == ids[i]);
> >>>
> >>>   /* If the array object is currently bound, the spec says
> "the binding
> >>>* for that object reverts to zero and the default vertex
> array
> >>>* becomes current."
> >>>*/
> >>>   if (obj == ctx->Array.VAO)
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] shader cache backward compatibility

2018-09-03 Thread Alexander Larsson

On Mon, Sep 3, 2018 at 10:41 AM Alexander Larsson  wrote:
>
> On Fri, Aug 31, 2018 at 4:05 PM Emil Velikov  wrote:
> >
> > Valid point - I forgot about that.
> >
> > A couple of ideas come to mind:
> >  - static link LLVM (Flatpak already does it)
> > No LLVM changes needed.
> >
> >  - shared link LLVM
> > LLVM add -Wl,--build-id=sha1
>
> As a very very simple workaround, can you add the file sizes (as well
> as the mtimes) to the staleness check? I mean, its possible that a
> rebuild generates the exact same size, but at least its better than
> always being wrong.

Also, valentin david (of the freedesktop sdk project) started working
on a solution based on build-ids:
 https://gitlab.com/freedesktop-sdk/freedesktop-sdk/merge_requests/487/diffs

This currently relies on the freedesktop sdk having build-ids, but it
would be easy for it to fall back on the mtime if that was not found.
Also, this code is not really tested yet, but you still get the idea
from looking at it, and it should work.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc
   al...@redhat.com alexander.lars...@gmail.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH libdrm] Add basic CONTRIBUTING file

2018-09-03 Thread Daniel Vetter

I picked up a bunch of the pieces from wayland's version:

https://gitlab.freedesktop.org/wayland/wayland/blob/master/CONTRIBUTING.md

The weston one is fairly similar. Then I rather massively trimmed it
down since in reality libdrm is a bit a dumping ground with very few
real rules. The commit rights and CoC sections I've copied verbatim
from igt respectively drm-misc. Weston/Wayland only differ in their
pick of how many patches you need (10 instead of 5). I think for
libdrm this is supremely relevant, since most everyone will get their
commit rights by contributing already to the kernel or mesa and having
commit rights there already.

Anyway, I figured this is good to get the rules documented, even if
there's mostly not many rules.

Note: This references maintainers in a MAINTAINERS file, which needs
to be created first.

Note: With the gitlab migration the entire commit rights process is
still a bit up in the air. But gitlab commit rights and roles are
hierarchical, so we can do libdrm-only maintainer/commiter roles
("Owner" and "Developer" in gitlab-speak). This should avoid
conflating libdrm roles with mesa roles, useful for those pushing to
libdrm as primarily kernel contributors.

v2: Comments from Emil:
- Recommend subject prefix.
- Fix copypaste fumbles, this isn't igt/wayland ...

v3: Comments from Marek:
- libdrm moved to mesa, update the document. Atm the entire account
  request situation is entirely not clear for gitlab and mesa
  projects, so that's a bit up in the air. Also, should probably send
  an announcement to dri-devel@, which didn't happen.
- amd folks don't submit their patches to dri-devel, document that.
  Probably applies to other drivers too.

v4: Comments from Rob:
- Also include kernel/userspace in the commit counts criteria, due to
  libdrm's special role as a glue library.

v5: Summarize the irc discussion on gitlab roles in the commit message
a bit.

v6: Some grammer stuff from Eric E.

v7: Use --local in git config (Eric E.)

Cc: Dave Airlie 
Cc: Michel Dänzer 
Cc: Emil Velikov 
Cc: Marek Olšák 
Cc: Rob Clark 
Cc: Eric Engestrom 
Reviewed-by: Rob Clark  (v4)
Reviewed-by: Eric Engestrom  (v6)
Acked-by: Emil Velikov  (v6)
Acked-by: Marek Olšák  (v5)
References: 
https://gitlab.freedesktop.org/wayland/weston/blob/master/CONTRIBUTING.md
References: 
https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-misc.html#commit-rights
References: https://cgit.freedesktop.org/drm/igt-gpu-tools/tree/CONTRIBUTING#n54
Signed-off-by: Daniel Vetter 
---
 CONTRIBUTING | 105 +++
 1 file changed, 105 insertions(+)
 create mode 100644 CONTRIBUTING

diff --git a/CONTRIBUTING b/CONTRIBUTING
new file mode 100644
index ..96f1e4fb0108
--- /dev/null
+++ b/CONTRIBUTING
@@ -0,0 +1,105 @@
+Contributing to libdrm
+==
+
+Submitting Patches
+--
+
+Patches should be sent to dri-de...@lists.freedesktop.org, using git
+send-email. For patches only touching driver specific code one of the driver
+mailing lists (like amd-...@lists.freedesktop.org) is also appropriate. See git
+documentation for help:
+
+http://git-scm.com/documentation
+
+Since dri-devel is a very busy mailing list please use --subject-prefix="PATCH
+libdrm" to make it easier to find libdrm patches. This is best done by running
+
+git config --local format.subjectprefix "PATCH libdrm"
+
+The first line of a commit message should contain a prefix indicating what part
+is affected by the patch followed by one sentence that describes the change. 
For
+examples:
+
+amdgpu: Use uint32_t i in amdgpu_find_bo_by_cpu_mapping
+
+The body of the commit message should describe what the patch changes and why,
+and also note any particular side effects. For a recommended reading on
+writing commit messages, see:
+
+http://who-t.blogspot.de/2009/12/on-commit-messages.html
+
+Your patches should also include a Signed-off-by line with your name and email
+address. If you're not the patch's original author, you should also gather
+S-o-b's by them (and/or whomever gave the patch to you.) The significance of
+this is that it certifies that you created the patch, that it was created under
+an appropriate open source license, or provided to you under those terms.  This
+lets us indicate a chain of responsibility for the copyright status of the 
code.
+For more details:
+
+https://developercertificate.org/
+
+We won't reject patches that lack S-o-b, but it is strongly recommended.
+
+Review and Merging
+--
+
+Patches should have at least one positive review (Reviewed-by: tag) or
+indication of approval (Acked-by: tag) before merging. For any code shared
+between drivers this is mandatory.
+
+Please note that kernel/userspace API header files have special rules, see
+include/drm/README.
+
+Coding style in the project loosely follows the CodingStyle of the linux 
kernel:
+

Re: [Mesa-dev] [PATCH 5/7] gallium: add PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER{S, _BUFFERS}

2018-09-03 Thread Marek Olšák

With my comments addressed, the first 5 patches are:

Reviewed-by: Marek Olšák 

Marek

On Mon, Sep 3, 2018 at 4:44 AM, Marek Olšák  wrote:
> Same answer as before - the screen.rst documentation is missing.
>
> Marek
>
> On Thu, Aug 30, 2018 at 9:40 AM, Erik Faye-Lund
>  wrote:
>> This moves the evergreen-specific max-sizes out as a driver-cap, so
>> other drivers with less strict requirements also can use hw-atomics.
>>
>> Remove ssbo_atomic as it's no longer needed.
>>
>> We should now be able to use hw-atomics for some stages and not for
>> other, if needed.
>>
>> Signed-off-by: Erik Faye-Lund 
>> ---
>>  src/gallium/drivers/etnaviv/etnaviv_screen.c  |  5 +
>>  .../drivers/freedreno/freedreno_screen.c  |  5 +
>>  .../drivers/nouveau/nv30/nv30_screen.c|  2 ++
>>  .../drivers/nouveau/nv50/nv50_screen.c|  2 ++
>>  .../drivers/nouveau/nvc0/nvc0_screen.c|  2 ++
>>  src/gallium/drivers/r300/r300_screen.c|  2 ++
>>  src/gallium/drivers/r600/r600_pipe.c  |  9 +
>>  src/gallium/drivers/radeonsi/si_get.c |  2 ++
>>  src/gallium/drivers/svga/svga_screen.c|  2 ++
>>  src/gallium/drivers/v3d/v3d_screen.c  |  2 ++
>>  src/gallium/drivers/vc4/vc4_screen.c  |  2 ++
>>  src/gallium/drivers/virgl/virgl_screen.c  |  2 ++
>>  src/gallium/include/pipe/p_defines.h  |  2 ++
>>  src/mesa/state_tracker/st_extensions.c| 20 +--
>>  14 files changed, 49 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.c 
>> b/src/gallium/drivers/etnaviv/etnaviv_screen.c
>> index 108b97d35c..95166a2db1 100644
>> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.c
>> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.c
>> @@ -372,6 +372,11 @@ etna_screen_get_param(struct pipe_screen *pscreen, enum 
>> pipe_cap param)
>>return 0;
>> case PIPE_CAP_UMA:
>>return 1;
>> +
>> +   /* hw atomic counters */
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>> +  return 0;
>> }
>>
>> debug_printf("unknown param %d", param);
>> diff --git a/src/gallium/drivers/freedreno/freedreno_screen.c 
>> b/src/gallium/drivers/freedreno/freedreno_screen.c
>> index af44ab698e..e6dfc2e48e 100644
>> --- a/src/gallium/drivers/freedreno/freedreno_screen.c
>> +++ b/src/gallium/drivers/freedreno/freedreno_screen.c
>> @@ -491,6 +491,11 @@ fd_screen_get_param(struct pipe_screen *pscreen, enum 
>> pipe_cap param)
>> return 1;
>> case PIPE_CAP_NATIVE_FENCE_FD:
>> return fd_device_version(screen->dev) >= FD_VERSION_FENCE_FD;
>> +
>> +   /* hw atomic counters */
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>> +   return 0;
>> }
>> debug_printf("unknown param %d\n", param);
>> return 0;
>> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
>> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> index d52d8f3988..3fdbadb6c6 100644
>> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
>> @@ -239,6 +239,8 @@ nv30_screen_get_param(struct pipe_screen *pscreen, enum 
>> pipe_cap param)
>> case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
>> case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
>> case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>>return 0;
>>
>> case PIPE_CAP_MAX_GS_INVOCATIONS:
>> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
>> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> index cd36795e56..a6dda5dfa0 100644
>> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
>> @@ -293,6 +293,8 @@ nv50_screen_get_param(struct pipe_screen *pscreen, enum 
>> pipe_cap param)
>> case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
>> case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
>> case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>>return 0;
>>
>> case PIPE_CAP_MAX_GS_INVOCATIONS:
>> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c 
>> b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> index 446e30dcc8..b628b24d76 100644
>> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
>> @@ -322,6 +322,8 @@ nvc0_screen_get_param(struct pipe_screen *pscreen, enum 
>> pipe_cap param)
>> case PIPE_CAP_CONSTBUF0_FLAGS:
>> case PIPE_CAP_PACKED_UNIFORMS:
>> case PIPE_CAP_CONSERVATIVE_RASTER_PRE_SNAP_POINTS_LINES:
>> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
>> +

Re: [Mesa-dev] [PATCH 5/7] gallium: add PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER{S, _BUFFERS}

2018-09-03 Thread Marek Olšák

Same answer as before - the screen.rst documentation is missing.

Marek

On Thu, Aug 30, 2018 at 9:40 AM, Erik Faye-Lund
 wrote:
> This moves the evergreen-specific max-sizes out as a driver-cap, so
> other drivers with less strict requirements also can use hw-atomics.
>
> Remove ssbo_atomic as it's no longer needed.
>
> We should now be able to use hw-atomics for some stages and not for
> other, if needed.
>
> Signed-off-by: Erik Faye-Lund 
> ---
>  src/gallium/drivers/etnaviv/etnaviv_screen.c  |  5 +
>  .../drivers/freedreno/freedreno_screen.c  |  5 +
>  .../drivers/nouveau/nv30/nv30_screen.c|  2 ++
>  .../drivers/nouveau/nv50/nv50_screen.c|  2 ++
>  .../drivers/nouveau/nvc0/nvc0_screen.c|  2 ++
>  src/gallium/drivers/r300/r300_screen.c|  2 ++
>  src/gallium/drivers/r600/r600_pipe.c  |  9 +
>  src/gallium/drivers/radeonsi/si_get.c |  2 ++
>  src/gallium/drivers/svga/svga_screen.c|  2 ++
>  src/gallium/drivers/v3d/v3d_screen.c  |  2 ++
>  src/gallium/drivers/vc4/vc4_screen.c  |  2 ++
>  src/gallium/drivers/virgl/virgl_screen.c  |  2 ++
>  src/gallium/include/pipe/p_defines.h  |  2 ++
>  src/mesa/state_tracker/st_extensions.c| 20 +--
>  14 files changed, 49 insertions(+), 10 deletions(-)
>
> diff --git a/src/gallium/drivers/etnaviv/etnaviv_screen.c 
> b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> index 108b97d35c..95166a2db1 100644
> --- a/src/gallium/drivers/etnaviv/etnaviv_screen.c
> +++ b/src/gallium/drivers/etnaviv/etnaviv_screen.c
> @@ -372,6 +372,11 @@ etna_screen_get_param(struct pipe_screen *pscreen, enum 
> pipe_cap param)
>return 0;
> case PIPE_CAP_UMA:
>return 1;
> +
> +   /* hw atomic counters */
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
> +  return 0;
> }
>
> debug_printf("unknown param %d", param);
> diff --git a/src/gallium/drivers/freedreno/freedreno_screen.c 
> b/src/gallium/drivers/freedreno/freedreno_screen.c
> index af44ab698e..e6dfc2e48e 100644
> --- a/src/gallium/drivers/freedreno/freedreno_screen.c
> +++ b/src/gallium/drivers/freedreno/freedreno_screen.c
> @@ -491,6 +491,11 @@ fd_screen_get_param(struct pipe_screen *pscreen, enum 
> pipe_cap param)
> return 1;
> case PIPE_CAP_NATIVE_FENCE_FD:
> return fd_device_version(screen->dev) >= FD_VERSION_FENCE_FD;
> +
> +   /* hw atomic counters */
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
> +   return 0;
> }
> debug_printf("unknown param %d\n", param);
> return 0;
> diff --git a/src/gallium/drivers/nouveau/nv30/nv30_screen.c 
> b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> index d52d8f3988..3fdbadb6c6 100644
> --- a/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> +++ b/src/gallium/drivers/nouveau/nv30/nv30_screen.c
> @@ -239,6 +239,8 @@ nv30_screen_get_param(struct pipe_screen *pscreen, enum 
> pipe_cap param)
> case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
> case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
> case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>return 0;
>
> case PIPE_CAP_MAX_GS_INVOCATIONS:
> diff --git a/src/gallium/drivers/nouveau/nv50/nv50_screen.c 
> b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> index cd36795e56..a6dda5dfa0 100644
> --- a/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> +++ b/src/gallium/drivers/nouveau/nv50/nv50_screen.c
> @@ -293,6 +293,8 @@ nv50_screen_get_param(struct pipe_screen *pscreen, enum 
> pipe_cap param)
> case PIPE_CAP_CONSERVATIVE_RASTER_POST_DEPTH_COVERAGE:
> case PIPE_CAP_MAX_CONSERVATIVE_RASTER_SUBPIXEL_PRECISION_BIAS:
> case PIPE_CAP_PROGRAMMABLE_SAMPLE_LOCATIONS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>return 0;
>
> case PIPE_CAP_MAX_GS_INVOCATIONS:
> diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c 
> b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> index 446e30dcc8..b628b24d76 100644
> --- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> +++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
> @@ -322,6 +322,8 @@ nvc0_screen_get_param(struct pipe_screen *pscreen, enum 
> pipe_cap param)
> case PIPE_CAP_CONSTBUF0_FLAGS:
> case PIPE_CAP_PACKED_UNIFORMS:
> case PIPE_CAP_CONSERVATIVE_RASTER_PRE_SNAP_POINTS_LINES:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTERS:
> +   case PIPE_CAP_MAX_COMBINED_HW_ATOMIC_COUNTER_BUFFERS:
>return 0;
>
> case PIPE_CAP_VENDOR_ID:
> diff --git a/src/gallium/drivers/r300/r300_screen.c 
> b/src/gallium/drivers/r300/r300_screen.c
> index d27c2b8f1d..f29d5244ff 100644
> ---

Re: [Mesa-dev] shader cache backward compatibility

2018-09-03 Thread Alexander Larsson

On Fri, Aug 31, 2018 at 4:05 PM Emil Velikov  wrote:
>
> Valid point - I forgot about that.
>
> A couple of ideas come to mind:
>  - static link LLVM (Flatpak already does it)
> No LLVM changes needed.
>
>  - shared link LLVM
> LLVM add -Wl,--build-id=sha1

As a very very simple workaround, can you add the file sizes (as well
as the mtimes) to the staleness check? I mean, its possible that a
rebuild generates the exact same size, but at least its better than
always being wrong.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=
 Alexander LarssonRed Hat, Inc
   al...@redhat.com alexander.lars...@gmail.com
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/2] winsys/virgl: correct resource and handle allocation

2018-09-03 Thread Gert Wollny

The ressource storage must already be resized when the count is equal to
the allocated size and the space for the handles must be resized
accordingly.

Fixes:
  Crashes with
  piglit/bin/map_buffer_range-invalidate CopyBufferSubData \
   increment-offset -auto -fbo

Invalid write of size 8
at 0xB72E4CF: virgl_drm_add_res (virgl_drm_winsys.c:629)
by 0xB72E4CF: virgl_drm_emit_res (virgl_drm_winsys.c:663)
by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
by 0x109A1E: upload (invalidate.c:169)
by 0x109C2F: piglit_display (invalidate.c:215)
by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
by 0x10949D: main (invalidate.c:47)
Address 0xbe07d30 is 0 bytes after a block of size 4,096 alloc'd
at 0x4C31B25: calloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-
linux.so)
by 0xB72DAAF: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:567)

and

Invalid write of size 4
at 0xB72E4F0: virgl_drm_add_res (virgl_drm_winsys.c:631)
by 0xB72E4F0: virgl_drm_emit_res (virgl_drm_winsys.c:663)
by 0xB72A44A: virgl_encode_resource_copy_region (virgl_encode.c:776)
by 0xB40CD12: st_copy_buffer_subdata (st_cb_bufferobjects.c:585)
by 0xB244A3B: _mesa_CopyBufferSubData (bufferobj.c:2940)
by 0x109A1E: upload (invalidate.c:169)
by 0x109C2F: piglit_display (invalidate.c:215)
by 0x4F80FBE: run_test (piglit_fbo_framework.c:52)
by 0x4F66E5F: piglit_gl_test_run (piglit-framework-gl.c:229)
by 0x10949D: main (invalidate.c:47)
Address 0xbe08570 is 0 bytes after a block of size 2,048 alloc'd
at 0x4C2FB0F: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-
linux.so)
by 0xB72DAC8: virgl_drm_cmd_buf_create (virgl_drm_winsys.c:572)

Signed-off-by: Gert Wollny 
---
 src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index aad6430c41..2976b46484 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -617,13 +617,18 @@ static void virgl_drm_add_res(struct virgl_drm_winsys 
*qdws,
 {
unsigned hash = res->res_handle & (sizeof(cbuf->is_handle_added)-1);
 
-   if (cbuf->cres > cbuf->nres) {
+   if (cbuf->cres >= cbuf->nres) {
   cbuf->nres += 256;
   cbuf->res_bo = realloc(cbuf->res_bo, cbuf->nres * sizeof(struct 
virgl_hw_buf*));
   if (!cbuf->res_bo) {
   fprintf(stderr,"failure to add relocation %d, %d\n", cbuf->cres, 
cbuf->nres);
   return;
   }
+  cbuf->res_hlist = realloc(cbuf->res_hlist, cbuf->nres * 
sizeof(uint32_t));
+  if (!cbuf->res_hlist) {
+  fprintf(stderr,"failure to add hlist relocation %d, %d\n", 
cbuf->cres, cbuf->nres);
+  return;
+  }
}
 
cbuf->res_bo[cbuf->cres] = NULL;
-- 
2.17.1

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

[Mesa-dev] [PATCH 2/2] winsys/virgl: Initialize value to silence valgrind

2018-09-03 Thread Gert Wollny

Silences:

  Conditional jump or move depends on uninitialised value(s)
  at 0xB72F2C0: virgl_drm_winsys_create (virgl_drm_winsys.c:854)
  by 0xB72F2C0: virgl_drm_screen_create (virgl_drm_winsys.c:926)
  by 0xB21C885: pipe_virgl_create_screen (drm_helper.h:275)
  by 0xB7201F0: pipe_loader_create_screen (pipe_loader.c:137)
  by 0xB639C91: dri2_init_screen (dri2.c:2112)
  by 0xB634F68: driCreateNewScreen2 (dri_util.c:153)
  by 0x63023E6: dri3_create_screen (dri3_glx.c:893)
  by 0x62D35BD: AllocAndFetchScreenConfigs (glxext.c:820)
  by 0x62D35BD: __glXInitialize (glxext.c:946)
  by 0x62CECB3: GetGLXPrivScreenConfig (glxcmds.c:174)
  by 0x62CF69C: glXQueryExtensionsString (glxcmds.c:1304)
  by 0x60AA7D9: ??? (in /usr/lib/x86_64-linux-gnu/libwaffle-1.so.0.5.2)
  by 0x4F81450: wfl_checked_display_connect (piglit-util-waffle.h:74)
  by 0x4F829E0: piglit_wfl_framework_init (piglit_wfl_framework.c:627)

Signed-off-by: Gert Wollny 
---
 src/gallium/winsys/virgl/drm/virgl_drm_winsys.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c 
b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
index 2976b46484..a53b015598 100644
--- a/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
+++ b/src/gallium/winsys/virgl/drm/virgl_drm_winsys.c
@@ -846,7 +846,7 @@ virgl_drm_winsys_create(int drmFD)
 
qdws->base.get_caps = virgl_drm_get_caps;
 
-   uint32_t value;
+   uint32_t value = 0;
getparam.param = VIRTGPU_PARAM_CAPSET_QUERY_FIX;
getparam.value = (uint64_t)(uintptr_t)
ret = drmIoctl(qdws->fd, DRM_IOCTL_VIRTGPU_GETPARAM, );
-- 
2.17.1

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

Re: [Mesa-dev] [PATCH v2] egl/android: rework device probing

2018-09-03 Thread Tapani Pälli


I've tested that Android Celadon continues to work with these changes ..

Tested-by: Tapani Pälli 

On 08/31/2018 07:59 PM, Emil Velikov wrote:

From: Emil Velikov 

Unlike the other platforms, here we aim do guess if the device that we
somewhat arbitrarily picked, is supported or not.

In particular: when a vendor is _not_ requested we loop through all
devices, picking the first one which can create a DRI screen.

When a vendor is requested - we use that and do _not_ fall-back to any
other device.

The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
EGL_MESA_query_renderer are MIA, this is the best we can do for the
moment.

With those (proposed) extensions userspace will be able to create a
separate EGL display for each device, query device details and make the
conscious decision which one to use.

v2:
  - update droid_open_device_drm_gralloc()
  - set the dri2_dpy->fd before using it
  - return a EGLBoolean for droid_{probe,open}_device*
  - do not warn on droid_load_driver failure (Tomasz)
  - plug mem leak on dri2_create_screen failure (Tomasz)
  - fixup function name typo (Tomasz, Rob)

Cc: Robert Foss 
Cc: Tomasz Figa 
Cc: Mauro Rossi 
Signed-off-by: Emil Velikov 
---
Rob, I choose not to keep your r-b since the patch has changed quite a
bit.

Mauro, please check that this version doesn't break the drm_gralloc case.

Thanks
Emil
---
  src/egl/drivers/dri2/platform_android.c | 116 +++-
  1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/src/egl/drivers/dri2/platform_android.c 
b/src/egl/drivers/dri2/platform_android.c
index 1f9fe27ab85..0586633a6db 100644
--- a/src/egl/drivers/dri2/platform_android.c
+++ b/src/egl/drivers/dri2/platform_android.c
@@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
_EGLDisplay *dpy)
  }
  
  #ifdef HAVE_DRM_GRALLOC

-static int
-droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
+static EGLBoolean
+droid_open_device_drm_gralloc(_EGLDisplay *disp)
  {
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
 int fd = -1, err = -EINVAL;
  
 if (dri2_dpy->gralloc->perform)

@@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
*dri2_dpy)
);
 if (err || fd < 0) {
_eglLog(_EGL_WARNING, "fail to get drm fd");
-  fd = -1;
+  return EGL_FALSE;
 }
  
-   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;

+   if (dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3) < 0)
+ return EGL_FALSE;
+
+   return droid_probe_device(disp);
  }
  #endif /* HAVE_DRM_GRALLOC */
  
@@ -1365,7 +1369,7 @@ static const __DRIextension *droid_image_loader_extensions[] = {

  EGLBoolean
  droid_load_driver(_EGLDisplay *disp)
  {
-   struct dri2_egl_display *dri2_dpy = disp->DriverData;
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
 const char *err;
  
 dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);

@@ -1404,6 +1408,17 @@ error:
 return false;
  }
  
+static void

+droid_unload_driver(_EGLDisplay *disp)
+{
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
+
+   dlclose(dri2_dpy->driver);
+   dri2_dpy->driver = NULL;
+   free(dri2_dpy->driver_name);
+   dri2_dpy->driver_name = NULL;
+}
+
  static int
  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
  {
@@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
char *vendor)
 return 0;
  }
  
-static int

+static EGLBoolean
+droid_probe_device(_EGLDisplay *disp)
+{
+  /* Check that the device is supported, by attempting to:
+   * - load the dri module
+   * - and, create a screen
+   */
+   if (!droid_load_driver(disp))
+  return EGL_FALSE;
+
+   if (!dri2_create_screen(disp)) {
+  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
+  droid_unload_driver(disp);
+  return EGL_FALSE;
+   }
+   return EGL_TRUE;
+}
+
+static EGLBoolean
  droid_open_device(_EGLDisplay *disp)
  {
  #define MAX_DRM_DEVICES 32
+   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
 drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
 int prop_set, num_devices;
-   int fd = -1, fallback_fd = -1;
  
 char *vendor_name = NULL;

 char vendor_buf[PROPERTY_VALUE_MAX];
@@ -1436,7 +1469,7 @@ droid_open_device(_EGLDisplay *disp)
  
 num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));

 if (num_devices < 0)
-  return num_devices;
+  return EGL_FALSE;
  
 for (int i = 0; i < num_devices; i++) {

device = devices[i];
@@ -1444,41 +1477,49 @@ droid_open_device(_EGLDisplay *disp)
if (!(device->available_nodes & (1 << DRM_NODE_RENDER)))
   continue;
  
-  fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);

-  if (fd < 0) {
+  dri2_dpy->fd = loader_open_device(device->nodes[DRM_NODE_RENDER]);
+  if (dri2_dpy->fd < 0) {
   _eglLog(_EGL_WARNING, "%s() Failed to

[Mesa-dev] [Bug 107734] [GLSL] glsl-fface-invariant, glsl-fcoord-invariant and glsl-pcoord-invariant should fail

2018-09-03 Thread bugzilla-daemon

https://bugs.freedesktop.org/show_bug.cgi?id=107734

--- Comment #2 from Tapani Pälli  ---
Seems a bit like : https://patchwork.freedesktop.org/patch/36417/

IIRC there was some issue with that patch ... or not. Vadym, please check if
this fixes the WebGL tests also?

-- 
You are receiving this mail because:
You are the assignee for the bug.___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v3] intel/decoder: fix the possible out of bounds group_iter

2018-09-03 Thread andrey simiklit

Hi all,

Could somebody push this small patch to mesa?

Regards,
Andrii.
On Mon, Aug 20, 2018 at 9:13 PM Lionel Landwerlin <
lionel.g.landwer...@intel.com> wrote:

> On 20/08/2018 17:20, asimiklit.w...@gmail.com wrote:
> > From: Andrii Simiklit 
> >
> > The "gen_group_get_length" function can return a negative value
> > and it can lead to the out of bounds group_iter.
> >
> > v2: printing of "unknown command type" was added
> > v3: just the asserts are added
> >
> > Signed-off-by: Andrii Simiklit 
>
> Reviewed-by: Lionel Landwerlin 
>
> Somebody should take a look at the other patches I sent out ;)
> Thanks!
>
> -
> Lionel
> > ---
> >   src/intel/common/gen_decoder.c | 5 -
> >   1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/intel/common/gen_decoder.c
> b/src/intel/common/gen_decoder.c
> > index ec0a486..2d9609a 100644
> > --- a/src/intel/common/gen_decoder.c
> > +++ b/src/intel/common/gen_decoder.c
> > @@ -803,8 +803,10 @@ static bool
> >   iter_more_groups(const struct gen_field_iterator *iter)
> >   {
> >  if (iter->group->variable) {
> > +  int length = gen_group_get_length(iter->group, iter->p);
> > +  assert(length >= 0 && "error the length is unknown!");
> > return iter_group_offset_bits(iter, iter->group_iter + 1) <
> > -  (gen_group_get_length(iter->group, iter->p) * 32);
> > +  (length * 32);
> >  } else {
> > return (iter->group_iter + 1) < iter->group->group_count ||
> >iter->group->next != NULL;
> > @@ -991,6 +993,7 @@ gen_field_iterator_init(struct gen_field_iterator
> *iter,
> >  iter->p_bit = p_bit;
> >
> >  int length = gen_group_get_length(iter->group, iter->p);
> > +   assert(length >= 0 && "error the length is unknown!");
> >  iter->p_end = length > 0 ? [length] : NULL;
> >  iter->print_colors = print_colors;
> >   }
>
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH v2] egl/android: rework device probing

2018-09-03 Thread Eric Engestrom

On Monday, 2018-09-03 08:40:47 +0100, Eric Engestrom wrote:
> On Friday, 2018-08-31 17:59:10 +0100, Emil Velikov wrote:
> > From: Emil Velikov 
> > 
> > Unlike the other platforms, here we aim do guess if the device that we
> > somewhat arbitrarily picked, is supported or not.
> > 
> > In particular: when a vendor is _not_ requested we loop through all
> > devices, picking the first one which can create a DRI screen.
> > 
> > When a vendor is requested - we use that and do _not_ fall-back to any
> > other device.
> > 
> > The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
> > EGL_MESA_query_renderer are MIA, this is the best we can do for the
> > moment.
> > 
> > With those (proposed) extensions userspace will be able to create a
> > separate EGL display for each device, query device details and make the
> > conscious decision which one to use.
> > 
> > v2:
> >  - update droid_open_device_drm_gralloc()
> >  - set the dri2_dpy->fd before using it
> >  - return a EGLBoolean for droid_{probe,open}_device*
> >  - do not warn on droid_load_driver failure (Tomasz)
> >  - plug mem leak on dri2_create_screen failure (Tomasz)
> >  - fixup function name typo (Tomasz, Rob)
> > 
> > Cc: Robert Foss 
> > Cc: Tomasz Figa 
> > Cc: Mauro Rossi 
> > Signed-off-by: Emil Velikov 
> > ---
> > Rob, I choose not to keep your r-b since the patch has changed quite a
> > bit.
> > 
> > Mauro, please check that this version doesn't break the drm_gralloc case.
> > 
> > Thanks
> > Emil
> > ---
> >  src/egl/drivers/dri2/platform_android.c | 116 +++-
> >  1 file changed, 73 insertions(+), 43 deletions(-)
> > 
> > diff --git a/src/egl/drivers/dri2/platform_android.c 
> > b/src/egl/drivers/dri2/platform_android.c
> > index 1f9fe27ab85..0586633a6db 100644
> > --- a/src/egl/drivers/dri2/platform_android.c
> > +++ b/src/egl/drivers/dri2/platform_android.c
> > @@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
> > _EGLDisplay *dpy)
> >  }
> >  
> >  #ifdef HAVE_DRM_GRALLOC
> > -static int
> > -droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
> > +static EGLBoolean
> > +droid_open_device_drm_gralloc(_EGLDisplay *disp)
> >  {
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > int fd = -1, err = -EINVAL;
> >  
> > if (dri2_dpy->gralloc->perform)
> > @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct 
> > dri2_egl_display *dri2_dpy)
> >);
> > if (err || fd < 0) {
> >_eglLog(_EGL_WARNING, "fail to get drm fd");
> > -  fd = -1;
> > +  return EGL_FALSE;
> > }
> >  
> > -   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
> > +   if (dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3) < 0)
> 
> This is doing `dpy->fd = (dup() < 0)`.
> 
> You could add parentheses to fix it, or simply split into
>   dpy->fd = dup();
>   if (dpy->fd < 0)
> 
> (voting for the latter)

Sorry, ignore this, I just saw that Tomasz pointed it out as well :]

(I should really read other people's replies before saying anything)

> 
> > + return EGL_FALSE;
> > +
> > +   return droid_probe_device(disp);
> >  }
> >  #endif /* HAVE_DRM_GRALLOC */
> >  
> > @@ -1365,7 +1369,7 @@ static const __DRIextension 
> > *droid_image_loader_extensions[] = {
> >  EGLBoolean
> >  droid_load_driver(_EGLDisplay *disp)
> >  {
> > -   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > const char *err;
> >  
> > dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> > @@ -1404,6 +1408,17 @@ error:
> > return false;
> >  }
> >  
> > +static void
> > +droid_unload_driver(_EGLDisplay *disp)
> > +{
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > +
> > +   dlclose(dri2_dpy->driver);
> > +   dri2_dpy->driver = NULL;
> > +   free(dri2_dpy->driver_name);
> > +   dri2_dpy->driver_name = NULL;
> > +}
> > +
> >  static int
> >  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
> >  {
> > @@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, 
> > const char *vendor)
> > return 0;
> >  }
> >  
> > -static int
> > +static EGLBoolean
> > +droid_probe_device(_EGLDisplay *disp)
> > +{
> > +  /* Check that the device is supported, by attempting to:
> > +   * - load the dri module
> > +   * - and, create a screen
> > +   */
> > +   if (!droid_load_driver(disp))
> > +  return EGL_FALSE;
> > +
> > +   if (!dri2_create_screen(disp)) {
> > +  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> > +  droid_unload_driver(disp);
> > +  return EGL_FALSE;
> > +   }
> > +   return EGL_TRUE;
> > +}
> > +
> > +static EGLBoolean
> >  droid_open_device(_EGLDisplay *disp)
> >  {
> >  #define MAX_DRM_DEVICES 32
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
> > int prop_set,

Re: [Mesa-dev] [PATCH v2] egl/android: rework device probing

2018-09-03 Thread Eric Engestrom

On Friday, 2018-08-31 17:59:10 +0100, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Unlike the other platforms, here we aim do guess if the device that we
> somewhat arbitrarily picked, is supported or not.
> 
> In particular: when a vendor is _not_ requested we loop through all
> devices, picking the first one which can create a DRI screen.
> 
> When a vendor is requested - we use that and do _not_ fall-back to any
> other device.
> 
> The former seems a bit fiddly, but considering EGL_EXT_explicit_device and
> EGL_MESA_query_renderer are MIA, this is the best we can do for the
> moment.
> 
> With those (proposed) extensions userspace will be able to create a
> separate EGL display for each device, query device details and make the
> conscious decision which one to use.
> 
> v2:
>  - update droid_open_device_drm_gralloc()
>  - set the dri2_dpy->fd before using it
>  - return a EGLBoolean for droid_{probe,open}_device*
>  - do not warn on droid_load_driver failure (Tomasz)
>  - plug mem leak on dri2_create_screen failure (Tomasz)
>  - fixup function name typo (Tomasz, Rob)
> 
> Cc: Robert Foss 
> Cc: Tomasz Figa 
> Cc: Mauro Rossi 
> Signed-off-by: Emil Velikov 
> ---
> Rob, I choose not to keep your r-b since the patch has changed quite a
> bit.
> 
> Mauro, please check that this version doesn't break the drm_gralloc case.
> 
> Thanks
> Emil
> ---
>  src/egl/drivers/dri2/platform_android.c | 116 +++-
>  1 file changed, 73 insertions(+), 43 deletions(-)
> 
> diff --git a/src/egl/drivers/dri2/platform_android.c 
> b/src/egl/drivers/dri2/platform_android.c
> index 1f9fe27ab85..0586633a6db 100644
> --- a/src/egl/drivers/dri2/platform_android.c
> +++ b/src/egl/drivers/dri2/platform_android.c
> @@ -1203,9 +1203,10 @@ droid_add_configs_for_visuals(_EGLDriver *drv, 
> _EGLDisplay *dpy)
>  }
>  
>  #ifdef HAVE_DRM_GRALLOC
> -static int
> -droid_open_device_drm_gralloc(struct dri2_egl_display *dri2_dpy)
> +static EGLBoolean
> +droid_open_device_drm_gralloc(_EGLDisplay *disp)
>  {
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> int fd = -1, err = -EINVAL;
>  
> if (dri2_dpy->gralloc->perform)
> @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct dri2_egl_display 
> *dri2_dpy)
>);
> if (err || fd < 0) {
>_eglLog(_EGL_WARNING, "fail to get drm fd");
> -  fd = -1;
> +  return EGL_FALSE;
> }
>  
> -   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
> +   if (dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3) < 0)

This is doing `dpy->fd = (dup() < 0)`.

You could add parentheses to fix it, or simply split into
  dpy->fd = dup();
  if (dpy->fd < 0)

(voting for the latter)

> + return EGL_FALSE;
> +
> +   return droid_probe_device(disp);
>  }
>  #endif /* HAVE_DRM_GRALLOC */
>  
> @@ -1365,7 +1369,7 @@ static const __DRIextension 
> *droid_image_loader_extensions[] = {
>  EGLBoolean
>  droid_load_driver(_EGLDisplay *disp)
>  {
> -   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> const char *err;
>  
> dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> @@ -1404,6 +1408,17 @@ error:
> return false;
>  }
>  
> +static void
> +droid_unload_driver(_EGLDisplay *disp)
> +{
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> +
> +   dlclose(dri2_dpy->driver);
> +   dri2_dpy->driver = NULL;
> +   free(dri2_dpy->driver_name);
> +   dri2_dpy->driver_name = NULL;
> +}
> +
>  static int
>  droid_filter_device(_EGLDisplay *disp, int fd, const char *vendor)
>  {
> @@ -1420,13 +1435,31 @@ droid_filter_device(_EGLDisplay *disp, int fd, const 
> char *vendor)
> return 0;
>  }
>  
> -static int
> +static EGLBoolean
> +droid_probe_device(_EGLDisplay *disp)
> +{
> +  /* Check that the device is supported, by attempting to:
> +   * - load the dri module
> +   * - and, create a screen
> +   */
> +   if (!droid_load_driver(disp))
> +  return EGL_FALSE;
> +
> +   if (!dri2_create_screen(disp)) {
> +  _eglLog(_EGL_WARNING, "DRI2: failed to create screen");
> +  droid_unload_driver(disp);
> +  return EGL_FALSE;
> +   }
> +   return EGL_TRUE;
> +}
> +
> +static EGLBoolean
>  droid_open_device(_EGLDisplay *disp)
>  {
>  #define MAX_DRM_DEVICES 32
> +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
> int prop_set, num_devices;
> -   int fd = -1, fallback_fd = -1;
>  
> char *vendor_name = NULL;
> char vendor_buf[PROPERTY_VALUE_MAX];
> @@ -1436,7 +1469,7 @@ droid_open_device(_EGLDisplay *disp)
>  
> num_devices = drmGetDevices2(0, devices, ARRAY_SIZE(devices));
> if (num_devices < 0)
> -  return num_devices;
> +  return EGL_FALSE;
>  
> for (int i = 0; i < num_devices; i++) {
>device = devices[i];
> @@ -1444,41 +1477,49 @@ droid_open_device(_EGLDisplay *disp)
>if

Re: [Mesa-dev] [PATCH 2/3] radv: Use a lower max offchip buffer count.

2018-09-03 Thread Samuel Pitoiset




On 9/3/18 2:35 AM, Bas Nieuwenhuizen wrote:

No clue what gets fixed by this but both radeonsi and amdvlk do it.

CC: 
---
  src/amd/vulkan/radv_device.c | 24 ++--
  1 file changed, 22 insertions(+), 2 deletions(-)

diff --git a/src/amd/vulkan/radv_device.c b/src/amd/vulkan/radv_device.c
index c300c88468a..3d208ab053d 100644
--- a/src/amd/vulkan/radv_device.c
+++ b/src/amd/vulkan/radv_device.c
@@ -1901,10 +1901,30 @@ radv_get_hs_offchip_param(struct radv_device *device, 
uint32_t *max_offchip_buff
device->physical_device->rad_info.family != CHIP_CARRIZO &&
device->physical_device->rad_info.family != CHIP_STONEY;
unsigned max_offchip_buffers_per_se = double_offchip_buffers ? 128 : 64;
-   unsigned max_offchip_buffers = max_offchip_buffers_per_se *
-   device->physical_device->rad_info.max_se;
+   unsigned max_offchip_buffers;
unsigned offchip_granularity;
unsigned hs_offchip_param;
+
+   /*
+* Per RadeonSI:
+* This must be one less than the maximum number due to a hw limitation.
+ * Various hardware bugs in SI, CIK, and GFX9 need this.
+*
+* Per AMDVLK:
+* Vega10 should limit max_offchip_buffers to 508 (4 * 127).
+* Gfx7 should limit max_offchip_buffers to 508
+* Gfx6 should limit max_offchip_buffers to 126 (2 * 63)
+*
+* Follow AMDVLK here.
+ */


Can you adjust the indentation here please?

Otherwise, series is:

Reviewed-by: Samuel Pitoiset 


+   if (device->physical_device->rad_info.family == CHIP_VEGA10 ||
+   device->physical_device->rad_info.chip_class == CIK ||
+   device->physical_device->rad_info.chip_class == SI)
+   --max_offchip_buffers_per_se;
+
+   max_offchip_buffers = max_offchip_buffers_per_se *
+   device->physical_device->rad_info.max_se;
+
switch (device->tess_offchip_block_dw_size) {
default:
assert(0);


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

Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.

2018-09-03 Thread Eric Engestrom

On Monday, 2018-09-03 14:59:49 +0900, Tomasz Figa wrote:
> On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov  
> wrote:
> >
> > On 30 August 2018 at 11:41, Eric Engestrom  wrote:
> > > On Thursday, 2018-08-30 17:55:14 +0900, Tomasz Figa wrote:
> > >> Hi Erik, Emil, Eric,
> > >>
> > >> On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund  
> > >> wrote:
> > >> >
> > >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov  
> > >> > wrote:
> > >> > >
> > >> > > On 5 July 2018 at 17:17, Eric Engestrom  
> > >> > > wrote:
> > >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote:
> > >> > > >> On 5 July 2018 at 10:53, Eric Engestrom 
> > >> > > >>  wrote:
> > >> > > >> > On Monday, 2018-07-02 14:12:44 +0530, samiuddi wrote:
> > >> > > >> >> This fixes crash due to NULL window when swap interval is set
> > >> > > >> >> for pbuffer surface.
> > >> > > >> >>
> > >> > > >> >> Test: CtsDisplayTestCases pass
> > >> > > >> >>
> > >> > > >> >> Signed-off-by: samiuddi 
> > >> > > >> >> ---
> > >> > > >> >>
> > >> > > >> >> Kindly ignore this patch
> > >> > > >> >> https://lists.freedesktop.org/archives/mesa-dev/2018-July/199098.html
> > >> > > >> >>
> > >> > > >> >>  src/egl/drivers/dri2/platform_android.c | 2 +-
> > >> > > >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >> > > >> >>
> > >> > > >> >> diff --git a/src/egl/drivers/dri2/platform_android.c 
> > >> > > >> >> b/src/egl/drivers/dri2/platform_android.c
> > >> > > >> >> index ca8708a..b5b960a 100644
> > >> > > >> >> --- a/src/egl/drivers/dri2/platform_android.c
> > >> > > >> >> +++ b/src/egl/drivers/dri2/platform_android.c
> > >> > > >> >> @@ -485,7 +485,7 @@ droid_swap_interval(_EGLDriver *drv, 
> > >> > > >> >> _EGLDisplay *dpy,
> > >> > > >> >> struct dri2_egl_surface *dri2_surf = 
> > >> > > >> >> dri2_egl_surface(surf);
> > >> > > >> >> struct ANativeWindow *window = dri2_surf->window;
> > >> > > >> >>
> > >> > > >> >> -   if (window->setSwapInterval(window, interval))
> > >> > > >> >> +   if (window && window->setSwapInterval(window, interval))
> > >> > > >> >>return EGL_FALSE;
> > >> > > >> >
> > >> > > >> > Shouldn't we return false if we couldn't set the swap interval?
> > >> > > >> >
> > >> > > >> > I think this should be:
> > >> > > >> >if (!window || window->setSwapInterval(window, interval))
> > >> > > >> >   return EGL_FALSE;
> > >> > > >> >
> > >> > > >> Changing the patch as above will lead to eglSwapInterval 
> > >> > > >> consistently
> > >> > > >> failing for pbuffer surfaces.
> > >> > > >
> > >> > > > I'm not that familiar with pbuffers, but does SwapInterval really 
> > >> > > > make
> > >> > > > sense for them? I thought you couldn't swap a pbuffer.
> > >> > > >
> > >> > > > If so, failing an invalid op seems like the right thing to do.
> > >> > > > Otherwise, I guess sure, no-op returning success is fine.
> > >> > > >
> > >> > > Looking at eglSwapInterval manpage [1] (with my annotation):
> > >> > > "eglSwapInterval — specifies the minimum number of video frame 
> > >> > > periods
> > >> > > per buffer swap for the _window_ associated with the current 
> > >> > > context."
> > >> > > While the spec does not mention window/pbuffer/pixmap at all - it
> > >> > > extensively refers to eglSwapBuffers.
> > >> > >
> > >> > > Wrt eglSwapBuffers manpage [2] and spec are consistent:
> > >> > >
> > >> > > "If surface is a pixel buffer or a pixmap, eglSwapBuffers has no
> > >> > > effect, and no error is generated."
> > >> > >
> > >> > > Perhaps it's wrong to set eglSwapInterval for !window surfaces, but
> > >> > > its sibling (eglSwapBuffers) does not error out.
> > >> > > Hence I doubt many users make a distinction between window and 
> > >> > > pbuffer
> > >> > > surfaces for eglSwap*.
> > >> > >
> > >> > > Worth bringing to the WG meeting - it' planned for 1st August.
> > >> > >
> > >> >
> > >> > As I pointed out when I proposed this variant here:
> > >> > https://patchwork.freedesktop.org/patch/219313/
> > >> >
> > >> > "Also, I don't think EGL_FALSE is the right return-value, as it doesn't
> > >> > seem the EGL 1.5 spec defines any such error. Also, for instance
> > >> > dri2_swap_interval returns EGL_TRUE when there's no driver-function,
> > >> > which further backs the "silent failure" in this case IMO."
> > >> >
> > >> > So I think EGL_TRUE is the correct return-value for now. If the spec
> > >> > gets changed, we can of course update our implementation.
> > >>
> > >> What happens to this patch in the end? It looks like we're observing a
> > >> similar problem in Chrome OS.
> > >>
> > >> Emil, was there any follow-up on the WG meeting?
> > >
> > > Meeting was cancelled, but I raised the issue here:
> > > https://gitlab.khronos.org/egl/API/merge_requests/17
> > >
> > > Right now we have ARM saying they return false + error and NVIDIA saying
> > > they return true + no error and that ARM has a bug.
> > > I think another party adding their opinion might nudge it forward :)
> > >
> > Fwiw I put forward a

Re: [Mesa-dev] [PATCH v2] egl/wayland: do not leak wl_buffer when it is locked

2018-09-03 Thread Juan A. Suarez Romero

On Sat, 2018-09-01 at 02:14 +0300, Andres Gomez wrote:
> Juan, should we also include this in the stable queues ?
> 

Unless Daniel has a different opinion, yes, it should be included. Forgot to CC
to stable.

Thanks!

J.A.


> 
> On Thu, 2018-08-30 at 13:59 +0200, Juan A. Suarez Romero wrote:
> > If color buffer is locked, do not set its wayland buffer to NULL;
> > otherwise it can not be freed later.
> > 
> > Rather, flag it in order to destroy it later on the release event.
> > 
> > v2: instruct release event to unlock only or free wl_buffer too (Daniel)
> > 
> > This also fixes dEQP-EGL.functional.swap_buffers_with_damage.* tests.
> > 
> > CC: Daniel Stone 
> > ---
> >  src/egl/drivers/dri2/egl_dri2.h |  1 +
> >  src/egl/drivers/dri2/platform_wayland.c | 22 +++---
> >  2 files changed, 16 insertions(+), 7 deletions(-)
> > 
> > diff --git a/src/egl/drivers/dri2/egl_dri2.h 
> > b/src/egl/drivers/dri2/egl_dri2.h
> > index d1e4e8dcf22..349f66a3506 100644
> > --- a/src/egl/drivers/dri2/egl_dri2.h
> > +++ b/src/egl/drivers/dri2/egl_dri2.h
> > @@ -297,6 +297,7 @@ struct dri2_egl_surface
> > struct {
> >  #ifdef HAVE_WAYLAND_PLATFORM
> >struct wl_buffer   *wl_buffer;
> > +  boolwl_release;
> >__DRIimage *dri_image;
> >/* for is_different_gpu case. NULL else */
> >__DRIimage *linear_copy;
> > diff --git a/src/egl/drivers/dri2/platform_wayland.c 
> > b/src/egl/drivers/dri2/platform_wayland.c
> > index 11c57de0f89..03a3e0993b0 100644
> > --- a/src/egl/drivers/dri2/platform_wayland.c
> > +++ b/src/egl/drivers/dri2/platform_wayland.c
> > @@ -182,9 +182,12 @@ wl_buffer_release(void *data, struct wl_buffer *buffer)
> >if (dri2_surf->color_buffers[i].wl_buffer == buffer)
> >   break;
> >  
> > -   if (i == ARRAY_SIZE(dri2_surf->color_buffers)) {
> > +   assert (i < ARRAY_SIZE(dri2_surf->color_buffers));
> > +
> > +   if (dri2_surf->color_buffers[i].wl_release) {
> >wl_buffer_destroy(buffer);
> > -  return;
> > +  dri2_surf->color_buffers[i].wl_release = false;
> > +  dri2_surf->color_buffers[i].wl_buffer = NULL;
> > }
> >  
> > dri2_surf->color_buffers[i].locked = false;
> > @@ -425,9 +428,14 @@ dri2_wl_release_buffers(struct dri2_egl_surface 
> > *dri2_surf)
> >dri2_egl_display(dri2_surf->base.Resource.Display);
> >  
> > for (int i = 0; i < ARRAY_SIZE(dri2_surf->color_buffers); i++) {
> > -  if (dri2_surf->color_buffers[i].wl_buffer &&
> > -  !dri2_surf->color_buffers[i].locked)
> > - wl_buffer_destroy(dri2_surf->color_buffers[i].wl_buffer);
> > +  if (dri2_surf->color_buffers[i].wl_buffer) {
> > + if (dri2_surf->color_buffers[i].locked) {
> > +dri2_surf->color_buffers[i].wl_release = true;
> > + } else {
> > +wl_buffer_destroy(dri2_surf->color_buffers[i].wl_buffer);
> > +dri2_surf->color_buffers[i].wl_buffer = NULL;
> > + }
> > +  }
> >if (dri2_surf->color_buffers[i].dri_image)
> >   
> > dri2_dpy->image->destroyImage(dri2_surf->color_buffers[i].dri_image);
> >if (dri2_surf->color_buffers[i].linear_copy)
> > @@ -436,11 +444,9 @@ dri2_wl_release_buffers(struct dri2_egl_surface 
> > *dri2_surf)
> >   munmap(dri2_surf->color_buffers[i].data,
> >  dri2_surf->color_buffers[i].data_size);
> >  
> > -  dri2_surf->color_buffers[i].wl_buffer = NULL;
> >dri2_surf->color_buffers[i].dri_image = NULL;
> >dri2_surf->color_buffers[i].linear_copy = NULL;
> >dri2_surf->color_buffers[i].data = NULL;
> > -  dri2_surf->color_buffers[i].locked = false;
> > }
> >  
> > if (dri2_dpy->dri2)
> > @@ -968,6 +974,8 @@ dri2_wl_swap_buffers_with_damage(_EGLDriver *drv,
> >dri2_surf->current->wl_buffer =
> >   create_wl_buffer(dri2_dpy, dri2_surf, image);
> >  
> > +  dri2_surf->current->wl_release = false;
> > +
> >wl_buffer_add_listener(dri2_surf->current->wl_buffer,
> >   _buffer_listener, dri2_surf);
> > }

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

Re: [Mesa-dev] [PATCH v2] egl/android: rework device probing

2018-09-03 Thread Tomasz Figa

On Mon, Sep 3, 2018 at 2:45 PM Tomasz Figa  wrote:
>
> Hi Emil,
>
> On Sat, Sep 1, 2018 at 2:03 AM Emil Velikov  wrote:
> >
> > From: Emil Velikov 
> >
>
> Thanks for the patch! Please see my comments below.
>
> [snip]
> > @@ -1214,10 +1215,13 @@ droid_open_device_drm_gralloc(struct 
> > dri2_egl_display *dri2_dpy)
> >);
> > if (err || fd < 0) {
> >_eglLog(_EGL_WARNING, "fail to get drm fd");
> > -  fd = -1;
> > +  return EGL_FALSE;
> > }
> >
> > -   return (fd >= 0) ? fcntl(fd, F_DUPFD_CLOEXEC, 3) : -1;
> > +   if (dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3) < 0)
>
> This will assign the boolean result of the comparison to dri2_dpy->fd.
> To avoid parenthesis hell, I'd rewrite this to:
>
> dri2_dpy->fd = fcntl(fd, F_DUPFD_CLOEXEC, 3);
> if (dri2_dpy->fd < 0)
>return EGL_FALSE;
>
> > + return EGL_FALSE;
> > +
> > +   return droid_probe_device(disp);
> >  }
> >  #endif /* HAVE_DRM_GRALLOC */
> >
> > @@ -1365,7 +1369,7 @@ static const __DRIextension 
> > *droid_image_loader_extensions[] = {
> >  EGLBoolean
> >  droid_load_driver(_EGLDisplay *disp)
>
> Not related to this patch, but I guess we could fix it up, while at
> it. Fails to build with:
>
> src/egl/drivers/dri2/platform_android
> .c:1369:1: error: no previous prototype for function
> 'droid_load_driver' [-Werror,-Wmissing-prototypes]
> droid_load_driver(_EGLDisplay *disp)
> ^
>
> The function should be static.
>
> >  {
> > -   struct dri2_egl_display *dri2_dpy = disp->DriverData;
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > const char *err;
> >
> > dri2_dpy->driver_name = loader_get_driver_for_fd(dri2_dpy->fd);
> > @@ -1404,6 +1408,17 @@ error:
> > return false;
> >  }
> [snip]
> > +static EGLBoolean
> >  droid_open_device(_EGLDisplay *disp)
> >  {
> >  #define MAX_DRM_DEVICES 32
> > +   struct dri2_egl_display *dri2_dpy = dri2_egl_display(disp);
> > drmDevicePtr device, devices[MAX_DRM_DEVICES] = { NULL };
> > int prop_set, num_devices;
>
> Not related to this patch, but prop_set is unused. We could add a
> fixup in this patch, while reworking this already.
>
> I'm going to test it on Chrome OS with the fixups above applied.

The probing itself seems to be working fine, but it looks like there
is some EGL image regression in master, which I don't have time to
investigate.

Tested without the vendor property set, with both cases of the desired
device being probed in first or some later iteration. In both cases
the initialization succeeded and I could see the right GL renderer
string.

Tested-by: Tomasz Figa 

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

Re: [Mesa-dev] [PATCH V2] Check if the window is non-NULL before setting swap interval.

2018-09-03 Thread Tomasz Figa

On Thu, Aug 30, 2018 at 11:23 PM Emil Velikov  wrote:
>
> On 30 August 2018 at 11:41, Eric Engestrom  wrote:
> > On Thursday, 2018-08-30 17:55:14 +0900, Tomasz Figa wrote:
> >> Hi Erik, Emil, Eric,
> >>
> >> On Tue, Jul 10, 2018 at 12:41 AM Erik Faye-Lund  
> >> wrote:
> >> >
> >> > On Thu, Jul 5, 2018 at 7:02 PM Emil Velikov  
> >> > wrote:
> >> > >
> >> > > On 5 July 2018 at 17:17, Eric Engestrom  
> >> > > wrote:
> >> > > > On Thursday, 2018-07-05 14:43:02 +0100, Emil Velikov wrote:
> >> > > >> On 5 July 2018 at 10:53, Eric Engestrom  
> >> > > >> wrote:
> >> > > >> > On Monday, 2018-07-02 14:12:44 +0530, samiuddi wrote:
> >> > > >> >> This fixes crash due to NULL window when swap interval is set
> >> > > >> >> for pbuffer surface.
> >> > > >> >>
> >> > > >> >> Test: CtsDisplayTestCases pass
> >> > > >> >>
> >> > > >> >> Signed-off-by: samiuddi 
> >> > > >> >> ---
> >> > > >> >>
> >> > > >> >> Kindly ignore this patch
> >> > > >> >> https://lists.freedesktop.org/archives/mesa-dev/2018-July/199098.html
> >> > > >> >>
> >> > > >> >>  src/egl/drivers/dri2/platform_android.c | 2 +-
> >> > > >> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> > > >> >>
> >> > > >> >> diff --git a/src/egl/drivers/dri2/platform_android.c 
> >> > > >> >> b/src/egl/drivers/dri2/platform_android.c
> >> > > >> >> index ca8708a..b5b960a 100644
> >> > > >> >> --- a/src/egl/drivers/dri2/platform_android.c
> >> > > >> >> +++ b/src/egl/drivers/dri2/platform_android.c
> >> > > >> >> @@ -485,7 +485,7 @@ droid_swap_interval(_EGLDriver *drv, 
> >> > > >> >> _EGLDisplay *dpy,
> >> > > >> >> struct dri2_egl_surface *dri2_surf = dri2_egl_surface(surf);
> >> > > >> >> struct ANativeWindow *window = dri2_surf->window;
> >> > > >> >>
> >> > > >> >> -   if (window->setSwapInterval(window, interval))
> >> > > >> >> +   if (window && window->setSwapInterval(window, interval))
> >> > > >> >>return EGL_FALSE;
> >> > > >> >
> >> > > >> > Shouldn't we return false if we couldn't set the swap interval?
> >> > > >> >
> >> > > >> > I think this should be:
> >> > > >> >if (!window || window->setSwapInterval(window, interval))
> >> > > >> >   return EGL_FALSE;
> >> > > >> >
> >> > > >> Changing the patch as above will lead to eglSwapInterval 
> >> > > >> consistently
> >> > > >> failing for pbuffer surfaces.
> >> > > >
> >> > > > I'm not that familiar with pbuffers, but does SwapInterval really 
> >> > > > make
> >> > > > sense for them? I thought you couldn't swap a pbuffer.
> >> > > >
> >> > > > If so, failing an invalid op seems like the right thing to do.
> >> > > > Otherwise, I guess sure, no-op returning success is fine.
> >> > > >
> >> > > Looking at eglSwapInterval manpage [1] (with my annotation):
> >> > > "eglSwapInterval — specifies the minimum number of video frame periods
> >> > > per buffer swap for the _window_ associated with the current context."
> >> > > While the spec does not mention window/pbuffer/pixmap at all - it
> >> > > extensively refers to eglSwapBuffers.
> >> > >
> >> > > Wrt eglSwapBuffers manpage [2] and spec are consistent:
> >> > >
> >> > > "If surface is a pixel buffer or a pixmap, eglSwapBuffers has no
> >> > > effect, and no error is generated."
> >> > >
> >> > > Perhaps it's wrong to set eglSwapInterval for !window surfaces, but
> >> > > its sibling (eglSwapBuffers) does not error out.
> >> > > Hence I doubt many users make a distinction between window and pbuffer
> >> > > surfaces for eglSwap*.
> >> > >
> >> > > Worth bringing to the WG meeting - it' planned for 1st August.
> >> > >
> >> >
> >> > As I pointed out when I proposed this variant here:
> >> > https://patchwork.freedesktop.org/patch/219313/
> >> >
> >> > "Also, I don't think EGL_FALSE is the right return-value, as it doesn't
> >> > seem the EGL 1.5 spec defines any such error. Also, for instance
> >> > dri2_swap_interval returns EGL_TRUE when there's no driver-function,
> >> > which further backs the "silent failure" in this case IMO."
> >> >
> >> > So I think EGL_TRUE is the correct return-value for now. If the spec
> >> > gets changed, we can of course update our implementation.
> >>
> >> What happens to this patch in the end? It looks like we're observing a
> >> similar problem in Chrome OS.
> >>
> >> Emil, was there any follow-up on the WG meeting?
> >
> > Meeting was cancelled, but I raised the issue here:
> > https://gitlab.khronos.org/egl/API/merge_requests/17
> >
> > Right now we have ARM saying they return false + error and NVIDIA saying
> > they return true + no error and that ARM has a bug.
> > I think another party adding their opinion might nudge it forward :)
> >
> Fwiw I put forward a suggestion to add a workaround in the
> Android/CrOS libEGL wrapper for the ARM drivers.
> Although one could also consider the Nvidia/Mesa drivers as
> non-compliant and add a workaround for those instead.
>
> I have to admit it's not pretty, but it seems consistent with all the
> other workarounds in the wrapper.
>
> As Eric said -

77 matches

Mail list logo