[Mesa-dev] [PATCH 04/12] mesa: Add a helper function _mesa_get_min_invocations_per_fragment()

[Paul Berry]

On 25 October 2013 16:45, Anuj Phogat <anuj.phogat at gmail.com> wrote:

> Thsi function is used to test if we need to do per sample shading or
> per fragment shading.
>

s/Thsi/This/


>
> Signed-off-by: Anuj Phogat <anuj.phogat at gmail.com>
> ---
>  src/mesa/program/program.c | 31 +++++++++++++++++++++++++++++++
>  src/mesa/program/program.h |  3 +++
>  2 files changed, 34 insertions(+)
>

This patch fails to compile because it relies on SYSTEM_BIT_SAMPLE_ID and
SYSTEM_BIT_SAMPLE_POS, which are added in patch 5.  I think you can fix
that by moving this patch after patch 5.


>
> diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
> index 093d372..e12e6ec 100644
> --- a/src/mesa/program/program.c
> +++ b/src/mesa/program/program.c
> @@ -1024,3 +1024,34 @@ _mesa_postprocess_program(struct gl_context *ctx,
> struct gl_program *prog)
>
>     }
>  }
> +
> +/* Gets the minimum number of shader invocations per fragment.
> + * This function is useful to determine if we need to do per
> + * sample shading or per fragment shading.
> + */
> +GLint
> +_mesa_get_min_invocations_per_fragment(struct gl_context *ctx,
> +                                       const struct gl_fragment_program
> *prog)
> +{
> +   /* From ARB_sample_shading specification:
> +    * "Using gl_SampleID in a fragment shader causes the entire shader
> +    *  to be evaluated per-sample."
> +    *
> +    * "Using gl_SamplePosition in a fragment shader causes the entire
> +    *  shader to be evaluated per-sample."
> +    *
> +    * "If MULTISAMPLE or SAMPLE_SHADING_ARB is disabled, sample shading
> +    *  has no effect."
>

There's a little more text in the spec right after this, which I think is
worth quoting:

    Otherwise, an implementation must provide a minimum of

        max(ceil(MIN_SAMPLE_SHADING_VALUE_ARB*SAMPLES),1)

    unique color values and sets of texture coordinates for each
    fragment.

Since that justifies the formula you use in the "else if" block.


> +    */
> +   if (ctx->Multisample.Enabled) {
> +      if (prog->Base.SystemValuesRead & SYSTEM_BIT_SAMPLE_ID ||
> +          prog->Base.SystemValuesRead & SYSTEM_BIT_SAMPLE_POS)
> +         return ctx->DrawBuffer->Visual.samples;
> +      else if (ctx->Multisample.SampleShading)
> +         return ceil(ctx->Multisample.MinSampleShadingValue *
> +                     ctx->DrawBuffer->Visual.samples);
>

If the user sets ctx->Multisample.MinSampleShadingValue to 0.0 (which is
allowed), then this function will return 0.  I think that risks bugs, since
it's the only situation in which this funciton can return 0, so the caller
might not realize they need to be prepared for that.

Let's just implement exactly the formula from the spec, which I believe
works out to:

         return MAX2(ceil(ctx->Multisample.MinSampleShadingValue *
                          ctx->DrawBuffer->Visual.samples), 1);



> +      else
> +         return 1;
> +   }
> +   return 1;
> +}
> diff --git a/src/mesa/program/program.h b/src/mesa/program/program.h
> index 34965ab..353ccab 100644
> --- a/src/mesa/program/program.h
> +++ b/src/mesa/program/program.h
> @@ -187,6 +187,9 @@ _mesa_valid_register_index(const struct gl_context
> *ctx,
>  extern void
>  _mesa_postprocess_program(struct gl_context *ctx, struct gl_program
> *prog);
>
> +extern GLint
> +_mesa_get_min_invocations_per_fragment(struct gl_context *ctx,
> +                                       const struct gl_fragment_program
> *prog);
>
>  static inline GLuint
>  _mesa_program_target_to_index(GLenum v)
> --
> 1.8.1.4
>
>
With those changes, the patch is:

Reviewed-by: Paul Berry <stereotype441 at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20131028/a6dffd9b/attachment-0001.html>