| Message ID | 1415042340-26404-1-git-send-email-bradley.d.volkin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Ping on this series. They're related to the batch copy series, but the changes are valid and tests should still pass even without the kernel changes being merged. On Mon, Nov 03, 2014 at 11:18:59AM -0800, Volkin, Bradley D wrote: > From: Brad Volkin <bradley.d.volkin@intel.com> > > The size of the batch buffer passed to the kernel is significantly > larger than the size of the batch buffer passed to the function. A > proposed optimization as part of the batch copy kernel series is to > use batch_len for the copy and parse operations, which leads to a > false "batch without MI_BATCH_BUFFER_END" failure for this test. > > To fix this, modify the test to set batch_start_offset and batch_len > such that they define the range of actual commands in the batch, > including a few of the surrounding nops for alignment purposes. > > v2: update batch_start_offset as well > > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> > --- > tests/gem_exec_parse.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c > index 1dc9103..e48b83a 100644 > --- a/tests/gem_exec_parse.c > +++ b/tests/gem_exec_parse.c > @@ -144,16 +144,18 @@ static void exec_split_batch(int fd, uint32_t *cmds, > struct drm_i915_gem_exec_object2 objs[1]; > uint32_t cmd_bo; > uint32_t noop[1024] = { 0 }; > + const int alloc_size = 4096 * 2; > + const int actual_start_offset = 4096-sizeof(uint32_t); > > // Allocate and fill a 2-page batch with noops > - cmd_bo = gem_create(fd, 4096 * 2); > + cmd_bo = gem_create(fd, alloc_size); > gem_write(fd, cmd_bo, 0, noop, sizeof(noop)); > gem_write(fd, cmd_bo, 4096, noop, sizeof(noop)); > > // Write the provided commands such that the first dword > // of the command buffer is the last dword of the first > // page (i.e. the command is split across the two pages). > - gem_write(fd, cmd_bo, 4096-sizeof(uint32_t), cmds, size); > + gem_write(fd, cmd_bo, actual_start_offset, cmds, size); > > objs[0].handle = cmd_bo; > objs[0].relocation_count = 0; > @@ -166,8 +168,14 @@ static void exec_split_batch(int fd, uint32_t *cmds, > > execbuf.buffers_ptr = (uintptr_t)objs; > execbuf.buffer_count = 1; > - execbuf.batch_start_offset = 0; > - execbuf.batch_len = size; > + // NB: We want batch_start_offset and batch_len to point to the block > + // of the actual commands (i.e. at the last dword of the first page), > + // but have to adjust both the start offset and length to meet the > + // kernel driver's requirements on the alignment of those fields. > + execbuf.batch_start_offset = actual_start_offset & ~0x7; > + execbuf.batch_len = > + ALIGN(size + actual_start_offset - execbuf.batch_start_offset, > + 0x8); > execbuf.cliprects_ptr = 0; > execbuf.num_cliprects = 0; > execbuf.DR1 = 0; > -- > 1.9.1 >
On Thu, Nov 06, 2014 at 02:39:53PM -0800, Volkin, Bradley D wrote: > Ping on this series. They're related to the batch copy series, but > the changes are valid and tests should still pass even without the > kernel changes being merged. Merged - I kinda forgotten that you don't have commit access. > > On Mon, Nov 03, 2014 at 11:18:59AM -0800, Volkin, Bradley D wrote: > > From: Brad Volkin <bradley.d.volkin@intel.com> > > > > The size of the batch buffer passed to the kernel is significantly > > larger than the size of the batch buffer passed to the function. A > > proposed optimization as part of the batch copy kernel series is to > > use batch_len for the copy and parse operations, which leads to a > > false "batch without MI_BATCH_BUFFER_END" failure for this test. > > > > To fix this, modify the test to set batch_start_offset and batch_len > > such that they define the range of actual commands in the batch, > > including a few of the surrounding nops for alignment purposes. > > > > v2: update batch_start_offset as well > > > > Signed-off-by: Brad Volkin <bradley.d.volkin@intel.com> > > --- > > tests/gem_exec_parse.c | 16 ++++++++++++---- > > 1 file changed, 12 insertions(+), 4 deletions(-) > > > > diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c > > index 1dc9103..e48b83a 100644 > > --- a/tests/gem_exec_parse.c > > +++ b/tests/gem_exec_parse.c > > @@ -144,16 +144,18 @@ static void exec_split_batch(int fd, uint32_t *cmds, > > struct drm_i915_gem_exec_object2 objs[1]; > > uint32_t cmd_bo; > > uint32_t noop[1024] = { 0 }; > > + const int alloc_size = 4096 * 2; > > + const int actual_start_offset = 4096-sizeof(uint32_t); > > > > // Allocate and fill a 2-page batch with noops > > - cmd_bo = gem_create(fd, 4096 * 2); > > + cmd_bo = gem_create(fd, alloc_size); > > gem_write(fd, cmd_bo, 0, noop, sizeof(noop)); > > gem_write(fd, cmd_bo, 4096, noop, sizeof(noop)); > > > > // Write the provided commands such that the first dword > > // of the command buffer is the last dword of the first > > // page (i.e. the command is split across the two pages). > > - gem_write(fd, cmd_bo, 4096-sizeof(uint32_t), cmds, size); > > + gem_write(fd, cmd_bo, actual_start_offset, cmds, size); > > > > objs[0].handle = cmd_bo; > > objs[0].relocation_count = 0; > > @@ -166,8 +168,14 @@ static void exec_split_batch(int fd, uint32_t *cmds, > > > > execbuf.buffers_ptr = (uintptr_t)objs; > > execbuf.buffer_count = 1; > > - execbuf.batch_start_offset = 0; > > - execbuf.batch_len = size; > > + // NB: We want batch_start_offset and batch_len to point to the block > > + // of the actual commands (i.e. at the last dword of the first page), > > + // but have to adjust both the start offset and length to meet the > > + // kernel driver's requirements on the alignment of those fields. Also pushed a patch on top to change all the comments to C-style, just for my OCD ;-) -Daniel > > + execbuf.batch_start_offset = actual_start_offset & ~0x7; > > + execbuf.batch_len = > > + ALIGN(size + actual_start_offset - execbuf.batch_start_offset, > > + 0x8); > > execbuf.cliprects_ptr = 0; > > execbuf.num_cliprects = 0; > > execbuf.DR1 = 0; > > -- > > 1.9.1 > > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/tests/gem_exec_parse.c b/tests/gem_exec_parse.c index 1dc9103..e48b83a 100644 --- a/tests/gem_exec_parse.c +++ b/tests/gem_exec_parse.c @@ -144,16 +144,18 @@ static void exec_split_batch(int fd, uint32_t *cmds, struct drm_i915_gem_exec_object2 objs[1]; uint32_t cmd_bo; uint32_t noop[1024] = { 0 }; + const int alloc_size = 4096 * 2; + const int actual_start_offset = 4096-sizeof(uint32_t); // Allocate and fill a 2-page batch with noops - cmd_bo = gem_create(fd, 4096 * 2); + cmd_bo = gem_create(fd, alloc_size); gem_write(fd, cmd_bo, 0, noop, sizeof(noop)); gem_write(fd, cmd_bo, 4096, noop, sizeof(noop)); // Write the provided commands such that the first dword // of the command buffer is the last dword of the first // page (i.e. the command is split across the two pages). - gem_write(fd, cmd_bo, 4096-sizeof(uint32_t), cmds, size); + gem_write(fd, cmd_bo, actual_start_offset, cmds, size); objs[0].handle = cmd_bo; objs[0].relocation_count = 0; @@ -166,8 +168,14 @@ static void exec_split_batch(int fd, uint32_t *cmds, execbuf.buffers_ptr = (uintptr_t)objs; execbuf.buffer_count = 1; - execbuf.batch_start_offset = 0; - execbuf.batch_len = size; + // NB: We want batch_start_offset and batch_len to point to the block + // of the actual commands (i.e. at the last dword of the first page), + // but have to adjust both the start offset and length to meet the + // kernel driver's requirements on the alignment of those fields. + execbuf.batch_start_offset = actual_start_offset & ~0x7; + execbuf.batch_len = + ALIGN(size + actual_start_offset - execbuf.batch_start_offset, + 0x8); execbuf.cliprects_ptr = 0; execbuf.num_cliprects = 0; execbuf.DR1 = 0;