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

[Ben Widawsky]

On Fri, May 29, 2015 at 12:07:36PM -0700, Chad Versace wrote:
> 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.
> 

Right. That was my thought too. (Combining comments from Matt + Ken in other
thread) I think !! is a very common idiom these days, but != 0 is definitely
clearer.

> > > > @@ -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,
>     ...,
> };
> 

Okay. I don't mind the change but I /think/ both ways work in GDB. It's less
effort for me to just change it than to check :-)

> 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.
> 

Yeah, here too you got my thoughts exactly, Chad. I initially had just flags,
which is okay, but, I realized most uses of it did have to do with layout - I
think the one big counter example is accelerated upload, which Topi mentioned
above. To me, that actually seems to suggest that we're probably abusing the way
in which we determine accelerated uploads (or at least being lazy about it).
Either way, for the sake of reducing churn, I'll keep layout and gladly allow
anyone to rename it in the future.

(Also, I would have liked to rename 'for_bo' but meh).

> > > 
> > > > +#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>

Thanks guys. I appreciate the feedback.