On 1 August 2012 08:18, Ian Romanick <<a href="mailto:idr@freedesktop.org" target="_blank">idr@freedesktop.org</a>> wrote: <div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On 07/27/2012 10:35 AM, Paul Berry wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> EXT_framebuffer_multisample is a required subpart of ARB_framebuffer_object, which means that we must support it even on platforms that don't support MSAA. Fortunately EXT_framebuffer_multisample allows for this by allowing GL_MAX_SAMPLES to be set to 1. This leads to a tricky quirk in the GL spec: since GlRenderbufferStorageMultisamples() accepts any value for its "samples" parameter up to and including GL_MAX_SAMPLES, that means that on platforms that don't support MSAA, GL_SAMPLES is allowed to be set to either 0 or 1. On platforms that do support MSAA, GL_SAMPLES=1 is not used; 0 means no MSAA, and 2 or higher means MSAA. In other words, GL_SAMPLES needs to be interpreted as follows: =0 no MSAA (possible on all platforms) =1 no MSAA (only possible on platforms where MSAA unsupported) >1 MSAA (only possible on platforms where MSAA supported) This patch modifies all MSAA-related code to choose between multisampling and single-sampling based on the condition (GL_SAMPLES > 1) instead of (GL_SAMPLES > 0) so that GL_SAMPLES=1 will be treated as "no MSAA". Note that since GL_SAMPLES=1 implies GL_SAMPLE_BUFFERS=1, we can no longer use GL_SAMPLE_BUFFERS to distinguish between MSAA and non-MSAA rendering. </blockquote> </div> It looks like you hit everything here. One question. Should quantize_num_samples quantize 1 to 1 or to 4 (as it does now)? </blockquote><div> Based on my reading of the spec, and the behaviour of my nVidia card, requesting samples=1 on a system that supports MSAA should select the minimum supported number of samples (i.e. 2 for nVidia, 4 for Intel chips). So I believe what we are doing is correct. Thanks for the review. I'll push the patches later today. </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"> If we decide to change that, it can be a follow-on patch. This series is Reviewed-by: Ian Romanick <<a href="mailto:ian.d.romanick@intel.com" target="_blank">ian.d.romanick@intel.com</a>><div class="HOEnZb"><div class="h5"> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> --- src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 10 +++++----- src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 4 ++-- src/mesa/drivers/dri/i965/gen6_blorp.cpp | 6 +++--- src/mesa/drivers/dri/i965/gen6_multisample_state.c | 3 ++- src/mesa/drivers/dri/i965/gen6_sf_state.c | 2 +- src/mesa/drivers/dri/i965/gen6_wm_state.c | 2 +- src/mesa/drivers/dri/i965/gen7_blorp.cpp | 4 ++-- src/mesa/drivers/dri/i965/gen7_sf_state.c | 2 +- src/mesa/drivers/dri/i965/gen7_wm_state.c | 2 +- src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 4 ++-- src/mesa/drivers/dri/intel/intel_mipmap_tree.c | 6 +++--- 11 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp index 296b99f..1206237 100644 --- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp +++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp @@ -1582,7 +1582,7 @@ inline intel_msaa_layout compute_msaa_layout_for_pipeline(struct brw_context *brw, unsigned num_samples, intel_msaa_layout true_layout) { - if (num_samples == 0) { + if (num_samples <= 1) { /* When configuring the GPU for non-MSAA, we can still accommodate IMS * format buffers, by transforming coordinates appropriately. */ @@ -1652,7 +1652,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw, dst.num_samples = 0; } - if (dst.map_stencil_as_y_tiled && dst.num_samples > 0) { + if (dst.map_stencil_as_y_tiled && dst.num_samples > 1) { /* If the destination surface is a W-tiled multisampled stencil buffer * that we're mapping as Y tiled, then we need to arrange for the WM * program to run once per sample rather than once per pixel, because @@ -1662,7 +1662,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw, wm_prog_key.persample_msaa_dispatch = true; } - if (src.num_samples > 0 && dst.num_samples > 0) { + if (src.num_samples > 0 && dst.num_samples > 1) { /* We are blitting from a multisample buffer to a multisample buffer, so * we must preserve samples within a pixel. This means we have to * arrange for the WM program to run once per sample rather than once @@ -1679,7 +1679,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw, GLenum base_format = _mesa_get_format_base_format(src_mt->format); if (base_format != GL_DEPTH_COMPONENT && /* TODO: what about depth/stencil? */ base_format != GL_STENCIL_INDEX && - src_mt->num_samples > 0 && dst_mt->num_samples == 0) { + src_mt->num_samples > 1 && dst_mt->num_samples <= 1) { /* We are downsampling a color buffer, so blend. */ wm_prog_key.blend = true; } @@ -1717,7 +1717,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw, wm_push_consts.x_transform.setup(src_x0, dst_x0, dst_x1, mirror_x); wm_push_consts.y_transform.setup(src_y0, dst_y0, dst_y1, mirror_y); - if (dst.num_samples == 0 && dst_mt->num_samples > 0) { + if (dst.num_samples <= 1 && dst_mt->num_samples > 1) { /* We must expand the rectangle we send through the rendering pipeline, * to account for the fact that we are mapping the destination region as * single-sampled when it is in fact multisampled. We must also align diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c index 65ca2fc..f577f4c 100644 --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c @@ -650,7 +650,7 @@ brw_get_surface_tiling_bits(uint32_t tiling) uint32_t brw_get_surface_num_multisamples(unsigned num_samples) { - if (num_samples > 0) + if (num_samples > 1) return BRW_SURFACE_MULTISAMPLECOUNT_4; else return BRW_SURFACE_MULTISAMPLECOUNT_1; @@ -992,7 +992,7 @@ brw_update_null_renderbuffer_surface(struct brw_context *brw, unsigned int unit) surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, 6 * 4, 32, &brw->wm.surf_offset[unit]); - if (fb->Visual.samples > 0) { + if (fb->Visual.samples > 1) { /* On Gen6, null render targets seem to cause GPU hangs when * multisampling. So work around this problem by rendering into dummy * color buffer. diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i965/gen6_blorp.cpp index b134ab4..995b507 100644 --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp @@ -415,7 +415,7 @@ gen6_blorp_emit_surface_state(struct brw_context *brw, uint32_t wm_surf_offset; uint32_t width, height; surface->get_miplevel_dims(&width, &height); - if (surface->num_samples > 0) { /* TODO: seems clumsy */ + if (surface->num_samples > 1) { /* TODO: seems clumsy */ width /= 2; height /= 2; } @@ -685,7 +685,7 @@ gen6_blorp_emit_sf_config(struct brw_context *brw, 1 << GEN6_SF_URB_ENTRY_READ_LENGTH_SHIFT | 0 << GEN6_SF_URB_ENTRY_READ_OFFSET_SHIFT); OUT_BATCH(0); /* dw2 */ - OUT_BATCH(params->num_samples > 0 ? GEN6_SF_MSRAST_ON_PATTERN : 0); + OUT_BATCH(params->num_samples > 1 ? GEN6_SF_MSRAST_ON_PATTERN : 0); for (int i = 0; i < 16; ++i) OUT_BATCH(0); ADVANCE_BATCH(); @@ -744,7 +744,7 @@ gen6_blorp_emit_wm_config(struct brw_context *brw, dw5 |= GEN6_WM_DISPATCH_ENABLE; /* We are rendering */ } - if (params->num_samples > 0) { + if (params->num_samples > 1) { dw6 |= GEN6_WM_MSRAST_ON_PATTERN; if (prog_data && prog_data->persample_msaa_dispatch) dw6 |= GEN6_WM_MSDISPMODE_PERSAMPLE; diff --git a/src/mesa/drivers/dri/i965/gen6_multisample_state.c b/src/mesa/drivers/dri/i965/gen6_multisample_state.c index 68d28dd..64ac292 100644 --- a/src/mesa/drivers/dri/i965/gen6_multisample_state.c +++ b/src/mesa/drivers/dri/i965/gen6_multisample_state.c @@ -42,6 +42,7 @@ gen6_emit_3dstate_multisample(struct brw_context *brw, switch (num_samples) { case 0: + case 1: number_of_multisamples = MS_NUMSAMPLES_1; break; case 4: @@ -121,7 +122,7 @@ gen6_emit_3dstate_sample_mask(struct brw_context *brw, BEGIN_BATCH(2); OUT_BATCH(_3DSTATE_SAMPLE_MASK << 16 | (2 - 2)); - if (num_samples > 0) { + if (num_samples > 1) { int coverage_int = (int) (num_samples * coverage + 0.5); uint32_t coverage_bits = (1 << coverage_int) - 1; if (coverage_invert) diff --git a/src/mesa/drivers/dri/i965/gen6_sf_state.c b/src/mesa/drivers/dri/i965/gen6_sf_state.c index 736e83a..c1bc252 100644 --- a/src/mesa/drivers/dri/i965/gen6_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen6_sf_state.c @@ -122,7 +122,7 @@ upload_sf_state(struct brw_context *brw) int i; /* _NEW_BUFFER */ bool render_to_fbo = _mesa_is_user_fbo(brw->intel.ctx.DrawBuffer); - bool multisampled_fbo = ctx->DrawBuffer->Visual.sampleBuffers; + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1; int attr = 0, input_index = 0; int urb_entry_read_offset = 1; diff --git a/src/mesa/drivers/dri/i965/gen6_wm_state.c b/src/mesa/drivers/dri/i965/gen6_wm_state.c index 63acbe3..dd43528 100644 --- a/src/mesa/drivers/dri/i965/gen6_wm_state.c +++ b/src/mesa/drivers/dri/i965/gen6_wm_state.c @@ -99,7 +99,7 @@ upload_wm_state(struct brw_context *brw) uint32_t dw2, dw4, dw5, dw6; /* _NEW_BUFFERS */ - bool multisampled_fbo = ctx->DrawBuffer->Visual.sampleBuffers; + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1; /* CACHE_NEW_WM_PROG */ if (brw->wm.prog_data->nr_params == 0) { diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp index cc28d8c..fddc7a8 100644 --- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp +++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp @@ -373,7 +373,7 @@ gen7_blorp_emit_sf_config(struct brw_context *brw, OUT_BATCH(_3DSTATE_SF << 16 | (7 - 2)); OUT_BATCH(params->depth_format << GEN7_SF_DEPTH_BUFFER_SURFACE_FORMAT_SHIFT); - OUT_BATCH(params->num_samples > 0 ? GEN6_SF_MSRAST_ON_PATTERN : 0); + OUT_BATCH(params->num_samples > 1 ? GEN6_SF_MSRAST_ON_PATTERN : 0); OUT_BATCH(0); OUT_BATCH(0); OUT_BATCH(0); @@ -432,7 +432,7 @@ gen7_blorp_emit_wm_config(struct brw_context *brw, dw1 |= GEN7_WM_DISPATCH_ENABLE; /* We are rendering */ } - if (params->num_samples > 0) { + if (params->num_samples > 1) { dw1 |= GEN7_WM_MSRAST_ON_PATTERN; if (prog_data && prog_data->persample_msaa_dispatch) dw2 |= GEN7_WM_MSDISPMODE_PERSAMPLE; diff --git a/src/mesa/drivers/dri/i965/gen7_sf_state.c b/src/mesa/drivers/dri/i965/gen7_sf_state.c index 2d258d2..871a8b7 100644 --- a/src/mesa/drivers/dri/i965/gen7_sf_state.c +++ b/src/mesa/drivers/dri/i965/gen7_sf_state.c @@ -161,7 +161,7 @@ upload_sf_state(struct brw_context *brw) float point_size; /* _NEW_BUFFERS */ bool render_to_fbo = _mesa_is_user_fbo(brw->intel.ctx.DrawBuffer); - bool multisampled_fbo = ctx->DrawBuffer->Visual.sampleBuffers; + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1; dw1 = GEN6_SF_STATISTICS_ENABLE | GEN6_SF_VIEWPORT_TRANSFORM_ENABLE; diff --git a/src/mesa/drivers/dri/i965/gen7_wm_state.c b/src/mesa/drivers/dri/i965/gen7_wm_state.c index e60027a..dc49a7d 100644 --- a/src/mesa/drivers/dri/i965/gen7_wm_state.c +++ b/src/mesa/drivers/dri/i965/gen7_wm_state.c @@ -42,7 +42,7 @@ upload_wm_state(struct brw_context *brw) uint32_t dw1, dw2; /* _NEW_BUFFERS */ - bool multisampled_fbo = ctx->DrawBuffer->Visual.sampleBuffers; + bool multisampled_fbo = ctx->DrawBuffer->Visual.samples > 1; dw1 = dw2 = 0; dw1 |= GEN7_WM_STATISTICS_ENABLE; diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c index 2522276..e0b0732 100644 --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c @@ -61,7 +61,7 @@ gen7_set_surface_msaa(struct gen7_surface_state *surf, unsigned num_samples, { if (num_samples > 4) surf->ss4.num_multisamples = GEN7_SURFACE_MULTISAMPLECOUNT_8; - else if (num_samples > 0) + else if (num_samples > 1) surf->ss4.num_multisamples = GEN7_SURFACE_MULTISAMPLECOUNT_4; else surf->ss4.num_multisamples = GEN7_SURFACE_MULTISAMPLECOUNT_1; @@ -280,7 +280,7 @@ gen7_update_texture_surface(struct gl_context *ctx, GLuint unit) /* We don't support MSAA for textures. */ assert(!mt->array_spacing_lod0); - assert(mt->num_samples == 0); + assert(mt->num_samples <= 1); intel_miptree_get_dimensions_for_image(firstImage, &width, &height, &depth); diff --git a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c index 3d15a8d..b92ae96 100644 --- a/src/mesa/drivers/dri/intel/intel_mipmap_tree.c +++ b/src/mesa/drivers/dri/intel/intel_mipmap_tree.c @@ -128,7 +128,7 @@ intel_miptree_create_internal(struct intel_context *intel, intel->vtbl.is_hiz_depth_format(intel, format)))) { /* MSAA stencil surfaces always use IMS layout. */ enum intel_msaa_layout msaa_layout = - num_samples > 0 ? INTEL_MSAA_LAYOUT_IMS : INTEL_MSAA_LAYOUT_NONE; + num_samples > 1 ? INTEL_MSAA_LAYOUT_IMS : INTEL_MSAA_LAYOUT_NONE; mt->stencil_mt = intel_miptree_create(intel, mt->target, MESA_FORMAT_S8, @@ -335,7 +335,7 @@ intel_miptree_create_for_renderbuffer(struct intel_context *intel, uint32_t depth = 1; enum intel_msaa_layout msaa_layout = INTEL_MSAA_LAYOUT_NONE; - if (num_samples > 0) { + if (num_samples > 1) { /* Adjust width/height/depth for MSAA */ msaa_layout = compute_msaa_layout(intel, format); if (msaa_layout == INTEL_MSAA_LAYOUT_IMS) { @@ -383,7 +383,7 @@ intel_miptree_create_for_renderbuffer(struct intel_context *intel, height = ALIGN(height, 2) * 2; break; default: - /* num_samples should already have been quantized to 0, 4, or + /* num_samples should already have been quantized to 0, 1, 4, or * 8. */ assert(false); </blockquote> </div></div></blockquote></div>