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

[Ilia Mirkin]

On Mon, Jun 23, 2014 at 3:52 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> 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!

Just to close the loop here, this was indeed it. With the patch that
fixes that (which is already in the repo), llvmpipe passes. I've also
pushed patches 1 + 2 of this series. The only thing left to test is
r600... anyone with the hw can grab the latest master, grab the piglit
tests from http://patchwork.freedesktop.org/patch/28375/, and run them
with

MESA_EXTENSION_OVERRIDE=GL_ARB_fragment_layer_viewport
bin/shader_runner ... -fbo -auto

If it passes, I think it's fine to push this out with a cap. If it fails... ugh.

  -ilia