<div dir="ltr">On 23 May 2013 09:57, Eric Anholt <span dir="ltr"><<a href="mailto:eric@anholt.net" target="_blank">eric@anholt.net</a>></span> wrote:<br>
<div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div>Paul Berry <<a href="mailto:stereotype441@gmail.com" target="_blank">stereotype441@gmail.com</a>> writes:<br>

<br>
> With the advent of fast color clears, it will no longer be safe for<br>
> the driver to access the data stored in a miptree with impunity.  For<br>
> example, sometimes a resolve will need to be performed first (to<br>
> ensure that deferred writes due to a fast clear are performed before<br>
> the buffer is accessed).  Other times, fast clear will need to be<br>
> disabled for the miptree, so that its contents can be safely shared<br>
> with an entity that Mesa can't synchronize with easily.<br>
><br>
> To prepare for that, this patch renames intel_mipmap_tree::region to<br>
> intel_mipmap_tree::region_private and creates an accessor function,<br>
> intel_miptree_get_region().  At the moment, the accessor function<br>
> simply returns region_private.  Later in the patch series, this<br>
> function will be expanded to take appropriate actions to maintain the<br>
> proper fast color clear state.<br>
><br>
> As much as possible, I've tried to restrict the functions which<br>
> directly access region_private to low-level miptree functions<br>
> (e.g. miptree initialization functions), so that it will be easy to<br>
> verify that those functions access the miptree contents safely.<br>
<br>
</div>I don't like this change. I think we should be explicitly resolving at<br>
the right points, like in patch 10.  In this patch, the places I see<br>
that look like they could trigger a resolve from ACCESS_RENDER would all<br>
break the GPU state, so you have to have things resolved before.  This<br>
means that these intel_miptree_get_region() functions just freak me out<br>
when I see them in some code -- "oh crap, would we resolve here?  that<br>
would be bad... oh, looks like we prevent that over in this codepath<br>
over here."<br>
<br>
Once the places that should absolutely never resolve get removed,<br>
there's hardly anything left in this patch.  It also goes against the<br>
work I've done to kill the region struct.<br>
</blockquote></div><br></div><div class="gmail_extra">We (me, Ken, Eric, and Chad) just had an in-person discussion about this, and came up with a new plan:<br><br></div><div class="gmail_extra">1. Eric will send out some patches that funnel all of the blitting operations* through a new intel_miptree_blit() function (whose arguments are miptrees rather than regions)<br>
</div><div class="gmail_extra">2. Once those land, I'll rework my series so that it does the resolve (and other state updating) inside intel_miptree_blit().  Hopefully that will allow us to drop this patch.<br><br></div>
<div class="gmail_extra">*Technically there is one blitting operation that can't go through intel_miptree_blit() (intelEmitImmediateColorExpandBlit(), which is used to accelerate glBitmap()).  We'll put a resolve hook in there as a special case.<br>
</div></div>