Re: [Mesa-dev] [PATCH v2] i965/sbe: fix number of inputs for active components
Can someone review this patch? The bug if fixes is blocking the 18.0
release.
Iago
On Tue, 2018-02-27 at 08:02 +0100, Iago Toral Quiroga wrote:
> In 16631ca30ea6 we fixed gen9 active components to account for padded
> inputs in the URB, which we can have with SSO programs. To do that,
> instead of going through the bitfield of inputs (which doesn't
> include
> padding information), we compute the number of inputs from the size
> of the URB entry.
>
> Unfortunately, there are some special inputs that are not stored in
> the URB and that we also need to account for. These special inputs
> are identified and handled during calculate_attr_overrides(), so this
> patch modifies this function to return a value with the total number
> of inputs, including the ones that are not stored in the URB, so we
> can use that number to program the correct number of active
> components.
>
> This fixes a regression in a WebGL program that uses Point Sprite
> functionality (specifically, VARYING_SLOT_PNTC).
>
> v2:
> - Add 'Fixes' tag (Mark Janes)
> - make no_vue_inputs int instead of uint32_t, and add const
> qualifier
>to num_inputs variable (Ian)
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105224
> Fixes: 16631ca30ea6 (i965/sbe: fix active components for SSO programs
> with over 16 inputs)
> ---
> src/mesa/drivers/dri/i965/genX_state_upload.c | 31
> ---
> 1 file changed, 23 insertions(+), 8 deletions(-)
>
> diff --git a/src/mesa/drivers/dri/i965/genX_state_upload.c
> b/src/mesa/drivers/dri/i965/genX_state_upload.c
> index 8668abd591..891bab1746 100644
> --- a/src/mesa/drivers/dri/i965/genX_state_upload.c
> +++ b/src/mesa/drivers/dri/i965/genX_state_upload.c
> @@ -1015,7 +1015,7 @@ genX(get_attr_override)(struct
> GENX(SF_OUTPUT_ATTRIBUTE_DETAIL) *attr,
> }
>
>
> -static void
> +static int
> genX(calculate_attr_overrides)(const struct brw_context *brw,
> struct
> GENX(SF_OUTPUT_ATTRIBUTE_DETAIL) *attr_overrides,
> uint32_t *point_sprite_enables,
> @@ -1064,6 +1064,7 @@ genX(calculate_attr_overrides)(const struct
> brw_context *brw,
> */
> bool drawing_points = brw_is_drawing_points(brw);
>
> + int no_vue_inputs = 0;
> for (int attr = 0; attr < VARYING_SLOT_MAX; attr++) {
>int input_index = wm_prog_data->urb_setup[attr];
>
> @@ -1097,6 +1098,12 @@ genX(calculate_attr_overrides)(const struct
> brw_context *brw,
> &max_source_attr);
>}
>
> + if (point_sprite ||
> + (attr == VARYING_SLOT_PRIMITIVE_ID &&
> + brw->vue_map_geom_out.varying_to_slot[attr] == -1)) {
> + no_vue_inputs++;
> + }
> +
>/* The hardware can only do the overrides on 16 overrides at a
> * time, and the other up to 16 have to be lined up so that
> the
> * input index = the output index. We'll need to do some
> @@ -1124,6 +1131,8 @@ genX(calculate_attr_overrides)(const struct
> brw_context *brw,
> * Similar text exists for Ivy Bridge.
> */
> *urb_entry_read_length = DIV_ROUND_UP(max_source_attr + 1, 2);
> +
> + return *urb_entry_read_length * 2 + no_vue_inputs;
> }
> #endif
>
> @@ -3434,11 +3443,12 @@ genX(upload_sbe)(struct brw_context *brw)
> * BRW_NEW_GS_PROG_DATA | BRW_NEW_PRIMITIVE |
> BRW_NEW_TES_PROG_DATA |
> * BRW_NEW_VUE_MAP_GEOM_OUT
> */
> - genX(calculate_attr_overrides)(brw,
> - attr_overrides,
> - &point_sprite_enables,
> - &urb_entry_read_length,
> - &urb_entry_read_offset);
> + const int num_inputs =
> + genX(calculate_attr_overrides)(brw,
> +attr_overrides,
> +&point_sprite_enables,
> +&urb_entry_read_length,
> +&urb_entry_read_offset);
>
>/* Typically, the URB entry read length and offset should be
> programmed
> * in 3DSTATE_VS and 3DSTATE_GS; SBE inherits it from the last
> active
> @@ -3459,8 +3469,13 @@ genX(upload_sbe)(struct brw_context *brw)
> #endif
>
> #if GEN_GEN >= 9
> - /* prepare the active component dwords */
> - const int num_inputs = urb_entry_read_length * 2;
> + /* prepare the active component dwords
> + *
> + * For this, we need to account for padded inputs (that we can
> have with
> + * SSO programs), so we take the number of inputs from the
> size of the
> + * URB entry. To that, we need to add the number of special
> inputs, if
> + * any, that are not in the URB (such as point sprite inputs).
> + */
>for (int input_index = 0; input_index < num_inputs;
> input_index++) {
> sbe.AttributeActiveComponentFormat[input_index] =
>
Re: [Mesa-dev] [PATCH v2] i965/sbe: fix number of inputs for active components
On Monday, February 26, 2018 11:02:08 PM PST Iago Toral Quiroga wrote: > In 16631ca30ea6 we fixed gen9 active components to account for padded > inputs in the URB, which we can have with SSO programs. To do that, > instead of going through the bitfield of inputs (which doesn't include > padding information), we compute the number of inputs from the size > of the URB entry. > > Unfortunately, there are some special inputs that are not stored in > the URB and that we also need to account for. These special inputs > are identified and handled during calculate_attr_overrides(), so this > patch modifies this function to return a value with the total number > of inputs, including the ones that are not stored in the URB, so we > can use that number to program the correct number of active components. > > This fixes a regression in a WebGL program that uses Point Sprite > functionality (specifically, VARYING_SLOT_PNTC). > > v2: > - Add 'Fixes' tag (Mark Janes) > - make no_vue_inputs int instead of uint32_t, and add const qualifier >to num_inputs variable (Ian) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105224 > Fixes: 16631ca30ea6 (i965/sbe: fix active components for SSO programs with > over 16 inputs) > --- > src/mesa/drivers/dri/i965/genX_state_upload.c | 31 > --- > 1 file changed, 23 insertions(+), 8 deletions(-) :( Thanks for fixing this, Iago. Reviewed-by: Kenneth Graunke signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965/sbe: fix number of inputs for active components
On Monday, February 26, 2018 11:02:08 PM PST Iago Toral Quiroga wrote: > In 16631ca30ea6 we fixed gen9 active components to account for padded > inputs in the URB, which we can have with SSO programs. To do that, > instead of going through the bitfield of inputs (which doesn't include > padding information), we compute the number of inputs from the size > of the URB entry. > > Unfortunately, there are some special inputs that are not stored in > the URB and that we also need to account for. These special inputs > are identified and handled during calculate_attr_overrides(), so this > patch modifies this function to return a value with the total number > of inputs, including the ones that are not stored in the URB, so we > can use that number to program the correct number of active components. > > This fixes a regression in a WebGL program that uses Point Sprite > functionality (specifically, VARYING_SLOT_PNTC). > > v2: > - Add 'Fixes' tag (Mark Janes) > - make no_vue_inputs int instead of uint32_t, and add const qualifier >to num_inputs variable (Ian) > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105224 > Fixes: 16631ca30ea6 (i965/sbe: fix active components for SSO programs with > over 16 inputs) Or you could just steal the code from anv and do: for (unsigned i = 0; i < 32; i++) sbe.AttributeActiveComponentFormat[i] = ACF_XYZW; instead of trying to count correctly. signature.asc Description: This is a digitally signed message part. ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH v2] i965/sbe: fix number of inputs for active components
On Wed, 2018-02-28 at 15:39 -0800, Kenneth Graunke wrote: > On Monday, February 26, 2018 11:02:08 PM PST Iago Toral Quiroga > wrote: > > In 16631ca30ea6 we fixed gen9 active components to account for > > padded > > inputs in the URB, which we can have with SSO programs. To do that, > > instead of going through the bitfield of inputs (which doesn't > > include > > padding information), we compute the number of inputs from the size > > of the URB entry. > > > > Unfortunately, there are some special inputs that are not stored in > > the URB and that we also need to account for. These special inputs > > are identified and handled during calculate_attr_overrides(), so > > this > > patch modifies this function to return a value with the total > > number > > of inputs, including the ones that are not stored in the URB, so we > > can use that number to program the correct number of active > > components. > > > > This fixes a regression in a WebGL program that uses Point Sprite > > functionality (specifically, VARYING_SLOT_PNTC). > > > > v2: > > - Add 'Fixes' tag (Mark Janes) > > - make no_vue_inputs int instead of uint32_t, and add const > > qualifier > >to num_inputs variable (Ian) > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=105224 > > Fixes: 16631ca30ea6 (i965/sbe: fix active components for SSO > > programs with over 16 inputs) > > Or you could just steal the code from anv and do: > >for (unsigned i = 0; i < 32; i++) > sbe.AttributeActiveComponentFormat[i] = ACF_XYZW; > > instead of trying to count correctly. Yeah, I was wondering if that would have any performance impact, so I have tried this approach with a couple of WebGL demos and I haven't seen much of a difference. I have also tried a terrain demo that I have around and gives me more precise frame timing and I seem to get ~0.03 more fps on average with the original patch, so I would say that performance impact is negligible, if it exists at all. Seeing how performance-wise there is not a clear difference I'll push the patch with your recommendation if Jenkins doesn't report anything weird, which I don't expect it to. Iago ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
