<div dir="ltr">On 9 August 2013 19:01, Anuj Phogat <span dir="ltr"><<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</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"><div class="im">Currently single sample scaled blits with GL_LINEAR filter falls<br>
back to meta path. Patch removes this limitation in BLORP engine<br>
and implements single sample scaled blit with bilinear filter.<br>
No piglit, gles3 regressions are obeserved with this patch. Piglit<br>
test case patches to verify this implementation are out on piglit<br>
mailing list.<br>
<br>
</div>V2:<br>
Use "sample" message in place of "LD" message to utilize the linear/nearest<br>
filtering (of single-sampled texture) functionality built in to hardware.<br>
<div class="im">Signed-off-by: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com">anuj.phogat@gmail.com</a>><br></div></blockquote><div><br></div><div>Thanks for re-working this, Anuj. I think we're getting close to converging on a solution.<br>
<br></div><div>My key concern right now is that there are several things that the blorp compiler should do differently we're doing scaled GL_LINEAR blits:<br><br></div><div>1 translate_dst_to_src() shouldn't round the floating point coordinates to integers<br>
</div><div>2 compile() should do its second call to sample() (the one that occurs when we're not blending)<br></div><div>3 compile() shouldn't call encode_msaa(), translate_tiling(), or decode_msaa()<br></div><div>
4 texture_lookup() should use retype({X,Y}, BRW_REGISTER_TYPE_F) as the source of its MOV operations<br></div><div><br></div><div>It's not obvious from reading the code that these will all happen under the right conditions.<br>
<br>I'd recommend adding a new boolean to brw_blorp_blit_prog_key to indicate when scaled GL_LINEAR blits are being done, and have all four behavioural changes above happen in response to that boolean.<br><br></div><div>
Two other minor comments 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"><div class="im">
---<br>
</div> src/mesa/drivers/dri/i965/brw_blorp.h | 5 +-<br>
src/mesa/drivers/dri/i965/brw_blorp_blit.cpp | 79 ++++++++++++++++-----------<br>
src/mesa/drivers/dri/i965/gen7_blorp.cpp | 9 ++-<br>
src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 4 +-<br>
4 files changed, 60 insertions(+), 37 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_blorp.h b/src/mesa/drivers/dri/i965/brw_blorp.h<br>
index 49862b8..6a3a429 100644<br>
<div class="im">--- a/src/mesa/drivers/dri/i965/brw_blorp.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_blorp.h<br>
@@ -44,7 +44,7 @@ brw_blorp_blit_miptrees(struct brw_context *brw,<br>
float src_x1, float src_y1,<br>
float dst_x0, float dst_y0,<br>
float dst_x1, float dst_y1,<br>
- bool mirror_x, bool mirror_y);<br>
+ GLenum filter, bool mirror_x, bool mirror_y);<br>
<br>
bool<br>
brw_blorp_clear_color(struct brw_context *brw, struct gl_framebuffer *fb,<br>
</div>@@ -232,6 +232,7 @@ public:<br>
bool use_wm_prog;<br>
brw_blorp_wm_push_constants wm_push_consts;<br>
bool color_write_disable[4];<br>
+ GLenum tex_filter;<br>
};<br>
<br>
<br>
@@ -347,7 +348,7 @@ public:<br>
<div class="im"> GLfloat src_x1, GLfloat src_y1,<br>
GLfloat dst_x0, GLfloat dst_y0,<br>
GLfloat dst_x1, GLfloat dst_y1,<br>
- bool mirror_x, bool mirror_y);<br>
+ GLenum filter, bool mirror_x, bool mirror_y);<br>
<br>
virtual uint32_t get_wm_prog(struct brw_context *brw,<br>
brw_blorp_prog_data **prog_data) const;<br>
diff --git a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
</div>index 8c0db48..128940d 100644<br>
<div><div class="h5">--- a/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
+++ b/src/mesa/drivers/dri/i965/brw_blorp_blit.cpp<br>
@@ -133,7 +133,7 @@ brw_blorp_blit_miptrees(struct brw_context *brw,<br>
float src_x1, float src_y1,<br>
float dst_x0, float dst_y0,<br>
float dst_x1, float dst_y1,<br>
- bool mirror_x, bool mirror_y)<br>
+ GLenum filter, bool mirror_x, bool mirror_y)<br>
{<br>
/* Get ready to blit. This includes depth resolving the src and dst<br>
* buffers if necessary. Note: it's not necessary to do a color resolve on<br>
@@ -161,7 +161,7 @@ brw_blorp_blit_miptrees(struct brw_context *brw,<br>
src_x1, src_y1,<br>
dst_x0, dst_y0,<br>
dst_x1, dst_y1,<br>
- mirror_x, mirror_y);<br>
+ filter, mirror_x, mirror_y);<br>
brw_blorp_exec(brw, ¶ms);<br>
<br>
intel_miptree_slice_set_needs_hiz_resolve(dst_mt, dst_level, dst_layer);<br>
@@ -173,7 +173,7 @@ do_blorp_blit(struct brw_context *brw, GLbitfield buffer_bit,<br>
struct intel_renderbuffer *dst_irb,<br>
GLfloat srcX0, GLfloat srcY0, GLfloat srcX1, GLfloat srcY1,<br>
GLfloat dstX0, GLfloat dstY0, GLfloat dstX1, GLfloat dstY1,<br>
- bool mirror_x, bool mirror_y)<br>
+ GLenum filter, bool mirror_x, bool mirror_y)<br>
{<br>
/* Find source/dst miptrees */<br>
struct intel_mipmap_tree *src_mt = find_miptree(buffer_bit, src_irb);<br>
@@ -185,7 +185,7 @@ do_blorp_blit(struct brw_context *brw, GLbitfield buffer_bit,<br>
dst_mt, dst_irb->mt_level, dst_irb->mt_layer,<br>
srcX0, srcY0, srcX1, srcY1,<br>
dstX0, dstY0, dstX1, dstY1,<br>
- mirror_x, mirror_y);<br>
+ filter, mirror_x, mirror_y);<br>
<br>
intel_renderbuffer_set_needs_downsample(dst_irb);<br>
}<br>
@@ -245,14 +245,6 @@ try_blorp_blit(struct brw_context *brw,<br>
fixup_mirroring(mirror_y, srcY0, srcY1);<br>
fixup_mirroring(mirror_y, dstY0, dstY1);<br>
<br>
- /* Linear filtering is not yet implemented in blorp engine. So, fallback<br>
- * to other blit paths.<br>
- */<br>
- if ((srcX1 - srcX0 != dstX1 - dstX0 ||<br>
- srcY1 - srcY0 != dstY1 - dstY0) &&<br>
- filter == GL_LINEAR)<br>
- return false;<br>
-<br>
/* If the destination rectangle needs to be clipped or scissored, do so.<br>
*/<br>
if (!(clip_or_scissor(mirror_x, srcX0, srcX1, dstX0, dstX1,<br>
@@ -304,7 +296,7 @@ try_blorp_blit(struct brw_context *brw,<br>
if (dst_irb)<br>
do_blorp_blit(brw, buffer_bit, src_irb, dst_irb, srcX0, srcY0,<br>
srcX1, srcY1, dstX0, dstY0, dstX1, dstY1,<br>
- mirror_x, mirror_y);<br>
+ filter, mirror_x, mirror_y);<br>
}<br>
break;<br>
case GL_DEPTH_BUFFER_BIT:<br>
@@ -316,7 +308,7 @@ try_blorp_blit(struct brw_context *brw,<br>
return false;<br>
do_blorp_blit(brw, buffer_bit, src_irb, dst_irb, srcX0, srcY0,<br>
srcX1, srcY1, dstX0, dstY0, dstX1, dstY1,<br>
- mirror_x, mirror_y);<br>
+ filter, mirror_x, mirror_y);<br>
break;<br>
case GL_STENCIL_BUFFER_BIT:<br>
src_irb =<br>
@@ -327,7 +319,7 @@ try_blorp_blit(struct brw_context *brw,<br>
return false;<br>
do_blorp_blit(brw, buffer_bit, src_irb, dst_irb, srcX0, srcY0,<br>
srcX1, srcY1, dstX0, dstY0, dstX1, dstY1,<br>
- mirror_x, mirror_y);<br>
+ filter, mirror_x, mirror_y);<br>
break;<br>
default:<br>
assert(false);<br>
@@ -396,7 +388,7 @@ brw_blorp_copytexsubimage(struct brw_context *brw,<br>
dst_mt, dst_image->Level, dst_image->Face + slice,<br>
srcX0, srcY0, srcX1, srcY1,<br>
dstX0, dstY0, dstX1, dstY1,<br>
- false, mirror_y);<br>
+ GL_NEAREST, false, mirror_y);<br>
<br>
/* If we're copying to a packed depth stencil texture and the source<br>
* framebuffer has separate stencil, we need to also copy the stencil data<br>
@@ -420,7 +412,7 @@ brw_blorp_copytexsubimage(struct brw_context *brw,<br>
dst_image->Face + slice,<br>
srcX0, srcY0, srcX1, srcY1,<br>
dstX0, dstY0, dstX1, dstY1,<br>
- false, mirror_y);<br>
+ GL_NEAREST, false, mirror_y);<br>
}<br>
}<br>
<br>
</div></div>@@ -873,15 +865,22 @@ brw_blorp_blit_program::compile(struct brw_context *brw,<br>
<div class="im"> decode_msaa(key->tex_samples, key->tex_layout);<br>
}<br>
<br>
- /* Now (X, Y, S) = decode_msaa(tex_samples, detile(tex_tiling, offset)).<br>
- *<br>
- * In other words: X, Y, and S now contain values which, when passed to<br>
- * the texturing unit, will cause data to be read from the correct<br>
- * memory location. So we can fetch the texel now.<br>
- */<br>
- if (key->tex_layout == INTEL_MSAA_LAYOUT_CMS)<br>
- mcs_fetch();<br>
- texel_fetch(texture_data[0]);<br>
</div>+ if (key->src_samples == 0 &&<br>
+ key->dst_samples == 0 &&<br>
+ !key->src_tiled_w) {<br>
+ sample(texture_data[0]);<br>
+ }<br>
<div class="im">+ else {<br></div></blockquote><div><br></div><div>Style nit: we usually combine "} else {" on one line.<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">
<div class="im">
+ /* Now (X, Y, S) = decode_msaa(tex_samples, detile(tex_tiling, offset)).<br>
+ *<br>
+ * In other words: X, Y, and S now contain values which, when passed to<br>
+ * the texturing unit, will cause data to be read from the correct<br>
+ * memory location. So we can fetch the texel now.<br>
+ */<br>
+ if (key->tex_layout == INTEL_MSAA_LAYOUT_CMS)<br>
+ mcs_fetch();<br>
+ texel_fetch(texture_data[0]);<br>
+ }<br>
}<br>
<br>
/* Finally, write the fetched (or blended) value to the render target and<br>
</div>@@ -1442,14 +1441,17 @@ brw_blorp_blit_program::translate_dst_to_src()<br>
<div class="im"> brw_RNDD(&func, Yp_f, Y_f);<br>
brw_MUL(&func, X_f, Xp_f, brw_imm_f(1 / key->x_scale));<br>
brw_MUL(&func, Y_f, Yp_f, brw_imm_f(1 / key->y_scale));<br>
</div>- } else {<br>
+ SWAP_XY_AND_XPYP();<br>
+ } else if (key->src_samples > 1 ||<br>
+ key->dst_samples > 1 ||<br>
+ key->src_tiled_w) {<br>
<div class="im"> /* Round the float coordinates down to nearest integer by moving to<br>
* UD registers.<br>
</div> */<br>
brw_MOV(&func, Xp, X_f);<br>
brw_MOV(&func, Yp, Y_f);<br>
+ SWAP_XY_AND_XPYP();<br>
}<br>
- SWAP_XY_AND_XPYP();<br>
brw_set_compression_control(&func, BRW_COMPRESSION_NONE);<br>
}<br>
<br>
@@ -1873,10 +1875,18 @@ brw_blorp_blit_program::texture_lookup(struct brw_reg dst,<br>
for (int arg = 0; arg < num_args; ++arg) {<br>
switch (args[arg]) {<br>
case SAMPLER_MESSAGE_ARG_U_FLOAT:<br>
- brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F), X);<br>
+ if (s_is_zero)<br>
+ brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F),<br>
+ retype(X, BRW_REGISTER_TYPE_F));<br>
+ else<br>
+ brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F), X);<br>
break;<br>
case SAMPLER_MESSAGE_ARG_V_FLOAT:<br>
- brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F), Y);<br>
+ if (s_is_zero)<br>
+ brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F),<br>
+ retype(Y, BRW_REGISTER_TYPE_F));<br>
+ else<br>
+ brw_MOV(&func, retype(mrf, BRW_REGISTER_TYPE_F), Y);<br>
break;<br>
case SAMPLER_MESSAGE_ARG_U_INT:<br>
brw_MOV(&func, mrf, X);<br>
@@ -2050,6 +2060,7 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,<br>
<div class="im"> GLfloat src_x1, GLfloat src_y1,<br>
GLfloat dst_x0, GLfloat dst_y0,<br>
GLfloat dst_x1, GLfloat dst_y1,<br>
+ GLenum filter,<br>
bool mirror_x, bool mirror_y)<br>
{<br>
struct gl_context *ctx = &brw->ctx;<br>
</div>@@ -2058,7 +2069,10 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,<br>
<div class="im"> src.set(brw, src_mt, src_level, src_layer);<br>
dst.set(brw, dst_mt, dst_level, dst_layer);<br>
<br>
- src.brw_surfaceformat = dst.brw_surfaceformat;<br>
+ if (src.num_samples > 1)<br>
+ src.brw_surfaceformat = dst.brw_surfaceformat;<br>
+ else<br>
+ dst.brw_surfaceformat = src.brw_surfaceformat;<br></div></blockquote><div><br></div><div>I thought we decided that it was a bug to ever set src.brw_surfaceformat = dst.brw_surfaceformat, and we should do dst.brw_surfaceformat = src.brw_surfaceformat always. Am I remembering wrong?<br>
<br>If I'm remembering right, we should probably do this fix in a separate patch with an explanation as to why (the reasoning, as I recall, is that even though we do multisample resolves at the time of the blit, the spec defines them as though they happen at the time of rendering, which means that the type of averaging we do during the resolve should depend only on whether the source buffer is SRGB; the destination format should be ignored). I can't remember if there are any piglit tests that need to be updated to account for this fix.<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"><div class="im">
<br>
use_wm_prog = true;<br>
memset(&wm_prog_key, 0, sizeof(wm_prog_key));<br>
</div>@@ -2129,6 +2143,9 @@ brw_blorp_blit_params::brw_blorp_blit_params(struct brw_context *brw,<br>
wm_prog_key.x_scale = 2.0;<br>
<div class="im"> wm_prog_key.y_scale = src_mt->num_samples / 2.0;<br>
<br>
</div>+ /* Filter used for framebuffer blitting. */<br>
+ tex_filter = filter;<br>
<div class="im">+<br>
/* The render path must be configured to use the same number of samples as<br>
* the destination buffer.<br>
</div> */<br>
diff --git a/src/mesa/drivers/dri/i965/gen7_blorp.cpp b/src/mesa/drivers/dri/i965/gen7_blorp.cpp<br>
index 518d7f5..5d01b73 100644<br>
--- a/src/mesa/drivers/dri/i965/gen7_blorp.cpp<br>
+++ b/src/mesa/drivers/dri/i965/gen7_blorp.cpp<br>
@@ -230,9 +230,14 @@ gen7_blorp_emit_sampler_state(struct brw_context *brw,<br>
32, &sampler_offset);<br>
memset(sampler, 0, sizeof(*sampler));<br>
<br>
- sampler->ss0.min_filter = BRW_MAPFILTER_LINEAR;<br>
+ if (params->tex_filter == GL_NEAREST) {<br>
+ sampler->ss0.min_filter = BRW_MAPFILTER_NEAREST;<br>
+ sampler->ss0.mag_filter = BRW_MAPFILTER_NEAREST;<br>
+ } else {<br>
+ sampler->ss0.min_filter = BRW_MAPFILTER_LINEAR;<br>
+ sampler->ss0.mag_filter = BRW_MAPFILTER_LINEAR;<br>
+ }<br>
sampler->ss0.mip_filter = BRW_MIPFILTER_NONE;<br>
- sampler->ss0.mag_filter = BRW_MAPFILTER_LINEAR;<br>
<br>
sampler->ss3.r_wrap_mode = BRW_TEXCOORDMODE_CLAMP;<br>
sampler->ss3.s_wrap_mode = BRW_TEXCOORDMODE_CLAMP;<br>
<div class=""><div class="h5">diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
index d6643ca..de5f8f2 100644<br>
--- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
+++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c<br>
@@ -1552,7 +1552,7 @@ intel_miptree_updownsample(struct brw_context *brw,<br>
width, height,<br>
dst_x0, dst_y0,<br>
width, height,<br>
- false, false /*mirror x, y*/);<br>
+ GL_NEAREST, false, false /*mirror x, y*/);<br>
<br>
if (src->stencil_mt) {<br>
brw_blorp_blit_miptrees(brw,<br>
@@ -1562,7 +1562,7 @@ intel_miptree_updownsample(struct brw_context *brw,<br>
width, height,<br>
dst_x0, dst_y0,<br>
width, height,<br>
- false, false /*mirror x, y*/);<br>
+ GL_NEAREST, false, false /*mirror x, y*/);<br>
}<br>
}<br>
<br>
--<br>
1.8.1.4<br>
<br>
</div></div></blockquote></div><br></div></div>