[Mesa-dev] [PATCH 1/2] i965: Clean up intel_batchbuffer

Discussion:

[Mesa-dev] [PATCH 1/2] i965: Clean up intel_batchbuffer_init().

Kenneth Graunke

2017-08-11 05:52:46 UTC

Passing screen lets us get the kernel features, devinfo, and bufmgr,
without needing container_of.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102062
Cc: Mauro Rossi <***@gmail.com>
Cc: Tapani Pälli <***@intel.com>
---
src/mesa/drivers/dri/i965/brw_context.c | 2 +-
src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++++++++--------
src/mesa/drivers/dri/i965/intel_batchbuffer.h | 5 ++---
3 files changed, 11 insertions(+), 12 deletions(-)

According to the bug report, something about container_of is causing
things to go haywire. I'm not seeing the problem, but eliminating
that is easy enough and arguably a bit nicer anyway...

diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
index 60b14571ed0..2d8f34f7efa 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -934,7 +934,7 @@ brwCreateContext(gl_api api,

intel_fbo_init(brw);

- intel_batchbuffer_init(&brw->batch, brw->bufmgr, brw->has_llc);
+ intel_batchbuffer_init(screen, &brw->batch);

if (brw->gen >= 6) {
/* Create a new hardware context. Using a hardware context means that
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 66b9a28129e..59488a2f969 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -58,13 +58,13 @@ uint_key_hash(const void *key)
}

void
-intel_batchbuffer_init(struct intel_batchbuffer *batch,
- struct brw_bufmgr *bufmgr,
- bool has_llc)
+intel_batchbuffer_init(struct intel_screen *screen,
+ struct intel_batchbuffer *batch)
{
- struct brw_context *brw = container_of(batch, brw, batch);
+ struct brw_bufmgr *bufmgr = screen->bufmgr;
+ const struct gen_device_info *devinfo = &screen->devinfo;

- if (!has_llc) {
+ if (!devinfo->has_llc) {
batch->cpu_map = malloc(BATCH_SZ);
batch->map = batch->cpu_map;
batch->map_next = batch->cpu_map;
@@ -87,14 +87,14 @@ intel_batchbuffer_init(struct intel_batchbuffer *batch,
}

batch->use_batch_first =
- brw->screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;
+ screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;

/* PIPE_CONTROL needs a w/a but only on gen6 */
batch->valid_reloc_flags = EXEC_OBJECT_WRITE;
- if (brw->gen == 6)
+ if (devinfo->gen == 6)
batch->valid_reloc_flags |= EXEC_OBJECT_NEEDS_GTT;

- intel_batchbuffer_reset(batch, bufmgr, has_llc);
+ intel_batchbuffer_reset(batch, bufmgr, devinfo->has_llc);
}

#define READ_ONCE(x) (*(volatile __typeof__(x) *)&(x))
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
index 4661a2a9f66..99d2747f282 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
@@ -38,9 +38,8 @@ extern "C" {

struct intel_batchbuffer;

-void intel_batchbuffer_init(struct intel_batchbuffer *batch,
- struct brw_bufmgr *bufmgr,
- bool has_llc);
+void intel_batchbuffer_init(struct intel_screen *screen,
+ struct intel_batchbuffer *batch);
void intel_batchbuffer_free(struct intel_batchbuffer *batch);
void intel_batchbuffer_save_state(struct brw_context *brw);
void intel_batchbuffer_reset_to_saved(struct brw_context *brw);

--
2.14.0

Kenneth Graunke

2017-08-11 05:52:47 UTC

Permalink

This should hopefully fix build issues on 32-bit Android-x86.

Cc: Mauro Rossi <***@gmail.com>
Cc: Tapani Pälli <***@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102050
---
src/mesa/drivers/dri/i965/intel_buffer_objects.c | 2 ++
1 file changed, 2 insertions(+)

Mauro, hopefully this helps?

diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
index ee591168283..9afd98edb87 100644
--- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
+++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
@@ -342,6 +342,7 @@ brw_get_buffer_subdata(struct gl_context *ctx,

unsigned int map_flags = MAP_READ;
mem_copy_fn memcpy_fn = memcpy;
+#ifdef USE_SSE4_1
if (!intel_obj->buffer->cache_coherent && cpu_has_sse4_1) {
/* Rather than acquire a new WB mmaping of the buffer object and pull
* it into the CPU cache, keep using the WC mmap that we have for writes,
@@ -350,6 +351,7 @@ brw_get_buffer_subdata(struct gl_context *ctx,
map_flags |= MAP_COHERENT;
memcpy_fn = (mem_copy_fn) _mesa_streaming_load_memcpy;
}
+#endif

void *map = brw_bo_map(brw, intel_obj->buffer, map_flags);
if (unlikely(!map)) {

--
2.14.0

Tapani Pälli

2017-08-11 06:50:30 UTC

Permalink

I do wonder what the target machine is (I haven't seen one that would
not have ARCH_X86_HAVE_SSE4_1 true, both 32bit and 64bit) but falling
back to memcpy makes perfect sense without USE_SSE4_1;

Post by Kenneth Graunke
This should hopefully fix build issues on 32-bit Android-x86.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102050
---
src/mesa/drivers/dri/i965/intel_buffer_objects.c | 2 ++
1 file changed, 2 insertions(+)
Mauro, hopefully this helps?
diff --git a/src/mesa/drivers/dri/i965/intel_buffer_objects.c b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
index ee591168283..9afd98edb87 100644
--- a/src/mesa/drivers/dri/i965/intel_buffer_objects.c
+++ b/src/mesa/drivers/dri/i965/intel_buffer_objects.c
@@ -342,6 +342,7 @@ brw_get_buffer_subdata(struct gl_context *ctx,
unsigned int map_flags = MAP_READ;
mem_copy_fn memcpy_fn = memcpy;
+#ifdef USE_SSE4_1
if (!intel_obj->buffer->cache_coherent && cpu_has_sse4_1) {
/* Rather than acquire a new WB mmaping of the buffer object and pull
* it into the CPU cache, keep using the WC mmap that we have for writes,
@@ -350,6 +351,7 @@ brw_get_buffer_subdata(struct gl_context *ctx,
map_flags |= MAP_COHERENT;
memcpy_fn = (mem_copy_fn) _mesa_streaming_load_memcpy;
}
+#endif
void *map = brw_bo_map(brw, intel_obj->buffer, map_flags);
if (unlikely(!map)) {

Kenneth Graunke

2017-08-11 17:14:15 UTC

Permalink

Post by Tapani PÃ¤lli
I do wonder what the target machine is (I haven't seen one that would
not have ARCH_X86_HAVE_SSE4_1 true, both 32bit and 64bit) but falling
back to memcpy makes perfect sense without USE_SSE4_1;

Mauro Rossi

2017-08-12 08:25:25 UTC

Permalink

I agree - you really want SSE 4.1 support for decent performance on Atoms.
The MOVNTDQA texture upload path, for example, gave us some really big
performance gains on some ChromeOS workloads.

So, while we should patch this, it'd still be worth figuring out if it's
possible to enable it in Android-x86.

--Ken

Hi,

I checked on nougat-x86 build 32bit and the patches are Ok.

The users of android-x86 have the option to build with silvermont.mk and
later
to have an optimized build, but we currently try to support SSE3 devices.
GNU/Linux distributions for desktop, like their kernel, may have that goal
too.

Thanks
M.

Kenneth Graunke

2017-08-12 08:40:59 UTC

Permalink

Post by Kenneth Graunke

I agree - you really want SSE 4.1 support for decent performance on Atoms.
The MOVNTDQA texture upload path, for example, gave us some really big
performance gains on some ChromeOS workloads.
So, while we should patch this, it'd still be worth figuring out if it's
possible to enable it in Android-x86.
--Ken
Hi,
I checked on nougat-x86 build 32bit and the patches are Ok.
The users of android-x86 have the option to build with silvermont.mk and
later
to have an optimized build, but we currently try to support SSE3 devices.
GNU/Linux distributions for desktop, like their kernel, may have that goal
too.
Thanks
M.

Well, right...but we do guard these with runtime checks. So you should
be able to build in optional SSE 4.1 support but still have such a build
work on SSE3-only hardware...

I'm glad to hear it's an option, at least...

Chris Wilson

2017-08-11 09:27:19 UTC

Permalink

Quoting Kenneth Graunke (2017-08-11 06:52:47)

Which symbol isn't there? I presume cpu_has_sse4_1 is defined to 0 if
!USE_SSE4_1 and so we could do

#ifndef USE_SSE4_1
static inline void *_mesa_streaming_load_memcpy(void *dst, void *src, size_t size)
{
unreachable("Illegal CPU");
}
#endif

The compiler should be quiet happy to do the dead-code elimination for
one less ifdef in the code. Worth it?
-Chris

Grazvydas Ignotas

2017-08-11 11:23:31 UTC

Permalink

I don't think anything is defining USE_SSE4_1 in mesa, so you are
disabling this code for everyone.

Post by Kenneth Graunke
if (!intel_obj->buffer->cache_coherent && cpu_has_sse4_1) {
/* Rather than acquire a new WB mmaping of the buffer object and pull
* it into the CPU cache, keep using the WC mmap that we have for writes,
@@ -350,6 +351,7 @@ brw_get_buffer_subdata(struct gl_context *ctx,
map_flags |= MAP_COHERENT;
memcpy_fn = (mem_copy_fn) _mesa_streaming_load_memcpy;
}
+#endif
void *map = brw_bo_map(brw, intel_obj->buffer, map_flags);
if (unlikely(!map)) {
--
2.14.0

Gražvydas

Tapani Pälli

2017-08-11 11:31:16 UTC

Permalink

Post by Grazvydas Ignotas

I don't think anything is defining USE_SSE4_1 in mesa, so you are
disabling this code for everyone.

Right .. it should be USE_SSE41, r-b with that

Post by Grazvydas Ignotas

Gražvydas
_______________________________________________
mesa-dev mailing list
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Emil Velikov

2017-08-11 12:16:04 UTC

Permalink

Post by Tapani PÃ¤lli

Post by Grazvydas Ignotas

I don't think anything is defining USE_SSE4_1 in mesa, so you are
disabling this code for everyone.

Right .. it should be USE_SSE41, r-b with that

Hmm you've beat me to it - yes USE_SSE41 it is. With that:
Reviewed-by: Emil Velikov <***@collabora.com>

Fwiw: The construct can be seen elsewhere - might want to move to a
helper at some later stage.

-Emil

Kenneth Graunke

2017-08-11 17:12:38 UTC

Permalink

Post by Grazvydas Ignotas

I don't think anything is defining USE_SSE4_1 in mesa, so you are
disabling this code for everyone.

Wow. That's embarassing - I noted this and fixed it for the commit
message, but not the actual code. Of course, it's annoying that the
runtime check is 4_1 and the preprocessor check is 41...

Thanks for catching this! Fixed locally.

--Ken

Tapani Pälli

2017-08-11 06:11:49 UTC

Permalink

heh I've just sent a patch that passes context (to get rid of
container_of) but I can see that screen is enough, thanks for fixing this!

Post by Kenneth Graunke
Passing screen lets us get the kernel features, devinfo, and bufmgr,
without needing container_of.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102062
---
src/mesa/drivers/dri/i965/brw_context.c | 2 +-
src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++++++++--------
src/mesa/drivers/dri/i965/intel_batchbuffer.h | 5 ++---
3 files changed, 11 insertions(+), 12 deletions(-)
According to the bug report, something about container_of is causing
things to go haywire. I'm not seeing the problem, but eliminating
that is easy enough and arguably a bit nicer anyway...
diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
index 60b14571ed0..2d8f34f7efa 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -934,7 +934,7 @@ brwCreateContext(gl_api api,
intel_fbo_init(brw);
- intel_batchbuffer_init(&brw->batch, brw->bufmgr, brw->has_llc);
+ intel_batchbuffer_init(screen, &brw->batch);
if (brw->gen >= 6) {
/* Create a new hardware context. Using a hardware context means that
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 66b9a28129e..59488a2f969 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -58,13 +58,13 @@ uint_key_hash(const void *key)
}
void
-intel_batchbuffer_init(struct intel_batchbuffer *batch,
- struct brw_bufmgr *bufmgr,
- bool has_llc)
+intel_batchbuffer_init(struct intel_screen *screen,
+ struct intel_batchbuffer *batch)
{
- struct brw_context *brw = container_of(batch, brw, batch);
+ struct brw_bufmgr *bufmgr = screen->bufmgr;
+ const struct gen_device_info *devinfo = &screen->devinfo;
- if (!has_llc) {
+ if (!devinfo->has_llc) {
batch->cpu_map = malloc(BATCH_SZ);
batch->map = batch->cpu_map;
batch->map_next = batch->cpu_map;
@@ -87,14 +87,14 @@ intel_batchbuffer_init(struct intel_batchbuffer *batch,
}
batch->use_batch_first =
- brw->screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;
+ screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;
/* PIPE_CONTROL needs a w/a but only on gen6 */
batch->valid_reloc_flags = EXEC_OBJECT_WRITE;
- if (brw->gen == 6)
+ if (devinfo->gen == 6)
batch->valid_reloc_flags |= EXEC_OBJECT_NEEDS_GTT;
- intel_batchbuffer_reset(batch, bufmgr, has_llc);
+ intel_batchbuffer_reset(batch, bufmgr, devinfo->has_llc);
}
#define READ_ONCE(x) (*(volatile __typeof__(x) *)&(x))
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
index 4661a2a9f66..99d2747f282 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
@@ -38,9 +38,8 @@ extern "C" {
struct intel_batchbuffer;
-void intel_batchbuffer_init(struct intel_batchbuffer *batch,
- struct brw_bufmgr *bufmgr,
- bool has_llc);
+void intel_batchbuffer_init(struct intel_screen *screen,
+ struct intel_batchbuffer *batch);
void intel_batchbuffer_free(struct intel_batchbuffer *batch);
void intel_batchbuffer_save_state(struct brw_context *brw);
void intel_batchbuffer_reset_to_saved(struct brw_context *brw);

Emil Velikov

2017-08-11 10:41:31 UTC

Permalink

Post by Kenneth Graunke
Passing screen lets us get the kernel features, devinfo, and bufmgr,
without needing container_of.

Hmm it seems to be crashing since sample (aka brw) cannot be NULL.
Which seems to be the case here.

Reviewed-by: Emil Velikov <***@collabora.com>

One could fix the macro, but will need to address a handful of others
that depend on it first.
Might give it a stab if I get the long and boring flight tomorrow :-)

-Emil

Iago Toral

2017-08-11 11:26:50 UTC

Permalink

Post by Kenneth Graunke
Passing screen lets us get the kernel features, devinfo, and bufmgr,
without needing container_of.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=102062
---
src/mesa/drivers/dri/i965/brw_context.c       |  2 +-
src/mesa/drivers/dri/i965/intel_batchbuffer.c | 16 ++++++++--------
src/mesa/drivers/dri/i965/intel_batchbuffer.h |  5 ++---
3 files changed, 11 insertions(+), 12 deletions(-)
According to the bug report, something about container_of is causing
things to go haywire.  I'm not seeing the problem, but eliminating
that is easy enough and arguably a bit nicer anyway...
diff --git a/src/mesa/drivers/dri/i965/brw_context.c
b/src/mesa/drivers/dri/i965/brw_context.c
index 60b14571ed0..2d8f34f7efa 100644
--- a/src/mesa/drivers/dri/i965/brw_context.c
+++ b/src/mesa/drivers/dri/i965/brw_context.c
@@ -934,7 +934,7 @@ brwCreateContext(gl_api api,

    intel_fbo_init(brw);

-   intel_batchbuffer_init(&brw->batch, brw->bufmgr, brw->has_llc);
+   intel_batchbuffer_init(screen, &brw->batch);

    if (brw->gen >= 6) {
       /* Create a new hardware context.  Using a hardware context
means that
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 66b9a28129e..59488a2f969 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -58,13 +58,13 @@ uint_key_hash(const void *key)
}

void
-intel_batchbuffer_init(struct intel_batchbuffer *batch,
-                       struct brw_bufmgr *bufmgr,
-                       bool has_llc)
+intel_batchbuffer_init(struct intel_screen *screen,
+                       struct intel_batchbuffer *batch)
{
-   struct brw_context *brw = container_of(batch, brw, batch);
+   struct brw_bufmgr *bufmgr = screen->bufmgr;
+   const struct gen_device_info *devinfo = &screen->devinfo;

-   if (!has_llc) {
+   if (!devinfo->has_llc) {
       batch->cpu_map = malloc(BATCH_SZ);
       batch->map = batch->cpu_map;
       batch->map_next = batch->cpu_map;
@@ -87,14 +87,14 @@ intel_batchbuffer_init(struct intel_batchbuffer *batch,
    }

    batch->use_batch_first =
-      brw->screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;
+      screen->kernel_features & KERNEL_ALLOWS_EXEC_BATCH_FIRST;

    /* PIPE_CONTROL needs a w/a but only on gen6 */
    batch->valid_reloc_flags = EXEC_OBJECT_WRITE;
-   if (brw->gen == 6)
+   if (devinfo->gen == 6)
       batch->valid_reloc_flags |= EXEC_OBJECT_NEEDS_GTT;

-   intel_batchbuffer_reset(batch, bufmgr, has_llc);
+   intel_batchbuffer_reset(batch, bufmgr, devinfo->has_llc);
}

#define READ_ONCE(x) (*(volatile __typeof__(x) *)&(x))
diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
index 4661a2a9f66..99d2747f282 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.h
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.h
@@ -38,9 +38,8 @@ extern "C" {

struct intel_batchbuffer;

-void intel_batchbuffer_init(struct intel_batchbuffer *batch,
-                            struct brw_bufmgr *bufmgr,
-                            bool has_llc);
+void intel_batchbuffer_init(struct intel_screen *screen,
+                            struct intel_batchbuffer *batch);
void intel_batchbuffer_free(struct intel_batchbuffer *batch);
void intel_batchbuffer_save_state(struct brw_context *brw);
void intel_batchbuffer_reset_to_saved(struct brw_context *brw);