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

[Iago Toral]

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?

Iago