[Mesa-dev] [PATCH 04/17] i965/miptree: Initialize the indirect clear color to zero

[Nanley Chery]

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.pohjolainen at 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 at lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/mesa-dev