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;
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
