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.vers...@intel.com>
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/mesa-dev
