[Mesa-dev] [PATCH 01/23] i965: Let caller of intel_miptree_create_layout() decide msaa layout

[Ben Widawsky]

On Wed, Feb 10, 2016 at 10:27:46AM +0200, Pohjolainen, Topi wrote:
> On Tue, Feb 09, 2016 at 12:05:52PM -0800, Ben Widawsky wrote:
> > On Mon, Feb 08, 2016 at 06:51:21PM +0200, Topi Pohjolainen wrote:
> > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > ---
> > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 15 +++++++++++----
> > >  1 file changed, 11 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > index 108dd87..0edd59f 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > @@ -64,8 +64,11 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
> > >   */
> > >  static enum intel_msaa_layout
> > >  compute_msaa_layout(struct brw_context *brw, mesa_format format,
> > > -                    bool disable_aux_buffers)
> > > +                    unsigned num_samples, bool disable_aux_buffers)
> > >  {
> > > +   if (num_samples <= 1)
> > > +      return INTEL_MSAA_LAYOUT_NONE;
> > > +
> > >     /* Prior to Gen7, all MSAA surfaces used IMS layout. */
> > >     if (brw->gen < 7)
> > >        return INTEL_MSAA_LAYOUT_IMS;
> > > @@ -299,6 +302,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> > >                              GLuint height0,
> > >                              GLuint depth0,
> > >                              GLuint num_samples,
> > > +                            enum intel_msaa_layout msaa_layout,
> > 
> > Is there a reason why you decided not to roll this into layout flags? It seems
> > to fit into that pretty well IMO.
> 
> If I understood you correctly, you would like to pass
> 
> enum intel_msaa_layout
> {
>    INTEL_MSAA_LAYOUT_NONE,
>    INTEL_MSAA_LAYOUT_IMS,
>    INTEL_MSAA_LAYOUT_UMS,
>    INTEL_MSAA_LAYOUT_CMS,
> };
> 
> along with
> 
> enum {
>    MIPTREE_LAYOUT_ACCELERATED_UPLOAD       = 1 << 0,
>    MIPTREE_LAYOUT_FORCE_ALL_SLICE_AT_LOD   = 1 << 1,
>    MIPTREE_LAYOUT_FOR_BO                   = 1 << 2,
>    MIPTREE_LAYOUT_DISABLE_AUX              = 1 << 3,
>    MIPTREE_LAYOUT_FORCE_HALIGN16           = 1 << 4,
> 
>    MIPTREE_LAYOUT_TILING_Y                 = 1 << 5,
>    MIPTREE_LAYOUT_TILING_NONE              = 1 << 6,
>    MIPTREE_LAYOUT_TILING_ANY               = MIPTREE_LAYOUT_TILING_Y |
>                                              MIPTREE_LAYOUT_TILING_NONE,
> };
> 
> in the same uint32_t. Values are conflicting and therefore one of the
> enumerations would need to re-introduce the other with non-conflicting
> values. Or did I misunderstand?

Well, I was thinking of introducing the minimum subset of whatever weird layout
you need to enforce. My assumption at the time was you're passing this info down
so that the function doing the actual layout could act up the type of MSAA. My
though is to keep the actual "layout" code as agnostic to the type of miptree,
only on the parameters provided via the flags. In that case, I think it makes
sense to create a new layout flag. For example...

MIPTREE_LAYOUT_WACKY_MSAA_THING 	     = 1 << 7
mt->msaa_layout = compute_msaa_layout(...)
if (mt->msaa_layout == whatever)
	layout_flags |= MIPTREE_LAYOUT_WACKY_MSAA_THING;
intel_miptree_create_layout(...)

After getting through about half of the patches, it seems like maybe you are
just saving the caller from mt->msaa = compute_msaa_layout(). If that's the
case, that's fine. However, I'm sort of wondering if this works correcting is
MIPTREE_LAYOUT_DISABLE_AUX is specified. It kind of seems like we could hit the
assert with this patch alone.

I think you'd need to remove the assertion completely
   if (mt->disable_aux_buffers)
         assert(mt->msaa_layout != INTEL_MSAA_LAYOUT_CMS)