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

[Pohjolainen, Topi]

On Mon, Sep 11, 2017 at 11:53:24AM +0200, Iago Toral wrote:
> 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?

Yes, that sounds good. I'm not a big fan of how we first filter tiling in ISL
and again apply these fallbacks later on. It used to be even more complicated
before for we switched to ISL - at least now we have them back-to-back and not
scattered. Problem is that some of the fallbacks are based on dimensions which
we just don't know before try to lay them out.

What you are proposing should make it a little clearer, I think.