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 <i...@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 <chr...@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);
>> }
>>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
