Re: [Mesa-dev] [PATCH 1/4] radv: move emitting VGT_GS_MODE into the HW VS path

2019-07-18 Thread Samuel Pitoiset 


On 7/18/19 2:10 AM, Bas Nieuwenhuizen wrote:

On Thu, Jul 18, 2019 at 2:05 AM Bas Nieuwenhuizen
 wrote:

On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
 wrote:

It's useless for NGG anyways.

Signed-off-by: Samuel Pitoiset 
---
  src/amd/vulkan/radv_pipeline.c | 43 ++
  1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
index fdeb31c453e..686fd371f0f 100644
--- a/src/amd/vulkan/radv_pipeline.c
+++ b/src/amd/vulkan/radv_pipeline.c
@@ -3272,27 +3272,18 @@ radv_pipeline_generate_vgt_gs_mode(struct radeon_cmdbuf 
*ctx_cs,

Can you rename the function?

Actually now that I see your later patches, how about we keep this
function, return immediately if ngg, and then move the primitive id
stuff for ngg to ngg?

Yes, looks better.






 pipeline->shaders[MESA_SHADER_TESS_EVAL] :
 pipeline->shaders[MESA_SHADER_VERTEX];
 unsigned vgt_primitiveid_en = 0;
-   uint32_t vgt_gs_mode = 0;

-   if (radv_pipeline_has_gs(pipeline)) {
-   const struct radv_shader_variant *gs =
-   pipeline->shaders[MESA_SHADER_GEOMETRY];
-
-   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
-
pipeline->device->physical_device->rad_info.chip_class);
-   } else if (radv_pipeline_has_ngg(pipeline)) {
+   if (radv_pipeline_has_ngg(pipeline)) {
 bool enable_prim_id =
 outinfo->export_prim_id || vs->info.info.uses_prim_id;

 vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(enable_prim_id) |
   
S_028A84_NGG_DISABLE_PROVOK_REUSE(enable_prim_id);
 } else if (outinfo->export_prim_id || vs->info.info.uses_prim_id) {
-   vgt_gs_mode = S_028A40_MODE(V_028A40_GS_SCENARIO_A);
 vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(1);
 }

 radeon_set_context_reg(ctx_cs, R_028A84_VGT_PRIMITIVEID_EN, 
vgt_primitiveid_en);
-   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
  }

  static void
@@ -3370,6 +3361,38 @@ radv_pipeline_generate_hw_vs(struct radeon_cmdbuf 
*ctx_cs,
cull_dist_mask << 8 |
clip_dist_mask);

+   /* We always write VGT_GS_MODE in the VS state, because every switch
+* between different shader pipelines involving a different GS or no GS
+* at all involves a switch of the VS (different GS use different copy
+* shaders). On the other hand, when the API switches from a GS to no
+* GS and then back to the same GS used originally, the GS state is not
+* sent again.
+*/
+   unsigned vgt_gs_mode;
+   if (!radv_pipeline_has_gs(pipeline)) {
+   const struct radv_vs_output_info *outinfo =
+   get_vs_output_info(pipeline);
+   const struct radv_shader_variant *vs =
+   pipeline->shaders[MESA_SHADER_TESS_EVAL] ?
+   pipeline->shaders[MESA_SHADER_TESS_EVAL] :
+   pipeline->shaders[MESA_SHADER_VERTEX];
+   unsigned mode = V_028A40_GS_OFF;
+
+   /* PrimID needs GS scenario A. */
+   if (outinfo->export_prim_id || vs->info.info.uses_prim_id)
+   mode = V_028A40_GS_SCENARIO_A;
+
+   vgt_gs_mode = S_028A40_MODE(mode);
+   } else {
+   const struct radv_shader_variant *gs =
+   pipeline->shaders[MESA_SHADER_GEOMETRY];
+
+   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
+
pipeline->device->physical_device->rad_info.chip_class);
+   }
+
+   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
+

Can you keep this in a separate function (possibly with the name
radv_pipeline_generate_vgt_gs_mode)?

 if (pipeline->device->physical_device->rad_info.chip_class <= GFX8)
 radeon_set_context_reg(ctx_cs, R_028AB4_VGT_REUSE_OFF,
outinfo->writes_viewport_index);
--
2.22.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] radv: move emitting VGT_GS_MODE into the HW VS path

2019-07-17 Thread Bas Nieuwenhuizen 
On Thu, Jul 18, 2019 at 2:05 AM Bas Nieuwenhuizen
 wrote:
>
> On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
>  wrote:
> >
> > It's useless for NGG anyways.
> >
> > Signed-off-by: Samuel Pitoiset 
> > ---
> >  src/amd/vulkan/radv_pipeline.c | 43 ++
> >  1 file changed, 33 insertions(+), 10 deletions(-)
> >
> > diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
> > index fdeb31c453e..686fd371f0f 100644
> > --- a/src/amd/vulkan/radv_pipeline.c
> > +++ b/src/amd/vulkan/radv_pipeline.c
> > @@ -3272,27 +3272,18 @@ radv_pipeline_generate_vgt_gs_mode(struct 
> > radeon_cmdbuf *ctx_cs,
>
> Can you rename the function?

Actually now that I see your later patches, how about we keep this
function, return immediately if ngg, and then move the primitive id
stuff for ngg to ngg?


>
>
> > pipeline->shaders[MESA_SHADER_TESS_EVAL] :
> > pipeline->shaders[MESA_SHADER_VERTEX];
> > unsigned vgt_primitiveid_en = 0;
> > -   uint32_t vgt_gs_mode = 0;
> >
> > -   if (radv_pipeline_has_gs(pipeline)) {
> > -   const struct radv_shader_variant *gs =
> > -   pipeline->shaders[MESA_SHADER_GEOMETRY];
> > -
> > -   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
> > -
> > pipeline->device->physical_device->rad_info.chip_class);
> > -   } else if (radv_pipeline_has_ngg(pipeline)) {
> > +   if (radv_pipeline_has_ngg(pipeline)) {
> > bool enable_prim_id =
> > outinfo->export_prim_id || 
> > vs->info.info.uses_prim_id;
> >
> > vgt_primitiveid_en |= 
> > S_028A84_PRIMITIVEID_EN(enable_prim_id) |
> >   
> > S_028A84_NGG_DISABLE_PROVOK_REUSE(enable_prim_id);
> > } else if (outinfo->export_prim_id || vs->info.info.uses_prim_id) {
> > -   vgt_gs_mode = S_028A40_MODE(V_028A40_GS_SCENARIO_A);
> > vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(1);
> > }
> >
> > radeon_set_context_reg(ctx_cs, R_028A84_VGT_PRIMITIVEID_EN, 
> > vgt_primitiveid_en);
> > -   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
> >  }
> >
> >  static void
> > @@ -3370,6 +3361,38 @@ radv_pipeline_generate_hw_vs(struct radeon_cmdbuf 
> > *ctx_cs,
> >cull_dist_mask << 8 |
> >clip_dist_mask);
> >
> > +   /* We always write VGT_GS_MODE in the VS state, because every switch
> > +* between different shader pipelines involving a different GS or 
> > no GS
> > +* at all involves a switch of the VS (different GS use different 
> > copy
> > +* shaders). On the other hand, when the API switches from a GS to 
> > no
> > +* GS and then back to the same GS used originally, the GS state is 
> > not
> > +* sent again.
> > +*/
> > +   unsigned vgt_gs_mode;
> > +   if (!radv_pipeline_has_gs(pipeline)) {
> > +   const struct radv_vs_output_info *outinfo =
> > +   get_vs_output_info(pipeline);
> > +   const struct radv_shader_variant *vs =
> > +   pipeline->shaders[MESA_SHADER_TESS_EVAL] ?
> > +   pipeline->shaders[MESA_SHADER_TESS_EVAL] :
> > +   pipeline->shaders[MESA_SHADER_VERTEX];
> > +   unsigned mode = V_028A40_GS_OFF;
> > +
> > +   /* PrimID needs GS scenario A. */
> > +   if (outinfo->export_prim_id || vs->info.info.uses_prim_id)
> > +   mode = V_028A40_GS_SCENARIO_A;
> > +
> > +   vgt_gs_mode = S_028A40_MODE(mode);
> > +   } else {
> > +   const struct radv_shader_variant *gs =
> > +   pipeline->shaders[MESA_SHADER_GEOMETRY];
> > +
> > +   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
> > +
> > pipeline->device->physical_device->rad_info.chip_class);
> > +   }
> > +
> > +   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
> > +
>
> Can you keep this in a separate function (possibly with the name
> radv_pipeline_generate_vgt_gs_mode)?
> > if (pipeline->device->physical_device->rad_info.chip_class <= GFX8)
> > radeon_set_context_reg(ctx_cs, R_028AB4_VGT_REUSE_OFF,
> >outinfo->writes_viewport_index);
> > --
> > 2.22.0
> >
> > ___
> > mesa-dev mailing list
> > mesa-dev@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH 1/4] radv: move emitting VGT_GS_MODE into the HW VS path

2019-07-17 Thread Bas Nieuwenhuizen 
On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
 wrote:
>
> It's useless for NGG anyways.
>
> Signed-off-by: Samuel Pitoiset 
> ---
>  src/amd/vulkan/radv_pipeline.c | 43 ++
>  1 file changed, 33 insertions(+), 10 deletions(-)
>
> diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
> index fdeb31c453e..686fd371f0f 100644
> --- a/src/amd/vulkan/radv_pipeline.c
> +++ b/src/amd/vulkan/radv_pipeline.c
> @@ -3272,27 +3272,18 @@ radv_pipeline_generate_vgt_gs_mode(struct 
> radeon_cmdbuf *ctx_cs,

Can you rename the function?


> pipeline->shaders[MESA_SHADER_TESS_EVAL] :
> pipeline->shaders[MESA_SHADER_VERTEX];
> unsigned vgt_primitiveid_en = 0;
> -   uint32_t vgt_gs_mode = 0;
>
> -   if (radv_pipeline_has_gs(pipeline)) {
> -   const struct radv_shader_variant *gs =
> -   pipeline->shaders[MESA_SHADER_GEOMETRY];
> -
> -   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
> -
> pipeline->device->physical_device->rad_info.chip_class);
> -   } else if (radv_pipeline_has_ngg(pipeline)) {
> +   if (radv_pipeline_has_ngg(pipeline)) {
> bool enable_prim_id =
> outinfo->export_prim_id || vs->info.info.uses_prim_id;
>
> vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(enable_prim_id) 
> |
>   
> S_028A84_NGG_DISABLE_PROVOK_REUSE(enable_prim_id);
> } else if (outinfo->export_prim_id || vs->info.info.uses_prim_id) {
> -   vgt_gs_mode = S_028A40_MODE(V_028A40_GS_SCENARIO_A);
> vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(1);
> }
>
> radeon_set_context_reg(ctx_cs, R_028A84_VGT_PRIMITIVEID_EN, 
> vgt_primitiveid_en);
> -   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
>  }
>
>  static void
> @@ -3370,6 +3361,38 @@ radv_pipeline_generate_hw_vs(struct radeon_cmdbuf 
> *ctx_cs,
>cull_dist_mask << 8 |
>clip_dist_mask);
>
> +   /* We always write VGT_GS_MODE in the VS state, because every switch
> +* between different shader pipelines involving a different GS or no 
> GS
> +* at all involves a switch of the VS (different GS use different copy
> +* shaders). On the other hand, when the API switches from a GS to no
> +* GS and then back to the same GS used originally, the GS state is 
> not
> +* sent again.
> +*/
> +   unsigned vgt_gs_mode;
> +   if (!radv_pipeline_has_gs(pipeline)) {
> +   const struct radv_vs_output_info *outinfo =
> +   get_vs_output_info(pipeline);
> +   const struct radv_shader_variant *vs =
> +   pipeline->shaders[MESA_SHADER_TESS_EVAL] ?
> +   pipeline->shaders[MESA_SHADER_TESS_EVAL] :
> +   pipeline->shaders[MESA_SHADER_VERTEX];
> +   unsigned mode = V_028A40_GS_OFF;
> +
> +   /* PrimID needs GS scenario A. */
> +   if (outinfo->export_prim_id || vs->info.info.uses_prim_id)
> +   mode = V_028A40_GS_SCENARIO_A;
> +
> +   vgt_gs_mode = S_028A40_MODE(mode);
> +   } else {
> +   const struct radv_shader_variant *gs =
> +   pipeline->shaders[MESA_SHADER_GEOMETRY];
> +
> +   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
> +
> pipeline->device->physical_device->rad_info.chip_class);
> +   }
> +
> +   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
> +

Can you keep this in a separate function (possibly with the name
radv_pipeline_generate_vgt_gs_mode)?
> if (pipeline->device->physical_device->rad_info.chip_class <= GFX8)
> radeon_set_context_reg(ctx_cs, R_028AB4_VGT_REUSE_OFF,
>outinfo->writes_viewport_index);
> --
> 2.22.0
>
> ___
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH 1/4] radv: move emitting VGT_GS_MODE into the HW VS path

2019-07-17 Thread Samuel Pitoiset 
It's useless for NGG anyways.

Signed-off-by: Samuel Pitoiset 
---
 src/amd/vulkan/radv_pipeline.c | 43 ++
 1 file changed, 33 insertions(+), 10 deletions(-)

diff --git a/src/amd/vulkan/radv_pipeline.c b/src/amd/vulkan/radv_pipeline.c
index fdeb31c453e..686fd371f0f 100644
--- a/src/amd/vulkan/radv_pipeline.c
+++ b/src/amd/vulkan/radv_pipeline.c
@@ -3272,27 +3272,18 @@ radv_pipeline_generate_vgt_gs_mode(struct radeon_cmdbuf 
*ctx_cs,
pipeline->shaders[MESA_SHADER_TESS_EVAL] :
pipeline->shaders[MESA_SHADER_VERTEX];
unsigned vgt_primitiveid_en = 0;
-   uint32_t vgt_gs_mode = 0;
 
-   if (radv_pipeline_has_gs(pipeline)) {
-   const struct radv_shader_variant *gs =
-   pipeline->shaders[MESA_SHADER_GEOMETRY];
-
-   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
-
pipeline->device->physical_device->rad_info.chip_class);
-   } else if (radv_pipeline_has_ngg(pipeline)) {
+   if (radv_pipeline_has_ngg(pipeline)) {
bool enable_prim_id =
outinfo->export_prim_id || vs->info.info.uses_prim_id;
 
vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(enable_prim_id) |
  
S_028A84_NGG_DISABLE_PROVOK_REUSE(enable_prim_id);
} else if (outinfo->export_prim_id || vs->info.info.uses_prim_id) {
-   vgt_gs_mode = S_028A40_MODE(V_028A40_GS_SCENARIO_A);
vgt_primitiveid_en |= S_028A84_PRIMITIVEID_EN(1);
}
 
radeon_set_context_reg(ctx_cs, R_028A84_VGT_PRIMITIVEID_EN, 
vgt_primitiveid_en);
-   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
 }
 
 static void
@@ -3370,6 +3361,38 @@ radv_pipeline_generate_hw_vs(struct radeon_cmdbuf 
*ctx_cs,
   cull_dist_mask << 8 |
   clip_dist_mask);
 
+   /* We always write VGT_GS_MODE in the VS state, because every switch
+* between different shader pipelines involving a different GS or no GS
+* at all involves a switch of the VS (different GS use different copy
+* shaders). On the other hand, when the API switches from a GS to no
+* GS and then back to the same GS used originally, the GS state is not
+* sent again.
+*/
+   unsigned vgt_gs_mode;
+   if (!radv_pipeline_has_gs(pipeline)) {
+   const struct radv_vs_output_info *outinfo =
+   get_vs_output_info(pipeline);
+   const struct radv_shader_variant *vs =
+   pipeline->shaders[MESA_SHADER_TESS_EVAL] ?
+   pipeline->shaders[MESA_SHADER_TESS_EVAL] :
+   pipeline->shaders[MESA_SHADER_VERTEX];
+   unsigned mode = V_028A40_GS_OFF;
+
+   /* PrimID needs GS scenario A. */
+   if (outinfo->export_prim_id || vs->info.info.uses_prim_id)
+   mode = V_028A40_GS_SCENARIO_A;
+
+   vgt_gs_mode = S_028A40_MODE(mode);
+   } else {
+   const struct radv_shader_variant *gs =
+   pipeline->shaders[MESA_SHADER_GEOMETRY];
+
+   vgt_gs_mode = ac_vgt_gs_mode(gs->info.gs.vertices_out,
+
pipeline->device->physical_device->rad_info.chip_class);
+   }
+
+   radeon_set_context_reg(ctx_cs, R_028A40_VGT_GS_MODE, vgt_gs_mode);
+
if (pipeline->device->physical_device->rad_info.chip_class <= GFX8)
radeon_set_context_reg(ctx_cs, R_028AB4_VGT_REUSE_OFF,
   outinfo->writes_viewport_index);
-- 
2.22.0

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev