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

[Jason Ekstrand]

On Mon, Sep 11, 2017 at 2:15 AM, Pohjolainen, Topi <
topi.pohjolainen at gmail.com> 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.


I think the fundamental problem here is that these decisions really need to
be made in ISL in the long term.  If we're going to support
GL_EXT_external_objects then we can't have any per-driver heuristics
because they'll break image compatibility between the two.  It's going to
depend on the need_to_retile_* condition whether it needs to be pulled into
ISL or removed entirely.  For instance, the "too big to blit" should
probably be solved by using blorp_copy instead of the blitter in those
cases rather than falling back to linear.

Sorry for not chipping in earlier. :-(  I think v2 of Iago's patch is fine
for now but we need to start moving in the direction where basically all
i965 ever does is call into ISL and ISL makes the decisions.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20170911/14597836/attachment-0001.html>