[Mesa-dev] [PATCH 09/11] nir: add lowering stage for user-clip-planes / clipdist

[Erik Faye-Lund]

On Tue, Sep 15, 2015 at 6:49 PM, Rob Clark <robdclark at gmail.com> wrote:
> On Tue, Sep 15, 2015 at 12:39 PM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>> On Mon, Sep 14, 2015 at 11:53 AM, Erik Faye-Lund <kusmabite at gmail.com> wrote:
>>> On Sun, Sep 13, 2015 at 5:51 PM, Rob Clark <robdclark at gmail.com> wrote:
>>>> From: Rob Clark <robclark at freedesktop.org>
>>>>
>>>> The vertex shader lowering adds calculation for CLIPDIST, if needed
>>>> (ie. user-clip-planes), and the frag shader lowering adds conditional
>>>> kills based on CLIPDIST value (which should be treated as a normal
>>>> interpolated varying by the driver).
>>>
>>> <snip>
>>>
>>>> +
>>>> +/*
>>>> + * FS lowering
>>>> + */
>>>> +
>>>> +static void
>>>> +lower_clip_fs(nir_function_impl *impl, unsigned ucp_enables,
>>>> +              nir_variable **in)
>>>> +{
>>>> +   nir_ssa_def *clipdist[MAX_CLIP_PLANES];
>>>> +   nir_builder b;
>>>> +
>>>> +   nir_builder_init(&b, impl);
>>>> +   b.cursor = nir_before_cf_list(&impl->body);
>>>> +
>>>> +   if (ucp_enables & 0x0f)
>>>> +      load_clipdist_input(&b, in[0], &clipdist[0]);
>>>> +   if (ucp_enables & 0xf0)
>>>> +      load_clipdist_input(&b, in[1], &clipdist[4]);
>>>> +
>>>> +   for (int plane = 0; plane < MAX_CLIP_PLANES; plane++) {
>>>> +      if (ucp_enables & (1 << plane)) {
>>>> +         nir_intrinsic_instr *discard;
>>>> +         nir_ssa_def *cond;
>>>> +
>>>> +         cond = nir_flt(&b, clipdist[plane], nir_imm_float(&b, 0.0));
>>>> +
>>>> +         discard = nir_intrinsic_instr_create(b.shader,
>>>> +                                              nir_intrinsic_discard_if);
>>>> +         discard->src[0] = nir_src_for_ssa(cond);
>>>> +         nir_builder_instr_insert(&b, &discard->instr);
>>>
>>> I think it's worth noting that this isn't *strictly* correct for
>>> multi-sampling, unless the shader is s run with per-sample shading
>>> (ala GL_ARB_sample_shading). Otherwise, all samples for a pixel will
>>> get the same discard-condition, leading to aliasing along the
>>> resulting edge.
>>>
>>> That being said, per-fragment clipping is better than no clipping, so
>>> it's clearly an improvement.
>>
>> So, in order to do this correctly for MSAA, I guess you'd need to use
>> SYSTEM_VALUE_SAMPLE_POS and FRAG_RESULT_SAMPLE_MASK, to perform
>> alpha-testing for each sample-point.
>
> I think it still involves being able to run the frag shader once per
> sample..  although blob does seem to expose OES_sample_shading so
> maybe that is possible?

No, that's the whole point of the above, to not have to run the entire
shader per sample, only the clipping-bit.

> I guess I'd care about it more once I supported MSAA ;-)
>
> (and tbh, all this effort was mostly just to get neverball to render
> correctly :-P)

Oh, I just meant that it might be neat to add to the commit message. I
wouldn't go ahead and implement this just yet.