Re: [Mesa-dev] [PATCH v3 02/10] gallium: add new fields to pipe_driver_query_info

2015-05-03 Thread Samuel Pitoiset 



On 04/26/2015 10:57 PM, Martin Peres wrote:

On 02/04/2015 21:41, Samuel Pitoiset wrote:

According to the spec of GL_AMD_performance_monitor, valid type values
returned are UNSIGNED_INT, UNSIGNED_INT64_AMD, PERCENTAGE_AMD, FLOAT.
This also introduces the new field group_id in order to categorize
queries into groups.

v3: fix incorrect query type for radeon and svga drivers

v2: add PIPE_DRIVER_QUERY_TYPE_BYTES


Weird way of ordering the versions. Usually, we get v2 before v3.
It's your call though :)


Okay, I'm going to change the order. :)



Signed-off-by: Samuel Pitoiset 
---
  src/gallium/auxiliary/hud/hud_driver_query.c  |  4 +++-
  src/gallium/drivers/nouveau/nvc0/nvc0_query.c |  6 ++
  src/gallium/drivers/radeon/r600_pipe_common.c | 14 +++---
  src/gallium/drivers/svga/svga_screen.c|  6 +++---
  src/gallium/include/pipe/p_defines.h  | 12 +++-
  5 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/src/gallium/auxiliary/hud/hud_driver_query.c 
b/src/gallium/auxiliary/hud/hud_driver_query.c

index 53771fc..1c8c485 100644
--- a/src/gallium/auxiliary/hud/hud_driver_query.c
+++ b/src/gallium/auxiliary/hud/hud_driver_query.c
@@ -187,6 +187,7 @@ hud_driver_query_install(struct hud_pane *pane, 
struct pipe_context *pipe,

 struct pipe_screen *screen = pipe->screen;
 struct pipe_driver_query_info query;
 unsigned num_queries, i;
+   boolean uses_byte_units;
 boolean found = FALSE;
   if (!screen->get_driver_query_info)
@@ -205,7 +206,8 @@ hud_driver_query_install(struct hud_pane *pane, 
struct pipe_context *pipe,

 if (!found)
return FALSE;
  +   uses_byte_units = query.type == PIPE_DRIVER_QUERY_TYPE_BYTES;
 hud_pipe_query_install(pane, pipe, query.name, query.query_type, 0,
-  query.max_value, query.uses_byte_units);
+  query.max_value, uses_byte_units);
 return TRUE;
  }
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c 
b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c

index f21deea..5be150e 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
@@ -1419,7 +1419,8 @@ nvc0_screen_get_driver_query_info(struct 
pipe_screen *pscreen,

info->name = nvc0_drv_stat_names[id];
info->query_type = NVC0_QUERY_DRV_STAT(id);
info->max_value = 0;
-  info->uses_byte_units = !!strstr(info->name, "bytes");
+  if (strstr(info->name, "bytes"))
+ info->type = PIPE_DRIVER_QUERY_TYPE_BYTES;
return 1;
 } else
  #endif
@@ -1428,14 +1429,12 @@ nvc0_screen_get_driver_query_info(struct 
pipe_screen *pscreen,
   info->name = nve4_pm_query_names[id - 
NVC0_QUERY_DRV_STAT_COUNT];
   info->query_type = NVE4_PM_QUERY(id - 
NVC0_QUERY_DRV_STAT_COUNT);
   info->max_value = (id < NVE4_PM_QUERY_METRIC_MP_OCCUPANCY) 
? 0 : 100;

- info->uses_byte_units = FALSE;
   return 1;
} else
if (screen->compute) {
   info->name = nvc0_pm_query_names[id - 
NVC0_QUERY_DRV_STAT_COUNT];
   info->query_type = NVC0_PM_QUERY(id - 
NVC0_QUERY_DRV_STAT_COUNT);

   info->max_value = 0;
- info->uses_byte_units = FALSE;
   return 1;
}
 }
@@ -1443,7 +1442,6 @@ nvc0_screen_get_driver_query_info(struct 
pipe_screen *pscreen,

 info->name = "this_is_not_the_query_you_are_looking_for";
 info->query_type = 0xdeadd01d;
 info->max_value = 0;
-   info->uses_byte_units = FALSE;
 return 0;
  }
  diff --git a/src/gallium/drivers/radeon/r600_pipe_common.c 
b/src/gallium/drivers/radeon/r600_pipe_common.c

index 0ef5fc2..238a934 100644
--- a/src/gallium/drivers/radeon/r600_pipe_common.c
+++ b/src/gallium/drivers/radeon/r600_pipe_common.c
@@ -656,13 +656,13 @@ static int r600_get_driver_query_info(struct 
pipe_screen *screen,
  struct r600_common_screen *rscreen = (struct 
r600_common_screen*)screen;

  struct pipe_driver_query_info list[] = {
  {"draw-calls", R600_QUERY_DRAW_CALLS, 0},
-{"requested-VRAM", R600_QUERY_REQUESTED_VRAM, 
rscreen->info.vram_size, TRUE},
-{"requested-GTT", R600_QUERY_REQUESTED_GTT, 
rscreen->info.gart_size, TRUE},

-{"buffer-wait-time", R600_QUERY_BUFFER_WAIT_TIME, 0, FALSE},
-{"num-cs-flushes", R600_QUERY_NUM_CS_FLUSHES, 0, FALSE},
-{"num-bytes-moved", R600_QUERY_NUM_BYTES_MOVED, 0, TRUE},
-{"VRAM-usage", R600_QUERY_VRAM_USAGE, 
rscreen->info.vram_size, TRUE},
-{"GTT-usage", R600_QUERY_GTT_USAGE, rscreen->info.gart_size, 
TRUE},
+{"requested-VRAM", R600_QUERY_REQUESTED_VRAM, 
rscreen->info.vram_size, PIPE_DRIVER_QUERY_TYPE_BYTES},
+{"requested-GTT", R600_QUERY_REQUESTED_GTT, 
rscreen->info.gart_size, PIPE_DRIVER_QUERY_TYPE_BYTES},

+{"buffer-wait-time", R600_QUERY_BUFFER_WAIT_TIME, 0},
+{"num-cs-flushes", R600_QUERY_NUM_CS_FLUSHES, 0},
+{"num-bytes-moved", R600_QUERY_NUM_BYTES

Re: [Mesa-dev] [PATCH v3 05/10] gallium: make pipe_context::begin_query return a boolean

2015-05-03 Thread Samuel Pitoiset 



On 04/26/2015 11:41 PM, Martin Peres wrote:

On 02/04/2015 21:41, Samuel Pitoiset wrote:

GL_AMD_performance_monitor must return an error when a monitoring
session cannot be started.

Signed-off-by: Samuel Pitoiset 
Reviewed-by: Marek Olšák 
Reviewed-by: Martin Peres 
---
  src/gallium/drivers/freedreno/freedreno_query.c|  4 ++--
  src/gallium/drivers/freedreno/freedreno_query.h|  2 +-
  src/gallium/drivers/freedreno/freedreno_query_hw.c |  3 ++-
  src/gallium/drivers/freedreno/freedreno_query_sw.c |  3 ++-
  src/gallium/drivers/i915/i915_query.c  |  3 ++-
  src/gallium/drivers/ilo/ilo_query.c|  3 ++-
  src/gallium/drivers/llvmpipe/lp_query.c|  3 ++-
  src/gallium/drivers/noop/noop_pipe.c   |  3 ++-
  src/gallium/drivers/nouveau/nv30/nv30_query.c  |  5 +++--
  src/gallium/drivers/nouveau/nv50/nv50_query.c  |  3 ++-
  src/gallium/drivers/nouveau/nvc0/nvc0_query.c  |  3 ++-
  src/gallium/drivers/r300/r300_query.c  |  9 +
  src/gallium/drivers/radeon/r600_query.c| 16 
+---

  src/gallium/drivers/rbug/rbug_context.c|  8 +---
  src/gallium/drivers/softpipe/sp_query.c|  3 ++-
  src/gallium/drivers/svga/svga_pipe_query.c |  3 ++-
  src/gallium/drivers/trace/tr_context.c |  6 --
  src/gallium/include/pipe/p_context.h   |  2 +-
  18 files changed, 50 insertions(+), 32 deletions(-)

diff --git a/src/gallium/drivers/freedreno/freedreno_query.c 
b/src/gallium/drivers/freedreno/freedreno_query.c

index 6f01e03..db2683c 100644
--- a/src/gallium/drivers/freedreno/freedreno_query.c
+++ b/src/gallium/drivers/freedreno/freedreno_query.c
@@ -59,11 +59,11 @@ fd_destroy_query(struct pipe_context *pctx, 
struct pipe_query *pq)

  q->funcs->destroy_query(fd_context(pctx), q);
  }
  -static void
+static boolean
  fd_begin_query(struct pipe_context *pctx, struct pipe_query *pq)
  {
  struct fd_query *q = fd_query(pq);
-q->funcs->begin_query(fd_context(pctx), q);
+return q->funcs->begin_query(fd_context(pctx), q);
  }
static void
diff --git a/src/gallium/drivers/freedreno/freedreno_query.h 
b/src/gallium/drivers/freedreno/freedreno_query.h

index bc9a7a2..c2c71da 100644
--- a/src/gallium/drivers/freedreno/freedreno_query.h
+++ b/src/gallium/drivers/freedreno/freedreno_query.h
@@ -37,7 +37,7 @@ struct fd_query;
  struct fd_query_funcs {
  void (*destroy_query)(struct fd_context *ctx,
  struct fd_query *q);
-void (*begin_query)(struct fd_context *ctx, struct fd_query *q);
+boolean (*begin_query)(struct fd_context *ctx, struct fd_query *q);
  void (*end_query)(struct fd_context *ctx, struct fd_query *q);
  boolean (*get_query_result)(struct fd_context *ctx,
  struct fd_query *q, boolean wait,
diff --git a/src/gallium/drivers/freedreno/freedreno_query_hw.c 
b/src/gallium/drivers/freedreno/freedreno_query_hw.c

index b29f9d4..a587178 100644
--- a/src/gallium/drivers/freedreno/freedreno_query_hw.c
+++ b/src/gallium/drivers/freedreno/freedreno_query_hw.c
@@ -136,7 +136,7 @@ fd_hw_begin_query(struct fd_context *ctx, struct 
fd_query *q)

  {
  struct fd_hw_query *hq = fd_hw_query(q);
  if (q->active)
-return;
+return true;

Should this be false instead of true?


Good catch! I fixed it locally.


/* begin_query() should clear previous results: */
  destroy_periods(ctx, &hq->periods);
@@ -149,6 +149,7 @@ fd_hw_begin_query(struct fd_context *ctx, struct 
fd_query *q)

  /* add to active list: */
  list_del(&hq->list);
  list_addtail(&hq->list, &ctx->active_queries);
+   return true;
  }
static void
diff --git a/src/gallium/drivers/freedreno/freedreno_query_sw.c 
b/src/gallium/drivers/freedreno/freedreno_query_sw.c

index 8d81698..514df14 100644
--- a/src/gallium/drivers/freedreno/freedreno_query_sw.c
+++ b/src/gallium/drivers/freedreno/freedreno_query_sw.c
@@ -85,7 +85,7 @@ is_rate_query(struct fd_query *q)
  }
  }
  -static void
+static boolean
  fd_sw_begin_query(struct fd_context *ctx, struct fd_query *q)
  {
  struct fd_sw_query *sq = fd_sw_query(q);
@@ -93,6 +93,7 @@ fd_sw_begin_query(struct fd_context *ctx, struct 
fd_query *q)

  sq->begin_value = read_counter(ctx, q->type);
  if (is_rate_query(q))
  sq->begin_time = os_time_get();
+   return true;
  }
static void
diff --git a/src/gallium/drivers/i915/i915_query.c 
b/src/gallium/drivers/i915/i915_query.c

index 1500d97..78d67ce 100644
--- a/src/gallium/drivers/i915/i915_query.c
+++ b/src/gallium/drivers/i915/i915_query.c
@@ -54,9 +54,10 @@ static void i915_destroy_query(struct pipe_context 
*ctx,

 FREE(query);
  }
  -static void i915_begin_query(struct pipe_context *ctx,
+static boolean i915_begin_query(struct pipe_context *ctx,
   struct pipe_query *query)
  {
+   return true;
  }
static void i915_end_query(struct pipe_context

Re: [Mesa-dev] [PATCH v3 06/10] st/mesa: implement GL_AMD_performance_monitor

2015-05-03 Thread Samuel Pitoiset 



On 04/27/2015 12:15 AM, Martin Peres wrote:

On 02/04/2015 21:41, Samuel Pitoiset wrote:

From: Christoph Bumiller 

This is based on the original patch of Christoph Bumiller.

v3:
  - only enable this extension when the underlying driver expose GPU 
counters

  - get rid of the ring buffer of queries

v2 (Samuel Pitoiset):
  - improve Gallium interface for this extension
  - rewrite some parts of the original code
  - fix compilation errors and piglit tests

Signed-off-by: Samuel Pitoiset 
---
  src/mesa/Makefile.sources  |   2 +
  src/mesa/state_tracker/st_cb_perfmon.c | 420 
+

  src/mesa/state_tracker/st_cb_perfmon.h |  64 +
  src/mesa/state_tracker/st_context.c|   9 +
  4 files changed, 495 insertions(+)
  create mode 100644 src/mesa/state_tracker/st_cb_perfmon.c
  create mode 100644 src/mesa/state_tracker/st_cb_perfmon.h

diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
index cc166ce..b1e56e2 100644
--- a/src/mesa/Makefile.sources
+++ b/src/mesa/Makefile.sources
@@ -432,6 +432,8 @@ STATETRACKER_FILES = \
  state_tracker/st_cb_flush.h \
  state_tracker/st_cb_msaa.c \
  state_tracker/st_cb_msaa.h \
+state_tracker/st_cb_perfmon.c \
+state_tracker/st_cb_perfmon.h \
  state_tracker/st_cb_program.c \
  state_tracker/st_cb_program.h \
  state_tracker/st_cb_queryobj.c \
diff --git a/src/mesa/state_tracker/st_cb_perfmon.c 
b/src/mesa/state_tracker/st_cb_perfmon.c

new file mode 100644
index 000..aba95f9
--- /dev/null
+++ b/src/mesa/state_tracker/st_cb_perfmon.c
@@ -0,0 +1,420 @@
+/*
+ * Copyright (C) 2013 Christoph Bumiller
+ * Copyright (C) 2015 Samuel Pitoiset
+ *
+ * Permission is hereby granted, free of charge, to any person 
obtaining a
+ * copy of this software and associated documentation files (the 
"Software"),
+ * to deal in the Software without restriction, including without 
limitation
+ * the rights to use, copy, modify, merge, publish, distribute, 
sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom 
the

+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice shall be 
included in

+ * all copies or substantial portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, 
EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF 
MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO 
EVENT SHALL

+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR
+ * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR 
OTHERWISE,
+ * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE 
USE OR

+ * OTHER DEALINGS IN THE SOFTWARE.
+ */
+
+/**
+ * Performance monitoring counters interface to gallium.
+ */
+
+#include "st_context.h"
+#include "st_cb_bitmap.h"
+#include "st_cb_perfmon.h"
+
+#include "util/bitset.h"
+
+#include "pipe/p_context.h"
+#include "pipe/p_screen.h"
+#include "util/u_memory.h"
+
+/**
+ * Return a PIPE_QUERY_x type >= PIPE_QUERY_DRIVER_SPECIFIC, or -1 if
+ * the driver-specific query doesn't exist.
+ */
+static int
+find_query_type(struct pipe_screen *screen, const char *name)
+{
+   int num_queries;
+   int type = -1;
+   int i;
+
+   num_queries = screen->get_driver_query_info(screen, 0, NULL);
+   if (!num_queries)
+  return type;
+
+   for (i = 0; i < num_queries; i++) {
+  struct pipe_driver_query_info info;
+
+  if (!screen->get_driver_query_info(screen, i, &info))
+ continue;
+
+  if (!strncmp(info.name, name, strlen(name))) {
+ type = info.query_type;
+ break;
+  }
+   }
+   return type;
+}
+
+/**
+ * Return TRUE if the underlying driver expose GPU counters.
+ */
+static bool
+has_gpu_counters(struct pipe_screen *screen)
+{
+   int num_groups, gid;
+
+   num_groups = screen->get_driver_query_group_info(screen, 0, NULL);
+   for (gid = 0; gid < num_groups; gid++) {
+  struct pipe_driver_query_group_info group_info;
+
+  if (!screen->get_driver_query_group_info(screen, gid, 
&group_info))

+ continue;
+
+  if (group_info.type == PIPE_DRIVER_QUERY_GROUP_TYPE_GPU)
+ return true;
+   }
+   return false;
+}
+
+static bool
+init_perf_monitor(struct gl_context *ctx, struct 
gl_perf_monitor_object *m)

+{
+   struct st_perf_monitor_object *stm = st_perf_monitor_object(m);
+   struct pipe_screen *screen = st_context(ctx)->pipe->screen;
+   struct pipe_context *pipe = st_context(ctx)->pipe;
+   int gid, cid;
+
+   st_flush_bitmap_cache(st_context(ctx));
+
+   /* Create a query for each active counter. */
+   for (gid = 0; gid < ctx->PerfMonitor.NumGroups; gid++) {
+  const struct gl_perf_monitor_group *g = 
&ctx->PerfMonitor.Groups[gid];

+
+  if (m->ActiveGroups[gid] > g->MaxActiveCounters) {
+ /* Maximum number of counters reached. Cannot start the 
session. */

Getting an error

Re: [Mesa-dev] [PATCH v3 07/10] nvc0: define driver-specific query groups

2015-05-03 Thread Samuel Pitoiset 



On 04/27/2015 12:20 AM, Martin Peres wrote:

On 27/04/2015 01:18, Martin Peres wrote:

On 02/04/2015 21:41, Samuel Pitoiset wrote:

This patch defines "Driver statistics" and "MP counters" groups, but
only the latter will be exposed through GL_AMD_performance_monitor.

Signed-off-by: Samuel Pitoiset 
---
   src/gallium/drivers/nouveau/nvc0/nvc0_query.c  | 67 
++

   src/gallium/drivers/nouveau/nvc0/nvc0_screen.c |  1 +
   src/gallium/drivers/nouveau/nvc0/nvc0_screen.h |  8 +++
   3 files changed, 76 insertions(+)

diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c 
b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c

index 01e7b37..071d179 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_query.c
@@ -1422,6 +1422,7 @@ nvc0_screen_get_driver_query_info(struct 
pipe_screen *pscreen,

 info->max_value.u64 = 0;
 if (strstr(info->name, "bytes"))
info->type = PIPE_DRIVER_QUERY_TYPE_BYTES;
+  info->group_id = NVC0_QUERY_DRV_STAT_GROUP;
 return 1;
  } else
   #endif
@@ -1431,22 +1432,88 @@ nvc0_screen_get_driver_query_info(struct 
pipe_screen *pscreen,
info->query_type = NVE4_PM_QUERY(id - 
NVC0_QUERY_DRV_STAT_COUNT);

info->max_value.u64 =
   (id < NVE4_PM_QUERY_METRIC_MP_OCCUPANCY) ? 0 : 100;
+ info->group_id = NVC0_QUERY_MP_COUNTER_GROUP;
return 1;
 } else
 if (screen->compute) {
info->name = nvc0_pm_query_names[id - 
NVC0_QUERY_DRV_STAT_COUNT];
info->query_type = NVC0_PM_QUERY(id - 
NVC0_QUERY_DRV_STAT_COUNT);

info->max_value.u64 = 0;
+ info->group_id = NVC0_QUERY_MP_COUNTER_GROUP;
return 1;
 }
  }
  /* user asked for info about non-existing query */
  info->name = "this_is_not_the_query_you_are_looking_for";
  info->query_type = 0xdeadd01d;
+   info->group_id = -1;
  info->max_value.u64 = 0;
  return 0;
   }
   +int
+nvc0_screen_get_driver_query_group_info(struct pipe_screen *pscreen,
+unsigned id,
+struct 
pipe_driver_query_group_info *info)

+{
+   struct nvc0_screen *screen = nvc0_screen(pscreen);
+   int count = 0;
+
+#ifdef NOUVEAU_ENABLE_DRIVER_STATISTICS
+   count++;
+#endif
+
+   if (screen->base.device->drm_version >= 0x01000101) {
+  if (screen->base.class_3d >= NVE4_3D_CLASS) {
+ count++;
+  } else if (screen->compute) {
+ count++; /* NVC0_COMPUTE is not always enabled */
+  }
+   }
+
+   if (!info)
+  return count;
+
+   if (id == NVC0_QUERY_MP_COUNTER_GROUP) {
+  info->name = "MP counters";
+  info->type = PIPE_DRIVER_QUERY_GROUP_TYPE_GPU;
+
+  if (screen->base.class_3d >= NVE4_3D_CLASS) {
+ info->num_queries = NVE4_PM_QUERY_COUNT;
+
+  /* On NVE4+, each multiprocessor have 8 hardware counters 
separated
+   * in two distinct domains, but we allow only one active 
query
+   * simultaneously because some of them use more than one 
hardware
+   * counter and this will result in an undefined 
behaviour. */
+  info->max_active_queries = 1; /* TODO: handle multiple hw 
counters */

+  return 1;
+  } else if (screen->compute) {
+ info->num_queries = NVC0_PM_QUERY_COUNT;
+
+ /* On NVC0:NVE4, each multiprocessor have 8 hardware counters
+  * in a single domain. */
+ info->max_active_queries = 8;
+ return 1;
+  }
+   }
+#ifdef NOUVEAU_ENABLE_DRIVER_STATISTICS

Why would anyone not want to enable driver statistics?


I see, it was already present. Fair-enough, we can argue about this in 
another patch :p




Yeah, maybe we should always enable driver statistics.


Patches 7-10 are:
Reviewed-by: Martin Peres 


Thanks Martin.






+   else if (id == NVC0_QUERY_DRV_STAT_GROUP) {
+  info->name = "Driver statistics";
+  info->type = PIPE_DRIVER_QUERY_GROUP_TYPE_CPU;
+  info->max_active_queries = NVC0_QUERY_DRV_STAT_COUNT;
+  info->num_queries = NVC0_QUERY_DRV_STAT_COUNT;
+  return 1;
+   }
+#endif
+
+   /* user asked for info about non-existing query group */
+   info->name = "this_is_not_the_query_group_you_are_looking_for";
+   info->max_active_queries = 0;
+   info->num_queries = 0;
+   info->type = 0;
+   return 0;
+}
+
   void
   nvc0_init_query_functions(struct nvc0_context *nvc0)
   {
diff --git a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c 
b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c

index 04c34f5..3c94cfa 100644
--- a/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
+++ b/src/gallium/drivers/nouveau/nvc0/nvc0_screen.c
@@ -651,6 +651,7 @@ nvc0_screen_create(struct nouveau_device *dev)
  pscreen->get_shader_param = nvc0_screen_get_shader_param;
  pscreen->get_paramf = nvc0_screen_get_paramf;
  pscreen->get_driver_query_info = 
nvc0_screen_get_driver_query_i

Re: [Mesa-dev] [PATCH 00/15] GL_AMD_performance_monitor

2015-05-03 Thread Samuel Pitoiset 



On 04/14/2015 11:02 PM, Robert Bragg wrote:

Hi Samuel,

On Tue, Mar 31, 2015 at 5:56 PM, Samuel Pitoiset
 wrote:

Hello Robert,

Sorry for the delay, I just saw your message few days ago, and I probably
removed the mail by mistake too...

And then I was on holiday; so more delay :-)


I have never heard about your work on this area, happy to know right now. :)

Well, regarding the backend stuff, I would prefer to keep the same for both
GL_AMD_performance_monitor and INTEL_performance_query.

My experience with the Intel backend where I initially aimed to update
both extensions behind one backend is that it was quite a hindrance
and there wasn't a clear benefit to it when there isn't really any
substantial code to speak of in the core infrastructure to share
between the extensions.

We should be careful not to talk cross purposes here though. In my
mind having orthogonal frontends and even different backend interfaces
wouldn't preclude a driver implementing both extensions with a unified
backend if desirable, there would just be two separate sets of entry
points for the frontend to interact with that unified backend.

Some of the issues I came across were:

The current design expects a common description of counters and their
types, but the current implementation doesn't fully support
INTEL_performance_query semantic types. Fixing this is awkward
because neither extension has data/semantic types that are a strict
subset of the other so to support all the types I imagine we'd also
need to introduce some mechanism for black/white listing counters for
each extension if we want to keep a common description. Then if we
wanted to utilize the full range of types for both extensions I have a
feeling a lot of the counters would end up being exclusively declared
for one extension or the other which would negate some of the benefit
of having a common structure.

The current infrastructure seems somewhat biased towards implementing
AMD_performance_monitor with the concept of groups and counter
selection which doesn't exist in the INTEL_performance_query extension
and that seems unfortunate when the selection mechanism looks to make
the allocation and tracking of query objects more costly/complex if we
don't need it for the INTEL_performance_query extension.

There's no substantial utility code associated with the core
infrastructure that the backends benefit from to help justify sharing
a backend for multiple extensions. The core support just does simple
frontend validation of user arguments to normalize things and handle
gl errors consistently before interacting with the backend so in
practice the INTEL_performance_query and AMD_performance_monitor code
is rather orthogonal.

I think the only things that connect the two extensions currently are
the shared declaration of counters and a tiny amount of utility code
for allocating/freeing monitor objects. Given the issue with the
counter types I found things became simpler if the counter
descriptions were instead moved into the backend. Given that
INTEL_performance_query doesn't need any active group/counter state
per object, the common object allocator also isn't ideal. So making
both of these changes (which seem to make sense even without the goal
of separating the extension) is enough to make the frontends
completely orthogonal. I also really like that with the counter
declarations in the backend that it's free to use whatever data
structures are appropriate for the various counters. As opposed to
statically declared arrays describing our counters, I needed to update
our backend to programatically build up the lists of available
counters and counter descriptions also necessarily became more
detailed so it was nice that this work could be self contained in the
backend and we can describe our Observation Architecture counters
differently from our pipeline statistics counters.

My thinking a.t.m is that if the current AMD_perfmon backend
architecture seems to be ok for your needs then it could be for the
best that the extensions can be easily made orthogonal so we can
develop support for both extensions without stepping on each other's
toes. Later if it's desirable to support both extensions in any driver
we can always evaluate what opportunities there are to have a common
backend interface if that could simplify things.


Currently, I'm trying to implement GL_AMD_perfmon as a state tracker which
is based on the query interface of Gallium and this looks quite good. Only
minor changes in the current interface are required to do that.

At this time, most of hardware performance counters are *only* exposed
through the Gallium HUD and I think it's not very helpful for a large number
of applications.
I'm pretty sure that GL_AMD_perfmon will be very useful for exposing GPU
counters and this is also a requirement for a GSoC project this year.

So, with respect to your work, my question is : why do you want to get rid
of AMD_perfmon in favour of INTEL_perf_query ?

 From my p

Re: [Mesa-dev] [PATCH 06/19] gallium: add tessellation shader properties

2015-05-03 Thread Glenn Kennard 
On Sat, 02 May 2015 22:16:30 +0200, Ilia Mirkin   
wrote:



Signed-off-by: Ilia Mirkin 
---
 src/gallium/auxiliary/tgsi/tgsi_strings.c  |  7 ++-
 src/gallium/docs/source/tgsi.rst   | 33  
++

 src/gallium/include/pipe/p_defines.h   |  7 +++
 src/gallium/include/pipe/p_shader_tokens.h |  7 ++-
 4 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.c  
b/src/gallium/auxiliary/tgsi/tgsi_strings.c

index dad503e..6781248 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_strings.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_strings.c
@@ -131,7 +131,12 @@ const char  
*tgsi_property_names[TGSI_PROPERTY_COUNT] =

"FS_DEPTH_LAYOUT",
"VS_PROHIBIT_UCPS",
"GS_INVOCATIONS",
-   "VS_WINDOW_SPACE_POSITION"
+   "VS_WINDOW_SPACE_POSITION",
+   "TCS_VERTICES_OUT",
+   "TES_PRIM_MODE",
+   "TES_SPACING",
+   "TES_VERTEX_ORDER_CW",
+   "TES_POINT_MODE",


Stray comma


 };
const char *tgsi_return_type_names[TGSI_RETURN_TYPE_COUNT] =
diff --git a/src/gallium/docs/source/tgsi.rst  
b/src/gallium/docs/source/tgsi.rst

index 0116842..f77702a 100644
--- a/src/gallium/docs/source/tgsi.rst
+++ b/src/gallium/docs/source/tgsi.rst
@@ -3071,6 +3071,39 @@ Naturally, clipping is not performed on window  
coordinates either.
 The effect of this property is undefined if a geometry or tessellation  
shader

 are in use.
+TCS_VERTICES_OUT
+
+
+The number of vertices written by the tessellation control shader. This
+effectively defines the patch input size of the tessellation evaluation  
shader

+as well.
+
+TES_PRIM_MODE
+"
+
+This sets the tessellation primitive mode, one of  
``PIPE_PRIM_TRIANGLES``,

+``PIPE_PRIM_QUADS``, or ``PIPE_PRIM_LINES``. (Unlike in GL, there is no
+separate isolines settings, the regular lines is assumed to mean  
isolines.)

+
+TES_SPACING
+"""
+
+This sets the spacing mode of the tessellation generator, one of
+``PIPE_TESS_SPACING_*``.
+
+TES_VERTEX_ORDER_CW
+"""
+
+This sets the vertex order to be clockwise if the value is 1, or
+counter-clockwise if set to 0.
+
+TES_POINT_MODE
+""
+
+If set to a non-zero value, this turns on point mode for the  
tessellator,

+which means that points will be generated instead of primitives.
+
+
 Texture Sampling and Texture Formats
 
diff --git a/src/gallium/include/pipe/p_defines.h  
b/src/gallium/include/pipe/p_defines.h

index 59b7486..14e0db3 100644
--- a/src/gallium/include/pipe/p_defines.h
+++ b/src/gallium/include/pipe/p_defines.h
@@ -432,6 +432,13 @@ enum pipe_flush_flags
/**
+ * Tessellator spacing types
+ */
+#define PIPE_TESS_SPACING_FRACT_ODD  0
+#define PIPE_TESS_SPACING_FRACT_EVEN 1


GL spec types out the FRACTIONAL which is easier to grep the spec for.


+#define PIPE_TESS_SPACING_EQUAL  2
+
+/**
  * Query object types
  */
 #define PIPE_QUERY_OCCLUSION_COUNTER 0
diff --git a/src/gallium/include/pipe/p_shader_tokens.h  
b/src/gallium/include/pipe/p_shader_tokens.h

index c6ab899..ff1f7d6 100644
--- a/src/gallium/include/pipe/p_shader_tokens.h
+++ b/src/gallium/include/pipe/p_shader_tokens.h
@@ -262,7 +262,12 @@ union tgsi_immediate_data
 #define TGSI_PROPERTY_VS_PROHIBIT_UCPS   7
 #define TGSI_PROPERTY_GS_INVOCATIONS 8
 #define TGSI_PROPERTY_VS_WINDOW_SPACE_POSITION 9
-#define TGSI_PROPERTY_COUNT  10
+#define TGSI_PROPERTY_TCS_VERTICES_OUT   10
+#define TGSI_PROPERTY_TES_PRIM_MODE  11
+#define TGSI_PROPERTY_TES_SPACING12
+#define TGSI_PROPERTY_TES_VERTEX_ORDER_CW13
+#define TGSI_PROPERTY_TES_POINT_MODE 14
+#define TGSI_PROPERTY_COUNT  15
struct tgsi_property {
unsigned Type : 4;  /**< TGSI_TOKEN_TYPE_PROPERTY */


With above niggles fixed
Reviewed-by: Glenn Kennard 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/19] gallium: basic tessellation support

2015-05-03 Thread Glenn Kennard 
On Sat, 02 May 2015 22:16:24 +0200, Ilia Mirkin   
wrote:



This series adds tokens and updates some helper gallium functions to
know about tessellation. This provides no actual support for
tessellation in either core or drivers, however this will make it
possible to work on the core and driver pieces without crazy
interdependencies, as well as be landed separately and without
(direct) dependency.

Most of these patches have existed for about a year already, and have
been part of my and Marek's trees enabling tessellation in the nvc0
and radeonsi drivers. I've taken this opportunity to fix up and fold
some of them though.

This should be pretty safe to land, since even if I messed something
up, having this in-tree will make it easier for others to identify and
fix any issues collaboratively.

Ilia Mirkin (11):
  gallium: add tessellation shader types
  gallium: add new PATCHES primitive type
  gallium: add new semantics for tessellation
  gallium: add interfaces for controlling tess program state
  gallium: add tessellation shader properties
  gallium: add patch_vertices to draw info
  gallium: add set_tess_state to configure default tessellation
parameters
  tgsi/scan: allow scanning tessellation shaders
  tgsi/sanity: set implicit in/out array sizes based on patch sizes
  tgsi/ureg: allow ureg_dst to have dimension indices
  tgsi/dump: fix declaration printing of tessellation inputs/outputs

Marek Olšák (8):
  gallium: bump shader input and output limits
  trace: implement new tessellation functions
  gallium/util: print patch_vertices in util_dump_draw_info
  gallium/u_blitter: disable tessellation for all operations
  gallium/cso: add support for tessellation shaders
  gallium/cso: set NULL shaders at context destruction
  gallium: disable tessellation shaders for meta ops
  tgsi/ureg: use correct limit for max input count

 src/gallium/auxiliary/cso_cache/cso_context.c | 100  
++

 src/gallium/auxiliary/cso_cache/cso_context.h |  12 
 src/gallium/auxiliary/hud/hud_context.c   |   6 ++
 src/gallium/auxiliary/postprocess/pp_run.c|   6 ++
 src/gallium/auxiliary/tgsi/tgsi_dump.c|  20 +-
 src/gallium/auxiliary/tgsi/tgsi_info.c|   4 ++
 src/gallium/auxiliary/tgsi/tgsi_sanity.c  |  36 --
 src/gallium/auxiliary/tgsi/tgsi_scan.c|   6 +-
 src/gallium/auxiliary/tgsi/tgsi_strings.c |  19 -
 src/gallium/auxiliary/tgsi/tgsi_strings.h |   2 +-
 src/gallium/auxiliary/tgsi/tgsi_ureg.c|  26 ++-
 src/gallium/auxiliary/tgsi/tgsi_ureg.h|  59 +--
 src/gallium/auxiliary/util/u_blit.c   |   6 ++
 src/gallium/auxiliary/util/u_blitter.c|  27 +++
 src/gallium/auxiliary/util/u_blitter.h|  16 -
 src/gallium/auxiliary/util/u_dump_state.c |   2 +
 src/gallium/docs/source/context.rst   |   5 ++
 src/gallium/docs/source/tgsi.rst  |  70 ++
 src/gallium/drivers/trace/tr_context.c|  26 +++
 src/gallium/drivers/trace/tr_dump_state.c |   2 +
 src/gallium/include/pipe/p_context.h  |  14 
 src/gallium/include/pipe/p_defines.h  |  16 -
 src/gallium/include/pipe/p_shader_tokens.h|  18 -
 src/gallium/include/pipe/p_state.h|   6 +-
 src/mesa/state_tracker/st_cb_bitmap.c |   8 ++-
 src/mesa/state_tracker/st_cb_clear.c  |   6 ++
 src/mesa/state_tracker/st_cb_drawpixels.c |   8 ++-
 src/mesa/state_tracker/st_cb_drawtex.c|   6 ++
 28 files changed, 501 insertions(+), 31 deletions(-)



Some minor nits for patches 1, 6 and 7, see separate mails

Patches 2-5, 8-19 are
Reviewed-by: Glenn Kennard 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 07/19] gallium: add patch_vertices to draw info

2015-05-03 Thread Glenn Kennard 
On Sat, 02 May 2015 22:16:31 +0200, Ilia Mirkin   
wrote:



Signed-off-by: Ilia Mirkin 
---
 src/gallium/include/pipe/p_state.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/gallium/include/pipe/p_state.h  
b/src/gallium/include/pipe/p_state.h

index e713a44..449c7f1 100644
--- a/src/gallium/include/pipe/p_state.h
+++ b/src/gallium/include/pipe/p_state.h
@@ -543,6 +543,8 @@ struct pipe_draw_info
unsigned start_instance; /**< first instance id */
unsigned instance_count; /**< number of instances */
+   unsigned patch_vertices; /**< the number of vertices per patch */
+


patch_vertex_count, this field isn't the actual patch vertices data
Don't forget to update patch 10 with the name


/**
 * For indexed drawing, these fields apply after index lookup.
 */


With above fixed,
Reviewed-by: Glenn Kennard 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 01/19] gallium: add tessellation shader types

2015-05-03 Thread Glenn Kennard 
On Sat, 02 May 2015 22:16:25 +0200, Ilia Mirkin   
wrote:



Signed-off-by: Ilia Mirkin 
---
 src/gallium/auxiliary/tgsi/tgsi_info.c | 4 
 src/gallium/auxiliary/tgsi/tgsi_strings.c  | 4 +++-
 src/gallium/auxiliary/tgsi/tgsi_strings.h  | 2 +-
 src/gallium/include/pipe/p_defines.h   | 6 --
 src/gallium/include/pipe/p_shader_tokens.h | 4 +++-
 5 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c  
b/src/gallium/auxiliary/tgsi/tgsi_info.c

index 3cab86e..eb447cb 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_info.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
@@ -302,6 +302,10 @@ tgsi_get_processor_name( uint processor )
   return "fragment shader";
case TGSI_PROCESSOR_GEOMETRY:
   return "geometry shader";
+   case TGSI_PROCESSOR_TESSCTRL:
+  return "tessellation control shader";
+   case TGSI_PROCESSOR_TESSEVAL:
+  return "tessellation evaluation shader";
default:
   return "unknown shader type!";
}
diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.c  
b/src/gallium/auxiliary/tgsi/tgsi_strings.c

index 9b727cf..e712f30 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_strings.c
+++ b/src/gallium/auxiliary/tgsi/tgsi_strings.c
@@ -32,11 +32,13 @@
 #include "tgsi_strings.h"
-const char *tgsi_processor_type_names[4] =
+const char *tgsi_processor_type_names[6] =


Don't forget to update the declaration in tgsi_strings.h


 {
"FRAG",
"VERT",
"GEOM",
+   "TESSC",
+   "TESSE",


A bit silly to shorten these when the dumps dedicate an entire line for  
printing the name.



"COMP"
 };
diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.h  
b/src/gallium/auxiliary/tgsi/tgsi_strings.h

index 90014a2..71e7437 100644
--- a/src/gallium/auxiliary/tgsi/tgsi_strings.h
+++ b/src/gallium/auxiliary/tgsi/tgsi_strings.h
@@ -38,7 +38,7 @@ extern "C" {
 #endif
-extern const char *tgsi_processor_type_names[4];
+extern const char *tgsi_processor_type_names[6];
extern const char *tgsi_semantic_names[TGSI_SEMANTIC_COUNT];
diff --git a/src/gallium/include/pipe/p_defines.h  
b/src/gallium/include/pipe/p_defines.h

index 67f48e4..48c182f 100644
--- a/src/gallium/include/pipe/p_defines.h
+++ b/src/gallium/include/pipe/p_defines.h
@@ -404,8 +404,10 @@ enum pipe_flush_flags
 #define PIPE_SHADER_VERTEX   0
 #define PIPE_SHADER_FRAGMENT 1
 #define PIPE_SHADER_GEOMETRY 2
-#define PIPE_SHADER_COMPUTE  3
-#define PIPE_SHADER_TYPES4
+#define PIPE_SHADER_TESSCTRL 3
+#define PIPE_SHADER_TESSEVAL 4


Most of the gallium names are typed out without contractions, ie  
PIPE_SHADER_TESSELLATION_CONTROL/EVALUATION



+#define PIPE_SHADER_COMPUTE  5
+#define PIPE_SHADER_TYPES6
/**
diff --git a/src/gallium/include/pipe/p_shader_tokens.h  
b/src/gallium/include/pipe/p_shader_tokens.h

index c14bcbc..776b0d4 100644
--- a/src/gallium/include/pipe/p_shader_tokens.h
+++ b/src/gallium/include/pipe/p_shader_tokens.h
@@ -43,7 +43,9 @@ struct tgsi_header
 #define TGSI_PROCESSOR_FRAGMENT  0
 #define TGSI_PROCESSOR_VERTEX1
 #define TGSI_PROCESSOR_GEOMETRY  2
-#define TGSI_PROCESSOR_COMPUTE   3
+#define TGSI_PROCESSOR_TESSCTRL  3
+#define TGSI_PROCESSOR_TESSEVAL  4
+#define TGSI_PROCESSOR_COMPUTE   5
struct tgsi_processor
 {


With above niggles fixed
Reviewed-by: Glenn Kennard 
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 01/19] gallium: add tessellation shader types

2015-05-03 Thread Fredrik Höglund 
On Sunday 03 May 2015, Glenn Kennard wrote:
> On Sat, 02 May 2015 22:16:25 +0200, Ilia Mirkin   
> wrote:
> 
> > Signed-off-by: Ilia Mirkin 
> > ---
> >  src/gallium/auxiliary/tgsi/tgsi_info.c | 4 
> >  src/gallium/auxiliary/tgsi/tgsi_strings.c  | 4 +++-
> >  src/gallium/auxiliary/tgsi/tgsi_strings.h  | 2 +-
> >  src/gallium/include/pipe/p_defines.h   | 6 --
> >  src/gallium/include/pipe/p_shader_tokens.h | 4 +++-
> >  5 files changed, 15 insertions(+), 5 deletions(-)
> >
> > diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c  
> > b/src/gallium/auxiliary/tgsi/tgsi_info.c
> > index 3cab86e..eb447cb 100644
> > --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
> > +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
> > @@ -302,6 +302,10 @@ tgsi_get_processor_name( uint processor )
> >return "fragment shader";
> > case TGSI_PROCESSOR_GEOMETRY:
> >return "geometry shader";
> > +   case TGSI_PROCESSOR_TESSCTRL:
> > +  return "tessellation control shader";
> > +   case TGSI_PROCESSOR_TESSEVAL:
> > +  return "tessellation evaluation shader";
> > default:
> >return "unknown shader type!";
> > }
> > diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.c  
> > b/src/gallium/auxiliary/tgsi/tgsi_strings.c
> > index 9b727cf..e712f30 100644
> > --- a/src/gallium/auxiliary/tgsi/tgsi_strings.c
> > +++ b/src/gallium/auxiliary/tgsi/tgsi_strings.c
> > @@ -32,11 +32,13 @@
> >  #include "tgsi_strings.h"
> > -const char *tgsi_processor_type_names[4] =
> > +const char *tgsi_processor_type_names[6] =
> 
> Don't forget to update the declaration in tgsi_strings.h
> 
> >  {
> > "FRAG",
> > "VERT",
> > "GEOM",
> > +   "TESSC",
> > +   "TESSE",
> 
> A bit silly to shorten these when the dumps dedicate an entire line for  
> printing the name.
> 
> > "COMP"
> >  };
> > diff --git a/src/gallium/auxiliary/tgsi/tgsi_strings.h  
> > b/src/gallium/auxiliary/tgsi/tgsi_strings.h
> > index 90014a2..71e7437 100644
> > --- a/src/gallium/auxiliary/tgsi/tgsi_strings.h
> > +++ b/src/gallium/auxiliary/tgsi/tgsi_strings.h
> > @@ -38,7 +38,7 @@ extern "C" {
> >  #endif
> > -extern const char *tgsi_processor_type_names[4];
> > +extern const char *tgsi_processor_type_names[6];
> > extern const char *tgsi_semantic_names[TGSI_SEMANTIC_COUNT];
> > diff --git a/src/gallium/include/pipe/p_defines.h  
> > b/src/gallium/include/pipe/p_defines.h
> > index 67f48e4..48c182f 100644
> > --- a/src/gallium/include/pipe/p_defines.h
> > +++ b/src/gallium/include/pipe/p_defines.h
> > @@ -404,8 +404,10 @@ enum pipe_flush_flags
> >  #define PIPE_SHADER_VERTEX   0
> >  #define PIPE_SHADER_FRAGMENT 1
> >  #define PIPE_SHADER_GEOMETRY 2
> > -#define PIPE_SHADER_COMPUTE  3
> > -#define PIPE_SHADER_TYPES4
> > +#define PIPE_SHADER_TESSCTRL 3
> > +#define PIPE_SHADER_TESSEVAL 4
> 
> Most of the gallium names are typed out without contractions, ie  
> PIPE_SHADER_TESSELLATION_CONTROL/EVALUATION

Another option, if the length is a concern, is to use the D3D names,
i.e. PIPE_SHADER_HULL and PIPE_SHADER_DOMAIN.

> > +#define PIPE_SHADER_COMPUTE  5
> > +#define PIPE_SHADER_TYPES6
> > /**
> > diff --git a/src/gallium/include/pipe/p_shader_tokens.h  
> > b/src/gallium/include/pipe/p_shader_tokens.h
> > index c14bcbc..776b0d4 100644
> > --- a/src/gallium/include/pipe/p_shader_tokens.h
> > +++ b/src/gallium/include/pipe/p_shader_tokens.h
> > @@ -43,7 +43,9 @@ struct tgsi_header
> >  #define TGSI_PROCESSOR_FRAGMENT  0
> >  #define TGSI_PROCESSOR_VERTEX1
> >  #define TGSI_PROCESSOR_GEOMETRY  2
> > -#define TGSI_PROCESSOR_COMPUTE   3
> > +#define TGSI_PROCESSOR_TESSCTRL  3
> > +#define TGSI_PROCESSOR_TESSEVAL  4
> > +#define TGSI_PROCESSOR_COMPUTE   5
> > struct tgsi_processor
> >  {
> 
> With above niggles fixed
> Reviewed-by: Glenn Kennard 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> 

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

Re: [Mesa-dev] [PATCH 01/27] i965: Define HW-binding table and resource streamer control opcodes

2015-05-03 Thread Pohjolainen, Topi 
On Tue, Apr 28, 2015 at 11:07:58PM +0300, Abdiel Janulgue wrote:
> Signed-off-by: Abdiel Janulgue 
> ---
>  src/mesa/drivers/dri/i965/brw_context.h |  1 +
>  src/mesa/drivers/dri/i965/brw_defines.h | 24 
>  src/mesa/drivers/dri/i965/intel_reg.h   |  3 +++
>  3 files changed, 28 insertions(+)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
> b/src/mesa/drivers/dri/i965/brw_context.h
> index a6d6787..07626af 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.h
> +++ b/src/mesa/drivers/dri/i965/brw_context.h
> @@ -1105,6 +1105,7 @@ struct brw_context
> bool no_simd8;
> bool use_rep_send;
> bool scalar_vs;
> +   bool has_resource_streamer;

This should go to the next patch. Other than that all looks good - I checked
the values against bspec and I couldn't find anything amiss.

Reviewed-by: Topi Pohjolainen 

>  
> /**
>  * Some versions of Gen hardware don't do centroid interpolation correctly
> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h 
> b/src/mesa/drivers/dri/i965/brw_defines.h
> index a97a944..da288d3 100644
> --- a/src/mesa/drivers/dri/i965/brw_defines.h
> +++ b/src/mesa/drivers/dri/i965/brw_defines.h
> @@ -1586,6 +1586,30 @@ enum brw_message_target {
>  #define _3DSTATE_BINDING_TABLE_POINTERS_GS   0x7829 /* GEN7+ */
>  #define _3DSTATE_BINDING_TABLE_POINTERS_PS   0x782A /* GEN7+ */
>  
> +#define _3DSTATE_BINDING_TABLE_POOL_ALLOC   0x7919 /* GEN7.5+ */
> +#define BRW_HW_BINDING_TABLE_ENABLE_SHIFT   11 /* GEN7.5+ */
> +#define BRW_HW_BINDING_TABLE_ENABLE_MASKINTEL_MASK(11, 11)
> +#define BRW_HW_BINDING_TABLE_ON 1
> +#define BRW_HW_BINDING_TABLE_OFF0
> +#define GEN7_HW_BT_MOCS_SHIFT   7
> +#define GEN7_HW_BT_MOCS_MASKINTEL_MASK(10, 7)
> +#define GEN8_HW_BT_MOCS_SHIFT   0
> +#define GEN8_HW_BT_MOCS_MASKINTEL_MASK(6, 0)
> +/* Only required in HSW */
> +#define HSW_HW_BINDING_TABLE_RESERVED   (3 << 5)
> +
> +#define _3DSTATE_BINDING_TABLE_EDIT_VS  0x7843 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_GS  0x7844 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_HS  0x7845 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_DS  0x7846 /* GEN7.5 */
> +#define _3DSTATE_BINDING_TABLE_EDIT_PS  0x7847 /* GEN7.5 */
> +#define BRW_BINDING_TABLE_INDEX_SHIFT   16
> +#define BRW_BINDING_TABLE_INDEX_MASKINTEL_MASK(23, 16)
> +
> +#define BRW_BINDING_TABLE_EDIT_TARGET_ALL   3
> +#define BRW_BINDING_TABLE_EDIT_TARGET_CORE1 2
> +#define BRW_BINDING_TABLE_EDIT_TARGET_CORE0 1
> +
>  #define _3DSTATE_SAMPLER_STATE_POINTERS  0x7802 /* GEN6+ */
>  # define PS_SAMPLER_STATE_CHANGE (1 << 12)
>  # define GS_SAMPLER_STATE_CHANGE (1 << 9)
> diff --git a/src/mesa/drivers/dri/i965/intel_reg.h 
> b/src/mesa/drivers/dri/i965/intel_reg.h
> index 488fb5b..9cdb3ca 100644
> --- a/src/mesa/drivers/dri/i965/intel_reg.h
> +++ b/src/mesa/drivers/dri/i965/intel_reg.h
> @@ -47,6 +47,9 @@
>  /* Load a value from memory into a register.  Only available on Gen7+. */
>  #define GEN7_MI_LOAD_REGISTER_MEM(CMD_MI | (0x29 << 23))
>  # define MI_LOAD_REGISTER_MEM_USE_GGTT   (1 << 22)
> +/* Haswell RS control */
> +#define MI_RS_CONTROL   (CMD_MI | (0x6 << 23))
> +#define MI_RS_STORE_DATA_IMM(CMD_MI | (0x2b << 23))
>  
>  /** @{
>   *
> -- 
> 1.9.1
> 
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 00/19] gallium: basic tessellation support

2015-05-03 Thread Ilia Mirkin 
On Sun, May 3, 2015 at 6:18 AM, Glenn Kennard  wrote:
> On Sat, 02 May 2015 22:16:24 +0200, Ilia Mirkin 
> wrote:
>
>> This series adds tokens and updates some helper gallium functions to
>> know about tessellation. This provides no actual support for
>> tessellation in either core or drivers, however this will make it
>> possible to work on the core and driver pieces without crazy
>> interdependencies, as well as be landed separately and without
>> (direct) dependency.
>>
>> Most of these patches have existed for about a year already, and have
>> been part of my and Marek's trees enabling tessellation in the nvc0
>> and radeonsi drivers. I've taken this opportunity to fix up and fold
>> some of them though.
>>
>> This should be pretty safe to land, since even if I messed something
>> up, having this in-tree will make it easier for others to identify and
>> fix any issues collaboratively.
>>
>> Ilia Mirkin (11):
>>   gallium: add tessellation shader types
>>   gallium: add new PATCHES primitive type
>>   gallium: add new semantics for tessellation
>>   gallium: add interfaces for controlling tess program state
>>   gallium: add tessellation shader properties
>>   gallium: add patch_vertices to draw info
>>   gallium: add set_tess_state to configure default tessellation
>> parameters
>>   tgsi/scan: allow scanning tessellation shaders
>>   tgsi/sanity: set implicit in/out array sizes based on patch sizes
>>   tgsi/ureg: allow ureg_dst to have dimension indices
>>   tgsi/dump: fix declaration printing of tessellation inputs/outputs
>>
>> Marek Olšák (8):
>>   gallium: bump shader input and output limits
>>   trace: implement new tessellation functions
>>   gallium/util: print patch_vertices in util_dump_draw_info
>>   gallium/u_blitter: disable tessellation for all operations
>>   gallium/cso: add support for tessellation shaders
>>   gallium/cso: set NULL shaders at context destruction
>>   gallium: disable tessellation shaders for meta ops
>>   tgsi/ureg: use correct limit for max input count
>>
>>  src/gallium/auxiliary/cso_cache/cso_context.c | 100
>> ++
>>  src/gallium/auxiliary/cso_cache/cso_context.h |  12 
>>  src/gallium/auxiliary/hud/hud_context.c   |   6 ++
>>  src/gallium/auxiliary/postprocess/pp_run.c|   6 ++
>>  src/gallium/auxiliary/tgsi/tgsi_dump.c|  20 +-
>>  src/gallium/auxiliary/tgsi/tgsi_info.c|   4 ++
>>  src/gallium/auxiliary/tgsi/tgsi_sanity.c  |  36 --
>>  src/gallium/auxiliary/tgsi/tgsi_scan.c|   6 +-
>>  src/gallium/auxiliary/tgsi/tgsi_strings.c |  19 -
>>  src/gallium/auxiliary/tgsi/tgsi_strings.h |   2 +-
>>  src/gallium/auxiliary/tgsi/tgsi_ureg.c|  26 ++-
>>  src/gallium/auxiliary/tgsi/tgsi_ureg.h|  59 +--
>>  src/gallium/auxiliary/util/u_blit.c   |   6 ++
>>  src/gallium/auxiliary/util/u_blitter.c|  27 +++
>>  src/gallium/auxiliary/util/u_blitter.h|  16 -
>>  src/gallium/auxiliary/util/u_dump_state.c |   2 +
>>  src/gallium/docs/source/context.rst   |   5 ++
>>  src/gallium/docs/source/tgsi.rst  |  70 ++
>>  src/gallium/drivers/trace/tr_context.c|  26 +++
>>  src/gallium/drivers/trace/tr_dump_state.c |   2 +
>>  src/gallium/include/pipe/p_context.h  |  14 
>>  src/gallium/include/pipe/p_defines.h  |  16 -
>>  src/gallium/include/pipe/p_shader_tokens.h|  18 -
>>  src/gallium/include/pipe/p_state.h|   6 +-
>>  src/mesa/state_tracker/st_cb_bitmap.c |   8 ++-
>>  src/mesa/state_tracker/st_cb_clear.c  |   6 ++
>>  src/mesa/state_tracker/st_cb_drawpixels.c |   8 ++-
>>  src/mesa/state_tracker/st_cb_drawtex.c|   6 ++
>>  28 files changed, 501 insertions(+), 31 deletions(-)
>>
>
> Some minor nits for patches 1, 6 and 7, see separate mails
>
> Patches 2-5, 8-19 are
> Reviewed-by: Glenn Kennard 

Thanks for taking a look! It's easy enough to make the naming changes
you proposed (just sed -i all the patches, and git am them), but I'd
rather not do so repeatedly. Marek, thoughts on Glenn's naming
suggestions?

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

Re: [Mesa-dev] [PATCH 00/19] gallium: basic tessellation support

2015-05-03 Thread Marek Olšák 
Renaming everything would be slightly annoying. I personally prefer
shorter names anyway (TESSC/TESSE, PIPE_SHADER_TESSEVAL, etc.). I
don't think longer names would make any difference in readability.

Marek

On Sun, May 3, 2015 at 5:06 PM, Ilia Mirkin  wrote:
> On Sun, May 3, 2015 at 6:18 AM, Glenn Kennard  wrote:
>> On Sat, 02 May 2015 22:16:24 +0200, Ilia Mirkin 
>> wrote:
>>
>>> This series adds tokens and updates some helper gallium functions to
>>> know about tessellation. This provides no actual support for
>>> tessellation in either core or drivers, however this will make it
>>> possible to work on the core and driver pieces without crazy
>>> interdependencies, as well as be landed separately and without
>>> (direct) dependency.
>>>
>>> Most of these patches have existed for about a year already, and have
>>> been part of my and Marek's trees enabling tessellation in the nvc0
>>> and radeonsi drivers. I've taken this opportunity to fix up and fold
>>> some of them though.
>>>
>>> This should be pretty safe to land, since even if I messed something
>>> up, having this in-tree will make it easier for others to identify and
>>> fix any issues collaboratively.
>>>
>>> Ilia Mirkin (11):
>>>   gallium: add tessellation shader types
>>>   gallium: add new PATCHES primitive type
>>>   gallium: add new semantics for tessellation
>>>   gallium: add interfaces for controlling tess program state
>>>   gallium: add tessellation shader properties
>>>   gallium: add patch_vertices to draw info
>>>   gallium: add set_tess_state to configure default tessellation
>>> parameters
>>>   tgsi/scan: allow scanning tessellation shaders
>>>   tgsi/sanity: set implicit in/out array sizes based on patch sizes
>>>   tgsi/ureg: allow ureg_dst to have dimension indices
>>>   tgsi/dump: fix declaration printing of tessellation inputs/outputs
>>>
>>> Marek Olšák (8):
>>>   gallium: bump shader input and output limits
>>>   trace: implement new tessellation functions
>>>   gallium/util: print patch_vertices in util_dump_draw_info
>>>   gallium/u_blitter: disable tessellation for all operations
>>>   gallium/cso: add support for tessellation shaders
>>>   gallium/cso: set NULL shaders at context destruction
>>>   gallium: disable tessellation shaders for meta ops
>>>   tgsi/ureg: use correct limit for max input count
>>>
>>>  src/gallium/auxiliary/cso_cache/cso_context.c | 100
>>> ++
>>>  src/gallium/auxiliary/cso_cache/cso_context.h |  12 
>>>  src/gallium/auxiliary/hud/hud_context.c   |   6 ++
>>>  src/gallium/auxiliary/postprocess/pp_run.c|   6 ++
>>>  src/gallium/auxiliary/tgsi/tgsi_dump.c|  20 +-
>>>  src/gallium/auxiliary/tgsi/tgsi_info.c|   4 ++
>>>  src/gallium/auxiliary/tgsi/tgsi_sanity.c  |  36 --
>>>  src/gallium/auxiliary/tgsi/tgsi_scan.c|   6 +-
>>>  src/gallium/auxiliary/tgsi/tgsi_strings.c |  19 -
>>>  src/gallium/auxiliary/tgsi/tgsi_strings.h |   2 +-
>>>  src/gallium/auxiliary/tgsi/tgsi_ureg.c|  26 ++-
>>>  src/gallium/auxiliary/tgsi/tgsi_ureg.h|  59 +--
>>>  src/gallium/auxiliary/util/u_blit.c   |   6 ++
>>>  src/gallium/auxiliary/util/u_blitter.c|  27 +++
>>>  src/gallium/auxiliary/util/u_blitter.h|  16 -
>>>  src/gallium/auxiliary/util/u_dump_state.c |   2 +
>>>  src/gallium/docs/source/context.rst   |   5 ++
>>>  src/gallium/docs/source/tgsi.rst  |  70 ++
>>>  src/gallium/drivers/trace/tr_context.c|  26 +++
>>>  src/gallium/drivers/trace/tr_dump_state.c |   2 +
>>>  src/gallium/include/pipe/p_context.h  |  14 
>>>  src/gallium/include/pipe/p_defines.h  |  16 -
>>>  src/gallium/include/pipe/p_shader_tokens.h|  18 -
>>>  src/gallium/include/pipe/p_state.h|   6 +-
>>>  src/mesa/state_tracker/st_cb_bitmap.c |   8 ++-
>>>  src/mesa/state_tracker/st_cb_clear.c  |   6 ++
>>>  src/mesa/state_tracker/st_cb_drawpixels.c |   8 ++-
>>>  src/mesa/state_tracker/st_cb_drawtex.c|   6 ++
>>>  28 files changed, 501 insertions(+), 31 deletions(-)
>>>
>>
>> Some minor nits for patches 1, 6 and 7, see separate mails
>>
>> Patches 2-5, 8-19 are
>> Reviewed-by: Glenn Kennard 
>
> Thanks for taking a look! It's easy enough to make the naming changes
> you proposed (just sed -i all the patches, and git am them), but I'd
> rather not do so repeatedly. Marek, thoughts on Glenn's naming
> suggestions?
>
>   -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 05/13] i965: Fix the untyped surface opcodes to deal with indirect surface access.

2015-05-03 Thread Pohjolainen, Topi 
On Sat, Mar 07, 2015 at 01:06:03AM -0800, Kenneth Graunke wrote:
> On Friday, March 06, 2015 03:20:26 PM Pohjolainen, Topi wrote:
> > On Fri, Mar 06, 2015 at 02:46:51PM +0200, Francisco Jerez wrote:
> > > "Pohjolainen, Topi"  writes:
> > > 
> > > > On Fri, Mar 06, 2015 at 02:29:15PM +0200, Francisco Jerez wrote:
> > > >> "Pohjolainen, Topi"  writes:
> > > >> 
> > > >> > On Fri, Feb 27, 2015 at 05:34:48PM +0200, Francisco Jerez wrote:
> > > >> >> Change brw_untyped_atomic() and brw_untyped_surface_read() to take 
> > > >> >> the
> > > >> >> surface index as a register instead of a constant and to use
> > > >> >> brw_send_indirect_message() to emit the indirect variant of send 
> > > >> >> with
> > > >> >> a dynamically calculated message descriptor.  This will be required 
> > > >> >> to
> > > >> >> support variable indexing of image arrays for
> > > >> >> ARB_shader_image_load_store.
> > > >> >> ---
> > > >> >>  src/mesa/drivers/dri/i965/brw_eu.h   |  10 +-
> > > >> >>  src/mesa/drivers/dri/i965/brw_eu_emit.c  | 158 
> > > >> >> +--
> > > >> >>  src/mesa/drivers/dri/i965/brw_fs_generator.cpp   |   4 +-
> > > >> >>  src/mesa/drivers/dri/i965/brw_vec4_generator.cpp |   4 +-
> > > >> >>  4 files changed, 96 insertions(+), 80 deletions(-)
> > > >> >> 
> > > >> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu.h 
> > > >> >> b/src/mesa/drivers/dri/i965/brw_eu.h
> > > >> >> index 87a9f3f..9cc9123 100644
> > > >> >> --- a/src/mesa/drivers/dri/i965/brw_eu.h
> > > >> >> +++ b/src/mesa/drivers/dri/i965/brw_eu.h
> > > >> >> @@ -398,18 +398,18 @@ void brw_CMP(struct brw_compile *p,
> > > >> >>  
> > > >> >>  void
> > > >> >>  brw_untyped_atomic(struct brw_compile *p,
> > > >> >> -   struct brw_reg dest,
> > > >> >> +   struct brw_reg dst,
> > > >> >> struct brw_reg payload,
> > > >> >> +   struct brw_reg surface,
> > > >> >> unsigned atomic_op,
> > > >> >> -   unsigned bind_table_index,
> > > >> >> unsigned msg_length,
> > > >> >> bool response_expected);
> > > >> >>  
> > > >> >>  void
> > > >> >>  brw_untyped_surface_read(struct brw_compile *p,
> > > >> >> - struct brw_reg dest,
> > > >> >> - struct brw_reg mrf,
> > > >> >> - unsigned bind_table_index,
> > > >> >> + struct brw_reg dst,
> > > >> >> + struct brw_reg payload,
> > > >> >> + struct brw_reg surface,
> > > >> >>   unsigned msg_length,
> > > >> >>   unsigned num_channels);
> > > >> >>  
> > > >> >> diff --git a/src/mesa/drivers/dri/i965/brw_eu_emit.c 
> > > >> >> b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > >> >> index 0b655d4..34695bf 100644
> > > >> >> --- a/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > >> >> +++ b/src/mesa/drivers/dri/i965/brw_eu_emit.c
> > > >> >> @@ -2518,6 +2518,48 @@ brw_send_indirect_message(struct brw_compile 
> > > >> >> *p,
> > > >> >> return setup;
> > > >> >>  }
> > > >> >>  
> > > >> >> +static struct brw_inst *
> > > >> >> +brw_send_indirect_surface_message(struct brw_compile *p,
> > > >> >> +  unsigned sfid,
> > > >> >> +  struct brw_reg dst,
> > > >> >> +  struct brw_reg payload,
> > > >> >> +  struct brw_reg surface,
> > > >> >> +  unsigned message_len,
> > > >> >> +  unsigned response_len,
> > > >> >> +  bool header_present)
> > > >> >> +{
> > > >> >> +   const struct brw_context *brw = p->brw;
> > > >> >> +   struct brw_inst *insn;
> > > >> >> +
> > > >> >> +   if (surface.file != BRW_IMMEDIATE_VALUE) {
> > > >> >> +  struct brw_reg addr = retype(brw_address_reg(0), 
> > > >> >> BRW_REGISTER_TYPE_UD);
> > > >> >> +
> > > >> >> +  brw_push_insn_state(p);
> > > >> >> +  brw_set_default_access_mode(p, BRW_ALIGN_1);
> > > >> >> +  brw_set_default_mask_control(p, BRW_MASK_DISABLE);
> > > >> >> +  brw_set_default_predicate_control(p, BRW_PREDICATE_NONE);
> > > >> >> +
> > > >> >> +  /* Mask out invalid bits from the surface index to avoid 
> > > >> >> hangs e.g. when
> > > >> >> +   * some surface array is accessed out of bounds.
> > > >> >> +   */
> > > >> >> +  insn = brw_AND(p, addr,
> > > >> >> + suboffset(vec1(retype(surface, 
> > > >> >> BRW_REGISTER_TYPE_UD)),
> > > >> >> +   
> > > >> >> BRW_GET_SWZ(surface.dw1.bits.swizzle, 0)),
> > > >> >> + brw_imm_ud(0xff));
> > > >> >> +
> > > >> >> +  brw_pop_insn_state(p);
> > > >> >> +
> > > >> >> +  surface = addr;
> > > >> >> +   }
> > > >> >> +
> > > >> >> +   insn = brw