<div dir="ltr">On 19 January 2013 11:06, Kenneth Graunke <span dir="ltr"><<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">The BLT engine has many limitations. Currently, it can only blit<br>
X-tiled buffers (since we don't have a kernel API to whack the BLT<br>
tiling mode register), which means all depth/stencil operations get<br>
punted to meta code, which can be very CPU-intensive.<br>
<br>
Even if we used the BLT engine, it can't blit between buffers with<br>
different tiling modes, such as an X-tiled non-MSAA ARGB8888 texture<br>
and a Y-tiled CMS ARGB8888 renderbuffer. This is a fundamental<br>
limitation, and the only way around that is to use BLORP.<br>
<br>
Previously, BLORP only handled BlitFramebuffer. This patch adds an<br>
additional frontend for doing CopyTexSubImage. It also makes it the<br>
default. This is partly to increase testing and avoid hiding bugs,<br>
and partly because the BLORP path can already handle more cases. With<br>
trivial extensions, it should be able to handle everything the BLT can.<br>
<br>
This helps PlaneShift massively, which tries to CopyTexSubImage2D<br>
between depth buffers whenever a player casts a spell. Since these<br>
are Y-tiled, we hit meta and software ReadPixels paths, eating 99% CPU<br>
while delivering ~1 FPS. This is particularly bad in an MMO setting<br>
because people cast spells all the time.<br>
<br>
It also helps Xonotic in 4X MSAA mode. At default power management<br>
settings, I measured a 6.35138% +/- 0.672548% performance boost (n=5).<br>
<br>
No Piglit regressions on Ivybridge. I have not tested Sandybridge.<br>
<br>
Cc: Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>><br></blockquote><div><br></div><div>I'm really glad you found this, and I'm really glad to see blorp getting more use. This was exactly the kind of purpose I had in mind when I wrote it--to avoid having to fall back to swrast for blit-like operations that couldn't be done with the hardware blitter. Thanks, Ken!<br>
<br></div><div>I found a couple of minor bugs, which I've commented on inline. I've also suggested a refactor that I think would eliminate a lot of code duplication.<br><br>I have some follow-up ideas at the end of the patch email that could possibly lead to additional performance improvements.<br>
</div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
---<br>
src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 106 +++++++++++++++++++++++++++<br>
src/mesa/drivers/dri/i965/brw_context.h | 8 ++<br>
src/mesa/drivers/dri/intel/intel_tex_copy.c | 32 ++++++--<br>
3 files changed, 138 insertions(+), 8 deletions(-)<br>
<br>
Paul,<br>
<br>
I'd appreciate your feedback on this patch. It's my first time really<br>
working with BLORP, so I'm bound to have screwed up something. :)<br>
<br>
I'm not sure about the HiZ and downsample resolves (hence the //'d out lines).<br></blockquote><div><br></div><div>Your interaction with blorp looks good. But I think there are some bugs in the resolves. My rules of thumb for depth/HiZ resolves are:<br>
<br></div><div>- Resolves take care of the mismatch in access patterns between HiZ-aware components (i.e. the depth buffer bound to the rendering pipeline) and non-HiZ-aware components (texture units, blorp, and swrast). After writing data using a HiZ-aware component and reading it using a non-HiZ-aware component, you need to do a depth resolve. After writing data using a non-HiZ-aware component and reading it using a HiZ-aware component, you need to do a HiZ resolve. For this determination, writing to a portion of the buffer (but not all of it) counts as both a write and a read.<br>
<br></div><div>- We do resolves at the last possible minute, so the way this is actually accomplished is to call intel_{miptree_slice,renderbuffer}_resolve_{depth,hiz} before reading/writing a buffer and intel_{miptree_slice,renderbuffer}_set_needs_{depth,hiz}_resolve after writing to a buffer. The latter calls simply flag a future resolve as being necessary; the former calls do the resolve only if it was previously flagged.<br>
</div><br>I'll comment below on the specific changes I think are necessary.<br><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
I'm also tempted to merge do_blorp_copytexsubimage into the caller.<br>
I mostly created it for symmetry with the existing code, but it's so small<br>
that the distinction is rather meaningless.<br></blockquote><div><br></div><div>See my refactoring suggestion below.<br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
Thanks in advance!<br>
--Ken<br>
<br>
Also, here's the Xonotic ministat in case anyone cares:<br>
<br>
x before<br>
+ after<br>
+--------------------------------------------------------------------------+<br>
|x xxx + + + ++|<br>
| |A| |_____A______||<br>
+--------------------------------------------------------------------------+<br>
N Min Max Median Avg Stddev<br>
x 5 110.64783 111.05587 110.87269 110.87019 0.15317753<br>
+ 5 117.07382 118.66559 117.92314 117.91198 0.70663046<br>
Difference at 95.0% confidence<br>
7.04179 +/- 0.745655<br>
6.35138% +/- 0.672548%<br>
(Student's t, pooled s = 0.511268)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
index bc7916a..b6d7738 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
@@ -295,6 +295,112 @@ try_blorp_blit(struct intel_context *intel,<br>
return true;<br>
}<br>
<br>
+static void<br>
+do_blorp_copytexsubimage(struct intel_context *intel,<br>
+ struct intel_renderbuffer *src_irb,<br>
+ struct gl_texture_image *dst_image,<br>
+ int srcX0, int srcY0,<br>
+ int dstX0, int dstY0,<br>
+ int dstX1, int dstY1,<br>
+ bool mirror_y)<br>
+{<br>
+ /* Find source/dst miptrees */<br>
+ struct intel_mipmap_tree *src_mt = src_irb->mt;<br>
+ struct intel_mipmap_tree *dst_mt = intel_texture_image(dst_image)->mt;<br>
+<br>
+ /* Get ready to blit. This includes depth resolving the src and dst<br>
+ * buffers if necessary.<br>
+ */<br>
+ intel_renderbuffer_resolve_depth(intel, src_irb);<br>
+ intel_miptree_slice_resolve_hiz(intel, dst_mt,<br>
+ dst_image->Level, dst_image->Face);<br></blockquote><div><br></div><div>The second call should be intel_miptree_slice_resolve_depth(), because we're about to access dst_image using blorp, which is not HiZ-aware. Presumably the reason you never saw a bug is because in your test cases, dst_image was only being used by non-HiZ-aware components (probably blorp and texturing) so no resolves were actually needed.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* Do the blit */<br>
+ brw_blorp_blit_miptrees(intel,<br>
+ src_mt, src_irb->mt_level, src_irb->mt_layer,<br>
+ dst_mt, dst_image->Level, dst_image->Face,<br>
+ srcX0, srcY0, dstX0, dstY0, dstX1, dstY1,<br>
+ false, mirror_y);<br>
+<br>
+ //intel_renderbuffer_set_needs_hiz_resolve(dst_irb);<br></blockquote><div><br></div><div>You're right to be concerned that something needs to be done here. The destination buffer needs to be marked as needing a HiZ resolve. Otherwise, if it is later used for rendering (which is HiZ-aware), depth tests might get performed using stale data from the HiZ buffer, which blorp didn't update.<br>
<br></div><div>Since we don't have a dst_irb, we can't use intel_renderbuffer_set_needs_hiz_resolve. Instead we have to do:<br><br></div><div>intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_image->Level, dst_image->Face);<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ //intel_renderbuffer_set_needs_downsample(dst_irb);<br></blockquote><div><br></div><div>I'm ok leaving this out--at the moment there's no way this code can be hit for a multisampled destination image. However, if we ever get around to implementing the GLES extension EXT_multisampled_render_to_texture, we'll have to have a hard think about what the correct behaviour should be. Perhaps we should put a comment here saying something like:<br>
<br></div><div>/* Note: there is no need to call intel_renderbuffer_set_needs_downsample() on the destination buffer, since automatic downsample is only needed for multisampled window system framebuffers, which are not allowed as destinations for CopyTexSubImage. However, this will need to be revisited if we ever support EXT_multisampled_render_to_texture. */<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+}<br></blockquote><div><br></div><div>As an alternative to those fixes, here's my refactoring suggestion: drop this function entirely, and instead, in brw_blorp_copytexsubimage(), create a temporary intel_renderbuffer to wrap around dst_image, and just call do_blorp_blit() directly. do_blorp_blit() already gets all the resolves correct, and it's well covered by existing tests. As a bonus, when we get around to implementing EXT_multisampled_render_to_texture, we'll probably remember to adjust do_blorp_blit() accordingly--it would be nice for this code to inherit that adjustment automatically.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
+bool<br>
+brw_blorp_copytexsubimage(struct intel_context *intel,<br>
+ struct gl_renderbuffer *src_rb,<br>
+ struct gl_texture_image *dst_image,<br>
+ int srcX0, int srcY0,<br>
+ int dstX0, int dstY0,<br>
+ int width, int height)<br>
+{<br>
+ struct gl_context *ctx = &intel->ctx;<br>
+<br>
+ /* BLORP is not supported before Gen6. */<br>
+ if (intel->gen < 6)<br>
+ return false;<br>
+<br>
+ /* CopyTexSubImage should happen even in conditional rendering. We could<br>
+ * turn it off and back on again. For now, just bail instead.<br>
+ */<br>
+ if (ctx->Query.CondRenderQuery)<br>
+ return false;<br></blockquote><div><br></div><div>This shouldn't be necessary. Blorp ignores any 3D pipeline state that the client has set up (including conditional rendering state), so if you just drop this check, the copy should happen just fine even if conditional rendering is on.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* BLORP requires the formats to match. In the future, we should relax<br>
+ * this for simple format mismatches like XRGB <-> ARGB.<br>
+ */<br>
+ if (_mesa_get_srgb_format_linear(src_rb->Format) !=<br>
+ _mesa_get_srgb_format_linear(dst_image->TexFormat))<br>
+ return false; <br></blockquote><div><br></div><div>Aren't you and Carl in the midst of relaxing that restriction right now? Perhaps we should refactor this check into a function that's shared by the blit and CopyTexSubImage paths so that once the restriction is relaxed, we don't have to remember to update both checks.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* Sync up the state of window system buffers. We need to do this before<br>
+ * we go looking for the buffers.<br>
+ */<br>
+ intel_prepare_render(intel);<br>
+<br>
+ /* Detect if the blit needs to be mirrored */<br>
+ bool mirror_y = false;<br>
+<br>
+ int srcX1 = srcX0 + width;<br>
+ int srcY1 = srcY0 + height;<br>
+ int dstX1 = dstX0 + width;<br>
+ int dstY1 = dstY0 + height;<br>
+<br>
+ /* If the destination rectangle needs to be clipped or scissored, do so. */<br>
+ if (!(clip_or_scissor(false, srcX0, srcX1, dstX0, dstX1,<br>
+ 0, dst_image->Width) &&<br>
+ clip_or_scissor(mirror_y, srcY0, srcY1, dstY0, dstY1,<br>
+ 0, dst_image->Height))) {<br>
+ /* Everything got clipped/scissored away, so the blit was successful. */<br>
+ return true;<br>
+ }<br></blockquote><div><br></div><div>Destination clipping shouldn't be necessary, because the restrictions on glCopyTexSubImage prevent the user from specifying a destination rectangle that falls outside the bounds of the destination texture. See error_check_subtexture_dimensions().<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* If the source rectangle needs to be clipped or scissored, do so. */<br>
+ if (!(clip_or_scissor(false, dstX0, dstX1, srcX0, srcX1,<br>
+ 0, src_rb->Width) &&<br>
+ clip_or_scissor(mirror_y, dstY0, dstY1, srcY0, srcY1,<br>
+ 0, src_rb->Height))) {<br>
+ /* Everything got clipped/scissored away, so the blit was successful. */<br>
+ return true;<br>
+ }<br></blockquote><div><br></div><div>Source clipping shouldn't be necessary either, because copytexsubimage (src/mesa/main/teximage.c) calls _mesa_clip_copytexsubimage, which takes care of it.<br></div> <blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ /* Account for the fact that in the system framebuffer, the origin is at<br>
+ * the lower left.<br>
+ */<br>
+ if (_mesa_is_winsys_fbo(ctx->ReadBuffer)) {<br>
+ GLint tmp = src_rb->Height - srcY0;<br>
+ srcY0 = src_rb->Height - srcY1;<br>
+ srcY1 = tmp;<br>
+ mirror_y = !mirror_y;<br>
+ }<br></blockquote><div><br></div><div>If you decide to go along with my refactoring suggestion, I'd suggest a follow-up patch where we move this check (and similar checks in try_blorp_blit) inside do_blorp_blit, just so that we don't have to duplicate this code.<br>
</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+ do_blorp_copytexsubimage(intel, intel_renderbuffer(src_rb), dst_image,<br>
+ srcX0, srcY0, dstX0, dstY0, dstX1, dstY1, mirror_y);<br>
+ return true;<br>
+} <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+<br>
+<br>
GLbitfield<br>
brw_blorp_framebuffer(struct intel_context *intel,<br>
GLint srcX0, GLint srcY0, GLint srcX1, GLint srcY1,<br>
diff --git a/src/mesa/drivers/dri/i965/brw_context.h b/src/mesa/drivers/dri/i965/brw_context.h<br>
index f3a3efe..3976ef8 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_context.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_context.h<br>
@@ -1219,6 +1219,14 @@ brw_blorp_framebuffer(struct intel_context *intel,<br>
GLint dstX0, GLint dstY0, GLint dstX1, GLint dstY1,<br>
GLbitfield mask, GLenum filter);<br>
<br>
+bool<br>
+brw_blorp_copytexsubimage(struct intel_context *intel,<br>
+ struct gl_renderbuffer *src_rb,<br>
+ struct gl_texture_image *dst_image,<br>
+ int srcX0, int srcY0,<br>
+ int dstX0, int dstY0,<br>
+ int width, int height);<br>
+<br>
/* gen6_multisample_state.c */<br>
void<br>
gen6_emit_3dstate_multisample(struct brw_context *brw,<br>
diff --git a/src/mesa/drivers/dri/intel/intel_tex_copy.c b/src/mesa/drivers/dri/intel/intel_tex_copy.c<br>
index 1af7b1c..0a3dfef 100644<br>
--- a/src/mesa/drivers/dri/intel/intel_tex_copy.c<br>
+++ b/src/mesa/drivers/dri/intel/intel_tex_copy.c<br>
@@ -41,6 +41,9 @@<br>
#include "intel_fbo.h"<br>
#include "intel_tex.h"<br>
#include "intel_blit.h"<br>
+#ifndef I915<br>
+#include "brw_context.h"<br>
+#endif<br>
<br>
#define FILE_DEBUG_FLAG DEBUG_TEXTURE<br>
<br>
@@ -156,15 +159,28 @@ intelCopyTexSubImage(struct gl_context *ctx, GLuint dims,<br>
GLint x, GLint y,<br>
GLsizei width, GLsizei height)<br>
{<br>
- if (dims == 3 || !intel_copy_texsubimage(intel_context(ctx),<br>
- intel_texture_image(texImage),<br>
- xoffset, yoffset,<br>
- intel_renderbuffer(rb), x, y, width, height)) {<br>
- fallback_debug("%s - fallback to swrast\n", __FUNCTION__);<br>
- _mesa_meta_CopyTexSubImage(ctx, dims, texImage,<br>
- xoffset, yoffset, zoffset,<br>
- rb, x, y, width, height);<br>
+ struct intel_context *intel = intel_context(ctx);<br>
+ if (dims != 3) {<br>
+#ifndef I915<br>
+ /* Try BLORP first. It can handle almost everything. */<br>
+ if (brw_blorp_copytexsubimage(intel, rb, texImage, x, y,<br>
+ xoffset, yoffset, width, height))<br>
+ return;<br>
+#endif<br>
+<br>
+ /* Next, try the BLT engine. */<br>
+ if (intel_copy_texsubimage(intel_context(ctx),<br>
+ intel_texture_image(texImage),<br>
+ xoffset, yoffset,<br>
+ intel_renderbuffer(rb), x, y, width, height))<br>
+ return;<br>
}<br>
+<br>
+ /* Finally, fall back to meta. This will likely be slow. */<br>
+ fallback_debug("%s - fallback to swrast\n", __FUNCTION__);<br>
+ _mesa_meta_CopyTexSubImage(ctx, dims, texImage,<br>
+ xoffset, yoffset, zoffset,<br>
+ rb, x, y, width, height);<br>
}<br>
<span class=""><font color="#888888"><br>
<br>
--<br>
1.8.1.1<br>
<br>
</font></span></blockquote></div><br><div>Possible further improvements (I'm not asking you to do them--just pointing them out so we can keep them in mind as we look for other places we can improve performance):<br><br>
</div>(1)
In theory, we ought to be able to optimize do_blorp_copytexsubimage and
do_blorp_blit to detect the case where the destination is being
entirely overwritten, and avoid having to do a depth resolve on the
destination in that case (there's no point in carefully computing values
to write into a depth buffer that's about to be overwritten). (Note: a further advantage of my refactoring suggestion is that we'd only have to make this improvement in one place).<br><br></div><div class="gmail_extra">
(2) It's unfortunate that we still fall back to swrast when dims == 3. Really we ought to be able to do handle the 3D case in both blorp and the hardware blitter--we just have to calculate the miptree layer more carefully. Your code uses dst_image->Face, but instead it should use a combination of dst_image->Face and zoffset, in much the same way that intel_render_texture() does (in fact, it would be nice if we had some common "intel_miptree_compute_layer" function for this purpose).<br>
<br></div><div class="gmail_extra">(3) I have a long-term plan to update blorp so that its access to the destination miptree is HiZ-aware (blorp uses the 3D pipeline after all, so this should be possible). That will of course change what resolves are necessary. I have no idea when we'll get around to doing this, but it's worth looking into if we continue to find blit-related performance bottlenecks like this one.<br>
<br></div><div class="gmail_extra">Thanks again for fixing this, Ken. Hopefully my suggestions aren't too onerous.<br></div></div>