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

[Ilia Mirkin]

On Fri, Mar 25, 2016 at 8:11 AM,  <eocallaghan at alterapraxis.com> wrote:
> On 2016-03-25 22:20, Ilia Mirkin wrote:
>>
>> 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.
>
>
> I was not referring to loop semantics or if a compiler can understand how
> to lower a loop correctly, you didn't really read what I said. Point is,
> parameterizing the loop with a function argument to save one line of code
> while losing some safety and determinism does hardly anything to make a
> argument for this to be changed imho. I much prefer how it is now, a simple
> constant deterministic loop, very clear.

OK, so you agree with everything I said wrt simplification of the loop
logic, except you want to keep starting at 32 instead of the upper
bound which you already computed earlier and is guaranteed to be the
maximum supported number of samples (and <= 32)?

That's confusing and openly inefficient. It's premature optimization
just like starting the loop at 32 and not 1024 is premature
optimization... it's not really optimization at all. If I look at that
in a year, I'll be like "huh, why does it start at 32 and not
MaxFramebufferSamples, must be something odd going on". I would like
to avoid such a thought process for myself or anyone else down the
line.

  -ilia