[Mesa-dev] [PATCH 3/7] gallium: add FBFETCH opcode to retrieve the current sample value

[Ilia Mirkin]

On Fri, Jan 6, 2017 at 6:05 AM, Nicolai Hähnle <nhaehnle at gmail.com> wrote:
> On 05.01.2017 17:59, Ilia Mirkin wrote:
>>
>> On Thu, Jan 5, 2017 at 11:30 AM, Nicolai Hähnle <nhaehnle at gmail.com>
>> wrote:
>>>
>>> On 05.01.2017 17:02, Ilia Mirkin wrote:
>>>>
>>>>
>>>> On Thu, Jan 5, 2017 at 10:48 AM, Nicolai Hähnle <nhaehnle at gmail.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On 02.01.2017 21:41, Marek Olšák wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On Mon, Jan 2, 2017 at 7:01 AM, Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>>> ---
>>>>>>>  src/gallium/auxiliary/tgsi/tgsi_info.c     |  2 +-
>>>>>>>  src/gallium/docs/source/tgsi.rst           | 11 +++++++++++
>>>>>>>  src/gallium/include/pipe/p_shader_tokens.h |  2 +-
>>>>>>>  3 files changed, 13 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>>>>> b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>>>>> index 37549aa..e34b8c7 100644
>>>>>>> --- a/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>>>>> +++ b/src/gallium/auxiliary/tgsi/tgsi_info.c
>>>>>>> @@ -106,7 +106,7 @@ static const struct tgsi_opcode_info
>>>>>>> opcode_info[TGSI_OPCODE_LAST] =
>>>>>>>     { 1, 3, 0, 0, 0, 0, 0, COMP, "CMP", TGSI_OPCODE_CMP },
>>>>>>>     { 1, 1, 0, 0, 0, 0, 0, CHAN, "SCS", TGSI_OPCODE_SCS },
>>>>>>>     { 1, 2, 1, 0, 0, 0, 0, OTHR, "TXB", TGSI_OPCODE_TXB },
>>>>>>> -   { 0, 1, 0, 0, 0, 0, 1, NONE, "", 69 },      /* removed */
>>>>>>> +   { 1, 1, 0, 0, 0, 0, 0, OTHR, "FBFETCH", TGSI_OPCODE_FBFETCH },
>>>>>>>     { 1, 2, 0, 0, 0, 0, 0, COMP, "DIV", TGSI_OPCODE_DIV },
>>>>>>>     { 1, 2, 0, 0, 0, 0, 0, REPL, "DP2", TGSI_OPCODE_DP2 },
>>>>>>>     { 1, 2, 1, 0, 0, 0, 0, OTHR, "TXL", TGSI_OPCODE_TXL },
>>>>>>> diff --git a/src/gallium/docs/source/tgsi.rst
>>>>>>> b/src/gallium/docs/source/tgsi.rst
>>>>>>> index d2d30b4..accbe1d 100644
>>>>>>> --- a/src/gallium/docs/source/tgsi.rst
>>>>>>> +++ b/src/gallium/docs/source/tgsi.rst
>>>>>>> @@ -2561,6 +2561,17 @@ Resource Access Opcodes
>>>>>>>    image, while .w will contain the number of samples for
>>>>>>> multi-sampled
>>>>>>>    images.
>>>>>>>
>>>>>>> +.. opcode:: FBFETCH - Load data from framebuffer
>>>>>>> +
>>>>>>> +  Syntax: ``FBFETCH dst, output``
>>>>>>> +
>>>>>>> +  Example: ``FBFETCH TEMP[0], OUT[0]``
>>>>>>> +
>>>>>>> +  Returns the color of the current position in the framebuffer from
>>>>>>> +  before this fragment shader invocation. Always returns the same
>>>>>>> +  value from multiple calls for a particular output within a single
>>>>>>> +  invocation.
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I'm not a fan of this last sentence. It's true that we could somehow
>>>>> bend
>>>>> things in the compiler to make the sentence true, but
>>>>>
>>>>> (a) The statement is clearly false with a straight-forward
>>>>> implementation
>>>>> of
>>>>> the instruction: multiple fragment shaders can be simultaneously
>>>>> in-flight
>>>>> on the same pixel/sample. A second FBFETCH could happen after an
>>>>> earlier
>>>>> invocation on the same pixel finished and get the new framebuffer
>>>>> value.
>>>>>
>>>>> (b) I'm not aware of an API that actually requires this guarantee.
>>>>>
>>>>>
>>>>>> If the value is always the same, can it be declared as a system value
>>>>>> instead?
>>>>>
>>>>>
>>>>>
>>>>>
>>>>> I don't know. I'd remove the statement about the value always being the
>>>>> same
>>>>> to begin with. And with an eye to how this actually ends up being
>>>>> implemented, and possible interactions with
>>>>> ARB_fragment_shader_interlock,
>>>>> I'd say it makes sense (and our lives easier) for the TGSI to define
>>>>> _when_
>>>>> the framebuffer value is supposed to be read, and for that it makes
>>>>> sense
>>>>> to
>>>>> have an instruction for it.
>>>>
>>>>
>>>>
>>>> In case it's not obvious, this is primarily for
>>>> KHR_blend_equation_advanced. It's illegal to use it with overdraw
>>>> without a barrier first.
>>>
>>>
>>>
>>> Well, you get an undefined value :)
>>>
>>> My point is that there's a plausible implementation of FBFETCH as an
>>> instruction in which "the same value will be returned within a single
>>> invocation" is only guaranteed if the application follows the rules
>>> involving BlendBarrier.
>>>
>>>
>>>> There's also
>>>> KHR_blend_equation_advanced_coherent and EXT_shader_framebuffer_fetch,
>>>> which do allow overdraw. I'm not sure how the ordering is specified
>>>> (or, how it's specified for regular blending). The gl_LastFragData
>>>> stuff are a non-writable space, and as such, it makes sense that
>>>> they'd be computed once on invocation and then kept constant
>>>> throughout the shader run.
>>>
>>>
>>>
>>> It still leaves open the question of _when_ they should be computed,
>>> especially in the future when guarantees about the order of fragment
>>> shaders
>>> are required (i.e. KHR_blend_equation_advanced_coherent): If you load the
>>> data too early, you may lose some potential for parallelism because you
>>> have
>>> to spend more time waiting for earlier invocations to finish. This
>>> obviously
>>> depends on the details of the hardware, but ARB_fragment_shader_interlock
>>> suggests that it is (or will be?) quite common to have synchronization
>>> that
>>> is rather fine-grained.
>>
>>
>> This is a related to the implementation issues I had on nouveau with
>> the sysval - basically the code looks like
>>
>> if (advanced blend is enabled) {
>> MOV TEMP[0], SV
>> IF
>>   use(sv)
>> ELSE
>>   use(sv)
>> use(sv some more)
>> }
>>
>> And then the st_glsl_to_tgsi copy propagation logic pushes those SV's
>> down to the uses. This is further complicated by the fact that the
>> xyzw values are logically independent at the TGSI level. And now I'm
>> stuck either
>>
>> (a) getting the value once at the top of the shader, even if advanced
>> blend might be disabled entirely
>> (b) fetching the texture once per bb
>> (c) figuring out some code motion heuristics which presently don't
>> exist in nouveau
>>
>> So... the operation makes the most sense to me. How about "once you
>> call FBFETCH, all further invocations should return identical values"?
>
>
> I don't know about "should". What about "may"? This would allow the driver
> to "cache" the fbfetch result or fetch it only once at the start of the
> shader, but doesn't force it to do that. So things are straightforward for
> the driver both when a simple texture-fetch-based implementation should be
> used _and_ when there really is fixed function hardware. How about this:

I like may.

>
>> Example: ``FBFETCH TEMP[0], OUT[0]``
>>
>> Returns the current color of the current position in the framebuffer.
>> The result is undefined if the current position is rendered to twice
>> without a blend barrier in between.
>
> I think that's pretty much the condition at the API level, isn't it?

Yes. I just want to point out that this opcode is tied to the API operation.

Marek, are you good with this non-SV approach? If so, I'll respin my
original series to take Nicolai's suggestions into account.

Cheers,

  -ilia