[Mesa-dev] [PATCH] i965: Add driconf option clamp_max_samples

[Kenneth Graunke]

On 11/02/2013 08:41 AM, Chad Versace wrote:
> This clamps GL_MAX_SAMPLES to the given value. If negative, then no
> clamping occurs.
> 
> CC: Paul Berry <stereotype441 at gmail.com>
> CC: Eric Anholt <eric at anholt.net>
> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
> ---
> 
> 
> This patch lives on my branch driconf-clamp-max_samples.
> 
> 
>  src/mesa/drivers/dri/i965/brw_context.c  | 41 ++++++++++++++++++++++++--------
>  src/mesa/drivers/dri/i965/intel_screen.c |  8 ++++++-
>  2 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/brw_context.c b/src/mesa/drivers/dri/i965/brw_context.c
> index 38147e9..32faf72 100644
> --- a/src/mesa/drivers/dri/i965/brw_context.c
> +++ b/src/mesa/drivers/dri/i965/brw_context.c
> @@ -273,6 +273,10 @@ brw_initialize_context_constants(struct brw_context *brw)
>  {
>     struct gl_context *ctx = &brw->ctx;
>  
> +   int max_samples;
> +   const int *legal_max_samples;
> +   int clamp_max_samples;
> +

I'd prefer to see these declared at their use (okay in C99), but it's up to you.

I also think "supported_msaa_modes" might be nicer than "legal_max_samples."
Again, up to you though.

>     ctx->Const.QueryCounterBits.Timestamp = 36;
>  
>     ctx->Const.StripTextureBorder = true;
> @@ -333,19 +337,36 @@ brw_initialize_context_constants(struct brw_context *brw)
>  
>     ctx->Const.AlwaysUseGetTransformFeedbackVertexCount = true;
>  
> -   if (brw->gen == 6) {
> -      ctx->Const.MaxSamples = 4;
> -      ctx->Const.MaxColorTextureSamples = 4;
> -      ctx->Const.MaxDepthTextureSamples = 4;
> -      ctx->Const.MaxIntegerSamples = 4;
> -   } else if (brw->gen >= 7) {
> -      ctx->Const.MaxSamples = 8;
> -      ctx->Const.MaxColorTextureSamples = 8;
> -      ctx->Const.MaxDepthTextureSamples = 8;
> -      ctx->Const.MaxIntegerSamples = 8;
> +   if (brw->gen >= 7)
>        ctx->Const.MaxProgramTextureGatherComponents = 4;
> +

Would you mind adding this:

if (brw->gen >= 8) {
   legal_max_samples = (int[]){16, 8, 4, 2, 0};
} else if (brw->gen >= 7) {

> +   if (brw->gen >= 7) {
> +      legal_max_samples = (int[]){8, 4, 0};
> +   } else if (brw->gen == 6) {
> +      legal_max_samples = (int[]){4, 0};
> +   } else {
> +      legal_max_samples = (int[]){0};
> +   }

Also, hey, you used one of my favorite C99 features!  Nice :)

> +
> +   clamp_max_samples = driQueryOptioni(&brw->optionCache, "clamp_max_samples");
> +   if (clamp_max_samples < 0) {
> +      clamp_max_samples = INT_MAX;
> +   }
> +
> +   /* Set max_samples = min(max(legal_max_samples), clamp_max_samples). */

It doesn't actually - if I specify clamp_max_samples=3, I won't get max_samples == 3 :)

> +   max_samples = 0;
> +   for (int i = 0; legal_max_samples[i] != 0; ++i) {
> +      if (legal_max_samples[i] <= clamp_max_samples) {
> +         max_samples = legal_max_samples[i];
> +         break;
> +      }
>     }

This might be clearer as:

   int max_samples = legal_max_samples[0];
   int clamp_max_samples = driQueryOptioni(&brw->optionCache, "clamp_max_samples");
   if (clamp_max_samples > 0) {
      /* Select the largest supported MSAA mode at or below clamp_max_samples. */
      for (int i = 0; legal_max_samples[i] != 0; ++i) {
         if (legal_max_samples[i] <= clamp_max_samples) {
            max_samples = legal_max_samples[i];
            break;
         }
      }
   }

Since the true maximum is always the first element, you can just set it to that,
and override it to a smaller value when clamped.

That said, there's another larger problem with this patch:

$ clamp_max_samples=2 glxinfo |& grep 'OpenGL version'
OpenGL version string: 2.1 Mesa 10.0.0-devel (git-1733459)

I would prefer to see it not affect the version advertised.  This probably means
overriding these values later.

>  
> +   ctx->Const.MaxSamples = max_samples;
> +   ctx->Const.MaxColorTextureSamples = max_samples;
> +   ctx->Const.MaxDepthTextureSamples = max_samples;
> +   ctx->Const.MaxIntegerSamples = max_samples;
> +
>     ctx->Const.MinLineWidth = 1.0;
>     ctx->Const.MinLineWidthAA = 1.0;
>     ctx->Const.MaxLineWidth = 5.0;
> diff --git a/src/mesa/drivers/dri/i965/intel_screen.c b/src/mesa/drivers/dri/i965/intel_screen.c
> index eafafa2..ce8124b 100644
> --- a/src/mesa/drivers/dri/i965/intel_screen.c
> +++ b/src/mesa/drivers/dri/i965/intel_screen.c
> @@ -63,11 +63,17 @@ DRI_CONF_BEGIN
>        DRI_CONF_OPT_BEGIN_B(disable_derivative_optimization, "false")
>  	 DRI_CONF_DESC(en, "Derivatives with finer granularity by default")
>        DRI_CONF_OPT_END
> -
>     DRI_CONF_SECTION_END
> +
>     DRI_CONF_SECTION_QUALITY
>        DRI_CONF_FORCE_S3TC_ENABLE("false")
> +
> +      DRI_CONF_OPT_BEGIN(clamp_max_samples, int, -1)
> +              DRI_CONF_DESC(en, "Clamp the value of GL_MAX_SAMPLES to the "
> +                            "given integer. If negative, then do not clamp.")
> +      DRI_CONF_OPT_END
>     DRI_CONF_SECTION_END
> +
>     DRI_CONF_SECTION_DEBUG
>        DRI_CONF_NO_RAST("false")
>        DRI_CONF_ALWAYS_FLUSH_BATCH("false")
>