Re: [Mesa-dev] [PATCH 08/20] anv/pipeline: Unify 3DSTATE_GS emission

[Timothy Arceri]

On Mon, 2016-11-14 at 08:46 -0800, Jason Ekstrand wrote:
> 
> 
> On Sat, Nov 12, 2016 at 2:54 PM, Timothy Arceri  bora.com> wrote:
> > On Sat, 2016-11-12 at 13:34 -0800, Jason Ekstrand wrote:
> > 
> > I have two questions and two suggestions below. 
> > 
> > With the suggestions addressed and assuming the answer to both
> > questions is yes, Patch 7-8 are:
> > 
> > Reviewed-by: Timothy Arceri 
> > 
> > > ---
> > >  src/intel/vulkan/gen7_pipeline.c  | 48 +--
> > 
> > >  src/intel/vulkan/gen8_pipeline.c  | 62 +--
> > 
> > > --
> > >  src/intel/vulkan/genX_pipeline_util.h | 73
> > > +++
> > >  3 files changed, 75 insertions(+), 108 deletions(-)
> > >
> > > diff --git a/src/intel/vulkan/gen7_pipeline.c
> > > b/src/intel/vulkan/gen7_pipeline.c
> > > index e604c25..52577f5 100644
> > > --- a/src/intel/vulkan/gen7_pipeline.c
> > > +++ b/src/intel/vulkan/gen7_pipeline.c
> > > @@ -105,53 +105,7 @@ genX(graphics_pipeline_create)(
> > >  #endif
> > >  
> > > emit_3dstate_vs(pipeline);
> > > -
> > > -   const struct brw_gs_prog_data *gs_prog_data =
> > > get_gs_prog_data(pipeline);
> > > -
> > > -   if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_GEOMETRY))
> > {
> > > -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs);
> > > -   } else {
> > > -  const struct anv_shader_bin *gs_bin =
> > > - pipeline->shaders[MESA_SHADER_GEOMETRY];
> > > -
> > > -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs) {
> > > - gs.KernelStartPointer = gs_bin->kernel.offset;
> > > -
> > > - gs.ScratchSpaceBasePointer = (struct anv_address) {
> > > -.bo = anv_scratch_pool_alloc(device, &device-
> > > >scratch_pool,
> > > - MESA_SHADER_GEOMETRY,
> > > - gs_prog_data-
> > > >base.base.total_scratch),
> > > -.offset = 0,
> > > - };
> > > - gs.PerThreadScratchSpace  =
> > > scratch_space(&gs_prog_data->base.base);
> > > -
> > > - gs.OutputVertexSize   = gs_prog_data-
> > > >output_vertex_size_hwords * 2 - 1;
> > > - gs.OutputTopology = gs_prog_data-
> > > >output_topology;
> > > - gs.VertexURBEntryReadLength   = gs_prog_data-
> > > >base.urb_read_length;
> > > - gs.IncludeVertexHandles   = gs_prog_data-
> > > >base.include_vue_handles;
> > > -
> > > - gs.DispatchGRFStartRegisterForURBData =
> > > -gs_prog_data->base.base.dispatch_grf_start_reg;
> > > -
> > > - gs.SamplerCount  =
> > get_sampler_count(gs_bin);
> > > - gs.BindingTableEntryCount=
> > > get_binding_table_entry_count(gs_bin);
> > > -
> > > - gs.MaximumNumberofThreads = devinfo->max_gs_threads 
> > -
> > > 1;
> > > - /* This in the next dword on HSW. */
> > > - gs.ControlDataFormat  = gs_prog_data-
> > > >control_data_format;
> > > - gs.ControlDataHeaderSize  = gs_prog_data-
> > > >control_data_header_size_hwords;
> > > - gs.InstanceControl= MAX2(gs_prog_data-
> > > >invocations, 1) - 1;
> > > - gs.DispatchMode   = gs_prog_data-
> > > >base.dispatch_mode;
> > > - gs.StatisticsEnable   = true;
> > > - gs.IncludePrimitiveID = gs_prog_data-
> > > >include_primitive_id;
> > > -# if (GEN_IS_HASWELL)
> > > - gs.ReorderMode= REORDER_TRAILING;
> > 
> > Shouldn't we have changed REORDER_TRAILING to TRAILING in
> > src/intel/genxml/gen75.xml in the previous patch?
> 
> Yeah, I'll do that.
>  
> > > -# else
> > > - gs.ReorderEnable  = true;
> > > -# endif
> > > - gs.FunctionEnable = true;
> > > -  }
> > > -   }
> > > +   emit_3dstate_gs(pipeline);
> > >  
> > > if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT))
> > {
> > >    anv_batch_emit(&pipeline->batch, GENX(3DSTATE_SBE), sbe);
> > > diff --git a/src/intel/vulkan/gen8_pipeline.c
> > > b/src/intel/vulkan/gen8_pipeline.c
> > > index 1320a13..5816bd4 100644
> > > --- a/src/intel/vulkan/gen8_pipeline.c
> > > +++ b/src/intel/vulkan/gen8_pipeline.c
> > > @@ -53,9 +53,6 @@ genX(graphics_pipeline_create)(
> > >  {
> > > ANV_FROM_HANDLE(anv_device, device, _device);
> > > ANV_FROM_HANDLE(anv_render_pass, pass, pCreateInfo-
> > >renderPass);
> > > -   const struct anv_physical_device *physical_device =
> > > -  &device->instance->physicalDevice;
> > > -   const struct gen_device_info *devinfo = &physical_device-
> > >info;
> > > struct anv_subpass *subpass = &pass->subpasses[pCreateInfo-
> > > >subpass];
> > > struct anv_pipeline *pipeline;
> > > VkResult result;
> > > @@ -112,64 +109,7 @@ genX(graphics_pipeline_create)(
> > >   wm_prog_data ? wm_prog_data->barycentric_interp_modes :
> > 0;
> > > }
> > >  
> > > -   if (!anv_pip

Re: [Mesa-dev] [PATCH 08/20] anv/pipeline: Unify 3DSTATE_GS emission

[Jason Ekstrand]

On Sat, Nov 12, 2016 at 2:54 PM, Timothy Arceri <
timothy.arc...@collabora.com> wrote:

> On Sat, 2016-11-12 at 13:34 -0800, Jason Ekstrand wrote:
>
> I have two questions and two suggestions below.
>
> With the suggestions addressed and assuming the answer to both
> questions is yes, Patch 7-8 are:
>
> Reviewed-by: Timothy Arceri 
>
> > ---
> >  src/intel/vulkan/gen7_pipeline.c  | 48 +--
> >  src/intel/vulkan/gen8_pipeline.c  | 62 +--
> > --
> >  src/intel/vulkan/genX_pipeline_util.h | 73
> > +++
> >  3 files changed, 75 insertions(+), 108 deletions(-)
> >
> > diff --git a/src/intel/vulkan/gen7_pipeline.c
> > b/src/intel/vulkan/gen7_pipeline.c
> > index e604c25..52577f5 100644
> > --- a/src/intel/vulkan/gen7_pipeline.c
> > +++ b/src/intel/vulkan/gen7_pipeline.c
> > @@ -105,53 +105,7 @@ genX(graphics_pipeline_create)(
> >  #endif
> >
> > emit_3dstate_vs(pipeline);
> > -
> > -   const struct brw_gs_prog_data *gs_prog_data =
> > get_gs_prog_data(pipeline);
> > -
> > -   if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_GEOMETRY)) {
> > -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs);
> > -   } else {
> > -  const struct anv_shader_bin *gs_bin =
> > - pipeline->shaders[MESA_SHADER_GEOMETRY];
> > -
> > -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs) {
> > - gs.KernelStartPointer = gs_bin->kernel.offset;
> > -
> > - gs.ScratchSpaceBasePointer = (struct anv_address) {
> > -.bo = anv_scratch_pool_alloc(device, &device-
> > >scratch_pool,
> > - MESA_SHADER_GEOMETRY,
> > - gs_prog_data-
> > >base.base.total_scratch),
> > -.offset = 0,
> > - };
> > - gs.PerThreadScratchSpace  =
> > scratch_space(&gs_prog_data->base.base);
> > -
> > - gs.OutputVertexSize   = gs_prog_data-
> > >output_vertex_size_hwords * 2 - 1;
> > - gs.OutputTopology = gs_prog_data-
> > >output_topology;
> > - gs.VertexURBEntryReadLength   = gs_prog_data-
> > >base.urb_read_length;
> > - gs.IncludeVertexHandles   = gs_prog_data-
> > >base.include_vue_handles;
> > -
> > - gs.DispatchGRFStartRegisterForURBData =
> > -gs_prog_data->base.base.dispatch_grf_start_reg;
> > -
> > - gs.SamplerCount  = get_sampler_count(gs_bin);
> > - gs.BindingTableEntryCount=
> > get_binding_table_entry_count(gs_bin);
> > -
> > - gs.MaximumNumberofThreads = devinfo->max_gs_threads -
> > 1;
> > - /* This in the next dword on HSW. */
> > - gs.ControlDataFormat  = gs_prog_data-
> > >control_data_format;
> > - gs.ControlDataHeaderSize  = gs_prog_data-
> > >control_data_header_size_hwords;
> > - gs.InstanceControl= MAX2(gs_prog_data-
> > >invocations, 1) - 1;
> > - gs.DispatchMode   = gs_prog_data-
> > >base.dispatch_mode;
> > - gs.StatisticsEnable   = true;
> > - gs.IncludePrimitiveID = gs_prog_data-
> > >include_primitive_id;
> > -# if (GEN_IS_HASWELL)
> > - gs.ReorderMode= REORDER_TRAILING;
>
> Shouldn't we have changed REORDER_TRAILING to TRAILING in
> src/intel/genxml/gen75.xml in the previous patch?
>

Yeah, I'll do that.


> > -# else
> > - gs.ReorderEnable  = true;
> > -# endif
> > - gs.FunctionEnable = true;
> > -  }
> > -   }
> > +   emit_3dstate_gs(pipeline);
> >
> > if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT)) {
> >anv_batch_emit(&pipeline->batch, GENX(3DSTATE_SBE), sbe);
> > diff --git a/src/intel/vulkan/gen8_pipeline.c
> > b/src/intel/vulkan/gen8_pipeline.c
> > index 1320a13..5816bd4 100644
> > --- a/src/intel/vulkan/gen8_pipeline.c
> > +++ b/src/intel/vulkan/gen8_pipeline.c
> > @@ -53,9 +53,6 @@ genX(graphics_pipeline_create)(
> >  {
> > ANV_FROM_HANDLE(anv_device, device, _device);
> > ANV_FROM_HANDLE(anv_render_pass, pass, pCreateInfo->renderPass);
> > -   const struct anv_physical_device *physical_device =
> > -  &device->instance->physicalDevice;
> > -   const struct gen_device_info *devinfo = &physical_device->info;
> > struct anv_subpass *subpass = &pass->subpasses[pCreateInfo-
> > >subpass];
> > struct anv_pipeline *pipeline;
> > VkResult result;
> > @@ -112,64 +109,7 @@ genX(graphics_pipeline_create)(
> >   wm_prog_data ? wm_prog_data->barycentric_interp_modes : 0;
> > }
> >
> > -   if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_GEOMETRY)) {
> > -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs);
> > -   } else {
> > -  const struct brw_gs_prog_data *gs_prog_data =
> > get_gs_prog_data(pipeline);
> > -  const struct anv_shader_bin *gs_bin =
> > - pipeline->shaders[MESA_SHADER_GEOMETRY];
> > 

Re: [Mesa-dev] [PATCH 08/20] anv/pipeline: Unify 3DSTATE_GS emission

[Timothy Arceri]

On Sat, 2016-11-12 at 13:34 -0800, Jason Ekstrand wrote:

I have two questions and two suggestions below. 

With the suggestions addressed and assuming the answer to both
questions is yes, Patch 7-8 are:

Reviewed-by: Timothy Arceri 

> ---
>  src/intel/vulkan/gen7_pipeline.c  | 48 +--
>  src/intel/vulkan/gen8_pipeline.c  | 62 +--
> --
>  src/intel/vulkan/genX_pipeline_util.h | 73
> +++
>  3 files changed, 75 insertions(+), 108 deletions(-)
> 
> diff --git a/src/intel/vulkan/gen7_pipeline.c
> b/src/intel/vulkan/gen7_pipeline.c
> index e604c25..52577f5 100644
> --- a/src/intel/vulkan/gen7_pipeline.c
> +++ b/src/intel/vulkan/gen7_pipeline.c
> @@ -105,53 +105,7 @@ genX(graphics_pipeline_create)(
>  #endif
>  
> emit_3dstate_vs(pipeline);
> -
> -   const struct brw_gs_prog_data *gs_prog_data =
> get_gs_prog_data(pipeline);
> -
> -   if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_GEOMETRY)) {
> -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs);
> -   } else {
> -  const struct anv_shader_bin *gs_bin =
> - pipeline->shaders[MESA_SHADER_GEOMETRY];
> -
> -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs) {
> - gs.KernelStartPointer = gs_bin->kernel.offset;
> -
> - gs.ScratchSpaceBasePointer = (struct anv_address) {
> -.bo = anv_scratch_pool_alloc(device, &device-
> >scratch_pool,
> - MESA_SHADER_GEOMETRY,
> - gs_prog_data-
> >base.base.total_scratch),
> -.offset = 0,
> - };
> - gs.PerThreadScratchSpace  =
> scratch_space(&gs_prog_data->base.base);
> -
> - gs.OutputVertexSize   = gs_prog_data-
> >output_vertex_size_hwords * 2 - 1;
> - gs.OutputTopology = gs_prog_data-
> >output_topology;
> - gs.VertexURBEntryReadLength   = gs_prog_data-
> >base.urb_read_length;
> - gs.IncludeVertexHandles   = gs_prog_data-
> >base.include_vue_handles;
> -
> - gs.DispatchGRFStartRegisterForURBData =
> -gs_prog_data->base.base.dispatch_grf_start_reg;
> -
> - gs.SamplerCount  = get_sampler_count(gs_bin);
> - gs.BindingTableEntryCount=
> get_binding_table_entry_count(gs_bin);
> -
> - gs.MaximumNumberofThreads = devinfo->max_gs_threads -
> 1;
> - /* This in the next dword on HSW. */
> - gs.ControlDataFormat  = gs_prog_data-
> >control_data_format;
> - gs.ControlDataHeaderSize  = gs_prog_data-
> >control_data_header_size_hwords;
> - gs.InstanceControl= MAX2(gs_prog_data-
> >invocations, 1) - 1;
> - gs.DispatchMode   = gs_prog_data-
> >base.dispatch_mode;
> - gs.StatisticsEnable   = true;
> - gs.IncludePrimitiveID = gs_prog_data-
> >include_primitive_id;
> -# if (GEN_IS_HASWELL)
> - gs.ReorderMode= REORDER_TRAILING;

Shouldn't we have changed REORDER_TRAILING to TRAILING in
src/intel/genxml/gen75.xml in the previous patch?

> -# else
> - gs.ReorderEnable  = true;
> -# endif
> - gs.FunctionEnable = true;
> -  }
> -   }
> +   emit_3dstate_gs(pipeline);
>  
> if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_FRAGMENT)) {
>    anv_batch_emit(&pipeline->batch, GENX(3DSTATE_SBE), sbe);
> diff --git a/src/intel/vulkan/gen8_pipeline.c
> b/src/intel/vulkan/gen8_pipeline.c
> index 1320a13..5816bd4 100644
> --- a/src/intel/vulkan/gen8_pipeline.c
> +++ b/src/intel/vulkan/gen8_pipeline.c
> @@ -53,9 +53,6 @@ genX(graphics_pipeline_create)(
>  {
> ANV_FROM_HANDLE(anv_device, device, _device);
> ANV_FROM_HANDLE(anv_render_pass, pass, pCreateInfo->renderPass);
> -   const struct anv_physical_device *physical_device =
> -  &device->instance->physicalDevice;
> -   const struct gen_device_info *devinfo = &physical_device->info;
> struct anv_subpass *subpass = &pass->subpasses[pCreateInfo-
> >subpass];
> struct anv_pipeline *pipeline;
> VkResult result;
> @@ -112,64 +109,7 @@ genX(graphics_pipeline_create)(
>   wm_prog_data ? wm_prog_data->barycentric_interp_modes : 0;
> }
>  
> -   if (!anv_pipeline_has_stage(pipeline, MESA_SHADER_GEOMETRY)) {
> -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs);
> -   } else {
> -  const struct brw_gs_prog_data *gs_prog_data =
> get_gs_prog_data(pipeline);
> -  const struct anv_shader_bin *gs_bin =
> - pipeline->shaders[MESA_SHADER_GEOMETRY];
> -
> -  uint32_t offset = 1;
> -  uint32_t length = (gs_prog_data->base.vue_map.num_slots + 1) /
> 2 - offset;
> -
> -  anv_batch_emit(&pipeline->batch, GENX(3DSTATE_GS), gs) {
> - gs.SingleProgramFlow   = false;
> - gs.KernelStartPointer  = gs_bin->kernel.offset;
> - gs.VectorMaskEnable= fals