<div dir="ltr">On 21 May 2013 11:09, Jordan Justen <<a href="mailto:jljusten@gmail.com" target="_blank">jljusten@gmail.com</a>> wrote: <div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div class="im">On Tue, May 21, 2013 at 10:27 AM, Paul Berry <<a href="mailto:stereotype441@gmail.com">stereotype441@gmail.com</a>> wrote: > On 17 May 2013 19:11, Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> wrote: >> >> Rather than pointing the surface_state directly at a single >> sub-image of the texture for rendering, we now point the >> surface_state at the top level of the texture, and configure >> the surface_state as needed based on this. >> >> We now also need to stop setting the FORCE_ZERO_RTAINDEX bit >> in the clip date so render target array values other than zero >> will be used. > > > 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? </div>It was my intent to essentially support gl_Layer with this patch. Of course, there are still missing pieces before it would be user visibible: * support layered depth/stencil * enable AMD_vertex_shader_layer (or GL 3.2) <div class="im"> > 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". </div>Would you prefer I delay the layered support portions of this patch, or just move it to a separate patch in this same series? I'm guessing you'd prefer to delay the layered support until a future series where everything can be enabled and made user-visible. </blockquote><div> I'm ok with the gl_Layer-related stuff going in now; I was mostly just confused by the commit subject. How about if we defer the gen7_clip_state.c part of the patch until later (since there's a question about whether it's going to be correct in the long run, and if I'm not mistaken it's not necessary now), and leave the rest of the patch as is? If we did that, and changed the subject line to something that doesn't imply that gl_Layer is finished yet (maybe "prepare for supporting layered color renderbuffers"?), that would be good enough for me. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> <div><div class="h5"> >> Signed-off-by: Jordan Justen <<a href="mailto:jordan.l.justen@intel.com">jordan.l.justen@intel.com</a>> >> --- >> src/mesa/drivers/dri/i965/brw_defines.h | 2 + >> src/mesa/drivers/dri/i965/gen7_clip_state.c | 3 +- >> src/mesa/drivers/dri/i965/gen7_wm_surface_state.c | 63 >> +++++++++++++++------ >> 3 files changed, 48 insertions(+), 20 deletions(-) >> >> diff --git a/src/mesa/drivers/dri/i965/brw_defines.h >> b/src/mesa/drivers/dri/i965/brw_defines.h >> index fedd78c..d61151f 100644 >> --- a/src/mesa/drivers/dri/i965/brw_defines.h >> +++ b/src/mesa/drivers/dri/i965/brw_defines.h >> @@ -539,6 +539,8 @@ >> #define GEN7_SURFACE_MULTISAMPLECOUNT_8 (3 << 3) >> #define GEN7_SURFACE_MSFMT_MSS (0 << 6) >> #define GEN7_SURFACE_MSFMT_DEPTH_STENCIL (1 << 6) >> +#define GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT 18 >> +#define GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT 7 >> >> /* Surface state DW5 */ >> #define BRW_SURFACE_X_OFFSET_SHIFT 25 >> diff --git a/src/mesa/drivers/dri/i965/gen7_clip_state.c >> b/src/mesa/drivers/dri/i965/gen7_clip_state.c >> index 29a5ed5..1256f32 100644 >> --- a/src/mesa/drivers/dri/i965/gen7_clip_state.c >> +++ b/src/mesa/drivers/dri/i965/gen7_clip_state.c >> @@ -107,8 +107,7 @@ upload_clip_state(struct brw_context *brw) >> GEN6_CLIP_XY_TEST | >> dw2); >> OUT_BATCH(U_FIXED(0.125, 3) << GEN6_CLIP_MIN_POINT_WIDTH_SHIFT | >> - U_FIXED(255.875, 3) << GEN6_CLIP_MAX_POINT_WIDTH_SHIFT | >> - GEN6_CLIP_FORCE_ZERO_RTAINDEX); >> + U_FIXED(255.875, 3) << GEN6_CLIP_MAX_POINT_WIDTH_SHIFT); > > > 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). </div></div>This goes along with the comment above. <div class="im"> > 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). </div>Hmm. Yeah, I'm not sure that scenario is handled properly by this. <div><div class="h5"> >> >> ADVANCE_BATCH(); >> } >> >> 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 6c01545..5f15eff 100644 >> --- a/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c >> +++ b/src/mesa/drivers/dri/i965/gen7_wm_surface_state.c >> @@ -23,6 +23,7 @@ >> #include "main/mtypes.h" >> #include "main/blend.h" >> #include "main/samplerobj.h" >> +#include "main/texformat.h" >> #include "program/prog_parameter.h" >> >> #include "intel_mipmap_tree.h" >> @@ -529,12 +530,13 @@ gen7_update_renderbuffer_surface(struct brw_context >> *brw, >> struct gl_context *ctx = &intel->ctx; >> struct intel_renderbuffer *irb = intel_renderbuffer(rb); >> struct intel_region *region = irb->mt->region; >> - uint32_t tile_x, tile_y; >> uint32_t format; >> /* _NEW_BUFFERS */ >> gl_format rb_format = _mesa_get_render_format(ctx, >> intel_rb_format(irb)); >> - >> - assert(!layered); >> + uint32_t surftype; >> + bool is_array = false; >> + int depth = rb->Depth > 0 ? rb->Depth - 1 : 0; > > 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;". </div></div>Okay, I'll update the depth code based on your suggestions here and below. -Jordan <div class="HOEnZb"><div class="h5"> >> >> + int min_array_element = 0; >> >> uint32_t *surf = brw_state_batch(brw, AUB_TRACE_SURFACE_STATE, >> 8 * 4, 32, >> &brw->wm.surf_offset[unit]); >> @@ -550,7 +552,23 @@ gen7_update_renderbuffer_surface(struct brw_context >> *brw, >> __FUNCTION__, _mesa_get_format_name(rb_format)); >> } >> >> - surf[0] = BRW_SURFACE_2D << BRW_SURFACE_TYPE_SHIFT | >> + if (rb->TexImage) { >> + surftype = translate_tex_target(rb->TexImage->TexObject->Target); >> + is_array = >> _mesa_tex_target_is_array(rb->TexImage->TexObject->Target); >> >> + if (rb->TexImage->TexObject->Target == GL_TEXTURE_CUBE_MAP_ARRAY) { >> + assert(rb->Depth > 0); >> + surftype = BRW_SURFACE_2D; >> + depth = (6 * (depth + 1)) - 1; >> + } else if (rb->TexImage->TexObject->Target == GL_TEXTURE_CUBE_MAP) >> { >> + surftype = BRW_SURFACE_2D; >> + depth = 5; >> + is_array = true; >> + } > > > 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: > > switch (rb->TexImage->TexObject->Target) { > case GL_TEXTURE_CUBE_MAP_ARRAY: > case GL_TEXTURE_CUBE_MAP: > surftype = BRW_SURFACE_2D; > is_array = true; > depth *=6; > break; > default: > surftype = translate_tex_target(...); > is_array = _mesa_tex_target_is_array(...); > break; > } > >> >> + } else { >> + surftype = BRW_SURFACE_2D; >> + } >> + >> + surf[0] = surftype << BRW_SURFACE_TYPE_SHIFT | >> format << BRW_SURFACE_FORMAT_SHIFT | >> (irb->mt->array_spacing_lod0 ? GEN7_SURFACE_ARYSPC_LOD0 >> : GEN7_SURFACE_ARYSPC_FULL) | >> @@ -561,24 +579,33 @@ gen7_update_renderbuffer_surface(struct brw_context >> *brw, >> if (irb->mt->align_w == 8) >> surf[0] |= GEN7_SURFACE_HALIGN_8; >> >> - /* reloc */ >> - surf[1] = intel_renderbuffer_tile_offsets(irb, &tile_x, &tile_y) + >> - region->bo->offset; /* reloc */ >> + if (is_array) { >> + surf[0] |= GEN7_SURFACE_IS_ARRAY; >> + } >> + >> + if (!layered) { >> + if (irb->mt->num_samples > 1) { >> + min_array_element = irb->mt_layer / irb->mt->num_samples; >> + } else { >> + min_array_element = irb->mt_layer; >> + } >> + } >> + >> + surf[1] = region->bo->offset; >> >> assert(brw->has_surface_tile_offset); >> - /* Note that the low bits of these fields are missing, so >> - * there's the possibility of getting in trouble. >> - */ >> - assert(tile_x % 4 == 0); >> - assert(tile_y % 2 == 0); >> - surf[5] = SET_FIELD(tile_x / 4, BRW_SURFACE_X_OFFSET) | >> - SET_FIELD(tile_y / 2, BRW_SURFACE_Y_OFFSET); >> >> - surf[2] = SET_FIELD(rb->Width - 1, GEN7_SURFACE_WIDTH) | >> - SET_FIELD(rb->Height - 1, GEN7_SURFACE_HEIGHT); >> - surf[3] = region->pitch - 1; >> + surf[5] = irb->mt_level; >> + >> + surf[2] = SET_FIELD(irb->mt->logical_width0 - 1, GEN7_SURFACE_WIDTH) | >> + SET_FIELD(irb->mt->logical_height0 - 1, >> GEN7_SURFACE_HEIGHT); >> + >> + surf[3] = (depth << BRW_SURFACE_DEPTH_SHIFT) | >> + (region->pitch - 1); >> >> - surf[4] = gen7_surface_msaa_bits(irb->mt->num_samples, >> irb->mt->msaa_layout); >> + surf[4] = gen7_surface_msaa_bits(irb->mt->num_samples, >> irb->mt->msaa_layout) | >> + min_array_element << GEN7_SURFACE_MIN_ARRAY_ELEMENT_SHIFT | >> + depth << GEN7_SURFACE_RENDER_TARGET_VIEW_EXTENT_SHIFT; >> >> if (irb->mt->msaa_layout == INTEL_MSAA_LAYOUT_CMS) { >> gen7_set_surface_mcs_info(brw, surf, brw->wm.surf_offset[unit], >> -- >> 1.7.10.4 >> >> _______________________________________________ >> mesa-dev mailing list >> <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> >> <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > _______________________________________________ > mesa-dev mailing list > <a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > <a href="http://lists.freedesktop.org/mailman/listinfo/mesa-dev" target="_blank">http://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > </div></div></blockquote></div> </div></div>