[Mesa-dev] [PATCH 06/14] i965: Add new SIMD8 VS prog data flag

[Kristian Høgsberg]

On Tue, Oct 28, 2014 at 5:48 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Tuesday, October 28, 2014 04:25:05 PM Matt Turner wrote:
>> On Tue, Oct 28, 2014 at 3:17 PM, Kristian Høgsberg <krh at bitplanet.net>
> wrote:
>> > This flag signals that we have a SIMD8 VS shader so we can set up the
>> > corresponding state accordingly.  This boils down to setting
>> > the BDW+ SIMD8 enable bit in 3DSTATE_VS and making UBO and pull
>> > constant buffers use dword pitch.
>> >
>> > Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_context.h          |  5 ++++-
>> >  src/mesa/drivers/dri/i965/brw_defines.h          |  2 ++
>> >  src/mesa/drivers/dri/i965/brw_gs_surface_state.c |  2 +-
>> >  src/mesa/drivers/dri/i965/brw_vs_surface_state.c | 10 ++++++++--
>> >  src/mesa/drivers/dri/i965/brw_wm_surface_state.c |  7 ++++---
>> >  src/mesa/drivers/dri/i965/gen8_vs_state.c        |  2 ++
>> >  6 files changed, 21 insertions(+), 7 deletions(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_context.h
> b/src/mesa/drivers/dri/i965/brw_context.h
>> > index eb37e75..e7cd30f 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_context.h
>> > +++ b/src/mesa/drivers/dri/i965/brw_context.h
>> > @@ -543,6 +543,8 @@ struct brw_vec4_prog_data {
>> >      * is the size of the URB entry used for output.
>> >      */
>> >     GLuint urb_entry_size;
>> > +
>> > +   bool simd8;
>>
>> brw_vec4_prog_data is going to be the prog_data struct for SIMD8
>> vertex shaders? :\
>>
>> brw_gs_prog_data, which inherits brw_vec4_prog_data, has
>>
>>    /**
>>     * Dispatch mode, can be any of:
>>     * GEN7_GS_DISPATCH_MODE_DUAL_OBJECT
>>     * GEN7_GS_DISPATCH_MODE_DUAL_INSTANCE
>>     * GEN7_GS_DISPATCH_MODE_SINGLE
>>     */
>>    int dispatch_mode;
>>
>> Maybe it shouldn't hold the values of the things in the comment
>> directly (since they're things like 2<<11) but shouldn't we pull this
>> field out and have an enum or something?
>
> I like that plan.  A bunch of stages can (or will be able to) run in 4x1
> (single), 4x2 dual object, 4x2 dual instance, or SIMD8 mode.
>
> enum shader_dispatch_mode {
>    DISPATCH_MODE_4X2_DUAL_OBJECT,
>    DISPATCH_MODE_4X2_DUAL_INSTANCE,
>    DISPATCH_MODE_4X1_SINGLE,
>    DISPATCH_MODE_SIMD8,
> };
>
> The state upload code would use these values.

I think that makes sense once we get SIMD8 GS and at that point it
will be an easy enough change to make. For now, we only have 4x2 or
SIMD8 for VS so a simd8 flag is sufficient.  I'm not saying it's a bad
idea, but let's do it when we need it.

As for using brw_vec4_prog_data for scalar vs, I agree that it looks
weird, but there's nothing vec4 specific in there.  A better name may
be brw_vue_prog_data, but is this really worse than using fs_visitor
to generate vs code?  Either way, we have some renaming to decide on,
which can do before or after this lands.

Kristian

>> >  const struct brw_tracked_state brw_gs_ubo_surfaces = {
>> > diff --git a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
> b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
>> > index 1cc96cf..24bc06d 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
>> > +++ b/src/mesa/drivers/dri/i965/brw_vs_surface_state.c
>> > @@ -112,6 +112,7 @@ static void
>> >  brw_upload_vs_pull_constants(struct brw_context *brw)
>> >  {
>> >     struct brw_stage_state *stage_state = &brw->vs.base;
>> > +   bool dword_pitch;
>>
>> I can't figure out the name of this variable. In most any context I
>> would imagine 'dword_pitch' is an integer.
>>
>> What does this mean, and can we name it something more descriptive?
>
> See brw_create_constant_surface in brw_wm_surface_state.c.  For SIMD8 shader
> access, we configure the constant buffer SURFACE_STATE to use a pitch of 4,
> while for SIMD4x2 access, we configure it to use a pitch of 16 (vec4 size).
>
> I believe Eric introduced the name, and it's been around for quite a while
> now.  Feel free to submit a patch to rename it.
>
> --Ken