[1/4] radv: move emitting VGT_GS_MODE into the HW VS path

Submitted by Samuel Pitoiset on July 17, 2019, 1:43 p.m.

Details

Message ID 20190717134353.5108-1-samuel.pitoiset@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Samuel Pitoiset July 17, 2019, 1:43 p.m.
It's useless for NGG anyways.

Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
---
 src/amd/vulkan/radv_pipeline.c | 43 ++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 10 deletions(-)

Patch hide | download patch | download mbox

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);

Comments

On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
<samuel.pitoiset@gmail.com> wrote:
>
> It's useless for NGG anyways.
>
> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> ---
>  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
On Thu, Jul 18, 2019 at 2:05 AM Bas Nieuwenhuizen
<bas@basnieuwenhuizen.nl> wrote:
>
> On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
> <samuel.pitoiset@gmail.com> wrote:
> >
> > It's useless for NGG anyways.
> >
> > Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
> > ---
> >  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
On 7/18/19 2:10 AM, Bas Nieuwenhuizen wrote:
> On Thu, Jul 18, 2019 at 2:05 AM Bas Nieuwenhuizen
> <bas@basnieuwenhuizen.nl> wrote:
>> On Wed, Jul 17, 2019 at 3:44 PM Samuel Pitoiset
>> <samuel.pitoiset@gmail.com> wrote:
>>> It's useless for NGG anyways.
>>>
>>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset@gmail.com>
>>> ---
>>>   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