[Mesa-dev] [PATCH 03/17] mesa/st: Set _NumSamples in update_framebuffer_state()

[eocallaghan at alterapraxis.com]

On 2016-03-25 14:02, Ilia Mirkin wrote:
> On Thu, Mar 24, 2016 at 8:11 PM, Edward O'Callaghan
> <eocallaghan at alterapraxis.com> wrote:
>> Using PIPE_FORMAT_NONE to indicate what MSAA modes are supported
>> with a framebuffer using no attachment.
>> 
>> Signed-off-by: Edward O'Callaghan <eocallaghan at alterapraxis.com>
>> ---
>>  src/mesa/state_tracker/st_atom_framebuffer.c | 51 
>> ++++++++++++++++++++++++++++
>>  1 file changed, 51 insertions(+)
>> 
>> diff --git a/src/mesa/state_tracker/st_atom_framebuffer.c 
>> b/src/mesa/state_tracker/st_atom_framebuffer.c
>> index ae883a2..07854ca 100644
>> --- a/src/mesa/state_tracker/st_atom_framebuffer.c
>> +++ b/src/mesa/state_tracker/st_atom_framebuffer.c
>> @@ -64,6 +64,44 @@ update_framebuffer_size(struct 
>> pipe_framebuffer_state *framebuffer,
>>     framebuffer->height = MIN2(framebuffer->height, surface->height);
>>  }
>> 
>> +/**
>> + * Round up the requested multisample count to the next supported 
>> sample size.
>> + */
>> +static unsigned
>> +framebuffer_quantize_num_samples(struct pipe_screen *screen, unsigned 
>> num_samples)
>> +{
>> +   int quantized_samples = 0;
>> +   bool msaa_mode_supported;
>> +
>> +   if (!num_samples)
>> +      return 0;
>> +
>> +   assert(screen);
>> +
>> +   /* Assumes the highest supported MSAA is x32 on any hardware */
>> +   for (unsigned msaa_mode = 32; msaa_mode >= 1; msaa_mode = 
>> msaa_mode/2) {
> 
> This should probably start at MaxFramebufferSamples right? Also
> msaa_mode >= num_samples? [then you can get rid of the if below]

I did it in this manner because I don't trust all C compilers to warn 
sufficiently
on `num_samples' overflows turning this into a infinite loop even though 
it is a
unsigned type. The micro-optimization serves no purpose because the 
optimizer will
trivially reduce the loop down, not that it has that many iterations any 
way. The
loop as-is is both well bounded and deterministic, nice qualities to 
have.

"Premature optimization is the root of all evil" ~ Donald Knuth's.

> 
> And lastly I don't know if it's a valid assumption that we can always
> just divide by 2. That said, I don't know of any hw that actually
> supports non-power-of-two MSAA levels, so perhaps it's OK.

You are way over engineering here; it is a totally reasonable assumption 
and if such
hardware does exist which we would support (I can`t see any in the tree 
currently as
far as I am aware) then they can provide follow up fixes.

> 
>> +      assert(!(msaa_mode > 32 || msaa_mode == 0)); /* be safe from 
>> int overflows */
>> +      if (msaa_mode >= num_samples) {
>> +         /**
>> +          * For ARB_framebuffer_no_attachment, A format of
>> +          * PIPE_FORMAT_NONE implies what number of samples is
>> +          * supported for a framebuffer with no attachment. Thus the
>> +          * drivers callback must be adjusted for this.
>> +          */
>> +         msaa_mode_supported = screen->is_format_supported(screen,
>> +                                     PIPE_FORMAT_NONE, 
>> PIPE_TEXTURE_2D,
>> +                                     msaa_mode, 
>> PIPE_BIND_RENDER_TARGET);
>> +         /**
>> +          * Check if the MSAA mode that is higher than the requested
>> +          * num_samples is supported, and if so returning it.
>> +          */
>> +         if (msaa_mode_supported)
>> +            quantized_samples = msaa_mode;
>> +      }
>> +   }
>> +
>> +   return quantized_samples;
>> +}
>> 
>>  /**
>>   * Update framebuffer state (color, depth, stencil, etc. buffers)
>> @@ -72,6 +110,8 @@ static void
>>  update_framebuffer_state( struct st_context *st )
>>  {
>>     struct pipe_framebuffer_state *framebuffer = 
>> &st->state.framebuffer;
>> +   struct pipe_context *pipe = st->pipe;
>> +   struct pipe_screen *screen = pipe->screen;
>>     struct gl_framebuffer *fb = st->ctx->DrawBuffer;
>>     struct st_renderbuffer *strb;
>>     GLuint i;
>> @@ -82,6 +122,17 @@ update_framebuffer_state( struct st_context *st )
>>     framebuffer->width  = UINT_MAX;
>>     framebuffer->height = UINT_MAX;
>> 
>> +   /**
>> +    * Quantize the derived default number of samples:
>> +    *
>> +    * A query to the driver of supported MSAA values the
>> +    * hardware supports is done as to legalize the number
>> +    * of application requested samples, NumSamples.
>> +    * See commit eb9cf3c for more information.
>> +    */
>> +   fb->DefaultGeometry._NumSamples =
>> +      framebuffer_quantize_num_samples(screen, 
>> fb->DefaultGeometry.NumSamples);
>> +
>>     /*printf("------ fb size %d x %d\n", fb->Width, fb->Height);*/
>> 
>>     /* Examine Mesa's ctx->DrawBuffer->_ColorDrawBuffers state
>> --
>> 2.5.5
>> 
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev