[Mesa-dev] [PATCH 1/6] i965: Consolidate certain miptree params to flags

[Chad Versace]

On Fri 29 May 2015, Pohjolainen, Topi wrote:
> On Fri, May 29, 2015 at 09:32:53AM +0300, Pohjolainen, Topi wrote:
> > On Thu, May 28, 2015 at 10:21:29AM -0700, Ben Widawsky wrote:
> > > I think pretty much everyone agrees that having more than a single bool as a
> > > function argument is bordering on a bad idea. What sucks about the current
> > > code is in several instances it's necessary to propagate these boolean
> > > selections down to lower layers of the code. This requires plumbing (mechanical,
> > > but still churn) pretty much all of the miptree functions each time.  By
> > > introducing the flags paramater, it is possible to add miptree constraints very
> > > easily.
> > 
> > I like this a lot. I have a few simple comments below.

Me too. This is a good cleanup.


> > > diff --git a/src/mesa/drivers/dri/i965/intel_fbo.c b/src/mesa/drivers/dri/i965/intel_fbo.c
> > > index aebed72..1b3a72f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_fbo.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_fbo.c
> > > @@ -390,7 +390,7 @@ intel_image_target_renderbuffer_storage(struct gl_context *ctx,
> > >                                           image->height,
> > >                                           1,
> > >                                           image->pitch,
> > > -                                         true /*disable_aux_buffers*/);
> > > +                                         MIPTREE_LAYOUT_DISABLE_AUX);
> > >     if (!irb->mt)
> > >        return;
> > >  
> > > @@ -1027,10 +1027,9 @@ intel_renderbuffer_move_to_temp(struct brw_context *brw,
> > >                                   intel_image->base.Base.Level,
> > >                                   intel_image->base.Base.Level,
> > >                                   width, height, depth,
> > > -                                 true,
> > >                                   irb->mt->num_samples,
> > >                                   INTEL_MIPTREE_TILING_ANY,
> > > -                                 false);
> > > +                                 MIPTREE_LAYOUT_ACCELERATED_UPLOAD);
> > >  
> > >     if (intel_miptree_wants_hiz_buffer(brw, new_mt)) {
> > >        intel_miptree_alloc_hiz(brw, new_mt);
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 24a5c3d..b243f8a 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -244,10 +244,8 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >                              GLuint width0,
> > >                              GLuint height0,
> > >                              GLuint depth0,
> > > -                            bool for_bo,
> > >                              GLuint num_samples,
> > > -                            bool force_all_slices_at_each_lod,
> > > -                            bool disable_aux_buffers)
> > > +                            uint32_t layout_flags)
> > >  {
> > >     struct intel_mipmap_tree *mt = calloc(sizeof(*mt), 1);
> > >     if (!mt)
> > > @@ -286,7 +284,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >     mt->logical_height0 = height0;
> > >     mt->logical_depth0 = depth0;
> > >     mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> > > -   mt->disable_aux_buffers = disable_aux_buffers;
> > > +   mt->disable_aux_buffers = !!(layout_flags & MIPTREE_LAYOUT_DISABLE_AUX);
> > 
> > You didn't mean to have double negation (!!), did you?
> 
> I actually meant that isn't "layout_flags & MIPTREE_LAYOUT_DISABLE_AUX" enough
> on its own?

I think `layout_flags & MIPTREE_LAYOUT_DISABLE_AUX` is sufficient solely
because mt->disable_aux_buffers has type bool. I prefer to keep Ben's
original `!!`, though`, because it feels more future-proof against
future code changes.

> > > @@ -422,12 +420,14 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >     mt->physical_height0 = height0;
> > >     mt->physical_depth0 = depth0;
> > >  
> > > -   if (!for_bo &&
> > > +   if (!(layout_flags & MIPTREE_LAYOUT_FOR_BO) &&
> > >         _mesa_get_format_base_format(format) == GL_DEPTH_STENCIL &&
> > >         (brw->must_use_separate_stencil ||
> > >  	(brw->has_separate_stencil &&
> > >           intel_miptree_wants_hiz_buffer(brw, mt)))) {
> > > -      const bool force_all_slices_at_each_lod = brw->gen == 6;
> > > +      uint32_t stencil_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD;
> > > +      if (brw->gen == 6)
> > > +         stencil_flags |= MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD;
> > 
> > Perhaps a separating line here, your choice.

+1

> > > @@ -527,6 +527,10 @@ bool
> > >  intel_miptree_alloc_non_msrt_mcs(struct brw_context *brw,
> > >                                   struct intel_mipmap_tree *mt);
> > >  
> > > +#define MIPTREE_LAYOUT_ACCELERATED_UPLOAD       (1<<0)
> > 
> > I was wondering if we could call the the flags something else than layout.
> > The manner of upload doesn't technically have much to do with layout. Maybe
> > just flags, shrug.

I have nitpick too :)

I'd like to see these defined in an anonymous enum block so the token
names show up in gdb.

enum {
    MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD = 1 << 1,
    ...,
};

I agree with Topi that "layout" isn't the best choice of name, but
I also can't think of anything better. So "layout" is good with me.
We can rename it later if it causes confusion.

> > 
> > > +#define MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD   (1<<1)
> > > +#define MIPTREE_LAYOUT_FOR_BO                   (1<<2)
> > > +#define MIPTREE_LAYOUT_DISABLE_AUX              (1<<3)
> > 
> > Maybe a separating new line here as well?

+1

...

This patch, with or without my and Topi's suggestions, is
Reviewed-by: Chad Versace <chad.versace at intel.com>