<div dir="ltr">On 17 May 2013 19:11, Jordan Justen <span dir="ltr"><<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.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">Rather than pointing the surface_state directly at a single<br>
sub-image of the texture for rendering, we now point the<br>
surface_state at the top level of the texture, and configure<br>
the surface_state as needed based on this.<br>
<br>
We now also need to stop setting the FORCE_ZERO_RTAINDEX bit<br>
in the clip date so render target array values other than zero<br>
will be used.<br></blockquote><div><br></div><div>Clarifying question to make sure I understand the purpose of this patch: it looks to me like you are *not* trying to enable support for gl_Layer (and layered framebuffer attachments) in this patch; you are merely changing the way SURFACE_STATE gets configured as a preparatory step, and you'll be adding gl_Layer support later. Is that correct?<br>
</div><div><br>If so, I think the subject line is a little misleading--"add support for" frequently means "add a user-visible feature" and I think someone could easily misunderstand this patch to be adding gl_Layer support. To clarify that there's no user-visible change involved here, perhaps we should say something like "use SURFACE_STATE fields to select level/layer to render to", or "stop using X/Y offsets to select level/layer to render to".<br>
<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>
Signed-off-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com" target="_blank">jordan.l.justen@intel.com</a>><br>
---<br>
src/mesa/drivers/dri/i965/brw_defines.h | 2 +<br>
src/mesa/drivers/dri/i965/gen7_clip_state.c | 3 +-<br>
src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 63 +++++++++++++++------<br>
3 files changed, 48 insertions(+), 20 deletions(-)<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/brw_defines.h b/src/mesa/drivers/dri/i965/brw_defines.h<br>
index fedd78c..d61151f 100644<br>
--- a/src/mesa/drivers/dri/i965/brw_defines.h<br>
+++ b/src/mesa/drivers/dri/i965/brw_defines.h<br>
@@ -539,6 +539,8 @@<br>
#define GEN7_SURFACE_MULTISAMPLECOUNT_8 (3 << 3)<br>
#define GEN7_SURFACE_MSFMT_MSS (0 << 6)<br>
#define GEN7_SURFACE_MSFMT_DEPTH_STENCIL (1 << 6)<br>
+#define GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT 18<br>
+#define GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT 7<br>
<br>
/* Surface state DW5 */<br>
#define BRW_SURFACE_X_OFFSET_SHIFT 25<br>
diff --git a/src/mesa/drivers/dri/i965/gen7_clip_state.c b/src/mesa/drivers/dri/i965/gen7_clip_state.c<br>
index 29a5ed5..1256f32 100644<br>
--- a/src/mesa/drivers/dri/i965/gen7_clip_state.c<br>
+++ b/src/mesa/drivers/dri/i965/gen7_clip_state.c<br>
@@ -107,8 +107,7 @@ upload_clip_state(struct brw_context *brw)<br>
GEN6_CLIP_XY_TEST |<br>
dw2);<br>
OUT_BATCH(U_FIXED(0.125, 3) << GEN6_CLIP_MIN_POINT_WIDTH_SHIFT |<br>
- U_FIXED(255.875, 3) << GEN6_CLIP_MAX_POINT_WIDTH_SHIFT |<br>
- GEN6_CLIP_FORCE_ZERO_RTAINDEX);<br>
+ U_FIXED(255.875, 3) << GEN6_CLIP_MAX_POINT_WIDTH_SHIFT);<br></blockquote><div><br></div><div>This doesn't seem right to me--are you sure this is needed? My reading of the bspec is that min_array_element gets applied after the clipper forces RTA index to 0, so it should still be ok for the clipper to force RTA index to 0 when we're rendering to a layer other than 0. It seems to me that the only time we should clear the GEN6_CLIP_FORCE_ZERO_RTAINDEX bit is when we want gl_Layer to take effect (which, if my earlier assumption is correct, we don't want to do yet). And even when we do get around to enabling gl_Layer, we won't want to do it unconditionally--we'll only want to enable it when the current draw framebuffer is layered (because gl_Layer is supposed to be ignored for non-layered framebuffers).<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">
ADVANCE_BATCH();<br>
}<br>
<br>
diff --git a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c<br>
index 6c01545..5f15eff 100644<br>
--- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c<br>
+++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c<br>
@@ -23,6 +23,7 @@<br>
#include "main/mtypes.h"<br>
#include "main/blend.h"<br>
#include "main/samplerobj.h"<br>
+#include "main/texformat.h"<br>
#include "program/prog_parameter.h"<br>
<br>
#include "intel_mipmap_tree.h"<br>
@@ -529,12 +530,13 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,<br>
struct gl_context *ctx = &intel->ctx;<br>
struct intel_renderbuffer *irb = intel_renderbuffer(rb);<br>
struct intel_region *region = irb->mt->region;<br>
- uint32_t tile_x, tile_y;<br>
uint32_t format;<br>
/* _NEW_BUFFERS */<br>
gl_format rb_format = _mesa_get_render_format(ctx, intel_rb_format(irb));<br>
-<br>
- assert(!layered);<br>
+ uint32_t surftype;<br>
+ bool is_array = false;<br>
+ int depth = rb->Depth > 0 ? rb->Depth - 1 : 0;<br></blockquote><div><br></div><div>As written, the "depth" variable holds the true depth minus one (since that's what the hardware expects in the "depth" field of SURFACE_STATE). That makes some of the math below confusing to follow (especially the formula "depth = (6 * (depth + 1)) - 1;"). I'd recommend storing the true depth in the "depth" variable, and don't subtract one until you store it in surf[3]. So this line would become "int depth = MAX(rb->Depth, 1);", and "depth = (6 * (depth + 1)) - 1;" would become "depth *= 6;".<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">
+ int min_array_element = 0;<br>
<br>
uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE,<br>
8 * 4, 32, &brw->wm.surf_offset[unit]);<br>
@@ -550,7 +552,23 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,<br>
__FUNCTION__, _mesa_get_format_name(rb_format));<br>
}<br>
<br>
- surf[0] = BRW_SURFACE_2D << BRW_SURFACE_TYPE_SHIFT |<br>
+ if (rb->TexImage) {<br>
+ surftype = translate_tex_target(rb->TexImage->TexObject->Target);<br>
+ is_array = _mesa_tex_target_is_array(rb->TexImage->TexObject->Target); <br></blockquote><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
+ if (rb->TexImage->TexObject->Target == GL_TEXTURE_CUBE_MAP_ARRAY) {<br>
+ assert(rb->Depth > 0);<br>
+ surftype = BRW_SURFACE_2D;<br>
+ depth = (6 * (depth + 1)) - 1;<br>
+ } else if (rb->TexImage->TexObject->Target == GL_TEXTURE_CUBE_MAP) {<br>
+ surftype = BRW_SURFACE_2D;<br>
+ depth = 5;<br>
+ is_array = true;<br>
+ }<br></blockquote><div><br><div>It reads awkwardly to me that in the case of
GL_TEXTURE_CUBE_MAP_ARRAY and GL_TEXTURE_CUBE_MAP we set surftype and
is_array incorrectly and then immediately override them with the correct
values. Consider something like this instead:<br>
<br></div><div>switch (rb->TexImage->TexObject->Target) {<br>case GL_TEXTURE_CUBE_MAP_ARRAY:<br></div><div><div>case GL_TEXTURE_CUBE_MAP:<br></div> surftype = BRW_SURFACE_2D;<br></div><div><div> is_array = true;<br>
</div>
depth *=6;<br></div> break;<br>default:<br><div> surftype = translate_tex_target(...);<br>
</div><div> is_array = _mesa_tex_target_is_array(...);<br></div><div> break;<br>}<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">
+ } else {<br>
+ surftype = BRW_SURFACE_2D;<br>
+ }<br>
+<br>
+ surf[0] = surftype << BRW_SURFACE_TYPE_SHIFT |<br>
format << BRW_SURFACE_FORMAT_SHIFT |<br>
(irb->mt->array_spacing_lod0 ? GEN7_SURFACE_ARYSPC_LOD0<br>
: GEN7_SURFACE_ARYSPC_FULL) |<br>
@@ -561,24 +579,33 @@ gen7_update_renderbuffer_surface(struct brw_context *brw,<br>
if (irb->mt->align_w == 8)<br>
surf[0] |= GEN7_SURFACE_HALIGN_8;<br>
<br>
- /* reloc */<br>
- surf[1] = intel_renderbuffer_tile_offsets(irb, &tile_x, &tile_y) +<br>
- region->bo->offset; /* reloc */<br>
+ if (is_array) {<br>
+ surf[0] |= GEN7_SURFACE_IS_ARRAY;<br>
+ }<br>
+<br>
+ if (!layered) {<br>
+ if (irb->mt->num_samples > 1) {<br>
+ min_array_element = irb->mt_layer / irb->mt->num_samples;<br>
+ } else {<br>
+ min_array_element = irb->mt_layer;<br>
+ }<br>
+ }<br>
+<br>
+ surf[1] = region->bo->offset;<br>
<br>
assert(brw->has_surface_tile_offset);<br>
- /* Note that the low bits of these fields are missing, so<br>
- * there's the possibility of getting in trouble.<br>
- */<br>
- assert(tile_x % 4 == 0);<br>
- assert(tile_y % 2 == 0);<br>
- surf[5] = SET_FIELD(tile_x / 4, BRW_SURFACE_X_OFFSET) |<br>
- SET_FIELD(tile_y / 2, BRW_SURFACE_Y_OFFSET);<br>
<br>
- surf[2] = SET_FIELD(rb->Width - 1, GEN7_SURFACE_WIDTH) |<br>
- SET_FIELD(rb->Height - 1, GEN7_SURFACE_HEIGHT);<br>
- surf[3] = region->pitch - 1;<br>
+ surf[5] = irb->mt_level;<br>
+<br>
+ surf[2] = SET_FIELD(irb->mt->logical_width0 - 1, GEN7_SURFACE_WIDTH) |<br>
+ SET_FIELD(irb->mt->logical_height0 - 1, GEN7_SURFACE_HEIGHT);<br>
+<br>
+ surf[3] = (depth << BRW_SURFACE_DEPTH_SHIFT) |<br>
+ (region->pitch - 1);<br>
<br>
- surf[4] = gen7_surface_msaa_bits(irb->mt->num_samples, irb->mt->msaa_layout);<br>
+ surf[4] = gen7_surface_msaa_bits(irb->mt->num_samples, irb->mt->msaa_layout) |<br>
+ min_array_element << GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT |<br>
+ depth << GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT;<br>
<br>
if (irb->mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS) {<br>
gen7_set_surface_mcs_info(brw, surf, brw->wm.surf_offset[unit],<br>
<span><font color="#888888">--<br>
1.7.10.4<br>
<br>
_______________________________________________<br>
mesa-dev mailing list<br>
<a href="mailto:mesa-dev@lists.freedesktop.org" target="_blank">mesa-dev@lists.freedesktop.org</a><br>
<a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a><br>
</font></span></blockquote></div><br></div></div>