[Mesa-dev] [PATCH 03/17] mesa/st: Set _NumSamples in update_framebuffer_state()
eocallaghan at alterapraxis.com
eocallaghan at alterapraxis.com
Fri Mar 25 12:11:31 UTC 2016
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.
>
>>>
>>> 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 [1]
>>
>>
>
>
> Links:
> ------
> [1] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
More information about the mesa-dev
mailing list