[Mesa-dev] [PATCH 2/2] i965: Respect driconf option clamp_max_samples

[Chad Versace]

On 11/01/2013 02:37 PM, Paul Berry wrote:
> On 31 October 2013 18:36, Chad Versace <chad.versace at linux.intel.com> wrote:
>
>> Clamp gen7 GL_MAX_SAMPLES to 0, 4, or 8.
>> Clamp gen6 GL_MAX_SAMPLES to 0 or 4.
>> Clamp other gens to 0.
>>
>> CC: Eric Anholt <eric at anholt.org>
>> Signed-off-by: Chad Versace <chad.versace at linux.intel.com>
>> ---
>>   src/mesa/drivers/dri/i965/brw_context.c  | 35
>> +++++++++++++++++++++++---------
>>   src/mesa/drivers/dri/i965/intel_screen.c |  1 +
>>   2 files changed, 26 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/mesa/drivers/dri/i965/brw_context.c
>> b/src/mesa/drivers/dri/i965/brw_context.c
>> index 38147e9..60a201f 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;
>>
>> +   uint32_t max_samples;
>> +   const uint32_t *legal_max_samples;
>> +   uint32_t clamp_max_samples;
>> +
>>      ctx->Const.QueryCounterBits.Timestamp = 36;
>>
>>      ctx->Const.StripTextureBorder = true;
>> @@ -333,19 +337,30 @@ 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) {
>> +      legal_max_samples = (uint32_t[]){8, 4, 0};
>>         ctx->Const.MaxProgramTextureGatherComponents = 4;
>> +   } else if (brw->gen == 6) {
>> +      legal_max_samples = (uint32_t[]){4, 0};
>> +   } else {
>> +      legal_max_samples = (uint32_t[]){0};
>> +   }
>> +
>> +   /* Set max_samples = min(max(legal_max_samples), clamp_max_samples). */
>>
>
> I don't think the comment matches your intention.  How about this instead?
>
>     /* Set max_samples to the first (and hence largest) element of
>      * legal_max_samples that is <= clamp_max_samples.  Or, if
>      * clamp_max_samples < 0, simply the first element of legal_max_samples.
>      */
>
>
>
>> +   max_samples = 0;
>> +   clamp_max_samples = driQueryOptioni(&brw->optionCache,
>> "clamp_max_samples");
>> +   for (uint32_t i = 0; legal_max_samples[i] != 0; ++i) {
>> +      if (legal_max_samples[i] <= clamp_max_samples) {
>> +         max_samples = legal_max_samples[i];
>> +         break;
>> +      }
>>
>
> If clamp_max_samples < 0, this code sets max_samples to 0.  That's not what
> you want; you want it to be set to legal_max_samples[0] in this case.

I tested the code and it works as I intended. If the user sets clamp_max_samples=-1,
then GL_MAX_SAMPLES remains unclamped.

But... I was accidentally too clever in this code. That's why it actually works
despite appearing otherwise. The cleverness is that driQueryOptioni() returns
an int, but I declared clamp_max_samples as a uint. Therefore, if the user
sets the driconf option clamp_max_samples tonegative, then clamp_max_samples
the variable becomes a very large uint. That cleverness is what makes
this comment true:
   /* Set max_samples = min(max(legal_max_samples), clamp_max_samples). */

I object to that sort of cleverness. So expect a new clever-free patch soon.