Re: [Mesa-dev] [PATCH 06/22] i965: Enable BatchbufferLogger in i965 driver

[Rogovin, Kevin]

>My worry is that batch->bo is not constant for the construction of a single 
>execbuf2.
> If intel_batchbuffer.c runs out of room inside the batch->bo, it will 
> allocate a new one and continue building it.

That will be ok, but if the (fd, GEM BO handle) changes, there is a serious 
issue.

> I'm just mentioning that may not be the same handle as some of the earlier 
> state calls.

This is a serious issue; sighs. The Logger will not miss any GPU state (since 
that is handled at ioctl interception time), but it will get the GL/GLES call 
ID's royally hosed because the api call markers will be on the log associated 
to that (old) (fd, GEM handle) pair.

Futz.

The solution is to add another function to the driver interface that the driver 
calls when it "migrates" stuff from one BO to another (i.e. when it grows a 
batchbuffer to fit in that last draw call).

Good catch on that, I had utterly forgotten that the batchbuffer split lets 
i965 "grow" the batchbuffer to fit in that one last call. All the stuff I have 
run on it so far have not hit that, but there will be loads where it gets hit.

I will fix this ASAP.

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

Re: [Mesa-dev] [PATCH 06/22] i965: Enable BatchbufferLogger in i965 driver

[Chris Wilson]

Quoting Rogovin, Kevin (2017-09-27 15:22:00)
> Hi,
> 
> > Hmm, this needs updating for the batch/state split; 
> 
> This was rebased (and working) against Mesa of Monday, Sept 25, 2017, which I 
> thought already had the batchbuffer split. I had thought the split was 
> already in master... but why would it need to know that the state is on a 
> separate BO? The logger reads into the batchbuffer, in particular, 
> STATE_BASE_ADDRESS, and uses that data to fetch the data structure placed 
> into the direct state jazz (which used to be at the end of the batchbuffer), 
> i.e. the logger chases GPU address to get GEM BO's and offsets into them. It 
> does this by tracking the creation (and destruction) all GEM BO's by 
> intercepting drmIoctl. The Logger chases everything so it can write out all 
> sorts of things, including shader disassembly (shaders are placed on a 
> different GEM BO as well).

No problem, I guessed it was assuming the old behaviour and knew that
everything (more or less) referred to the batch->bo.

> 
> > and of particular note that the batch->bo  is no longer constant. It 
> > shouldn't 
> > matter overall as the contents/offsets are preserved.
> > I would need to go back and see why you need to know the handle before the 
> > submit, 
> > but the obvious solution to me would be to record the submission.
> 
> The system if perfectly fine (and happy) if the value behind batch->bo 
> changes on every execbuffer2. The logger works as follows:
> Application calls pre_call() before issuing a GL API call. That function then 
> has the Logger ask the driver "what is the active batch buffer on this 
> thread?" whose answer is by calling intel_active_batchbuffer (). The active 
> batch buffer is identified by the pair (fd, GEM BO handle). The driver_data 
> pointer is not used by the Logger to track, rather it is for the driver to 
> use to point to whatever data structure it has for tracking batchbuffer (in 
> i965's case the address of brw->batch) so that can tell where it is in a 
> given batchbuffer. Now that the logger has the ID of the batchbuffer, i.e. 
> the pair (fd, GEM BO handle), it then knows to what batchbuffer the GL API 
> call is to be associated and it appends the call information to the log for 
> that batchbuffer. What is also important, is that the system will also work 
> if there are multiple threads in the calling application each with their own 
> context current. 

My worry is that batch->bo is not constant for the construction of a
single execbuf2. If intel_batchbuffer.c runs out of room inside the
batch->bo, it will allocate a new one and continue building it.

> When drmIoctl happens, the logger then trivially extracts the (fd, GEM BO 
> handle) pair of the batchbuffer to be executed and from there knows what log 
> to emit.

I'm just mentioning that may not be the same handle as some of the
earlier state calls.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 06/22] i965: Enable BatchbufferLogger in i965 driver

[Chris Wilson]

Quoting kevin.rogo...@intel.com (2017-09-25 11:34:06)
> +static
> +uint32_t
> +intel_batchbuffer_state(const struct i965_logged_batchbuffer *st)
> +{
> +   struct intel_batchbuffer *batch
> +  = (struct intel_batchbuffer*) st->driver_data;
> +
> +   assert(batch->bo->gem_handle == st->gem_bo);

Hmm, this needs updating for the batch/state split; and of particular
note that the batch->bo is no longer constant. It shouldn't matter
overall as the contents/offsets are preserved.

I would need to go back and see why you need to know the handle before
the submit, but the obvious solution to me would be to record the
submission.

> +   return batch->map_next - batch->map;
> +}
> +
> +static
> +void
> +intel_active_batchbuffer(struct i965_logged_batchbuffer *st)
> +{
> +   GET_CURRENT_CONTEXT(ctx);
> +   struct brw_context *brw = brw_context(ctx);
> +
> +   if(brw != NULL && brw->have_active_batchbuffer) {
> +  st->driver_data = >batch;
> +  st->gem_bo = brw->batch.bo->gem_handle;
> +  st->fd = brw_bufmgr_fd(brw->bufmgr);
> +   } else {
> +  st->driver_data = NULL;
> +  st->gem_bo = -1;

Fwiw, invalid handle is 0.
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 06/22] i965: Enable BatchbufferLogger in i965 driver

[kevin . rogovin]

From: Kevin Rogovin 

The interface for BatchbufferLogger is that it is active
only if it is LD_PRELOAD'ed. Thus, the i965 driver is to
use dlsym to see if it is there, and if so fetch the object
at intel_screen creation.

Signed-off-by: Kevin Rogovin 
---
 src/mesa/drivers/dri/i965/brw_bufmgr.c   | 22 -
 src/mesa/drivers/dri/i965/brw_bufmgr.h   |  8 -
 src/mesa/drivers/dri/i965/brw_context.c  |  2 ++
 src/mesa/drivers/dri/i965/brw_context.h  |  6 
 src/mesa/drivers/dri/i965/intel_screen.c | 55 ++--
 src/mesa/drivers/dri/i965/intel_screen.h |  3 ++
 6 files changed, 91 insertions(+), 5 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.c 
b/src/mesa/drivers/dri/i965/brw_bufmgr.c
index 44aaf02..3473be7 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.c
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.c
@@ -66,6 +66,8 @@
 #include "string.h"
 
 #include "i915_drm.h"
+#include "intel_screen.h"
+#include "tools/i965_batchbuffer_logger.h"
 
 #ifdef HAVE_VALGRIND
 #include 
@@ -105,6 +107,7 @@ struct bo_cache_bucket {
 };
 
 struct brw_bufmgr {
+   struct intel_screen *screen;
int fd;
 
pthread_mutex_t lock;
@@ -619,9 +622,14 @@ bo_unreference_final(struct brw_bo *bo, time_t time)
 {
struct brw_bufmgr *bufmgr = bo->bufmgr;
struct bo_cache_bucket *bucket;
+   struct i965_batchbuffer_logger *bb_logger =
+  bufmgr->screen->batchbuffer_logger;
 
DBG("bo_unreference final: %d (%s)\n", bo->gem_handle, bo->name);
 
+   if(bb_logger != NULL) {
+  bb_logger->aborted_batchbuffer(bb_logger, bufmgr->fd, bo->gem_handle);
+   }
bucket = bucket_for_size(bufmgr, bo->size);
/* Put the buffer into our internal cache for reuse if we can. */
if (bufmgr->bo_reuse && bo->reusable && bucket != NULL &&
@@ -1061,6 +1069,12 @@ brw_bufmgr_destroy(struct brw_bufmgr *bufmgr)
free(bufmgr);
 }
 
+int
+brw_bufmgr_fd(const struct brw_bufmgr *bufmgr)
+{
+   return bufmgr->fd;
+}
+
 static int
 bo_set_tiling_internal(struct brw_bo *bo, uint32_t tiling_mode,
uint32_t stride)
@@ -1334,9 +1348,14 @@ gem_param(int fd, int name)
  * \param fd File descriptor of the opened DRM device.
  */
 struct brw_bufmgr *
-brw_bufmgr_init(struct gen_device_info *devinfo, int fd)
+brw_bufmgr_init(struct intel_screen *screen)
 {
struct brw_bufmgr *bufmgr;
+   struct gen_device_info *devinfo;
+   int fd;
+
+   devinfo = >devinfo;
+   fd = screen->driScrnPriv->fd;
 
bufmgr = calloc(1, sizeof(*bufmgr));
if (bufmgr == NULL)
@@ -1352,6 +1371,7 @@ brw_bufmgr_init(struct gen_device_info *devinfo, int fd)
 * fd so that its namespace does not clash with another.
 */
bufmgr->fd = fd;
+   bufmgr->screen = screen;
 
if (pthread_mutex_init(>lock, NULL) != 0) {
   free(bufmgr);
diff --git a/src/mesa/drivers/dri/i965/brw_bufmgr.h 
b/src/mesa/drivers/dri/i965/brw_bufmgr.h
index de0ba1d..b78907b 100644
--- a/src/mesa/drivers/dri/i965/brw_bufmgr.h
+++ b/src/mesa/drivers/dri/i965/brw_bufmgr.h
@@ -46,6 +46,7 @@ extern "C" {
 
 struct gen_device_info;
 struct brw_context;
+struct intel_screen;
 
 struct brw_bo {
/**
@@ -275,6 +276,11 @@ void brw_bo_wait_rendering(struct brw_bo *bo);
 void brw_bufmgr_destroy(struct brw_bufmgr *bufmgr);
 
 /**
+ * Returns the file descriptor of the buffer manager
+ */
+int brw_bufmgr_fd(const struct brw_bufmgr *bufmgr);
+
+/**
  * Get the current tiling (and resulting swizzling) mode for the bo.
  *
  * \param buf Buffer to get tiling mode for
@@ -313,7 +319,7 @@ int brw_bo_busy(struct brw_bo *bo);
 int brw_bo_madvise(struct brw_bo *bo, int madv);
 
 /* drm_bacon_bufmgr_gem.c */
-struct brw_bufmgr *brw_bufmgr_init(struct gen_device_info *devinfo, int fd);
+struct brw_bufmgr *brw_bufmgr_init(struct intel_screen *screen);
 struct brw_bo *brw_bo_gem_create_from_name(struct brw_bufmgr *bufmgr,
const char *name,
unsigned int handle);
diff --git a/src/mesa/drivers/dri/i965/brw_context.c 
b/src/mesa/drivers/dri/i965/brw_context.c
index ee1badd..d084272 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -1009,6 +1009,7 @@ brwCreateContext(gl_api api,
 
vbo_use_buffer_objects(ctx);
vbo_always_unmap_buffers(ctx);
+   brw->have_active_batchbuffer = true;
 
return true;
 }
@@ -1021,6 +1022,7 @@ intelDestroyContext(__DRIcontext * driContextPriv)
struct gl_context *ctx = >ctx;
const struct gen_device_info *devinfo = >screen->devinfo;
 
+   brw->have_active_batchbuffer = false;
_mesa_meta_free(>ctx);
 
if (INTEL_DEBUG & DEBUG_SHADER_TIME) {
diff --git a/src/mesa/drivers/dri/i965/brw_context.h 
b/src/mesa/drivers/dri/i965/brw_context.h
index e130fbf..2d1f5d8 100644
--- a/src/mesa/drivers/dri/i965/brw_context.h
+++ b/src/mesa/drivers/dri/i965/brw_context.h
@@ -1211,6 +1211,12 @@ struct brw_context