On Mon, May 07, 2018 at 11:30:15AM +0300, Pohjolainen, Topi wrote: > On Thu, May 03, 2018 at 12:03:51PM -0700, Nanley Chery wrote: > > The indirect clear color isn't correctly tracked in > > intel_miptree::fast_clear_color. The initial value of ::fast_clear_color > > is zero, while that of the indirect clear color is undefined or > > non-zero. > > > > Topi Pohjolainen discovered this issue with MCS buffers. This issue is > > apparent when fast-clearing an MCS buffer for the first time with > > glClearColor = {0.0,}. Although the indirect clear color is non-zero, > > the initial aux state of the MCS is CLEAR and the tracked clear color is > > zero, so we avoid updating the indirect clear color with {0.0,}. > > > > Make the indirect clear color match the initial value of > > ::fast_clear_color. > > > > --- > > > > Hey Topi, > > > > Just FYI, this patch should fix the MCS bug you reported earlier. > > > > src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 33 > > ++++++++++++++++++--------- > > 1 file changed, 22 insertions(+), 11 deletions(-) > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > index 5d3ee569bd8..e70c9ff1ef4 100644 > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c > > @@ -978,11 +978,11 @@ create_ccs_buf_for_image(struct brw_context *brw, > > * system with CCS, we don't have the extra space at the end of the aux > > * buffer. So create a new bo here that will store that clear color. > > */ > > - const struct gen_device_info *devinfo = &brw->screen->devinfo; > > - if (devinfo->gen >= 10) { > > + if (brw->isl_dev.ss.clear_color_state_size > 0) { > > mt->aux_buf->clear_color_bo = > > - brw_bo_alloc(brw->bufmgr, "clear_color_bo", > > - brw->isl_dev.ss.clear_color_state_size); > > + brw_bo_alloc_tiled(brw->bufmgr, "clear_color_bo", > > + brw->isl_dev.ss.clear_color_state_size, > > + I915_TILING_NONE, 0, BO_ALLOC_ZEROED); > > if (!mt->aux_buf->clear_color_bo) { > > free(mt->aux_buf); > > mt->aux_buf = NULL; > > @@ -1673,9 +1673,9 @@ intel_alloc_aux_buffer(struct brw_context *brw, > > > > buf->size = aux_surf->size; > > > > - const struct gen_device_info *devinfo = &brw->screen->devinfo; > > - if (devinfo->gen >= 10) { > > - /* On CNL, instead of setting the clear color in the SURFACE_STATE, > > we > > + const bool has_indirect_clear = brw->isl_dev.ss.clear_color_state_size > > > 0; > > + if (has_indirect_clear) { > > + /* On CNL+, instead of setting the clear color in the SURFACE_STATE, > > we > > * will set a pointer to a dword somewhere that contains the color. > > So, > > * allocate the space for the clear color value here on the aux > > buffer. > > */ > > @@ -1698,7 +1698,8 @@ intel_alloc_aux_buffer(struct brw_context *brw, > > } > > > > /* Initialize the bo to the desired value */ > > - if (wants_memset) { > > + const bool needs_memset = wants_memset || has_indirect_clear; > > + if (needs_memset) { > > assert(!(alloc_flags & BO_ALLOC_BUSY)); > > > > void *map = brw_bo_map(brw, buf->bo, MAP_WRITE | MAP_RAW); > > @@ -1706,11 +1707,21 @@ intel_alloc_aux_buffer(struct brw_context *brw, > > intel_miptree_aux_buffer_free(buf); > > return NULL; > > } > > - memset(map, memset_value, mt->aux_buf->size); > > + > > + /* Memset the aux_surf portion of the BO. */ > > + if (wants_memset) > > + memset(map, memset_value, aux_surf->size); > > + > > + /* Zero the indirect clear color to match ::fast_clear_color. */ > > + if (has_indirect_clear) { > > + memset((char *)map + buf->clear_color_offset, 0, > > + brw->isl_dev.ss.clear_color_state_size); > > + } > > + > > brw_bo_unmap(buf->bo); > > } > > > > - if (devinfo->gen >= 10) { > > + if (has_indirect_clear) { > > buf->clear_color_bo = buf->bo; > > brw_bo_reference(buf->clear_color_bo); > > } > > @@ -1869,7 +1880,7 @@ intel_miptree_alloc_hiz(struct brw_context *brw, > > isl_surf_get_hiz_surf(&brw->isl_dev, &mt->surf, &temp_hiz_surf); > > assert(ok); > > > > - const uint32_t alloc_flags = BO_ALLOC_BUSY; > > + const uint32_t alloc_flags = 0; > > This change is unrelated (and really should be _BUSY as we don't memset() in > case of hiz), right? >
This change is needed because the HiZ buffer's clear value/color becomes indirect on gen10+. Since we allocate the indirect clear color as part of the auxiliary surface, we will memset this bo. > Otherwise this looks good and given there isn't any major change in the > previous patch, this is: > > Reviewed-by: Topi Pohjolainen <topi.pohjolai...@intel.com> > Thanks! -Nanley > > mt->aux_buf = intel_alloc_aux_buffer(brw, "hiz-miptree", &temp_hiz_surf, > > alloc_flags, false, 0, mt); > > > > -- > > 2.16.2 > > > > _______________________________________________ > > mesa-dev mailing list > > mesa-dev@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev