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

[Jason Ekstrand]

On Wed, Apr 17, 2019 at 3:31 PM Nanley Chery <nanleychery at gmail.com> wrote:

> On Wed, Apr 17, 2019 at 11:34:15AM -0700, Rafael Antognolli wrote:
> > 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...
>
> This fix looks good to me. I'm a little confused about the title. I see
> how this change pads the buffer, but I don't see how it aligns it.
>

I agree that this doesn't really "align" anything.  We get the alignment
because the size of each surface is a multiple of 4K.  Here's an
interesting bspec quote:

      The memory layout of the clear color pointed to by this address is a
value stored in the lower-order bytes of a 64-byte cache-line.

The fact that it refers to a cache line is important.  It's possible (but I
really don't know) something weird is happening with MI writes and caching.
:-/

--Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20190418/ebe1d2bb/attachment-0001.html>