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

[Iago Toral]

On Mon, 2017-09-11 at 12:15 +0300, Pohjolainen, Topi wrote:
> 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.

So the key things here is that for these we rely ISL_TILING_ANY_MASK
and not ISL_TILING_Y0_BIT being specifically requested... that sounds a
bit finicky since should we ever create a color renderbuffer with Y-
tiling specifically instead of ANY_MASK then we wold skip retiling
strategies and that may not be the correct thing to do.

Seeing that, maybe using the usage fields is a better idea, so instead
of this:

if (tiling_flags & ~(ISL_TILING_Y0_BIT | ISL_TILING_W_BIT)) {

we can do this:

if (mt->surf.usage & (ISL_SURF_USAGE_STENCIL_BIT |   
                      ISL_SURF_USAGE_DEPTH_BIT)) {

And that way we make re-tiling exclusions more explicit and avoid the
issue I mention above.

Would that be a good idea?

Iago