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

[Jordan Justen]

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?

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

-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