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.pohjolai...@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.jus...@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@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
