[Mesa-dev] [PATCH] etnaviv: bugfix: Don't do resolve-in-place without valid TS

[Lucas Stach]

Hi Andres,

Am Freitag, den 03.11.2017, 14:39 +0200 schrieb Andres Gomez:
> On Wed, 2017-11-01 at 12:47 +0100, Lucas Stach wrote:
> > Am Mittwoch, den 01.11.2017, 12:34 +0100 schrieb Wladimir J. van
> > der Laan:
> > > On Wed, Nov 01, 2017 at 11:52:55AM +0100, Lucas Stach wrote:
> > > > Am Mittwoch, den 01.11.2017, 11:17 +0100 schrieb Wladimir J.
> > > > van der Laan:
> > > > > GC3000 resolve-in-place assumes that the TS state is
> > > > > configured.
> > > > > If it is not, this will result in MMU errors. This is
> > > > > especially
> > > > > apparent when using glGenMipmaps().
> > > > > 
> > > > > Fixes a problem introduced in
> > > > > 78ade659569ee6fe9bd244170956139f19dd8c6c.
> > > > > 
> > > > > > Signed-off-by: Wladimir J. van der Laan <laanwj at gmail.com>
> > > > > 
> > > > > ---
> > > > >  src/gallium/drivers/etnaviv/etnaviv_clear_blit.c | 4 ++++
> > > > >  src/gallium/drivers/etnaviv/etnaviv_emit.c       | 4 ++++
> > > > >  src/gallium/drivers/etnaviv/etnaviv_rs.c         | 1 +
> > > > >  src/gallium/drivers/etnaviv/etnaviv_rs.h         | 2 ++
> > > > >  4 files changed, 11 insertions(+)
> > > > > 
> > > > > Ooops. This seems like an obvious oversight but I hadn't
> > > > > thought we would get
> > > > > into this path at all when there is no TS to "flush".
> > > > 
> > > > And that's probably what we should fix. The self-resolve cases
> > > > on
> > > > resource flush and sampler update don't check the TS status,
> > > > but they
> > > > are only useful if there is a valid TS.
> > > > 
> > > > With the change you did here we are still wasting bandwidth for
> > > > a no-op 
> > > > blit on older cores like GC880 when generating mipmaps.
> > > 
> > > Yes, just a bugfix (my original commit did not introduce the
> > > higher-level
> > > behavior). This particular case should not result in a MMU error.
> > > If we fix the
> > > higher level, then this could be replaced with an assertion
> > > instead.
> > > 
> > > On the longer run I'd personally prefer to make "Flush resource
> > > level TS" a
> > > separate, explicit operation, for example a method on the
> > > context, and not make
> > > it go through the blit path with source==destination. It's a
> > > hardware operation
> > > implemented differently on GC3000 and GC7000, that just happens
> > > to use the RS
> > > blit on <GC3000.
> > 
> > Agreed. I'll make sure this commit lands in 17.3 and will work on a
> > follow up change to fix the high-level behavior.
> 
> Lucas, this landed with the "mesa-stable at lists.freedesktop.org"
> without
> specifying the branch.
> 
> For 17.2, the fix claimed and the commit 78ade659569ee6fe9bd2 are yet
> not present. However, the patch can still be applied solving a
> trivial
> conflict.
> 
> Do you want this patch in 17.2 or the nomination was only intended
> for
> 17.3 ?

It's only relevant for the 17.3 branch. There is no point in
backporting to 17.2.

Regards,
Lucas