[Mesa-dev] [PATCH 5/6] Implement the proper packing for the stencil payload

[Matt Turner]

On Tue, Oct 20, 2015 at 3:25 PM, Ben Widawsky <ben at bwidawsk.net> wrote:
> On Tue, Oct 20, 2015 at 03:17:38PM -0700, Matt Turner wrote:
>> On Tue, Oct 20, 2015 at 2:29 PM, Ben Widawsky
>> <benjamin.widawsky at intel.com> wrote:
>> > This patch is split out for review. It will be squashed before pushing.
>> > ---
>> >  src/mesa/drivers/dri/i965/brw_defines.h        |  1 +
>> >  src/mesa/drivers/dri/i965/brw_fs.cpp           |  6 +++-
>> >  src/mesa/drivers/dri/i965/brw_fs.h             |  2 ++
>> >  src/mesa/drivers/dri/i965/brw_fs_generator.cpp | 44 ++++++++++++++++++++++++++
>> >  src/mesa/drivers/dri/i965/brw_shader.cpp       |  2 ++
>> >  5 files changed, 54 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h
>> > index c67728b..2c5cd7a 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_defines.h
>> > +++ b/src/mesa/drivers/dri/i965/brw_defines.h
>> > @@ -915,6 +915,7 @@ enum opcode {
>> >      */
>> >     FS_OPCODE_FB_WRITE_LOGICAL,
>> >
>> > +   FS_OPCODE_PACK_STENCIL_REF,
>>
>> Listing this in the middle of four fbwrite opcodes seems wrong.
>>
>
> Can you please find me a better place? I searched all over and really wasn't
> sure where it belongs. It is FB write related (unless there is a more generic
> use of such packing), but it is indeed not an FB write.

Immediately after the fbwrites would be better. Or how about above
VEC4_OPCODE_MOV_BYTES? The block started by VEC4_OPCODE_MOV_BYTES all
do regioned MOVs similar to this opcode?

My only concern was putting it in the middle of a group of opcodes it
doesn't belong to.

>> >     FS_OPCODE_BLORP_FB_WRITE,
>> >     FS_OPCODE_REP_FB_WRITE,
>> >     SHADER_OPCODE_RCP,
>> > diff --git a/src/mesa/drivers/dri/i965/brw_fs.cpp b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > index 560eb91..c962043 100644
>> > --- a/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > +++ b/src/mesa/drivers/dri/i965/brw_fs.cpp
>> > @@ -3440,7 +3440,11 @@ lower_fb_write_logical_send(const fs_builder &bld, fs_inst *inst,
>> >     if (src_stencil.file != BAD_FILE) {
>> >        assert(devinfo->gen >= 9);
>> >        assert(bld.dispatch_width() != 16);
>> > -      sources[length] = src_stencil;
>> > +
>> > +      sources[length] = bld.vgrf(BRW_REGISTER_TYPE_UD);
>> > +      bld.exec_all().annotate("FB write OS")
>>
>> OS?
>>
>
> "Output Stencil" It's the common form in all the payload definitions in the
> docs. I kind of liked keeping the doc term so it's searchable, even though we
> hadn't done that for other things like oMask (should be OM). I can rename it to
> whatever you like instead - just tell me what you want.

I don't have a preference. I just wouldn't have any idea what the
annotation meant if I saw it in INTEL_DEBUG=fs output, which seems
like it reduces the usefulness.