[Mesa-dev] [PATCH] i965: do not fallback to linear tiling for depth/stencil surfaces

[Pohjolainen, Topi]

On Mon, Sep 11, 2017 at 10:55:36AM +0200, Iago Toral wrote:
> On Fri, 2017-09-08 at 11:44 +0300, Pohjolainen, Topi wrote:
> > On Fri, Sep 08, 2017 at 09:41:11AM +0200, Iago Toral wrote:
> > > On Fri, 2017-09-08 at 00:34 -0700, Kenneth Graunke wrote:
> > > > On Thursday, September 7, 2017 11:44:04 PM PDT Iago Toral Quiroga
> > > > wrote:
> > > > > It is not supported by the hardware and the driver assumes
> > > > > W-tiling for stencil and Y-tiling for depth everywhere anyway.
> > > > > 
> > > > > This fixes a regression in a CTS test introduced with commit
> > > > > 4ea63fab77f0 that started applying re-tiling for these surfaces
> > > > > in certain scenarios.
> > > > > 
> > > > > Fixes:
> > > > > KHR-GL45.direct_state_access.renderbuffers_storage
> > > > > ---
> > > > >  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 8 ++++++--
> > > > >  1 file changed, 6 insertions(+), 2 deletions(-)
> > > > > 
> > > > > diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > index 79afdc5342..cfc1212353 100644
> > > > > --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> > > > > @@ -572,9 +572,13 @@ make_surface(struct brw_context *brw,
> > > > > GLenum
> > > > > target, mesa_format format,
> > > > >  
> > > > >     /* In case caller doesn't specifically request Y-tiling
> > > > > (needed
> > > > >      * unconditionally for depth), check for corner cases
> > > > > needing
> > > > > special
> > > > > -    * treatment.
> > > > > +    * treatment. Stencil and depth surfaces are always W  and
> > > > > Y
> > > > > tiled
> > > > > +    * respectively, so we should not attempt to retile them to
> > > > > different
> > > > > +    * layouts.
> > > > >      */
> > > > > -   if (tiling_flags & ~ISL_TILING_Y0_BIT) {
> > > > 
> > > > Based on Topi's comment, it sounds like depth surfaces should be
> > > > passing
> > > > tiling_flags == ISL_TILING_Y0_BIT, so we shouldn't hit this for
> > > > depth.
> > > > 
> > > > Separate stencil passes ISL_TILING_W_BIT, so I think you're right
> > > > that
> > > > it'll incorrectly hit this case.  Perhaps we need to just change
> > > > it
> > > > to:
> > > > 
> > > >    if (tiling_flags & ~(ISL_TILING_Y0_BIT | ISL_TILING_W_BIT))
> > > > 
> > > > ...?  Though, basing it on usage doesn't seem bad either...
> > > 
> > > Yeah, I noticed that the test for depth usage wasn't actually
> > > required
> > > after sending the patch.
> > > 
> > > Is there anything other than depth/stencil that can use Y/W tiling
> > > and
> > > could be retiled to linear or X? If that is the case then I think
> > > we
> > > want to check against usage, otherwise we could just check against
> > > the
> > > tiling flags.
> > 
> > Color renderbuffers can be X or Y (latter preferred).
> 
> Mmm... the current implementation will attempt to retile things that
> are not Y-tiled, but that restriction seems to come from depth surfaces
> specifically... Topi: are Y-tiled color renderbuffers ever supposed to
> be retiled to linear or X? If they are, then we need to modify the code
> consider the usage flags instead of the tiling flags to decide if want
> to try retiling strategies. What do you think?

In make_surface():

   /* In case caller doesn't specifically request Y-tiling (needed
    * unconditionally for depth), check for corner cases needing special
    * treatment.
    */
   if (tiling_flags & ~ISL_TILING_Y0_BIT) {
      if (need_to_retile_as_linear(brw, mt->surf.row_pitch,
                                   mt->surf.tiling, mt->surf.samples)) {
         init_info.tiling_flags = 1u << ISL_TILING_LINEAR;
         if (!isl_surf_init_s(&brw->isl_dev, &mt->surf, &init_info))
            goto fail;
      } else if (need_to_retile_as_x(brw, mt->surf.size, mt->surf.tiling)) {
         init_info.tiling_flags = 1u << ISL_TILING_X;
         if (!isl_surf_init_s(&brw->isl_dev, &mt->surf, &init_info))
            goto fail;
      }
   }


Color buffers normally are created with ISL_TILING_ANY_MASK. If
isl_surf_init_s() chose Y-tiling than the block above will consider both linear
and X-tiled fallbacks. Same thing if isl_surf_init_s() chose X-tiling, the
block is hit and linear considered.