[Mesa-dev] [PATCH 1/3] st/mesa: fix handling of NumSamples=1
Brian Paul
brianp at vmware.com
Wed Aug 2 19:49:57 UTC 2017
On 08/02/2017 01:18 PM, Roland Scheidegger wrote:
> Am 02.08.2017 um 20:35 schrieb Brian Paul:
>> On 08/02/2017 11:59 AM, Roland Scheidegger wrote:
>>> I think the problem here is that msaa surfaces with sample count 1 are
>>> not really supposed to exist in gallium.
>>> This is a rather awkward gl-ism, which isn't possible anywhere else,
>>> other apis have no distinction between a multisampled surface with
>>> sample count 1 and a non-multisampled surface with effectively sample
>>> count 1 too.
>>> So, drivers should not have to distinguish between msaa surfaces with
>>> sample count 1 and non-msaa surfaces, those should be the same (the
>>> state tracker takes care of not accidentally enabling things like
>>> alpha_to_coverage for non-msaa surfaces even if multisampling and
>>> alpha_to_coverage itself is enabled). This is how d3d10 (and our device)
>>> operate.
>>>
>>> I believe the correct solution would be to translate whatever is failing
>>> for those fake multsample surfaces (you mentioned
>>> glRenderbufferStorageMultisample()) away to something more appropriate
>>> for non-msaa surfaces (in the state tracker or driver,
>>
>> I'm not sure I understand what you mean there.
>>
>>
>>> I have no idea
>>> what actually goes wrong).
>>> But creating a non-msaa surface for a gl msaa surface with sample count
>>> 1 is the right thing to do.
>>
>> Let me give you a concrete example of what's happening.
>>
>> Several Piglit tests create FBO surfaces with
>> glRenderbufferStorageMultisample(samples=1). This means we're asking GL
>> for a multisampled renderbuffer with at least one sample. NVIDIA, for
>> example, creates a 2x msaa surface in this case. As-is, the state
>> tracker code for searching for a supported sample count is skipped so we
>> pass pipe_resource::nr_sample=1 to resource_create() and we get a
>> non-msaa surface. That's wrong.
> Why?
Because the caller of glRenderbufferStorageMultisample(samples=1)
expects to get a msaa surface. As it is now, we're not satisfying that
request.
> Like I said, there should be no difference in gallium between a msaa
> surface (with sample count 1) and a non-msaa surface.
I'm not disputing that. Though, with my svga_is_format_supported()
patch, I guess I am making that distinction. (more below)
> Things like alpha-to-coverage are still supposed to work (just as they
> do with d3d10).
>
> I just don't see the need to refuse a msaa surface with sample count 1.
> Albeit I suppose if a driver can't do the addtional stuff implied by
> msaa (such as alpha-to-coverage) then indeed there needs to be a way to
> distinguish this.
OK, so you're concerned with the is_format_supported() query.
>
> As far as I know, all drivers do not distinguish between resources with
> sample count 0 and 1 (at least they are not supposed to).
> Thus, I can't quite see how that additional code in the state tracker
> will help you - if a driver accepts some format with sample count 0, it
> will accept it with sample count 1 too, because in gallium there is
> supposed to be no difference between them (that both 0 and 1 are
> accepted for sample count is more of a historical accident).
So, the other way I could do my patch would be:
diff --git a/src/mesa/state_tracker/st_cb_fbo.c
b/src/mesa/state_tracker/st_cb_f
index 23cbcdc..afc7700 100644
--- a/src/mesa/state_tracker/st_cb_fbo.c
+++ b/src/mesa/state_tracker/st_cb_fbo.c
@@ -156,12 +156,11 @@ st_renderbuffer_alloc_storage(struct gl_context * ctx,
* by the implementation.
*
* So let's find the supported number of samples closest to NumSamples.
- * (NumSamples == 1) is treated the same as (NumSamples == 0).
*/
- if (rb->NumSamples > 1) {
+ if (rb->NumSamples > 0) {
unsigned i;
- for (i = rb->NumSamples; i <= ctx->Const.MaxSamples; i++) {
+ for (i = MAX2(2, rb->NumSamples); i <= ctx->Const.MaxSamples; i++) {
format = st_choose_renderbuffer_format(st, internalFormat, i);
if (format != PIPE_FORMAT_NONE) {
So that is_format_supported() is not called with samples=1.
Does that sound better to you?
-Brian
>
> Roland
>
>
>
>>
>> I'm pretty sure we need to do the search.
>>
>> -Brian
>>
>>>
>>> Roland
>>>
>>>
>>> Am 02.08.2017 um 19:07 schrieb Brian Paul:
>>>> We pretty much use the convention that if gl_renderbuffer::NumSamples
>>>> or gl_texture_image::NumSamples is zero, it's a non-MSAA surface.
>>>> Otherwise, it's an MSAA surface.
>>>>
>>>> This patch changes the sample count checks in st_AllocTextureStorage()
>>>> and st_renderbuffer_alloc_storage() to test for samples > 0 instead
>>>> of > 1.
>>>> As it is, if samples==1 we skip the search for the next higher number of
>>>> supported samples and ask the gallium driver to create a MSAA surface
>>>> with
>>>> one sample, which no driver supports (AFAIK). Instead, drivers create a
>>>> non-MSAA surface.
>>>>
>>>> A specific example of this problem is the Piglit
>>>> arb_framebuffer_srgb-blit
>>>> test. It calls glRenderbufferStorageMultisample() with samples=1 to
>>>> request an MSAA renderbuffer with the minimum supported number of MSAA
>>>> samples. Instead of creating a 4x or 8x, etc. MSAA surface, we wound up
>>>> creating a non-MSAA surface.
>>>>
>>>> Finally, add a comment on the gl_renderbuffer::NumSamples field.
>>>>
>>>> There is one piglit regression with the VMware driver:
>>>> ext_framebuffer_multisample-blit-mismatched-formats fails because
>>>> now we're actually creating real MSAA surfaces (the requested sample
>>>> count is 1) and we're hitting some sort of bug in the blitter code.
>>>> That
>>>> will have to be fixed separately. Other drivers may find regressions
>>>> too now that MSAA surfaces are really being created.
>>>> ---
>>>> src/mesa/main/mtypes.h | 2 +-
>>>> src/mesa/state_tracker/st_cb_fbo.c | 3 +--
>>>> src/mesa/state_tracker/st_cb_texture.c | 2 +-
>>>> 3 files changed, 3 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/src/mesa/main/mtypes.h b/src/mesa/main/mtypes.h
>>>> index 404d586..1dec89c 100644
>>>> --- a/src/mesa/main/mtypes.h
>>>> +++ b/src/mesa/main/mtypes.h
>>>> @@ -3303,7 +3303,7 @@ struct gl_renderbuffer
>>>> * called without a rb->TexImage.
>>>> */
>>>> GLboolean NeedsFinishRenderTexture;
>>>> - GLubyte NumSamples;
>>>> + GLubyte NumSamples; /**< zero means not multisampled */
>>>> GLenum InternalFormat; /**< The user-specified format */
>>>> GLenum _BaseFormat; /**< Either GL_RGB, GL_RGBA,
>>>> GL_DEPTH_COMPONENT or
>>>> GL_STENCIL_INDEX. */
>>>> diff --git a/src/mesa/state_tracker/st_cb_fbo.c
>>>> b/src/mesa/state_tracker/st_cb_fbo.c
>>>> index 23cbcdc..6986eaa 100644
>>>> --- a/src/mesa/state_tracker/st_cb_fbo.c
>>>> +++ b/src/mesa/state_tracker/st_cb_fbo.c
>>>> @@ -156,9 +156,8 @@ st_renderbuffer_alloc_storage(struct gl_context *
>>>> ctx,
>>>> * by the implementation.
>>>> *
>>>> * So let's find the supported number of samples closest to
>>>> NumSamples.
>>>> - * (NumSamples == 1) is treated the same as (NumSamples == 0).
>>>> */
>>>> - if (rb->NumSamples > 1) {
>>>> + if (rb->NumSamples > 0) {
>>>> unsigned i;
>>>>
>>>> for (i = rb->NumSamples; i <= ctx->Const.MaxSamples; i++) {
>>>> diff --git a/src/mesa/state_tracker/st_cb_texture.c
>>>> b/src/mesa/state_tracker/st_cb_texture.c
>>>> index db2913e..de6b57e 100644
>>>> --- a/src/mesa/state_tracker/st_cb_texture.c
>>>> +++ b/src/mesa/state_tracker/st_cb_texture.c
>>>> @@ -2680,7 +2680,7 @@ st_AllocTextureStorage(struct gl_context *ctx,
>>>> bindings = default_bindings(st, fmt);
>>>>
>>>> /* Raise the sample count if the requested one is unsupported. */
>>>> - if (num_samples > 1) {
>>>> + if (num_samples > 0) {
>>>> enum pipe_texture_target ptarget =
>>>> gl_target_to_pipe(texObj->Target);
>>>> boolean found = FALSE;
>>>>
>>>>
>>>
>>
>
More information about the mesa-dev
mailing list