On 1 August 2012 08:18, Ian Romanick <i...@freedesktop.org> wrote:
> On 07/27/2012 10:35 AM, Paul Berry wrote:
>
>> 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
>> GlRenderbufferStorageMultisamp**les() 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.
>>
>
> It looks like you hit everything here.
>
> One question. Should quantize_num_samples quantize 1 to 1 or to 4 (as it
> does now)?
>
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.
>
> If we decide to change that, it can be a follow-on patch. This series is
>
> Reviewed-by: Ian Romanick <ian.d.roman...@intel.com>
>
>
> ---
>> 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);
>>
>>
>
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
