On Sep 7, 2016 9:30 PM, "Pohjolainen, Topi" <<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: > > On Wed, Sep 07, 2016 at 03:25:30PM -0700, Jason Ekstrand wrote: > > On Sep 7, 2016 10:24 AM, "Topi Pohjolainen" > > <[1]<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: > > > > > > Once mcs buffer gets allocated without delay for lossless > > > compression (same as we do for msaa), one gets regression in: > > > > > > GL45-CTS.texture_barrier_ARB.same-texel-rw > > > > > > Setting the auxiliary surface for both sampling engine and data > > > port seems to fix this. I haven't found any hardware documentation > > > backing this though. > > > > > > v2 (Jason): Prepare also for the case where surface is sampled with > > > non-compressible format forcing also rendering without > > > compression. > > > v3: Split asserts and decision making. > > > > > > Signed-off-by: Topi Pohjolainen <[2]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a>> > > > --- > > > src/mesa/drivers/dri/i965/brw_wm_surface_state.c | 63 > > +++++++++++++++++++++--- > > > 1 file changed, 56 insertions(+), 7 deletions(-) > > > > > > diff --git a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > > index c1273c5..054c5c8 100644 > > > --- a/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > > +++ b/src/mesa/drivers/dri/i965/brw_wm_surface_state.c > > > @@ -140,9 +140,7 @@ brw_emit_surface_state(struct brw_context *brw, > > > struct isl_surf *aux_surf = NULL, aux_surf_s; > > > uint64_t aux_offset = 0; > > > enum isl_aux_usage aux_usage = ISL_AUX_USAGE_NONE; > > > - if (mt->mcs_mt && > > > - ((view.usage & ISL_SURF_USAGE_RENDER_TARGET_BIT) || > > > - mt->fast_clear_state != INTEL_FAST_CLEAR_STATE_RESOLVED)) { > > > + if (mt->mcs_mt && !(flags & INTEL_AUX_BUFFER_DISABLED)) { > > > intel_miptree_get_aux_isl_surf(brw, mt, &aux_surf_s, > > &aux_usage); > > > aux_surf = &aux_surf_s; > > > assert(mt->mcs_mt->offset == 0); > > > @@ -425,6 +423,54 @@ swizzle_to_scs(GLenum swizzle, bool > > need_green_to_blue) > > > return (need_green_to_blue && scs == HSW_SCS_GREEN) ? > > HSW_SCS_BLUE : scs; > > > } > > > > > > +static unsigned > > > +brw_find_matching_rb(const struct gl_framebuffer *fb, > > > + const struct intel_mipmap_tree *mt) > > > +{ > > > + for (unsigned i = 0; i < fb->_NumColorDrawBuffers; i++) { > > > + const struct intel_renderbuffer *irb = > > > + intel_renderbuffer(fb->_ColorDrawBuffers[i]); > > > + > > > + if (irb->mt == mt) > > > + return i; > > > + } > > > + > > > + return fb->_NumColorDrawBuffers; > > > +} > > > + > > > +static bool > > > +brw_disable_aux_surface(const struct brw_context *brw, > > > + const struct intel_mipmap_tree *mt) > > > +{ > > > + /* Nothing to disable. */ > > > + if (!mt->mcs_mt) > > > + return false; > > > + > > > + /* There are special cases only for lossless compression. */ > > > + if (!intel_miptree_is_lossless_compressed(brw, mt)) > > > + return mt->fast_clear_state == > > INTEL_FAST_CLEAR_STATE_RESOLVED; > > > + > > > + const struct gl_framebuffer *fb = brw->ctx.DrawBuffer; > > > + const unsigned rb_index = brw_find_matching_rb(fb, mt); > > > + > > > + /* In practise it looks that setting the same lossless compressed > > surface > > > + * to be sampled without auxiliary surface and to be written with > > auxiliary > > > + * surface confuses the hardware. Therefore sampler engine must > > be provided > > > + * with auxiliary buffer regardless of the fast clear state if > > the same > > > + * surface is also going to be written during the same rendering > > pass with > > > + * auxiliary buffer enabled. > > > + */ > > > + if (rb_index < fb->_NumColorDrawBuffers) { > > > + if (brw->draw_aux_buffer_disabled[rb_index]) { > > > + assert(mt->fast_clear_state == > > INTEL_FAST_CLEAR_STATE_RESOLVED); > > > + } > > > + > > > + return brw->draw_aux_buffer_disabled[rb_index]; > > > > Can we use "mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_RESOLVED" > > here as well? For render targets we want to look at > > aux_buffer_disabled but for textures (which is the only case that uses > > this function), fast_clear_state should be sufficient. > > Like the comment says, we can't skip setting aux buffer for sampling in case > the same surface is written as render target with aux enabled. Hardware > doesn't seem to like this. This is specifically for that purpose checking > the flag. Let me see if I understand this correctly: this handles the case where the miptree is bound as both a render target and a texture and there is nothing about the texture view scenario causing it to skip compression and, for some reason, the surface starts of the draw call fully resolved. If *any* drawing has been done without a resolve, we would be in the UNRESOLVED state and this wouldn't be needed. That's terribly specific but also a valid use case. We could potentially handle this by setting the render target surfaces to UNRESOLVED a bit earlier in the pipeline setup code. I believe we flag things as unresolved rather late; in particular, after we've done our texture state setup. The other option is to do exactly what you did and add a comment. That's probably the best option. With a good comment added, Reviewed-by: Jason Ekstrand <<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a>> --Jason > > > > > + } > > > + > > > + return mt->fast_clear_state == INTEL_FAST_CLEAR_STATE_RESOLVED; > > > +} > > > + > > > void > > > brw_update_texture_surface(struct gl_context *ctx, > > > unsigned unit, > > > @@ -542,7 +588,8 @@ brw_update_texture_surface(struct gl_context > > *ctx, > > > obj->Target == GL_TEXTURE_CUBE_MAP_ARRAY) > > > view.usage |= ISL_SURF_USAGE_CUBE_BIT; > > > > > > - const int flags = 0; > > > + const int flags = > > > + brw_disable_aux_surface(brw, mt) ? > > INTEL_AUX_BUFFER_DISABLED : 0; > > > brw_emit_surface_state(brw, mt, flags, mt->target, view, > > > surface_state_infos[brw->gen].tex_mocs, > > > surf_offset, surf_index, > > > @@ -1113,7 +1160,8 @@ update_renderbuffer_read_surfaces(struct > > brw_context *brw) > > > .usage = ISL_SURF_USAGE_TEXTURE_BIT, > > > }; > > > > > > - const int flags = 0; > > > + const int flags = brw->draw_aux_buffer_disabled[i] ? > > > + INTEL_AUX_BUFFER_DISABLED : 0; > > > brw_emit_surface_state(brw, irb->mt, flags, target, > > view, > > > > > surface_state_infos[brw->gen].tex_mocs, > > > surf_offset, surf_index, > > > @@ -1672,8 +1720,9 @@ update_image_surface(struct brw_context *brw, > > > }; > > > > > > const int surf_index = surf_offset - > > &brw->wm.base.surf_offset[0]; > > > - > > > - const int flags = 0; > > > + const int flags = > > > + mt->fast_clear_state == > > INTEL_FAST_CLEAR_STATE_RESOLVED ? > > > + INTEL_AUX_BUFFER_DISABLED : 0; > > > > According to brw_context, images should *always* be resolved. Can we > > make this check an assert and always pass AUX_BUFFER_DISABLED? > > > > > brw_emit_surface_state(brw, mt, flags, mt->target, view, > > > > > surface_state_infos[brw->gen].tex_mocs, > > > surf_offset, surf_index, > > > -- > > > 2.5.5 > > > > > > _______________________________________________ > > > mesa-dev mailing list > > > [3]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > [4]<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > References > > > > 1. mailto:<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a> > > 2. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > 3. mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > 4. <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>