Re: [Mesa-dev] [PATCH] i965: Fix batch-last mode to properly swap BOs.

2018-06-04 Thread Lionel Landwerlin 

Reviewed-by: Lionel Landwerlin 

On 04/06/18 11:18, Kenneth Graunke wrote:

On pre-4.13 kernels, which don't support I915_EXEC_BATCH_FIRST, we move
the validation list entry to the end...but incorrectly left the exec_bo
array alone, causing a mismatch where exec_bos[0] no longer corresponded
with validation_list[0] (and similarly for the last entry).

One example of resulting breakage is that we'd update bo->gtt_offset
based on the wrong buffer.  This wreaked total havoc when trying to use
softpin, and likely caused unnecessary relocations in the normal case.

Fixes: 29ba502a4e28471f67e4e904ae503157087efd20 (i965: Use 
I915_EXEC_BATCH_FIRST when available.)
---
  src/mesa/drivers/dri/i965/intel_batchbuffer.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 2cee2376c0e..462789e8cfb 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -798,11 +798,16 @@ submit_batch(struct brw_context *brw, int in_fence_fd, 
int *out_fence_fd)
} else {
   /* Move the batch to the end of the validation list */
   struct drm_i915_gem_exec_object2 tmp;
+ struct brw_bo *tmp_bo;
   const unsigned index = batch->exec_count - 1;
  
   tmp = *entry;

   *entry = batch->validation_list[index];
   batch->validation_list[index] = tmp;
+
+ tmp_bo = batch->exec_bos[0];
+ batch->exec_bos[0] = batch->exec_bos[index];
+ batch->exec_bos[index] = tmp_bo;
}
  
ret = execbuffer(dri_screen->fd, batch, brw->hw_ctx,



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

Re: [Mesa-dev] [PATCH] i965: Fix batch-last mode to properly swap BOs.

2018-06-04 Thread Chris Wilson 
Quoting Kenneth Graunke (2018-06-04 11:18:37)
> On pre-4.13 kernels, which don't support I915_EXEC_BATCH_FIRST, we move
> the validation list entry to the end...but incorrectly left the exec_bo
> array alone, causing a mismatch where exec_bos[0] no longer corresponded
> with validation_list[0] (and similarly for the last entry).
> 
> One example of resulting breakage is that we'd update bo->gtt_offset
> based on the wrong buffer.  This wreaked total havoc when trying to use
> softpin, and likely caused unnecessary relocations in the normal case.
> 
> Fixes: 29ba502a4e28471f67e4e904ae503157087efd20 (i965: Use 
> I915_EXEC_BATCH_FIRST when available.)

Reviewed-by: Chris Wilson 

One thing that may have helped is if we do the post-execbuf processing
in submit_batch; execbuffer() then just becomes stuffing the pointers
into struct drm_i915_gem_execbuffer2 and calling the ioctl. At the
moment, it isn't clear where batch->exec_bos was being used again, as
one expects it to be consumed by submit_batch().
-Chris
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] i965: Fix batch-last mode to properly swap BOs.

2018-06-04 Thread Kenneth Graunke 
On pre-4.13 kernels, which don't support I915_EXEC_BATCH_FIRST, we move
the validation list entry to the end...but incorrectly left the exec_bo
array alone, causing a mismatch where exec_bos[0] no longer corresponded
with validation_list[0] (and similarly for the last entry).

One example of resulting breakage is that we'd update bo->gtt_offset
based on the wrong buffer.  This wreaked total havoc when trying to use
softpin, and likely caused unnecessary relocations in the normal case.

Fixes: 29ba502a4e28471f67e4e904ae503157087efd20 (i965: Use 
I915_EXEC_BATCH_FIRST when available.)
---
 src/mesa/drivers/dri/i965/intel_batchbuffer.c | 5 +
 1 file changed, 5 insertions(+)

diff --git a/src/mesa/drivers/dri/i965/intel_batchbuffer.c 
b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
index 2cee2376c0e..462789e8cfb 100644
--- a/src/mesa/drivers/dri/i965/intel_batchbuffer.c
+++ b/src/mesa/drivers/dri/i965/intel_batchbuffer.c
@@ -798,11 +798,16 @@ submit_batch(struct brw_context *brw, int in_fence_fd, 
int *out_fence_fd)
   } else {
  /* Move the batch to the end of the validation list */
  struct drm_i915_gem_exec_object2 tmp;
+ struct brw_bo *tmp_bo;
  const unsigned index = batch->exec_count - 1;
 
  tmp = *entry;
  *entry = batch->validation_list[index];
  batch->validation_list[index] = tmp;
+
+ tmp_bo = batch->exec_bos[0];
+ batch->exec_bos[0] = batch->exec_bos[index];
+ batch->exec_bos[index] = tmp_bo;
   }
 
   ret = execbuffer(dri_screen->fd, batch, brw->hw_ctx,
-- 
2.17.0

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