[Mesa-dev] [PATCH 5/5] intel: Remove intel_renderbuffer::hiz_region

[Chad Versace]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 10/19/2011 11:12 AM, Chad Versace wrote:
> On 10/18/2011 03:57 PM, Eric Anholt wrote:
>> On Mon, 17 Oct 2011 07:40:28 -0700, Chad Versace <chad at chad-versace.us> wrote:
>>> Replace it with intel_renderbuffer::region::hiz::region.
>>>
>>> Signed-off-by: Chad Versace <chad at chad-versace.us>
>>> ---
>>>  src/mesa/drivers/dri/i965/brw_misc_state.c     |   27 +++++++++--------
>>>  src/mesa/drivers/dri/intel/intel_context.c     |   10 ++++--
>>>  src/mesa/drivers/dri/intel/intel_fbo.c         |   36 +++++------------------
>>>  src/mesa/drivers/dri/intel/intel_fbo.h         |    3 --
>>>  src/mesa/drivers/dri/intel/intel_mipmap_tree.c |    1 -
>>>  src/mesa/drivers/dri/intel/intel_mipmap_tree.h |   14 ---------
>>>  6 files changed, 29 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/src/mesa/drivers/dri/i965/brw_misc_state.c b/src/mesa/drivers/dri/i965/brw_misc_state.c
>>> index 2e6780b..6b521e0 100644
>>> --- a/src/mesa/drivers/dri/i965/brw_misc_state.c
>>> +++ b/src/mesa/drivers/dri/i965/brw_misc_state.c
> 
>>> @@ -583,21 +574,10 @@ intel_update_tex_wrapper_regions(struct intel_context *intel,
>>>  
>>>     /* Allocate the texture's hiz region if necessary. */
>>>     if (intel->vtbl.is_hiz_depth_format(intel, rb->Format)
>>> -       && !intel_image->mt->hiz_region) {
>>> -      intel_image->mt->hiz_region =
>>> -         intel_region_alloc(intel->intelScreen,
>>> -                            I915_TILING_Y,
>>> -                            _mesa_get_format_bytes(rb->Format),
>>> -                            rb->Width,
>>> -                            rb->Height,
>>> -                            GL_TRUE);
>>> -      if (!intel_image->mt->hiz_region)
>>> -         return GL_FALSE;
>>> -   }
>>> -
>>> -   /* Point the renderbuffer's hiz region to the texture's hiz region. */
>>> -   if (irb->hiz_region != intel_image->mt->hiz_region) {
>>> -      intel_region_reference(&irb->hiz_region, intel_image->mt->hiz_region);
>>> +       && irb->region->hiz.region == NULL) {
>>> +      bool ok = intel_renderbuffer_alloc_hiz(intel, irb);
>>> +      if (!ok)
>>> +	 return false;
>>>     }
> 
>> I think you're aware of this already, but just in case:
> 
>> The hiz region allocated here using the new rb_alloc_hiz looks like it
>> will be based on the size of the whole texture miptree (since it looks
>> up the region's size, not rb w/h), while all that should be needed is
>> the maximum size of any texture image.
> 
> I was aware of that, and chose to do that because it was compatible with my
> vague plan of handling miptrees. But...
> 
> 
>> I think we could just use Width/Height, if we resolved at
>> FinishRenderTexture time and freed the renderbuffer's hiz region (which
>> I think would be reasonable for now, and should make mipmapped depth hiz
>> pretty straightforward).
> 
> I like that approach. It's much simpler than my original plan, and it shouldn't cause
> unnecessary resolves for apps that are written correctly. After rendering to a depth texture,
> the app, if sensible, will eventually sample from the texture. The depth resolve must happen
> eventually, so we might as well straightforwardly do it when detaching the texture.
> 
> At first, I though this plan would cause problems when multiple images/layers/levels of the same texture
> object were attached to an FBO. But I no longer see a problem there. An FBO has only one
> depth attachment point, so each image would be attached to a different FBO, which ensures
> that FinishRenderTexture gets called each time we render to a new image/layer/level.

Sad times. This simple approach you suggested won't work. Depending on the situation, it
causes infinite recursion or mutex deadlock.

The infinite recursion occurs here:
  intel_finish_render_texture()
    gen6_hiz_resolve_depth_buffer()
      glBindFramebuffer(hiz->fbo) // Bind the FBO used for the meta-op.
        intel_finish_render_texture() // Try resolving the same texture again.
          ...
       
Of course, I experimented by adding a new context bit that prevented recursion
into intel_finish_render_texture if a HiZ meta-op was in progress. But that
introduces a new problem: deadlock on the FBO's mutex. Mesa really doesn't
want the FBO altered under its feet (by a meta-op) while Mesa itself is
of altering the FBO (in FinishRenderTexture).

I don't see a workaround.

I'm now returning to my original plan. Create a HiZ mipmap, maintain a need_resolve
bit for each mipmap layer, and postpone the texture resolves until intel_region_map()
or brw_predraw_resolve_buffers().

- -- 
Chad Versace
chad at chad-versace.us
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.11 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iQIcBAEBAgAGBQJOoboyAAoJEAIvNt057x8iEjMP/296NFy9Ba5a9L/v7fK7jgOs
DnbFOEq5gAZeBLV+mdxwuB2wH6M/2v7uCt3pwXSzVPJmVo9OQd8vba1IVXDciN4V
99uEJaFVGV9At3KjCAP93WaMRvTa1WNQZFCcnZQkFn/x4ERJsatpDjzQ+mcEv0hz
XHJBMFhrUoM9YbwqoiNWsiWWxVV9UbTVOKm5wSRBB+/uQZ+fDLlQ+FPCQgS4vmpc
jvQxMzLO6g5ybHWFEQ40ywPNnoX6kGMYf6e0U6UCWNyvH4HkYEBVAwsQfqEXoU1P
Y/CgrrO9gR5bMLqAqu78UedE91tjZ8PSO9oRafTYcIYQa0zaxR+M4A6/rGUyqyCS
P4UUSxWv4iV/M5eGubDjOasAEwMmk2DjH7qUjYe3qdd/xMN6wMCKEHs/nDXH2Som
e+ln8EInKrjNF3vlQLRdJCEK92XKXHKu07KznUmPU38cqDNkNtJB9q0tBbwduwtl
EiAtO4S8u5v82G0rGqOHZ942xbRfXSUVgI6YrawCgPBI8MkkVHIogBWjAjbK2sWd
Z5rvSSEdkcU674XE01zKDI9tAcuzqfuUP1E5oKQ/dbh9FoTC+oMlPg8SD73GsVuc
oNRYHNoWZHzAXTNG3lWefZcakv2vDAaa5GOR8xWRPxaZAFnviYSPsdbmEy3PIEMK
oAgp33KM0CpL2f4K+yKx
=Lj82
-----END PGP SIGNATURE-----