[igt-dev] [PATCH i-g-t v2 1/2] lib/rendercopy_gen7: Reorganizing batch allocations

[Kalamarz, Lukasz]

On Mon, 2018-04-16 at 13:50 -0700, Daniele Ceraolo Spurio wrote:
> 
> On 16/04/18 08:22, Lukasz Kalamarz wrote:
> > This lib was written in a different manner than all other libs,
> > which was causing some issues during refactoring. Previous
> > implementation was allocating data only in a state part of
> > batchbuffer.
> > New implementation take usage of splitting batch into two parts,
> > one for batch commands and second for various states.
> > 
> 
> This commit message is a bit unclear. There are no changes in how
> data 
> is allocated/written as both the old and the new implementations
> split 
> the batch; the difference is that the old style used 2 different 
> pointers (ptr and state) to access the 2 parts at the same time
> while 
> the new implementation re-uses the same pointer (ptr) and only
> access 
> one section at a time.
> 

Fixed that in new series

> Also, if I'm not missing anything after this patch there are no more 
> users of batch->state (grep only returns results in null_state_gen, 
> which has its own definition of intel_batchbuffer), so we can remove
> it 
> from the structure.
> 

Added new patch to remove state from intel_batchbuffer

> > Signed-off-by: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > Cc: Katarzyna Dec <katarzyna.dec at intel.com>
> > Cc: Radoslaw Szwichtenberg <radoslaw.szwichtenberg at intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > ---
> 
> <snip>
> 
> > @@ -535,12 +531,27 @@ void gen7_render_copyfunc(struct
> > intel_batchbuffer *batch,
> >   			  unsigned width, unsigned height,
> >   			  struct igt_buf *dst, unsigned dst_x,
> > unsigned dst_y)
> >   {
> > +	uint32_t ps_binding_table, ps_sampler_off, ps_kernel_off;
> > +	uint32_t blend_state, cc_viewport;
> > +	uint32_t vertex_buffer;
> >   	uint32_t batch_end;
> >   
> >   	intel_batchbuffer_flush_with_context(batch, context);
> >   
> > -	batch->state = &batch->buffer[BATCH_STATE_SPLIT];
> > +	batch->ptr = &batch->buffer[BATCH_STATE_SPLIT];
> >   
> > +
> > +	blend_state = gen7_create_blend_state(batch);
> > +	cc_viewport = gen7_create_cc_viewport(batch);
> > +	ps_sampler_off = gen7_create_sampler(batch);
> > +	ps_kernel_off = batch_copy(batch, ps_kernel,
> > sizeof(ps_kernel), 64);
> > +	vertex_buffer = gen7_create_vertex_buffer(batch,
> > +						  src_x, src_y,
> > +						  dst_x, dst_y,
> > +						  width, height);
> > +	ps_binding_table = gen7_bind_surfaces(batch, src, dst);
> > +
> 
> igt_assert(batch->ptr < &batch->buffer[4095]);
> 

Missed that. Fixed in new version

> > +	batch->ptr = batch->buffer;
> >   	OUT_BATCH(GEN7_PIPELINE_SELECT | PIPELINE_SELECT_3D);
> >   
> >   	gen7_emit_state_base_address(batch);
> > @@ -556,18 +567,18 @@ void gen7_render_copyfunc(struct
> > intel_batchbuffer *batch,
> >   	gen7_emit_wm(batch);
> >   	gen7_emit_streamout(batch);
> >   	gen7_emit_null_depth_buffer(batch);
> > -
> > -	gen7_emit_cc(batch);
> > -        gen7_emit_sampler(batch);
> > +	gen7_emit_cc(batch, blend_state, cc_viewport);
> > +	gen7_emit_sampler(batch, ps_sampler_off);
> >           gen7_emit_sbe(batch);
> > -        gen7_emit_ps(batch);
> > +	gen7_emit_ps(batch, ps_kernel_off);
> >           gen7_emit_vertex_elements(batch);
> > -        gen7_emit_vertex_buffer(batch,
> > -				src_x, src_y, dst_x, dst_y, width,
> > height);
> > -	gen7_emit_binding_table(batch, src, dst);
> > +	gen7_emit_vertex_buffer(batch, src_x, src_y,
> > +				dst_x, dst_y, width,
> > +				height, vertex_buffer);
> > +	gen7_emit_binding_table(batch, src, dst,
> > ps_binding_table);
> >   	gen7_emit_drawing_rectangle(batch, dst);
> >   
> > -        OUT_BATCH(GEN7_3DPRIMITIVE | (7- 2));
> > +	OUT_BATCH(GEN7_3DPRIMITIVE | (7 - 2));
> >           OUT_BATCH(GEN7_3DPRIMITIVE_VERTEX_SEQUENTIAL |
> > _3DPRIM_RECTLIST);
> >           OUT_BATCH(3);
> >           OUT_BATCH(0);
> > 
> 
> It might be worth adding a patch to change the spaces to tabs before 
> this one to have cleaner result. Not a blocker since it isn't
> strictly 
> related to this change.
> Daniele

Created a new patch to fix that.
---
Lukasz