[Mesa-dev] [PATCH 03/17] i965/miptree: Move init_mcs into alloc_aux_buffer

[Nanley Chery]

On Mon, May 07, 2018 at 10:10:16AM -0700, Nanley Chery wrote:
> On Mon, May 07, 2018 at 11:51:50AM +0300, Pohjolainen, Topi wrote:
> > On Fri, May 04, 2018 at 11:04:40AM -0700, Nanley Chery wrote:
> > > On Fri, May 04, 2018 at 10:00:32AM -0700, Nanley Chery wrote:
> > > > On Fri, May 04, 2018 at 09:42:34AM -0700, Nanley Chery wrote:
> > > > > On Thu, May 03, 2018 at 12:03:50PM -0700, Nanley Chery wrote:
> > > > > > Add infrastructure for initializing the clear color BO.
> > > > > > ---
> > > > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 68 ++++++++++++---------------
> > > > > >  1 file changed, 31 insertions(+), 37 deletions(-)
> > > > > > 
> > > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > index 182a896e23a..5d3ee569bd8 100644
> > > > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > > @@ -1658,41 +1658,13 @@ intel_miptree_copy_teximage(struct brw_context *brw,
> > > > > >     intel_obj->needs_validate = true;
> > > > > >  }
> > > > > >  
> > > > > > -static bool
> > > > > > -intel_miptree_init_mcs(struct brw_context *brw,
> > > > > > -                       struct intel_mipmap_tree *mt,
> > > > > > -                       int init_value)
> > > > > > -{
> > > > > > -   assert(mt->aux_buf != NULL);
> > > > > > -
> > > > > > -   /* From the Ivy Bridge PRM, Vol 2 Part 1 p326:
> > > > > > -    *
> > > > > > -    *     When MCS buffer is enabled and bound to MSRT, it is required that it
> > > > > > -    *     is cleared prior to any rendering.
> > > > > > -    *
> > > > > > -    * Since we don't use the MCS buffer for any purpose other than rendering,
> > > > > > -    * it makes sense to just clear it immediately upon allocation.
> > > > > > -    *
> > > > > > -    * Note: the clear value for MCS buffers is all 1's, so we memset to 0xff.
> > > > > > -    */
> > > > > > -   void *map = brw_bo_map(brw, mt->aux_buf->bo, MAP_WRITE | MAP_RAW);
> > > > > > -   if (unlikely(map == NULL)) {
> > > > > > -      fprintf(stderr, "Failed to map mcs buffer into GTT\n");
> > > > > > -      intel_miptree_aux_buffer_free(mt->aux_buf);
> > > > > > -      mt->aux_buf = NULL;
> > > > > > -      return false;
> > > > > > -   }
> > > > > > -   void *data = map;
> > > > > > -   memset(data, init_value, mt->aux_buf->size);
> > > > > > -   brw_bo_unmap(mt->aux_buf->bo);
> > > > > > -   return true;
> > > > > > -}
> > > > > > -
> > > > > >  static struct intel_miptree_aux_buffer *
> > > > > >  intel_alloc_aux_buffer(struct brw_context *brw,
> > > > > >                         const char *name,
> > > > > >                         const struct isl_surf *aux_surf,
> > > > > >                         uint32_t alloc_flags,
> > > > > > +                       bool wants_memset,
> > > > > > +                       uint8_t memset_value,
> > > > > >                         struct intel_mipmap_tree *mt)
> > > > > >  {
> > > > > >     struct intel_miptree_aux_buffer *buf = calloc(sizeof(*buf), 1);
> > > > > > @@ -1725,6 +1697,19 @@ intel_alloc_aux_buffer(struct brw_context *brw,
> > > > > >        return NULL;
> > > > > >     }
> > > > > >  
> > > > > > +   /* Initialize the bo to the desired value */
> > > > > > +   if (wants_memset) {
> > > > > > +      assert(!(alloc_flags & BO_ALLOC_BUSY));
> > > > > > +
> > > > > > +      void *map = brw_bo_map(brw, buf->bo, MAP_WRITE | MAP_RAW);
> > > > > > +      if (map == NULL) {
> > > > > > +         intel_miptree_aux_buffer_free(buf);
> > > > > > +         return NULL;
> > > > > > +      }
> > > > > > +      memset(map, memset_value, mt->aux_buf->size);
> > > > > 
> > > > > Found a bug here. The last argument should be buf->size because
> > > > > mt->aux_buf hasn't been assigned yet. Will fix locally.
> > > > > 
> > > > > -Nanley
> > > > > 
> > > > 
> > > > False alarm. Sorry for the noise.
> > 
> > You are passing "aux_surf" as one of the arguments. Could you just use
> > "aux_surf->size"? That gives the value for "buf->size" before "buf->size"
> > gets augmented by the indirect clear color.
> > 
> 
> Sure. The only extra change I'll have to make is to rephrase this commit
> message to explain that this patch adds a memset capability to
> intel_alloc_aux_buffer and uses that instead of init_mcs. It'll no
> longer be a refactoring patch that inlines a broken
> intel_miptree_init_mcs function.
> 

I've fixed the commit locally and changed the commit message to:

    i965/miptree: Add and use a memset option in alloc_aux_buffer
    
    Add infrastructure for initializing the clear color BO.
    intel_miptree_init_mcs is no longer needed with change.
    
    v2: Only memset aux_surf->size (Topi Pohjolainen).

Let me know if you'd like me to send the v2 to the list.

-Nanley

> -Nanley
> 
> 
> > > > 
> > > > -Nanley
> > > > 
> > > 
> > > ... I was looking at the wrong code. I'll fix this.
> > > _______________________________________________
> > > mesa-dev mailing list
> > > mesa-dev at lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/mesa-dev