[Mesa-dev] [PATCH] i965: Always use Y-tiled buffers on SKL+

[Pohjolainen, Topi]

On Thu, Apr 21, 2016 at 08:03:02AM +0300, Pohjolainen, Topi wrote:
> On Wed, Apr 20, 2016 at 09:49:02PM -0700, Ben Widawsky wrote:
> > From: Ben Widawsky <benjamin.widawsky at intel.com>
> > 
> > Starting with Skylake, the display engine is capable of scanning out from
> > Y-tiled buffers. As such, we can and should use Y-tiling for better efficiency.
> > This also has the added benefit of being able to fast clear the winsys buffer.
> > 
> > Note that the buffer allocation done for mipmaps will already never allocate an
> > X-tiled buffer for GEN9.
> > 
> > Here is the statistically significant difference (percentage) with this patch. I
> > do not know why we did see these kind of results earlier. This is on a SKL gt3e
> > with a resolution of 3200x1800.
> 
> Yes, please. A few comments but:
> 
> Reviewed-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> 
> > 
> > Benchmark          a-master->b-yscanout
> > OglBatch0          8.56
> > OglBatch1          6.85
> > OglBatch2          5.0
> > OglBatch3          4.48
> > OglBatch4          4.16
> > OglBatch6          3.57
> > OglBatch7          2.96
> > OglFillPixel       1.91
> > OglGeomPoint       4.43
> > OglGeomTriList     2.83
> > OglPSBump8         0.32
> > OglShMapVsm        6.61
> > OglTerrainFlyInst  5.2
> > OglVSTangent       3.03
> > OglZBuffer         8.88
> > egypt              14.54
> > manhattan          1.59
> > manhattanoff       -0.25
> > triangle           15.35
> > warsow             2.26
> > xonotic            7.9
> > 
> > Signed-off-by: Ben Widawsky <ben at bwidawsk.net>
> > ---
> >  src/mesa/drivers/dri/i965/brw_meta_fast_clear.c |  4 ++--
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c   |  9 +++++++--
> >  src/mesa/drivers/dri/i965/intel_mipmap_tree.h   |  3 ++-
> >  src/mesa/drivers/dri/i965/intel_screen.c        | 21 ++++++++++++++++++---
> >  4 files changed, 29 insertions(+), 8 deletions(-)
> > 
> > diff --git a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > index 1fb5dc8..cf634a2 100644
> > --- a/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > +++ b/src/mesa/drivers/dri/i965/brw_meta_fast_clear.c
> > @@ -240,7 +240,7 @@ get_fast_clear_rect(struct brw_context *brw, struct gl_framebuffer *fb,
> >         * alignment size returned by intel_get_non_msrt_mcs_alignment(), but
> >         * with X alignment multiplied by 16 and Y alignment multiplied by 32.
> >         */
> > -      intel_get_non_msrt_mcs_alignment(irb->mt, &x_align, &y_align);
> > +      intel_get_non_msrt_mcs_alignment(brw, irb->mt, &x_align, &y_align);
> >        x_align *= 16;
> >  
> >        /* SKL+ line alignment requirement for Y-tiled are half those of the prior
> > @@ -821,7 +821,7 @@ get_resolve_rect(struct brw_context *brw,
> >      * by a factor of 2.
> >      */
> >  
> > -   intel_get_non_msrt_mcs_alignment(mt, &x_align, &y_align);
> > +   intel_get_non_msrt_mcs_alignment(brw, mt, &x_align, &y_align);
> >     if (brw->gen >= 9) {
> >        x_scaledown = x_align * 8;
> >        y_scaledown = y_align * 8;
> > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > index d263ff8..a793370 100644
> > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > @@ -144,7 +144,8 @@ compute_msaa_layout(struct brw_context *brw, mesa_format format,
> >   *   by half the block width, and Y coordinates by half the block height.
> >   */
> >  void
> > -intel_get_non_msrt_mcs_alignment(struct intel_mipmap_tree *mt,
> > +intel_get_non_msrt_mcs_alignment(const struct brw_context *brw,
> > +                                 struct intel_mipmap_tree *mt,
> 
> I think you could mark mt as const while you are at it if you like.
> 
> >                                   unsigned *width_px, unsigned *height)
> >  {
> >     switch (mt->tiling) {
> > @@ -156,6 +157,10 @@ intel_get_non_msrt_mcs_alignment(struct intel_mipmap_tree *mt,
> >        *height = 4;
> >        break;
> >     case I915_TILING_X:
> > +      /* The docs are somewhat confusing with the way the tables are displayed.
> > +       * However, it does clearly state: "MCS and Lossless compression is
> > +       * supported for TiledY/TileYs/TileYf non-MSRTs only." */
> 
> Comment closing on its own line. I'm also a little confused if this comment is
> really related to this patch.

It does belong here, my mistake, sorry.