[igt-dev] [PATCH i-g-t] lib/gpgpu_fill: Adding missing configuration parameters for gpgpu_fill function

[Daniele Ceraolo Spurio]

+ igt-dev

On 21/03/18 07:23, Katarzyna Dec wrote:
> During debugging gpgpu_fill test on various platforms, I found out few
> things that can affect newer gens:

this is slightly confusing, as you start with saying that you've found 
something but then below you start with what you've changed. I'd reword 
to make it clearer what were the issues and how they've been fixed.

>    Seting number of threads in TS in gen8_fill_interface_descriptor to 1.
> This field was omitted in earlier platforms (apparently without any side
> effects). Not setting it for newer platforms results in GPU hang.
>    Adding pipeline selection mask to gen9_gpgpu_fillfunc, which is
> necessary from SKL.
>    Changes gen7_emit_interface_descriptor_load to gen8 version.

This last point applies to the gen9 fillfunc only. I'd also mention that 
the gen8 interface descriptor applies to gen9+ as well for extra clarity.

> 
> 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>
> ---
>   lib/gpgpu_fill.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/gpgpu_fill.c b/lib/gpgpu_fill.c
> index 4d98643d..e7a1edaa 100644
> --- a/lib/gpgpu_fill.c
> +++ b/lib/gpgpu_fill.c
> @@ -336,6 +336,8 @@ gen8_fill_interface_descriptor(struct intel_batchbuffer *batch, struct igt_buf *
>   	idd->desc5.constant_urb_entry_read_offset = 0;
>   	idd->desc5.constant_urb_entry_read_length = 1; /* grf 1 */
>   
> +	idd->desc6.num_threads_in_tg = 1;
> +

1 seems like a safe value which applies to all platforms and it doesn't 
change execution time (gem_gpgpu_fill still completes in ~0.2s on SKL 
with this change applied) so it seems reasonable to me to use it, but 
it'd be nice to get an ack by someone more experienced with the gpgpu 
pipeline.

>   	return offset;
>   }
>   
> @@ -790,12 +792,13 @@ gen9_gpgpu_fillfunc(struct intel_batchbuffer *batch,
>   	batch->ptr = batch->buffer;
>   
>   	/* GPGPU pipeline */
> -	OUT_BATCH(GEN7_PIPELINE_SELECT | PIPELINE_SELECT_GPGPU);
> +	OUT_BATCH(GEN7_PIPELINE_SELECT | GEN9_PIPELINE_SELECTION_MASK |
> +	PIPELINE_SELECT_GPGPU);

need more indent here.

Apart from the various nitpicks the change looks good to me.

Daniele

>   
>   	gen9_emit_state_base_address(batch);
>   	gen8_emit_vfe_state_gpgpu(batch);
>   	gen7_emit_curbe_load(batch, curbe_buffer);
> -	gen7_emit_interface_descriptor_load(batch, interface_descriptor);
> +	gen8_emit_interface_descriptor_load(batch, interface_descriptor);
>   	gen8_emit_gpgpu_walk(batch, x, y, width, height);
>   
>   	OUT_BATCH(MI_BATCH_BUFFER_END);
>