[Mesa-dev] [PATCH] i965: Rewrite ir_tex to ir_txl with lod 0 for vertex shaders

[Kristian Høgsberg]

On Wed, Apr 15, 2015 at 4:28 PM, Kenneth Graunke <kenneth at whitecape.org> wrote:
> On Wednesday, April 15, 2015 02:47:01 PM Ian Romanick wrote:
>> On 04/15/2015 12:41 PM, Kristian Høgsberg wrote:
>> > The ir_tex opcode turns into a sample or sample_c message, which will try to
>> > compute derivatives to determine the lod. This produces garbage for
>> > non-fragment shaders where the sample coordinates don't correspond to
>> > subspans.
>> >
>> > We fix this by rewriting the opcode from ir_tex to ir_txl and setting the
>> > lod to 0.
>>
>> This seems logical, but... why the heck hasn't this been a problem
>> before?  Is this some weirdness of just BSW?  Should we perhaps only do
>> this on BSW?
>
> In the vec4 backend, we've always made ir_tex use the SAMPLE_LOD message
> with lod set to 0.0 - so Gen4-7.5 have always done this workaround.
>
> When Kristian added SIMD8 VS support (Oct 2014, Gen8+), we started using
> SAMPLE (by virtue of not changing the FS behavior).  We honestly have no
> idea why it appears to work on Broadwell - it sure seems like it ought
> to be broken there.  Maybe we've just been lucky.
>
> There's also a message header bit for "Force LOD to Zero", which the
> docs say is mandatory if you're going to use the "SAMPLE" message in
> SIMD4x2 stages (i.e. not fragment).  We've wondered whether headerless
> messages in non-fragment stages magically get that right.  That could
> make the issue less noticable...

More speculation: since SKL adds a new message type (sample_c_lz) for
sample_c with lod=0, it could be that some of that got pulled into
BSW. Maybe pre-SKL, the thread dispatcher set the header bit in the
thread payload for VS so that it carries over in message header for
sample and sample_c and gets the right behavior. If BSW removes that
special case in favor of a dedicated opcode, it would explain why it
breaks on BSW but not BDW. The docs doesn't mention sample_c_lz for
BSW, so who knows...

> Using SAMPLE_LOD is easier than setting the message header bit, and
> seems just as effective.  I think we should do it on Broadwell too.
>
> Thanks for fixing this, Kristian!

Thanks for the help.

Kristian

> Cc: "10.5" <mesa-stable at lists.freedesktop.org>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>
>
>>
>> > https://bugs.freedesktop.org/show_bug.cgi?id=89457
>>
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89457
>>
>> That allows the bin/bugzilla_mesa.sh script to find it for generating
>> release notes.
>>
>> > Signed-off-by: Kristian Høgsberg <krh at bitplanet.net>
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_fs_visitor.cpp | 9 +++++++++
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> > index 0049b2d..4e99366 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs_visitor.cpp
>> > @@ -1839,6 +1839,15 @@ fs_visitor::emit_texture_gen7(ir_texture_opcode op, fs_reg dst,
>> >        offset_value.file != BAD_FILE && offset_value.file != IMM;
>> >     bool coordinate_done = false;
>> >
>> > +   /* The sampler can only meaningfully compute LOD for fragment shader
>> > +    * messages. For all other stages, we change the opcode to ir_txl and
>> > +    * hardcode the LOD to 0.
>> > +    */
>> > +   if (stage != MESA_SHADER_FRAGMENT && op == ir_tex) {
>> > +      op = ir_txl;
>> > +      lod = fs_reg(0.0f);
>> > +   }
>> > +
>> >     /* Set up the LOD info */
>> >     switch (op) {
>> >     case ir_tex:
>> >
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>>