[Mesa-dev] [PATCH 5/5] i965 gen7: add support for layered color renderbuffers
Paul Berry
stereotype441 at gmail.com
Tue May 21 11:19:47 PDT 2013
On 21 May 2013 11:09, Jordan Justen <jljusten at gmail.com> wrote:
> On Tue, May 21, 2013 at 10:27 AM, Paul Berry <stereotype441 at gmail.com>
> wrote:
> > On 17 May 2013 19:11, Jordan Justen <jordan.l.justen at 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.
>
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.
>
> >> Signed-off-by: Jordan Justen <jordan.l.justen at 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 at lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
> >
> >
> > _______________________________________________
> > mesa-dev mailing list
> > mesa-dev at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130521/f31bf385/attachment-0001.html>
More information about the mesa-dev
mailing list