[Mesa-dev] [PATCH] intel/isl: Align clear color buffer to full cacheline

[Rafael Antognolli]

On Wed, Apr 17, 2019 at 09:04:09AM -0700, Kenneth Graunke wrote:
> On Wednesday, April 17, 2019 7:16:28 AM PDT Topi Pohjolainen wrote:
> > From: Rafael Antognolli <rafael.antognolli at intel.com>
> > 
> > Fixes MCS fast clear gpu hangs with Vulkan CTS on ICL in CI.
> > 
> > CC: Anuj Phogat <anuj.phogat at gmail.com>
> > CC: Kenneth Graunke <kenneth at whitecape.org>
> > Tested-by: Topi Pohjolainen <topi.pohjolainen at intel.com>
> > Signed-off-by: Rafael Antognolli <rafael.antognolli at intel.com>
> > ---
> >  src/intel/isl/isl.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c
> > index 6b9e6c9e0f0..acfed5119ba 100644
> > --- a/src/intel/isl/isl.c
> > +++ b/src/intel/isl/isl.c
> > @@ -122,7 +122,8 @@ isl_device_init(struct isl_device *dev,
> >     dev->ss.size = RENDER_SURFACE_STATE_length(info) * 4;
> >     dev->ss.align = isl_align(dev->ss.size, 32);
> >  
> > -   dev->ss.clear_color_state_size = CLEAR_COLOR_length(info) * 4;
> > +   dev->ss.clear_color_state_size =
> > +      isl_align(CLEAR_COLOR_length(info) * 4, 64);
> >     dev->ss.clear_color_state_offset =
> >        RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 * 4;
> >  
> > 
> 
> I'm not as familiar with Vulkan, but it looks like we're storing this
> clear color data as part of the underlying image's BO, rather than as
> a separate piece of data.  I wonder if it has anything to do with that
> BO being considered tiled, so something is trying to access an entire
> cacheline around here.  Or it's offsetting following data to not be
> cacheline aligned...

Hmmm... Yeah, we store it after the aux buffer, in the same BO as the
image one.

What I think it's the biggest issue in Vulkan is that we store some
data (resolve type and tracking) right after the clear color data. And
the data size is 32B, but the docs say it should be the lower 32B of a
cacheline. For some reason I thought it was safe to write stuff into the
higher 32B, but apparently it wasn't :-/

> I did notice that the clear address has to be 64B aligned.

My understanding is that the image and aux surface were always 4K
aligned, so this restriction would be met. I guess making an assert for
it wouldn't hurt, though...