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? Like I said, there should be no difference in gallium between a msaa surface (with sample count 1) and a non-msaa surface. 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. 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). 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; >>> >>> >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev