On Tue, May 21, 2013 at 10:27 AM, Paul Berry <stereotype...@gmail.com> wrote:
> On 17 May 2013 19:11, Jordan Justen <jordan.l.jus...@intel.com> 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?
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)
> 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".
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.
>> Signed-off-by: Jordan Justen <jordan.l.jus...@intel.com>
>> ---
>> 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).
This goes along with the comment above.
> 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).
Hmm. Yeah, I'm not sure that scenario is handled properly by this.
>>
>> 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;".
Okay, I'll update the depth code based on your suggestions here and below.
-Jordan
>>
>> + 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
>> mesa-dev@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
>
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
