<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=us-ascii">
</head>
<body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;" class="">
Reviewed-by: Tim Rowley <<a href="mailto:timothy.o.rowley@intel.com" class="">timothy.o.rowley@intel.com</a>>
<div class=""><br class="">
<div>
<blockquote type="cite" class="">
<div class="">On May 4, 2017, at 7:33 PM, Bruce Cherniak <<a href="mailto:bruce.cherniak@intel.com" class="">bruce.cherniak@intel.com</a>> wrote:</div>
<br class="Apple-interchange-newline">
<div class="">
<div class="">v3: list piglit tests fixed by this patch. Fixed typo Tim pointed out.<br class="">
v2: Reword commit message to more closely adhere to community<br class="">
guidelines.<br class="">
<br class="">
This patch moves msaa resolve down into core/StoreTiles where the<br class="">
surface format conversion routines are available.  The previous<br class="">
"experimental" resolve was limited to 8-bit unsigned render targets.<br class="">
<br class="">
This fixes a number of piglit msaa tests by adding resolve support for<br class="">
all the render target formats we support.<br class="">
<br class="">
Specifically:<br class="">
layered-rendering/gl-layer-render: fail->pass<br class="">
layered-rendering/gl-layer-render-storage: fail->pass<br class="">
multisample-formats *[2,4,8,16] gl_arb_texture_rg: crash->pass<br class="">
multisample-formats *[2,4,8,16] gl_ext_texture_snorm: crash->pass<br class="">
multisample-formats *[2,4,8,16] gl_arb_texture_float: fail->pass<br class="">
multisample-formats *[2,4,8,16] gl_arb_texture_rg-float: fail->pass<br class="">
<br class="">
MSAA is still disabled by default, but can be enabled with<br class="">
"export SWR_MSAA_MAX_COUNT=4" (1,2,4,8,16 are options)<br class="">
The default is 0, which is disabled.<br class="">
<br class="">
This patch improves the number of multisample-formats supported by swr,<br class="">
and fixes several crashes currently in the 17.1 branch.  Therefore, it<br class="">
should be considered for inclusion in the 17.1 stable release.  Being<br class="">
disabled by default, it poses no risk to most users of swr.<br class="">
<br class="">
cc: <a href="mailto:mesa-stable@lists.freedesktop.org" class="">mesa-stable@lists.freedesktop.org</a><br class="">
---<br class="">
.../drivers/swr/rasterizer/memory/StoreTile.h      | 75 +++++++++++++++++++++<br class="">
src/gallium/drivers/swr/swr_context.cpp            | 77 +---------------------<br class="">
src/gallium/drivers/swr/swr_screen.cpp             | 10 +--<br class="">
3 files changed, 82 insertions(+), 80 deletions(-)<br class="">
<br class="">
diff --git a/src/gallium/drivers/swr/rasterizer/memory/StoreTile.h b/src/gallium/drivers/swr/rasterizer/memory/StoreTile.h<br class="">
index ffde574c03..12a5f3d8ce 100644<br class="">
--- a/src/gallium/drivers/swr/rasterizer/memory/StoreTile.h<br class="">
+++ b/src/gallium/drivers/swr/rasterizer/memory/StoreTile.h<br class="">
@@ -1133,6 +1133,64 @@ struct StoreRasterTile<br class="">
            }<br class="">
        }<br class="">
    }<br class="">
+<br class="">
+    //////////////////////////////////////////////////////////////////////////<br class="">
+    /// @brief Resolves an 8x8 raster tile to the resolve destination surface.<br class="">
+    /// @param pSrc - Pointer to raster tile.<br class="">
+    /// @param pDstSurface - Destination surface state<br class="">
+    /// @param x, y - Coordinates to raster tile.<br class="">
+    /// @param sampleOffset - Offset between adjacent multisamples<br class="">
+    INLINE static void Resolve(<br class="">
+        uint8_t *pSrc,<br class="">
+        SWR_SURFACE_STATE* pDstSurface,<br class="">
+        uint32_t x, uint32_t y, uint32_t sampleOffset, uint32_t renderTargetArrayIndex) // (x, y) pixel coordinate to start of raster tile.<br class="">
+    {<br class="">
+        uint32_t lodWidth = std::max(pDstSurface->width >> pDstSurface->lod, 1U);<br class="">
+        uint32_t lodHeight = std::max(pDstSurface->height >> pDstSurface->lod, 1U);<br class="">
+<br class="">
+        float oneOverNumSamples = 1.0f / pDstSurface->numSamples;<br class="">
+<br class="">
+        // For each raster tile pixel (rx, ry)<br class="">
+        for (uint32_t ry = 0; ry < KNOB_TILE_Y_DIM; ++ry)<br class="">
+        {<br class="">
+            for (uint32_t rx = 0; rx < KNOB_TILE_X_DIM; ++rx)<br class="">
+            {<br class="">
+                // Perform bounds checking.<br class="">
+                if (((x + rx) < lodWidth) &&<br class="">
+                        ((y + ry) < lodHeight))<br class="">
+                {<br class="">
+                    // Sum across samples<br class="">
+                    float resolveColor[4] = {0};<br class="">
+                    for (uint32_t sampleNum = 0; sampleNum < pDstSurface->numSamples; sampleNum++)<br class="">
+                    {<br class="">
+                        float sampleColor[4] = {0};<br class="">
+                        uint8_t *pSampleSrc = pSrc + sampleOffset * sampleNum;<br class="">
+                        GetSwizzledSrcColor(pSampleSrc, rx, ry, sampleColor);<br class="">
+                        resolveColor[0] += sampleColor[0];<br class="">
+                        resolveColor[1] += sampleColor[1];<br class="">
+                        resolveColor[2] += sampleColor[2];<br class="">
+                        resolveColor[3] += sampleColor[3];<br class="">
+                    }<br class="">
+<br class="">
+                    // Divide by numSamples to average<br class="">
+                    resolveColor[0] *= oneOverNumSamples;<br class="">
+                    resolveColor[1] *= oneOverNumSamples;<br class="">
+                    resolveColor[2] *= oneOverNumSamples;<br class="">
+                    resolveColor[3] *= oneOverNumSamples;<br class="">
+<br class="">
+                    // Use the resolve surface state<br class="">
+                    SWR_SURFACE_STATE* pResolveSurface = (SWR_SURFACE_STATE*)pDstSurface->pAuxBaseAddress;<br class="">
+                    uint8_t *pDst = (uint8_t*)ComputeSurfaceAddress<false, false>((x + rx), (y + ry),<br class="">
+                        pResolveSurface->arrayIndex + renderTargetArrayIndex, pResolveSurface->arrayIndex + renderTargetArrayIndex,<br class="">
+                        0, pResolveSurface->lod, pResolveSurface);<br class="">
+                    {<br class="">
+                        ConvertPixelFromFloat<DstFormat>(pDst, resolveColor);<br class="">
+                    }<br class="">
+                }<br class="">
+            }<br class="">
+        }<br class="">
+    }<br class="">
+<br class="">
};<br class="">
<br class="">
template<typename TTraits, SWR_FORMAT SrcFormat, SWR_FORMAT DstFormat><br class="">
@@ -2316,6 +2374,9 @@ struct StoreMacroTile<br class="">
            pfnStore[sampleNum] = (bForceGeneric || KNOB_USE_GENERIC_STORETILE) ? StoreRasterTile<TTraits, SrcFormat, DstFormat>::Store : OptStoreRasterTile<TTraits, SrcFormat, DstFormat>::Store;<br class="">
        }<br class="">
<br class="">
+        // Save original for pSrcHotTile resolve.<br class="">
+        uint8_t *pResolveSrcHotTile = pSrcHotTile;<br class="">
+<br class="">
        // Store each raster tile from the hot tile to the destination surface.<br class="">
        for(uint32_t row = 0; row < KNOB_MACROTILE_Y_DIM; row += KNOB_TILE_Y_DIM)<br class="">
        {<br class="">
@@ -2328,6 +2389,20 @@ struct StoreMacroTile<br class="">
                }<br class="">
            }<br class="">
        }<br class="">
+<br class="">
+        if (pDstSurface->pAuxBaseAddress)<br class="">
+        {<br class="">
+            uint32_t sampleOffset = KNOB_TILE_X_DIM * KNOB_TILE_Y_DIM * (FormatTraits<SrcFormat>::bpp / 8);<br class="">
+            // Store each raster tile from the hot tile to the destination surface.<br class="">
+            for(uint32_t row = 0; row < KNOB_MACROTILE_Y_DIM; row += KNOB_TILE_Y_DIM)<br class="">
+            {<br class="">
+                for(uint32_t col = 0; col < KNOB_MACROTILE_X_DIM; col += KNOB_TILE_X_DIM)<br class="">
+                {<br class="">
+                    StoreRasterTile<TTraits, SrcFormat, DstFormat>::Resolve(pResolveSrcHotTile, pDstSurface, (x + col), (y + row), sampleOffset, renderTargetArrayIndex);<br class="">
+                    pResolveSrcHotTile += sampleOffset * pDstSurface->numSamples;<br class="">
+                }<br class="">
+            }<br class="">
+        }<br class="">
    }<br class="">
};<br class="">
<br class="">
diff --git a/src/gallium/drivers/swr/swr_context.cpp b/src/gallium/drivers/swr/swr_context.cpp<br class="">
index e8e465196d..11ce3464d4 100644<br class="">
--- a/src/gallium/drivers/swr/swr_context.cpp<br class="">
+++ b/src/gallium/drivers/swr/swr_context.cpp<br class="">
@@ -267,65 +267,6 @@ swr_resource_copy(struct pipe_context *pipe,<br class="">
}<br class="">
<br class="">
<br class="">
-/* XXX: This resolve is incomplete and suboptimal. It will be removed once the<br class="">
- * pipelined resolve blit works. */<br class="">
-void<br class="">
-swr_do_msaa_resolve(struct pipe_resource *src_resource,<br class="">
-                    struct pipe_resource *dst_resource)<br class="">
-{<br class="">
-   /* This is a pretty dumb inline resolve.  It only supports 8-bit formats<br class="">
-    * (ex RGBA8/BGRA8) - which are most common display formats anyway.<br class="">
-    */<br class="">
-<br class="">
-   /* quick check for 8-bit and number of components */<br class="">
-   uint8_t bits_per_component =<br class="">
-      util_format_get_component_bits(src_resource->format,<br class="">
-            UTIL_FORMAT_COLORSPACE_RGB, 0);<br class="">
-<br class="">
-   /* Unsupported resolve format */<br class="">
-   assert(src_resource->format == dst_resource->format);<br class="">
-   assert(bits_per_component == 8);<br class="">
-   if ((src_resource->format != dst_resource->format) ||<br class="">
-       (bits_per_component != 8)) {<br class="">
-      return;<br class="">
-   }<br class="">
-<br class="">
-   uint8_t src_num_comps = util_format_get_nr_components(src_resource->format);<br class="">
-<br class="">
-   SWR_SURFACE_STATE *src_surface = &swr_resource(src_resource)->swr;<br class="">
-   SWR_SURFACE_STATE *dst_surface = &swr_resource(dst_resource)->swr;<br class="">
-<br class="">
-   uint32_t *src, *dst, offset;<br class="">
-   uint32_t num_samples = src_surface->numSamples;<br class="">
-   float recip_num_samples = 1.0f / num_samples;<br class="">
-   for (uint32_t y = 0; y < src_surface->height; y++) {<br class="">
-      for (uint32_t x = 0; x < src_surface->width; x++) {<br class="">
-         float r = 0.0f;<br class="">
-         float g = 0.0f;<br class="">
-         float b = 0.0f;<br class="">
-         float a = 0.0f;<br class="">
-         for (uint32_t sampleNum = 0;  sampleNum < num_samples; sampleNum++) {<br class="">
-            offset = ComputeSurfaceOffset<false>(x, y, 0, 0, sampleNum, 0, src_surface);<br class="">
-            src = (uint32_t *) src_surface->pBaseAddress + offset/src_num_comps;<br class="">
-            const uint32_t sample = *src;<br class="">
-            r += (float)((sample >> 24) & 0xff) / 255.0f * recip_num_samples;<br class="">
-            g += (float)((sample >> 16) & 0xff) / 255.0f * recip_num_samples;<br class="">
-            b += (float)((sample >>  8) & 0xff) / 255.0f * recip_num_samples;<br class="">
-            a += (float)((sample      ) & 0xff) / 255.0f * recip_num_samples;<br class="">
-         }<br class="">
-         uint32_t result = 0;<br class="">
-         result  = ((uint8_t)(r * 255.0f) & 0xff) << 24;<br class="">
-         result |= ((uint8_t)(g * 255.0f) & 0xff) << 16;<br class="">
-         result |= ((uint8_t)(b * 255.0f) & 0xff) <<  8;<br class="">
-         result |= ((uint8_t)(a * 255.0f) & 0xff);<br class="">
-         offset = ComputeSurfaceOffset<false>(x, y, 0, 0, 0, 0, src_surface);<br class="">
-         dst = (uint32_t *) dst_surface->pBaseAddress + offset/src_num_comps;<br class="">
-         *dst = result;<br class="">
-      }<br class="">
-   }<br class="">
-}<br class="">
-<br class="">
-<br class="">
static void<br class="">
swr_blit(struct pipe_context *pipe, const struct pipe_blit_info *blit_info)<br class="">
{<br class="">
@@ -342,28 +283,14 @@ swr_blit(struct pipe_context *pipe, const struct pipe_blit_info *blit_info)<br class="">
      debug_printf("swr_blit: color resolve : %d -> %d\n",<br class="">
            info.src.resource->nr_samples, info.dst.resource->nr_samples);<br class="">
<br class="">
-      /* Because the resolve is being done inline (not pipelined),<br class="">
-       * resources need to be stored out of hottiles and the pipeline empty.<br class="">
-       *<br class="">
-       * Resources are marked unused following fence finish because all<br class="">
-       * pipeline operations are complete.  Validation of the blit will mark<br class="">
-       * them are read/write again.<br class="">
-       */<br class="">
+      /* Resolve is done as part of the surface store. */<br class="">
      swr_store_dirty_resource(pipe, info.src.resource, SWR_TILE_RESOLVED);<br class="">
-      swr_store_dirty_resource(pipe, info.dst.resource, SWR_TILE_RESOLVED);<br class="">
-      swr_fence_finish(pipe->screen, NULL, swr_screen(pipe->screen)->flush_fence, 0);<br class="">
-      swr_resource_unused(info.src.resource);<br class="">
-      swr_resource_unused(info.dst.resource);<br class="">
<br class="">
      struct pipe_resource *src_resource = info.src.resource;<br class="">
      struct pipe_resource *resolve_target =<br class="">
         swr_resource(src_resource)->resolve_target;<br class="">
<br class="">
-      /* Inline resolve samples into resolve target resource, then continue<br class="">
-       * the blit. */<br class="">
-      swr_do_msaa_resolve(src_resource, resolve_target);<br class="">
-<br class="">
-      /* The resolve target becomes the new source for the blit.  */<br class="">
+      /* The resolve target becomes the new source for the blit. */<br class="">
      info.src.resource = resolve_target;<br class="">
   }<br class="">
<br class="">
diff --git a/src/gallium/drivers/swr/swr_screen.cpp b/src/gallium/drivers/swr/swr_screen.cpp<br class="">
index 04e613745a..9254e73508 100644<br class="">
--- a/src/gallium/drivers/swr/swr_screen.cpp<br class="">
+++ b/src/gallium/drivers/swr/swr_screen.cpp<br class="">
@@ -891,6 +891,10 @@ swr_create_resolve_resource(struct pipe_screen *_screen,<br class="">
<br class="">
      /* Attach it to the multisample resource */<br class="">
      msaa_res->resolve_target = alt;<br class="">
+<br class="">
+      /* Hang resolve surface state off the multisample surface state to so<br class="">
+       * StoreTiles knows where to resolve the surface. */<br class="">
+      msaa_res->swr.pAuxBaseAddress =  (uint8_t *)&swr_resource(alt)->swr;<br class="">
   }<br class="">
<br class="">
   return true; /* success */<br class="">
@@ -1009,14 +1013,10 @@ swr_flush_frontbuffer(struct pipe_screen *p_screen,<br class="">
      SwrEndFrame(swr_context(pipe)->swrContext);<br class="">
   }<br class="">
<br class="">
-   /* Multisample surfaces need to be resolved before present */<br class="">
+   /* Multisample resolved into resolve_target at flush with store_resource */<br class="">
   if (pipe && spr->swr.numSamples > 1) {<br class="">
      struct pipe_resource *resolve_target = spr->resolve_target;<br class="">
<br class="">
-      /* Do an inline surface resolve into the resolve target resource<br class="">
-       * XXX: This works, just not optimal. Work on using a pipelined blit. */<br class="">
-      swr_do_msaa_resolve(resource, resolve_target);<br class="">
-<br class="">
      /* Once resolved, copy into display target */<br class="">
      SWR_SURFACE_STATE *resolve = &swr_resource(resolve_target)->swr;<br class="">
<br class="">
-- <br class="">
2.11.0<br class="">
<br class="">
_______________________________________________<br class="">
mesa-dev mailing list<br class="">
<a href="mailto:mesa-dev@lists.freedesktop.org" class="">mesa-dev@lists.freedesktop.org</a><br class="">
https://lists.freedesktop.org/mailman/listinfo/mesa-dev<br class="">
</div>
</div>
</blockquote>
</div>
<br class="">
</div>
</body>
</html>