Re: [PATCH 1/2] tests/gem_exec_parse: fix batch_len setting for cmd-crossing-page

[Daniel Vetter <daniel@ffwll.ch>]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx