[Mesa-dev] [04.5/11] i965: Add flag telling if miptree is for client consumption

[Pohjolainen, Topi]

On Mon, May 09, 2016 at 10:26:59AM -0700, Ben Widawsky wrote:
> On Fri, May 06, 2016 at 09:08:41PM +0300, Pohjolainen, Topi wrote:
> > On Thu, May 05, 2016 at 07:08:15PM -0700, Ben Widawsky wrote:
> > > On Mon, Apr 25, 2016 at 08:10:01PM +0300, Topi Pohjolainen wrote:
> > > > Signed-off-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > > > ---
> > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 7 ++++---
> > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h | 9 +++++++++
> > > >  2 files changed, 13 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > index b11fafd..cdf2fbd 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > @@ -394,6 +394,7 @@ intel_miptree_create_layout(struct brw_context *brw,
> > > >     mt->logical_depth0 = depth0;
> > > >     mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_NO_MCS;
> > > >     mt->disable_aux_buffers = (layout_flags & MIPTREE_LAYOUT_DISABLE_AUX) != 0;
> > > > +   mt->is_scanout = (layout_flags & MIPTREE_LAYOUT_FOR_SCANOUT) != 0;
> > > >     exec_list_make_empty(&mt->hiz_map);
> > > >     mt->cpp = _mesa_get_format_bytes(format);
> > > >     mt->num_samples = num_samples;
> > > > @@ -900,7 +901,7 @@ intel_update_winsys_renderbuffer_miptree(struct brw_context *intel,
> > > >                                                   height,
> > > >                                                   1,
> > > >                                                   pitch,
> > > > -                                                 0);
> > > > +                                                 MIPTREE_LAYOUT_FOR_SCANOUT);
> > > >     if (!singlesample_mt)
> > > >        goto fail;
> > > >  
> > > > @@ -959,8 +960,8 @@ intel_miptree_create_for_renderbuffer(struct brw_context *brw,
> > > >     bool ok;
> > > >     GLenum target = num_samples > 1 ? GL_TEXTURE_2D_MULTISAMPLE : GL_TEXTURE_2D;
> > > >     const uint32_t layout_flags = MIPTREE_LAYOUT_ACCELERATED_UPLOAD |
> > > > -                                 MIPTREE_LAYOUT_TILING_ANY;
> > > > -
> > > > +                                 MIPTREE_LAYOUT_TILING_ANY |
> > > > +                                 MIPTREE_LAYOUT_FOR_SCANOUT;
> > > >  
> > > >     mt = intel_miptree_create(brw, target, format, 0, 0,
> > > >                               width, height, depth, num_samples,
> > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > index d4f8563..bb06522 100644
> > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > > @@ -657,6 +657,13 @@ struct intel_mipmap_tree
> > > >      */
> > > >     bool disable_aux_buffers;
> > > >  
> > > > +   /**
> > > > +    * Tells if the underlying buffer is to be also consumed by entities other
> > > > +    * than the driver. This allows logic to turn off features such as lossless
> > > > +    * compression which is not currently understood by client applications.
> > > > +    */
> > > > +   bool is_scanout;
> > > > +
> > > >     /* These are also refcounted:
> > > >      */
> > > >     GLuint refcount;
> > > > @@ -697,6 +704,8 @@ enum {
> > > >     MIPTREE_LAYOUT_TILING_NONE              = 1 << 6,
> > > >     MIPTREE_LAYOUT_TILING_ANY               = MIPTREE_LAYOUT_TILING_Y |
> > > >                                               MIPTREE_LAYOUT_TILING_NONE,
> > > > +
> > > > +   MIPTREE_LAYOUT_FOR_SCANOUT              = 1 << 7,
> > > >  };
> > > >  
> > > >  struct intel_mipmap_tree *intel_miptree_create(struct brw_context *brw,
> > > 
> > > Since we hope that we one day can use these buffers for scanout, I'd go for
> > > something more generic, like (requires other changes as well):
> > > 
> > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > index 21e4718..deea30d 100644
> > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.h
> > > @@ -687,11 +687,14 @@ 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_DISABLE_AUX_MCS /* CCS_D */ = 1 << 3,
> > > +   MIPTREE_LAYOUT_DISABLE_AUX_CCS_E        = 1 << 4,
> > > +   MIPTREE_LAYOUT_DISABLE_AUX              = MIPTREE_LAYOUT_DISABLE_AUX_MCS |
> > > +                                             MIPTREE_LAYOUT_DISABLE_AUX_CCS_E,
> > > +   MIPTREE_LAYOUT_FORCE_HALIGN16           = 1 << 5,
> > > +
> > > +   MIPTREE_LAYOUT_TILING_Y                 = 1 << 6,
> > > +   MIPTREE_LAYOUT_TILING_NONE              = 1 << 7,
> > >     MIPTREE_LAYOUT_TILING_ANY               = MIPTREE_LAYOUT_TILING_Y |
> > >                                               MIPTREE_LAYOUT_TILING_NONE,
> > >  };
> > > 
> > > 
> > > I do like mt->is_scanout though. That seems useful. Either way:
> > > Reviewed-by: Ben Widawsky <ben at bwidawsk.net>
> > 
> > I'm wondering what you had in mind in addition to the _LAYOUT_DISABLE_ flags?
> > These are not available anymore when intel_miptree_prepare_mcs() (next patch,
> > the deferred allocation of aux buffer) gets called. We would need something
> > in "struct intel_mipmap_tree", perhaps "bool disable_something"?
> 
> My only point was that I thought it might be useful to be able to selective
> disable MCS/CCS_D, vs. CCS_E, vs. both. If you don't think so, then you can
> ignore my comment, and we can always fix it later.

No, I think it might be useful. I'm just checking that you are ok with adding
equivalent flags to miptree. Cause that what it pretty much comes down to.