[Mesa-dev] [PATCH v2 3/3] mesa/st: enable ARB_fragment_layer_viewport

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 3:50 PM, Roland Scheidegger <sroland at vmware.com> wrote:
> Am 23.06.2014 21:14, schrieb Ilia Mirkin:
>> On Mon, Jun 23, 2014 at 12:01 PM, Roland Scheidegger <sroland at vmware.com> wrote:
>>> Am 23.06.2014 17:18, schrieb Ilia Mirkin:
>>>> On Mon, Jun 23, 2014 at 11:06 AM, Roland Scheidegger <sroland at vmware.com> wrote:
>>>>> Am 23.06.2014 16:43, schrieb Ilia Mirkin:
>>>>>> On Mon, Jun 23, 2014 at 9:51 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>> On Mon, Jun 23, 2014 at 9:39 AM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
>>>>>>>> If multiple viewports are supported, that implies the presence of a GS
>>>>>>>> and layered rendering, so we can enable ARB_fragment_layer_viewport as
>>>>>>>> well.
>>>>>>>>
>>>>>>>> Signed-off-by: Ilia Mirkin <imirkin at alum.mit.edu>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Untested on r600, but nv50/nvc0/llvmpipe seem to pass basic testing.
>>>>>>>
>>>>>>> Grr... actually llvmpipe fails for viewport, but works for layer.
>>>>>>
>>>>>> But... it draws all black instead of red, which means it's probably
>>>>>> either my or llvmpipe's understanding of default viewport state that's
>>>>>> wrong. I assumed that by default viewports were initialized s.t. they
>>>>>> would "Just Work" without having to be touched. Or... it's not
>>>>>> clamping the viewport index properly. Will investigate later.
>>>>> Hmm I'm not sure. This was tested with d3d10 rules which of course
>>>>> doesn't have the commands to set all viewports at once. But this is
>>>>> handled by the state tracker.
>>>>
>>>> Well, the tests don't set viewports at all. I was under the
>>>> (potentially incorrect) assumption that the default state was good
>>>> enough.
>>> Yes it should be - the viewport should be set somewhere (I'm not exactly
>>> sure where it gets set) and since in GL the "old" commands just set all
>>> viewports this should work.
>>
>> This is the first time I'm looking at llvmpipe in detail, so forgive
>> the potential stupidity...
>>
>> It seems like lp_state_fs.c:lp_llvm_viewport isn't clamping the
>> viewport index. Should it be? [Or is there some non-obvious place
>> where it's actually being clamped?] It seems like the layer is clamped
>> explicitly in lp_setup_tri and lp_setup_point.
>
> This is sort of complicated.
> Note that for ordinary fs input handling, you don't see viewport index
> handling anywhere in llvmpipe - this is just handled like any other
> ordinary attribute (through the interpolation stuff).
> The code in lp_state_fs for viewport handling is just to get the correct
> depth range, so you are right the viewport index must be clamped. The
> clamping is done in lp_setup_tri() however (watch out for
> viewport_index), the value then gets passed to rasterization (via
> tri->inputs) and there written into the thread_data (in
> lp_rast_shade_tile / lp_rast_shade_quads_mask). This is the value the
> code in lp_state_fs.c will then pick up to fetch the correct depthrange
> values (for depth clamping).
>
> Though now that I look at it, this looks wrong in draw:
>
> draw_clamp_viewport_idx(int idx)
> {
>    return ((PIPE_MAX_VIEWPORTS > idx || idx < 0) ? idx : 0);
> }
>
>
> The same clamping in llvmpipe looks like this:
>
> lp_clamp_viewport_idx(int idx)
> {
>    return (PIPE_MAX_VIEWPORTS > idx && idx >= 0) ? idx : 0;
> }
>
> Would only make a difference for negative viewport indices though (and
> don't ask me what would happen but draw might indeed fetch the viewport
> values from "random" memory locations so the result might be quite
> unpredictable...).
>
> Roland

And a viewport index of -1 was one of my test cases. That's probably it!