[Mesa-dev] [PATCH 09/12] i965/fs: Add support for gl_ViewportIndex input

[Chris Forbes]

Ian,

I'd thought about that a bit while building this, and struggled to
find cases where this was observable in a defined fragment shader
execution.

The ARB_viewport_array spec says:

    If the value of the viewport index is outside the range zero to the value
    of MAX_VIEWPORTS minus one, the results of the viewport
    transformation are undefined.

It seems absurd to carry around an extra real slot which only adds any
value in a case where we're not required to be performing any
particular fragment shader invocations at all.

I can see cases where an out-of-range gl_Layer *almost* makes sense --
only interactions with the framebuffer are undefined, so you could
have no framebuffer writes, no fragment tests, and then do something
based on gl_Layer with atomics, images, or shader storage buffers. But
it's still a mad thing to do.

Do you know the rationale for having this language in the spec?

In any case, happy to park this for now -- it just looked like an easy
win, and it turns out it's not quite.

-- Chris

On Mon, Jan 27, 2014 at 5:59 AM, Ian Romanick <idr at freedesktop.org> wrote:
> On 01/24/2014 10:51 PM, Chris Forbes wrote:
>> Same idea as gl_Layer -- this is delivered in part of R0.0.
>
> NAK.
>
> The spec says:
>
>     "... the fragment stage will read the same value written by
>     the geometry stage, even if that value is out of range."
>
> If the geometry shader passes 0xDEADBEEF, the fragment shader is
> supposed to read 0xDEADBEEF.  That won't fit in the 4-bits available in
> R0.0.  That's why I didn't implement this when I did the
> GL_ARB_viewport_array work. :)
>
> I think I want to provide an Intel extension that provides the value the
> hardware will actually use.  I'm thinking take the existing ARB spec and
> replace that one sentence with
>
>     "If the value written by the geometry stage is out of range, the
>     value read by the fragment stage is undefined."
>
> We would also replace the next sentence with:
>
>     "If a fragment shader contains a static access to gl_ViewportIndex,
>     it will NOT count against the implementation defined limit for the
>     maximum number of inputs to the fragment stage."
>
> There are probably a couple other little edits too.
>
> I'm also concerned about interactions with this extension and SSO.
> Since we have to assign a real slot for gl_ViewportIndex, a separable
> geometry shader that writes it will always have to write to the shadow
> copy.  If the separable fragment shader doesn't read it, the varying
> layouts may not match. :(
>
>> Signed-off-by: Chris Forbes <chrisf at ijw.co.nz>
>> ---
>>  src/mesa/drivers/dri/i965/brw_fs.cpp         | 22 ++++++++++++++++++++++
>>  src/mesa/drivers/dri/i965/brw_fs.h           |  1 +
>>  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp |  2 ++
>>  3 files changed, 25 insertions(+)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> index 17d5237..e32133b 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> @@ -1294,6 +1294,28 @@ fs_visitor::emit_layer_setup(ir_variable *ir)
>>     return reg;
>>  }
>>
>> +fs_reg *
>> +fs_visitor::emit_viewport_index_setup(ir_variable *ir)
>> +{
>> +   /* The value for gl_ViewportIndex is provided in bits 30:27 of R0.0. */
>> +
>> +   /* These bits are actually present on all Gen4+ h/w, but until GS is enabled
>> +    * on earlier platforms we don't expect to get here on anything earlier
>> +    * than Gen7.
>> +    */
>> +   assert(brw->gen >= 7);
>> +
>> +   this->current_annotation = "gl_ViewportIndex";
>> +   fs_reg *reg = new(this->mem_ctx) fs_reg(this, ir->type);
>> +   fs_reg temp = fs_reg(this, glsl_type::int_type);
>> +   emit(BRW_OPCODE_SHR, temp,
>> +         fs_reg(retype(brw_vec1_grf(0, 0), BRW_REGISTER_TYPE_D)),
>> +         fs_reg(brw_imm_d(27)));
>> +   emit(BRW_OPCODE_AND, *reg, temp,
>> +         fs_reg(brw_imm_d(0x0f)));
>> +   return reg;
>> +}
>> +
>>  fs_reg
>>  fs_visitor::fix_math_operand(fs_reg src)
>>  {
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs.h b/src/mesa/drivers/dri/i965/brw_fs.h
>> index e04c341..e47fff4 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs.h
>> +++ b/src/mesa/drivers/dri/i965/brw_fs.h
>> @@ -346,6 +346,7 @@ public:
>>     fs_reg *emit_sampleid_setup(ir_variable *ir);
>>     fs_reg *emit_samplemaskin_setup(ir_variable *ir);
>>     fs_reg *emit_layer_setup(ir_variable *ir);
>> +   fs_reg *emit_viewport_index_setup(ir_variable *ir);
>>     fs_reg *emit_general_interpolation(ir_variable *ir);
>>     void emit_interpolation_setup_gen4();
>>     void emit_interpolation_setup_gen6();
>> diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> index e949f4b..8864cf2 100644
>> --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> @@ -63,6 +63,8 @@ fs_visitor::visit(ir_variable *ir)
>>        reg = emit_frontfacing_interpolation(ir);
>>        } else if (!strcmp(ir->name, "gl_Layer")) {
>>           reg = emit_layer_setup(ir);
>> +      } else if (!strcmp(ir->name, "gl_ViewportIndex")) {
>> +         reg = emit_viewport_index_setup(ir);
>>        } else {
>>        reg = emit_general_interpolation(ir);
>>        }
>>
>