[Mesa-dev] [Mesa-stable] [PATCH] i965: Fix buffer overruns in MSAA MCS buffer clearing.

[Ian Romanick]

On 04/14/2014 05:33 PM, Eric Anholt wrote:
> This manifested as rendering failures or sometimes GPU hangs in
> compositors when they accidentally got MSAA visuals due to a bug in the X
> Server.  Today we decided that the problem in compositors was equivalent
> to a corruption bug we'd noticed recently in resizing MSAA-visual
> glxgears, and debugging got a lot easier.
> 
> When we allocate our MCS MT, libdrm takes the size we request, aligns it
> to Y tile size (blowing it up from 300x300=900000 bytes to 384*320=122880
                                              ^^^^^
Infinitesimal nit...       one too many zeros here.

> bytes, 30 pages), then puts it into a power-of-two-sized BO (131072 bytes,
> 32 pages).  Because it's Y tiled, we attach a 384-byte-stride fence to it.
> When we memset by the BO size in Mesa, between bytes 122880 and 131072 the
> data gets stored to the first 20 or so scanlines of each of the 3 tiled
> pages in that row, even though only 2 of those pages were allocated by
> libdrm.  In the glxgears case, the missing 3rd page happened to
> consistently be the static VBO that got mapped right after the first MCS
> allocation, so corruption only appeared once window resize made us throw
> out the old MCS and then allocate the same BO to back the new MCS.

Oh man...  I didn't realize that bo->size wasn't necessarily the amount
of backing storage.  It makes sense... do we track the "backed size"
anywhere?  It seems possible to have similar problems elsewhere...

> Instead, just memset the amount of data we actually asked libdrm to
> allocate for, which will be smaller (more efficient) and not overrun.
> Thanks go to Kenneth for doing most of the hard debugging to eliminate a
> lot of the search space for the bug.
> 
> Cc: "10.0 10.1" <mesa-stable at lists.freedesktop.org>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77207
> ---
> 
> Ken: how I eventually figured this out was I thought "well, I'm
> clearing the whole surface at the start of rendering glxgears, so I
> don't *really* need to memset because I'll be initializing the whole
> buffer during fallback glClear() anyway, so let me just drop the whole
> memset step to definitely eliminate that as a potential problem.  Huh,
> the problem's gone."
> 
> The worst is I remember briefly thinking about this code when it was
> landing, and thought "nope, seems safe."
> 
>  src/mesa/drivers/dri/i965/intel_mipmap_tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> index 5996a1b..59700ed 100644
> --- a/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> +++ b/src/mesa/drivers/dri/i965/intel_mipmap_tree.c
> @@ -1219,7 +1219,7 @@ intel_miptree_alloc_mcs(struct brw_context *brw,
>      * Note: the clear value for MCS buffers is all 1's, so we memset to 0xff.
>      */
>     void *data = intel_miptree_map_raw(brw, mt->mcs_mt);
> -   memset(data, 0xff, mt->mcs_mt->region->bo->size);
> +   memset(data, 0xff, mt->mcs_mt->region->height * mt->mcs_mt->region->pitch);
>     intel_miptree_unmap_raw(brw, mt->mcs_mt);
>     mt->fast_clear_state = INTEL_FAST_CLEAR_STATE_CLEAR;
>  
>