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

[Ilia Mirkin]

On Tue, Jun 24, 2014 at 12:44 PM, Tobias Droste <tdroste at gmx.de> wrote:
> Hi,
>
> this does not seem to work on r600g (or I'm too stupid ;-)).
> I updated to latest mesa master, patched piglit and this is the result:
>
> $ glxinfo
> [...]
> OpenGL vendor string: X.Org
> OpenGL renderer string: Gallium 0.4 on AMD JUNIPER
> OpenGL core profile version string: 3.3 (Core Profile) Mesa 10.3.0-devel
> (git-0c181cd)
> OpenGL core profile shading language version string: 3.30
> OpenGL core profile context flags: (none)
> OpenGL core profile profile mask: core profile
> [...]
>
> $ MESA_EXTENSION_OVERRIDE=GL_ARB_fragment_layer_viewport bin/shader_runner
> tests/spec/arb_fragment_layer_viewport/layer-gs-write-simple.shader_test -fbo
> -auto
> piglit: info: Requested an OpenGL 3.2 Core Context, and received a matching
> 3.3 context
>
> Probe color at (0,0)
>   Expected: 0.000000 1.000000 0.000000
>   Observed: 1.000000 0.000000 0.000000
> PIGLIT: {'result': 'fail' }
>
> $ MESA_EXTENSION_OVERRIDE=GL_ARB_fragment_layer_viewport bin/shader_runner
> tests/spec/arb_fragment_layer_viewport/viewport-gs-write-simple.shader_test -
> fbo -auto
> piglit: info: Requested an OpenGL 3.2 Core Context, and received a matching
> 3.3 context
>
> Probe color at (0,0)
>   Expected: 0.000000 1.000000 0.000000
>   Observed: 1.000000 0.000000 0.000000
> PIGLIT: {'result': 'fail' }

Boooo :(

Could I trouble you to figure out which of the 4 draws (in each) is
failing? (Just comment out the "probe" lines, leaving only one in for
each)

Thanks for testing, BTW!

  -ilia

>
> Tobias
>
> Am Mo, 23. Juni 2014, 23:00:15 schrieb 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
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev