On Mon, Mar 26, 2018 at 02:28:48PM -0700, Daniele Ceraolo Spurio wrote: > + 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. > Actually, this changes will be included in refactoring series and I will explain everything (I hope) in there. > > 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. > Gen9+ functions will be based on Gen9, so this change will be included there there as well. > > > > Signed-off-by: Katarzyna Dec <katarzyna.dec@xxxxxxxxx> > > Cc: Lukasz Kalamarz <lukasz.kalamarz@xxxxxxxxx> > > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx> > > --- > > 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. > Missing seting number of threads in thread group seems like a 'bug' here. According to documentation from BDW this field cannot be set to 0. We cannot be sure that it is not zero unless we set it explicitly. > > 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 > Refactoring of these libraries will be a good oportunity to adjust code style :) Thanks for important feedback! Kasia > > 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); > > _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
