<div dir="ltr"><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Apr 17, 2019 at 3:31 PM Nanley Chery <<a href="mailto:nanleychery@gmail.com">nanleychery@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">On Wed, Apr 17, 2019 at 11:34:15AM -0700, Rafael Antognolli wrote:<br>
> On Wed, Apr 17, 2019 at 09:04:09AM -0700, Kenneth Graunke wrote:<br>
> > On Wednesday, April 17, 2019 7:16:28 AM PDT Topi Pohjolainen wrote:<br>
> > > From: Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>><br>
> > > <br>
> > > Fixes MCS fast clear gpu hangs with Vulkan CTS on ICL in CI.<br>
> > > <br>
> > > CC: Anuj Phogat <<a href="mailto:anuj.phogat@gmail.com" target="_blank">anuj.phogat@gmail.com</a>><br>
> > > CC: Kenneth Graunke <<a href="mailto:kenneth@whitecape.org" target="_blank">kenneth@whitecape.org</a>><br>
> > > Tested-by: Topi Pohjolainen <<a href="mailto:topi.pohjolainen@intel.com" target="_blank">topi.pohjolainen@intel.com</a>><br>
> > > Signed-off-by: Rafael Antognolli <<a href="mailto:rafael.antognolli@intel.com" target="_blank">rafael.antognolli@intel.com</a>><br>
> > > ---<br>
> > >  src/intel/isl/isl.c | 3 ++-<br>
> > >  1 file changed, 2 insertions(+), 1 deletion(-)<br>
> > > <br>
> > > diff --git a/src/intel/isl/isl.c b/src/intel/isl/isl.c<br>
> > > index 6b9e6c9e0f0..acfed5119ba 100644<br>
> > > --- a/src/intel/isl/isl.c<br>
> > > +++ b/src/intel/isl/isl.c<br>
> > > @@ -122,7 +122,8 @@ isl_device_init(struct isl_device *dev,<br>
> > >     dev->ss.size = RENDER_SURFACE_STATE_length(info) * 4;<br>
> > >     dev->ss.align = isl_align(dev->ss.size, 32);<br>
> > >  <br>
> > > -   dev->ss.clear_color_state_size = CLEAR_COLOR_length(info) * 4;<br>
> > > +   dev->ss.clear_color_state_size =<br>
> > > +      isl_align(CLEAR_COLOR_length(info) * 4, 64);<br>
> > >     dev->ss.clear_color_state_offset =<br>
> > >        RENDER_SURFACE_STATE_ClearValueAddress_start(info) / 32 * 4;<br>
> > >  <br>
> > > <br>
> > <br>
> > I'm not as familiar with Vulkan, but it looks like we're storing this<br>
> > clear color data as part of the underlying image's BO, rather than as<br>
> > a separate piece of data.  I wonder if it has anything to do with that<br>
> > BO being considered tiled, so something is trying to access an entire<br>
> > cacheline around here.  Or it's offsetting following data to not be<br>
> > cacheline aligned...<br>
> <br>
> Hmmm... Yeah, we store it after the aux buffer, in the same BO as the<br>
> image one.<br>
> <br>
> What I think it's the biggest issue in Vulkan is that we store some<br>
> data (resolve type and tracking) right after the clear color data. And<br>
> the data size is 32B, but the docs say it should be the lower 32B of a<br>
> cacheline. For some reason I thought it was safe to write stuff into the<br>
> higher 32B, but apparently it wasn't :-/<br>
> <br>
> > I did notice that the clear address has to be 64B aligned.<br>
> <br>
> My understanding is that the image and aux surface were always 4K<br>
> aligned, so this restriction would be met. I guess making an assert for<br>
> it wouldn't hurt, though...<br>
<br>
This fix looks good to me. I'm a little confused about the title. I see<br>
how this change pads the buffer, but I don't see how it aligns it.<br></blockquote><div><br></div><div>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:</div><div><br></div><div>      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.</div><div><br></div><div>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. :-/</div><div><br></div><div>--Jason<br></div></div></div>