[5/8] radeonsi: don't set spi_ps_input_* for monolithic shaders

Submitted by Marek Olšák on June 20, 2019, 4:19 a.m.

Details

Message ID 20190620041941.14001-5-maraeo@gmail.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Marek Olšák June 20, 2019, 4:19 a.m.
From: Marek Olšák <marek.olsak@amd.com>

The driver doesn't use these values and ac_rtld has assertions
expecting the value of 0.
---
 src/gallium/drivers/radeonsi/si_shader.c | 39 ++++++++++++++++--------
 1 file changed, 26 insertions(+), 13 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
index 54b29d0ae01..0489399b827 100644
--- a/src/gallium/drivers/radeonsi/si_shader.c
+++ b/src/gallium/drivers/radeonsi/si_shader.c
@@ -6128,21 +6128,22 @@  static void si_get_ps_prolog_key(struct si_shader *shader,
 		 key->ps_prolog.states.bc_optimize_for_linear);
 	key->ps_prolog.ancillary_vgpr_index = shader->info.ancillary_vgpr_index;
 
 	if (info->colors_read) {
 		unsigned *color = shader->selector->color_attr_index;
 
 		if (shader->key.part.ps.prolog.color_two_side) {
 			/* BCOLORs are stored after the last input. */
 			key->ps_prolog.num_interp_inputs = info->num_inputs;
 			key->ps_prolog.face_vgpr_index = shader->info.face_vgpr_index;
-			shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
+			if (separate_prolog)
+				shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
 		}
 
 		for (unsigned i = 0; i < 2; i++) {
 			unsigned interp = info->input_interpolate[color[i]];
 			unsigned location = info->input_interpolate_loc[color[i]];
 
 			if (!(info->colors_read & (0xf << i*4)))
 				continue;
 
 			key->ps_prolog.color_attr_index[i] = color[i];
@@ -6159,66 +6160,78 @@  static void si_get_ps_prolog_key(struct si_shader *shader,
 			case TGSI_INTERPOLATE_COLOR:
 				/* Force the interpolation location for colors here. */
 				if (shader->key.part.ps.prolog.force_persp_sample_interp)
 					location = TGSI_INTERPOLATE_LOC_SAMPLE;
 				if (shader->key.part.ps.prolog.force_persp_center_interp)
 					location = TGSI_INTERPOLATE_LOC_CENTER;
 
 				switch (location) {
 				case TGSI_INTERPOLATE_LOC_SAMPLE:
 					key->ps_prolog.color_interp_vgpr_index[i] = 0;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_PERSP_SAMPLE_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_PERSP_SAMPLE_ENA(1);
+					}
 					break;
 				case TGSI_INTERPOLATE_LOC_CENTER:
 					key->ps_prolog.color_interp_vgpr_index[i] = 2;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_PERSP_CENTER_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_PERSP_CENTER_ENA(1);
+					}
 					break;
 				case TGSI_INTERPOLATE_LOC_CENTROID:
 					key->ps_prolog.color_interp_vgpr_index[i] = 4;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_PERSP_CENTROID_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_PERSP_CENTROID_ENA(1);
+					}
 					break;
 				default:
 					assert(0);
 				}
 				break;
 			case TGSI_INTERPOLATE_LINEAR:
 				/* Force the interpolation location for colors here. */
 				if (shader->key.part.ps.prolog.force_linear_sample_interp)
 					location = TGSI_INTERPOLATE_LOC_SAMPLE;
 				if (shader->key.part.ps.prolog.force_linear_center_interp)
 					location = TGSI_INTERPOLATE_LOC_CENTER;
 
 				/* The VGPR assignment for non-monolithic shaders
 				 * works because InitialPSInputAddr is set on the
 				 * main shader and PERSP_PULL_MODEL is never used.
 				 */
 				switch (location) {
 				case TGSI_INTERPOLATE_LOC_SAMPLE:
 					key->ps_prolog.color_interp_vgpr_index[i] =
 						separate_prolog ? 6 : 9;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_LINEAR_SAMPLE_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_LINEAR_SAMPLE_ENA(1);
+					}
 					break;
 				case TGSI_INTERPOLATE_LOC_CENTER:
 					key->ps_prolog.color_interp_vgpr_index[i] =
 						separate_prolog ? 8 : 11;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_LINEAR_CENTER_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_LINEAR_CENTER_ENA(1);
+					}
 					break;
 				case TGSI_INTERPOLATE_LOC_CENTROID:
 					key->ps_prolog.color_interp_vgpr_index[i] =
 						separate_prolog ? 10 : 13;
-					shader->config.spi_ps_input_ena |=
-						S_0286CC_LINEAR_CENTROID_ENA(1);
+					if (separate_prolog) {
+						shader->config.spi_ps_input_ena |=
+							S_0286CC_LINEAR_CENTROID_ENA(1);
+					}
 					break;
 				default:
 					assert(0);
 				}
 				break;
 			default:
 				assert(0);
 			}
 		}
 	}

Comments

Doesn't this cause assertions in si_shader_ps() for monolithic
shaders? Some of these assertions check that at least one bit in a
group is set and I think we end up with input_ena = 0 for monolithic
shaders now?

On Thu, Jun 20, 2019 at 6:20 AM Marek Olšák <maraeo@gmail.com> wrote:
>
> From: Marek Olšák <marek.olsak@amd.com>
>
> The driver doesn't use these values and ac_rtld has assertions
> expecting the value of 0.
> ---
>  src/gallium/drivers/radeonsi/si_shader.c | 39 ++++++++++++++++--------
>  1 file changed, 26 insertions(+), 13 deletions(-)
>
> diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
> index 54b29d0ae01..0489399b827 100644
> --- a/src/gallium/drivers/radeonsi/si_shader.c
> +++ b/src/gallium/drivers/radeonsi/si_shader.c
> @@ -6128,21 +6128,22 @@ static void si_get_ps_prolog_key(struct si_shader *shader,
>                  key->ps_prolog.states.bc_optimize_for_linear);
>         key->ps_prolog.ancillary_vgpr_index = shader->info.ancillary_vgpr_index;
>
>         if (info->colors_read) {
>                 unsigned *color = shader->selector->color_attr_index;
>
>                 if (shader->key.part.ps.prolog.color_two_side) {
>                         /* BCOLORs are stored after the last input. */
>                         key->ps_prolog.num_interp_inputs = info->num_inputs;
>                         key->ps_prolog.face_vgpr_index = shader->info.face_vgpr_index;
> -                       shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
> +                       if (separate_prolog)
> +                               shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
>                 }
>
>                 for (unsigned i = 0; i < 2; i++) {
>                         unsigned interp = info->input_interpolate[color[i]];
>                         unsigned location = info->input_interpolate_loc[color[i]];
>
>                         if (!(info->colors_read & (0xf << i*4)))
>                                 continue;
>
>                         key->ps_prolog.color_attr_index[i] = color[i];
> @@ -6159,66 +6160,78 @@ static void si_get_ps_prolog_key(struct si_shader *shader,
>                         case TGSI_INTERPOLATE_COLOR:
>                                 /* Force the interpolation location for colors here. */
>                                 if (shader->key.part.ps.prolog.force_persp_sample_interp)
>                                         location = TGSI_INTERPOLATE_LOC_SAMPLE;
>                                 if (shader->key.part.ps.prolog.force_persp_center_interp)
>                                         location = TGSI_INTERPOLATE_LOC_CENTER;
>
>                                 switch (location) {
>                                 case TGSI_INTERPOLATE_LOC_SAMPLE:
>                                         key->ps_prolog.color_interp_vgpr_index[i] = 0;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_PERSP_SAMPLE_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_PERSP_SAMPLE_ENA(1);
> +                                       }
>                                         break;
>                                 case TGSI_INTERPOLATE_LOC_CENTER:
>                                         key->ps_prolog.color_interp_vgpr_index[i] = 2;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_PERSP_CENTER_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_PERSP_CENTER_ENA(1);
> +                                       }
>                                         break;
>                                 case TGSI_INTERPOLATE_LOC_CENTROID:
>                                         key->ps_prolog.color_interp_vgpr_index[i] = 4;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_PERSP_CENTROID_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_PERSP_CENTROID_ENA(1);
> +                                       }
>                                         break;
>                                 default:
>                                         assert(0);
>                                 }
>                                 break;
>                         case TGSI_INTERPOLATE_LINEAR:
>                                 /* Force the interpolation location for colors here. */
>                                 if (shader->key.part.ps.prolog.force_linear_sample_interp)
>                                         location = TGSI_INTERPOLATE_LOC_SAMPLE;
>                                 if (shader->key.part.ps.prolog.force_linear_center_interp)
>                                         location = TGSI_INTERPOLATE_LOC_CENTER;
>
>                                 /* The VGPR assignment for non-monolithic shaders
>                                  * works because InitialPSInputAddr is set on the
>                                  * main shader and PERSP_PULL_MODEL is never used.
>                                  */
>                                 switch (location) {
>                                 case TGSI_INTERPOLATE_LOC_SAMPLE:
>                                         key->ps_prolog.color_interp_vgpr_index[i] =
>                                                 separate_prolog ? 6 : 9;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_LINEAR_SAMPLE_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_LINEAR_SAMPLE_ENA(1);
> +                                       }
>                                         break;
>                                 case TGSI_INTERPOLATE_LOC_CENTER:
>                                         key->ps_prolog.color_interp_vgpr_index[i] =
>                                                 separate_prolog ? 8 : 11;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_LINEAR_CENTER_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_LINEAR_CENTER_ENA(1);
> +                                       }
>                                         break;
>                                 case TGSI_INTERPOLATE_LOC_CENTROID:
>                                         key->ps_prolog.color_interp_vgpr_index[i] =
>                                                 separate_prolog ? 10 : 13;
> -                                       shader->config.spi_ps_input_ena |=
> -                                               S_0286CC_LINEAR_CENTROID_ENA(1);
> +                                       if (separate_prolog) {
> +                                               shader->config.spi_ps_input_ena |=
> +                                                       S_0286CC_LINEAR_CENTROID_ENA(1);
> +                                       }
>                                         break;
>                                 default:
>                                         assert(0);
>                                 }
>                                 break;
>                         default:
>                                 assert(0);
>                         }
>                 }
>         }
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev

sorry, totally missed that it was set in src/amd/common as well.

r-b for this patch too.

On Tue, Jun 25, 2019 at 2:17 AM Marek Olšák <maraeo@gmail.com> wrote:
>
> If the shader is monolithic, the value of SPI_PS_INPUT_ENA that is generated by LLVM is used as-is.
>
> Marek
>
> On Fri, Jun 21, 2019 at 7:03 PM Bas Nieuwenhuizen <bas@basnieuwenhuizen.nl> wrote:
>>
>> Doesn't this cause assertions in si_shader_ps() for monolithic
>> shaders? Some of these assertions check that at least one bit in a
>> group is set and I think we end up with input_ena = 0 for monolithic
>> shaders now?
>>
>> On Thu, Jun 20, 2019 at 6:20 AM Marek Olšák <maraeo@gmail.com> wrote:
>> >
>> > From: Marek Olšák <marek.olsak@amd.com>
>> >
>> > The driver doesn't use these values and ac_rtld has assertions
>> > expecting the value of 0.
>> > ---
>> >  src/gallium/drivers/radeonsi/si_shader.c | 39 ++++++++++++++++--------
>> >  1 file changed, 26 insertions(+), 13 deletions(-)
>> >
>> > diff --git a/src/gallium/drivers/radeonsi/si_shader.c b/src/gallium/drivers/radeonsi/si_shader.c
>> > index 54b29d0ae01..0489399b827 100644
>> > --- a/src/gallium/drivers/radeonsi/si_shader.c
>> > +++ b/src/gallium/drivers/radeonsi/si_shader.c
>> > @@ -6128,21 +6128,22 @@ static void si_get_ps_prolog_key(struct si_shader *shader,
>> >                  key->ps_prolog.states.bc_optimize_for_linear);
>> >         key->ps_prolog.ancillary_vgpr_index = shader->info.ancillary_vgpr_index;
>> >
>> >         if (info->colors_read) {
>> >                 unsigned *color = shader->selector->color_attr_index;
>> >
>> >                 if (shader->key.part.ps.prolog.color_two_side) {
>> >                         /* BCOLORs are stored after the last input. */
>> >                         key->ps_prolog.num_interp_inputs = info->num_inputs;
>> >                         key->ps_prolog.face_vgpr_index = shader->info.face_vgpr_index;
>> > -                       shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
>> > +                       if (separate_prolog)
>> > +                               shader->config.spi_ps_input_ena |= S_0286CC_FRONT_FACE_ENA(1);
>> >                 }
>> >
>> >                 for (unsigned i = 0; i < 2; i++) {
>> >                         unsigned interp = info->input_interpolate[color[i]];
>> >                         unsigned location = info->input_interpolate_loc[color[i]];
>> >
>> >                         if (!(info->colors_read & (0xf << i*4)))
>> >                                 continue;
>> >
>> >                         key->ps_prolog.color_attr_index[i] = color[i];
>> > @@ -6159,66 +6160,78 @@ static void si_get_ps_prolog_key(struct si_shader *shader,
>> >                         case TGSI_INTERPOLATE_COLOR:
>> >                                 /* Force the interpolation location for colors here. */
>> >                                 if (shader->key.part.ps.prolog.force_persp_sample_interp)
>> >                                         location = TGSI_INTERPOLATE_LOC_SAMPLE;
>> >                                 if (shader->key.part.ps.prolog.force_persp_center_interp)
>> >                                         location = TGSI_INTERPOLATE_LOC_CENTER;
>> >
>> >                                 switch (location) {
>> >                                 case TGSI_INTERPOLATE_LOC_SAMPLE:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] = 0;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_PERSP_SAMPLE_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_PERSP_SAMPLE_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 case TGSI_INTERPOLATE_LOC_CENTER:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] = 2;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_PERSP_CENTER_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_PERSP_CENTER_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 case TGSI_INTERPOLATE_LOC_CENTROID:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] = 4;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_PERSP_CENTROID_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_PERSP_CENTROID_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 default:
>> >                                         assert(0);
>> >                                 }
>> >                                 break;
>> >                         case TGSI_INTERPOLATE_LINEAR:
>> >                                 /* Force the interpolation location for colors here. */
>> >                                 if (shader->key.part.ps.prolog.force_linear_sample_interp)
>> >                                         location = TGSI_INTERPOLATE_LOC_SAMPLE;
>> >                                 if (shader->key.part.ps.prolog.force_linear_center_interp)
>> >                                         location = TGSI_INTERPOLATE_LOC_CENTER;
>> >
>> >                                 /* The VGPR assignment for non-monolithic shaders
>> >                                  * works because InitialPSInputAddr is set on the
>> >                                  * main shader and PERSP_PULL_MODEL is never used.
>> >                                  */
>> >                                 switch (location) {
>> >                                 case TGSI_INTERPOLATE_LOC_SAMPLE:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] =
>> >                                                 separate_prolog ? 6 : 9;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_LINEAR_SAMPLE_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_LINEAR_SAMPLE_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 case TGSI_INTERPOLATE_LOC_CENTER:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] =
>> >                                                 separate_prolog ? 8 : 11;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_LINEAR_CENTER_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_LINEAR_CENTER_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 case TGSI_INTERPOLATE_LOC_CENTROID:
>> >                                         key->ps_prolog.color_interp_vgpr_index[i] =
>> >                                                 separate_prolog ? 10 : 13;
>> > -                                       shader->config.spi_ps_input_ena |=
>> > -                                               S_0286CC_LINEAR_CENTROID_ENA(1);
>> > +                                       if (separate_prolog) {
>> > +                                               shader->config.spi_ps_input_ena |=
>> > +                                                       S_0286CC_LINEAR_CENTROID_ENA(1);
>> > +                                       }
>> >                                         break;
>> >                                 default:
>> >                                         assert(0);
>> >                                 }
>> >                                 break;
>> >                         default:
>> >                                 assert(0);
>> >                         }
>> >                 }
>> >         }
>> > --
>> > 2.17.1
>> >
>> > _______________________________________________
>> > mesa-dev mailing list
>> > mesa-dev@lists.freedesktop.org
>> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev