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

[Ilia Mirkin]

On Mar 25, 2016 4:43 AM, <eocallaghan at alterapraxis.com> wrote:
>
> 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.
>
>

I was going for clarity and simplicity, not runtime efficiency. Fewer lines
of code to read, fewer conditions. For loop semantics are fairly well
defined, compilers tend to get those things right.

>>
>> 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.

I'm flexible on this one... If no one else cares, I don't care either.

>
>
>>
>>> +      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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20160325/cef4dec7/attachment-0001.html>