[Mesa-dev] [PATCH v3 10/19] i965/gen6 depth surface: calculate minimum array element being rendered

[Pohjolainen, Topi]

On Fri, Aug 01, 2014 at 12:43:38PM -0700, Jordan Justen wrote:
> On Fri, Aug 1, 2014 at 2:44 AM, Pohjolainen, Topi
> <topi.pohjolainen at intel.com> wrote:
> > On Fri, Aug 01, 2014 at 12:53:40AM -0700, Jordan Justen wrote:
> >> (a23cfb8 for gen6)
> >>
> >> In layered rendering this will be 0. Otherwise it will be the
> >> selected slice.
> >>
> >> Signed-off-by: Jordan Justen <jordan.l.justen at intel.com>
> >> ---
> >>  src/mesa/drivers/dri/i965/gen6_blorp.cpp     |  3 +++
> >>  src/mesa/drivers/dri/i965/gen6_depth_state.c | 10 ++++++++++
> >>  2 files changed, 13 insertions(+)
> >>
> >> diff --git a/src/mesa/drivers/dri/i965/gen6_blorp.cpp b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> >> index 131c4aa..ff1732d 100644
> >> --- a/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> >> +++ b/src/mesa/drivers/dri/i965/gen6_blorp.cpp
> >> @@ -793,6 +793,7 @@ gen6_blorp_emit_depth_stencil_config(struct brw_context *brw,
> >>     uint32_t tile_mask_x, tile_mask_y;
> >>     uint32_t surftype;
> >>     unsigned int depth = MAX2(params->depth.mt->logical_depth0, 1);
> >> +   unsigned int min_array_element;
> >>     GLenum gl_target = params->depth.mt->target;
> >>     unsigned int lod;
> >>
> >> @@ -818,6 +819,8 @@ gen6_blorp_emit_depth_stencil_config(struct brw_context *brw,
> >>                                     NULL,
> >>                                     &tile_mask_x, &tile_mask_y);
> >>
> >> +   min_array_element = params->depth.layer;
> >
> > This could be slightly simpler:
> >
> >       const unsigned min_array_element = params->depth.layer;
> >
> >> +
> >>     lod = params->depth.level - params->depth.mt->first_level;
> >>
> >>     /* 3DSTATE_DEPTH_BUFFER */
> >> diff --git a/src/mesa/drivers/dri/i965/gen6_depth_state.c b/src/mesa/drivers/dri/i965/gen6_depth_state.c
> >> index 1f36ed8..5e3981c 100644
> >> --- a/src/mesa/drivers/dri/i965/gen6_depth_state.c
> >> +++ b/src/mesa/drivers/dri/i965/gen6_depth_state.c
> >> @@ -48,6 +48,7 @@ gen6_emit_depth_stencil_hiz(struct brw_context *brw,
> >>     struct gl_framebuffer *fb = ctx->DrawBuffer;
> >>     uint32_t surftype;
> >>     unsigned int depth = 1;
> >> +   unsigned int min_array_element;
> >>     GLenum gl_target = GL_TEXTURE_2D;
> >>     unsigned int lod;
> >>     const struct intel_renderbuffer *irb = NULL;
> >> @@ -100,6 +101,15 @@ gen6_emit_depth_stencil_hiz(struct brw_context *brw,
> >>        break;
> >>     }
> >>
> >> +   if (fb->MaxNumLayers > 0 || !irb) {
> >> +      min_array_element = 0;
> >> +   } else if (irb->mt->num_samples > 1) {
> >> +      /* Convert physical layer to logical layer. */
> >> +      min_array_element = irb->mt_layer / irb->mt->num_samples;
> >
> > I still don't understand this. This would be needed only for surfaces were
> > samples indices are in their own layers. On gen6 this is never the case as
> > all the surfaces are interleaved (i.e., samples are in the same physical
> > layer). Moreover, this is true also on gen7 for depth surfaces - and hence
> > I'm also questioning the existing logic in the gen7 specific path.
> 
> I looked again, and I think gen7/gen8 does not divide by num_samples
> for depth state. (Chris changed this in 77d55ef4.) It does in
> gen7_wm_surface_state.c.
> 
> A piglit run verifies that this works for gen6:
>   const unsigned min_array_element = irb ? irb->mt_layer : 0;
> This matches gen7 & gen8 for depth state, so I'll change to this.
> Would that get an r-b from you for this patch?

Sure.

> 
> But delving further into the "divide by num_samples" question,
> git grep -C5 "mt_layer =" src/mesa/drivers/dri/i965
> turned up some interesting results.
> 
> intel_fbo.c:
>   irb->mt_layer = layer_multiplier * layer;
>   (For gen7+ UMS/CMS layer_multiplier = mt->num_samples)
> 
> intel_fbo.h:
>  * Note: for a 2D multisample array texture on Gen7+ using
>  * INTEL_MSAA_LAYOUT_UMS or INTEL_MSAA_LAYOUT_CMS, mt_layer is the physical
>  * layer holding sample 0.  So, for example, if mt->num_samples == 4, then
>  * logical layer n corresponds to mt_layer == 4*n.
> 
> intel_mipmap_tree.c:compute_msaa_layout always returns IMS for depth,
> so that explains why in all cases we don't divide by num_samples for
> depth.
> 
> It might be better to make a helper routine that adjusts layer based
> on the msaa layout type. Basically if UMS/CMS, divide by num_samples.
> 
> Right now there is a lot of code path knowledge in this calculation.
> For instance:
>  * on depth paths we are using IMS, so don't divide
>  * on gen6 we only use IMS for color, so don't divide
>  * on gen7/8 color, we are using UMS/CMS, so divide

Thanks for clarifying all this. And the helper sounds like a really good idea.

> 
> -Jordan
> 
> >> +   } else {
> >> +      min_array_element = irb->mt_layer;
> >> +   }
> >> +
> >>     lod = irb ? irb->mt_level - irb->mt->first_level : 0;
> >>
> >>     BEGIN_BATCH(7);
> >> --
> >> 2.0.1
> >>
> >> _______________________________________________
> >> 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