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

["Szwichtenberg, Radoslaw" <radoslaw.szwichtenberg@intel.com>]

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@intel.com>
> Cc: Lukasz Kalamarz <lukasz.kalamarz@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev