[igt-dev] [PATCH i-g-t v7 2/4] lib: Remove duplications in gpu_fill library

[Katarzyna Dec]

On Wed, Apr 11, 2018 at 12:53:24PM +0100, Szwichtenberg, Radoslaw wrote:
> On Wed, 2018-04-11 at 10:14 +0200, Katarzyna Dec wrote:
> > After moving all functions needed for gpgpu and media fill testing
> > there is a lot of duplications which can be removed:
> >   Library media_fill_gen8 and media_fill_gen8lp for CHT was removed,
> > media state flush for !CHT was added to gen7_emit_media_objects.
> >   Many gen8 functions were replaced with gen7 version with devid
> > parameter (gen7_fill_curbe_load, gen7_emit_interface_descriptor,
> > gen7_fill_binding_table, gen7_emit_media_objects). Unified fill kernel
> > function so it is applicable to all gens and both media and gpgpu
> > (merged gen7_fill_media_kernel and gen8_fill_media_kernel).
> >   Duplicated constants like GEN8_MEDIA_VFE_STATE, GEN8_MEDIA_CURBE_LOAD,
> > GEN8_MEDIA_INTERFACE_DESCRIPTOR_LOAD, GEN8_MEDIA_OBJECT were
> > replaced by GEN7 version. However this constants were not removed
> > from gen8_media.h library, because they are used by other tests
> > for Gen8+. More refactoring in this gen*_media.h libraries is needed.
> > 
> > It seems that further unification of *_fillfunc functions will
> > introduce more confusion in understanding what the tests are doing
> > and what were changes between Gens.
> > 
> > v2: Moved some reduntant changes from Move gpgpu/media fill to gpu_fill...
> > to this patch. Applied comments from review.
> > 
> > v3: rebase
> > 
> > Signed-off-by: Katarzyna Dec <katarzyna.dec at intel.com>
> > Cc: Lukasz Kalamarz <lukasz.kalamarz at intel.com>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > 
> > diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> > index 45e65dd7..9c0150c1 100644
> > --- a/lib/Makefile.sources
> > +++ b/lib/Makefile.sources
> > @@ -58,7 +58,6 @@ lib_source_list =	 	\
> >  	media_fill.h            \
> >  	media_fill_gen7.c       \
> >  	media_fill_gen8.c       \
> > -	media_fill_gen8lp.c     \
> >  	media_fill_gen9.c       \
> >  	media_spin.h		\
> >  	media_spin.c		\
> > diff --git a/lib/gpgpu_fill.c b/lib/gpgpu_fill.c
> > index f2765fd6..579ce78d 100644
> > --- a/lib/gpgpu_fill.c
> > +++ b/lib/gpgpu_fill.c
> > @@ -180,7 +180,7 @@ gen8_gpgpu_fillfunc(struct intel_batchbuffer *batch,
> >  	gen8_emit_state_base_address(batch);
> >  	gen8_emit_vfe_state_gpgpu(batch);
> >  	gen7_emit_curbe_load(batch, curbe_buffer);
> > -	gen8_emit_interface_descriptor_load(batch, interface_descriptor);
> > +	gen7_emit_interface_descriptor_load(batch, interface_descriptor);
> >  	gen8_emit_gpgpu_walk(batch, x, y, width, height);
> >  
> >  	OUT_BATCH(MI_BATCH_BUFFER_END);
> > diff --git a/lib/gpu_fill.c b/lib/gpu_fill.c
> > index d7a2dd4c..a53e5533 100644
> > --- a/lib/gpu_fill.c
> > +++ b/lib/gpu_fill.c
> > @@ -142,26 +142,18 @@ gen7_fill_binding_table(struct intel_batchbuffer *batch,
> What do you think about changing name from gen7_fill to gen_fill? This function
> now acts in slightly different way depending on gen.
> >  
> >  	binding_table = batch_alloc(batch, 32, 64);
> >  	offset = batch_offset(batch, binding_table);
> > -	binding_table[0] = gen7_fill_surface_state(batch, dst,
> > +	if (IS_GEN7(batch->devid))
> > +		binding_table[0] = gen7_fill_surface_state(batch, dst,
> >  						GEN7_SURFACEFORMAT_R8_UNORM,
> > 1);
> > +	else
> > +		binding_table[0] = gen8_fill_surface_state(batch, dst,
> > +						GEN8_SURFACEFORMAT_R8_UNORM,
> > 1);
> >  
> >  	return offset;
> >  }
> >  
> >  uint32_t
> > -gen7_fill_media_kernel(struct intel_batchbuffer *batch,
> > -		const uint32_t kernel[][4],
> > -		size_t size)
> > -{
> > -	uint32_t offset;
> > -
> > -	offset = batch_copy(batch, kernel, size, 64);
> > -
> > -	return offset;
> > -}
> > -
> > -uint32_t
> > -gen8_fill_media_kernel(struct intel_batchbuffer *batch,
> > +gen7_fill_kernel(struct intel_batchbuffer *batch,
> Maybe this also could be just gen_fill_kernel if it is very same for all gens
> and also doesn't have anything introduced by genX?
> 
> Otherwise LGTM
It may be a bit confusing. It should not be that hard to add one more patch with                                                                                                                                  
name unification. Generally, the idea with staying with gen7_naming was to show
in media_fillfunc/gpgpu_fillfunc changes between gens, e.g. what did change
in gen8, gen9 etc.
If anyone who will be reviewing this code has similar concerns as you Radek, it
will not be a problem to add another patch :)

Kasia