On Sep 8, 2016 11:13 PM, "Pohjolainen, Topi" <<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a>> wrote: > > On Thu, Sep 08, 2016 at 08:49:56AM -0700, Jason Ekstrand wrote: > > On Sep 7, 2016 9:30 PM, "Pohjolainen, Topi" > > <[1]<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][2]<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][3]<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, > > So, above there is already: > > * 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. > > What would you add? Hmmm... I think my reservations about that convent are because it sounds like a workaround. How about this: If we are drawing into this with compression enabled, then we must also enable compression when texturing from it regardless of fast_clear_state. If we don't then, after the first draw call with this setup, there will be data in the CCS which won't get picked up by subsequent texturing operations as required by ARB_texture_barrier. Since we don't want to re-emit the binding table or do a resolve operation every draw call, the easiest thing to do is just enable compression on the texturing side. This is completely safe to do since, if compressed texturing weren't allowed, we would have disabled compression of render targets in whatever_that_function_is_called(). > > > > Reviewed-by: Jason Ekstrand <[4]<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][5]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > > [4][6]<a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > > > > > > > References > > > > > > > > 1. mailto:[7]<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a> > > > > 2. mailto:[8]<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > > > 3. mailto:[9]<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > > > 4. [10]<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@gmail.com">topi.pohjolainen@gmail.com</a> > > 3. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > 4. mailto:<a href="mailto:jason@jlekstrand.net">jason@jlekstrand.net</a> > > 5. mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > 6. <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a> > > 7. mailto:<a href="mailto:topi.pohjolainen@gmail.com">topi.pohjolainen@gmail.com</a> > > 8. mailto:<a href="mailto:topi.pohjolainen@intel.com">topi.pohjolainen@intel.com</a> > > 9. mailto:<a href="mailto:mesa-dev@lists.freedesktop.org">mesa-dev@lists.freedesktop.org</a> > > 10. <a href="https://lists.freedesktop.org/mailman/listinfo/mesa-dev">https://lists.freedesktop.org/mailman/listinfo/mesa-dev</a>