[Mesa-dev] [PATCH 1/4] i965: Fix fulsim assertion failures by aligning HiZ ops to 8x4.

[Kenneth Graunke]

On 02/28/2013 09:08 AM, Eric Anholt wrote:
> Chad Versace <chad.versace at linux.intel.com> writes:
>
>> On 02/27/2013 11:39 AM, Eric Anholt wrote:
>>> Chad Versace <chad.versace at linux.intel.com> writes:
>>>
>>>> On 02/26/2013 11:15 PM, Eric Anholt wrote:
>>>>> I have some debug of HiZ rendering that looks like some rendering is not
>>>>> landing in my HiZ buffer.  Unfortunately, fulsim choking on us violating
>>>>> hiz rendering rules was preventing me from using it as a debug aid.
>>>>>
>>>>> Once we get things reliable, we'll also be able to take advantage of this
>>>>> to get fast clears on modes like 1366x768.
>>>>> ---
>>>>>   src/mesa/drivers/dri/i965/brw_blorp.cpp |   10 ++++++++++
>>>>>   1 file changed, 10 insertions(+)
>>>>>
>>>>> diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>>>>> index 5f72b5d..49dcacb 100644
>>>>> --- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
>>>>> +++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
>>>>> @@ -181,6 +181,16 @@ brw_hiz_op_params::brw_hiz_op_params(struct intel_mipmap_tree *mt,
>>>>>      this->hiz_op = op;
>>>>>
>>>>>      depth.set(mt, level, layer);
>>>>> +
>>>>> +   /* HiZ operations require alignment to 8x4.  Our depth and hiz miplevels
>>>>> +    * should have their start offsets aligned to that (except for a bug on
>>>>> +    * non-Z16) so we won't draw into a neighboring miplevel, and we allocate
>>>>> +    * memory aligned to pages (128bytesx32), so we won't draw into memory not
>>>>> +    * owned by our miptree.
>>>>> +    */
>>>>> +   depth.width = ALIGN(depth.width, 8);
>>>>> +   depth.height = ALIGN(depth.height, 4);
>>>>> +
>>>>
>>>> This should be moved into the brw_hiz_op_params() constructor.
>>>
>>> That's where this is.  Were you thinking of a version that was posted in
>>> my tree at one point instead?
>>
>> No, I was talking with my foot in my mouth. :/
>>
>> I meant it should be set by call to depth.set() above, which is
>> brw_blorp_mip_info::set(), like this:
>>
>>     this->width = ALIGN(mt->level[level].width, 8);
>>     this->height = ALIGN(mt->level[level].height, 4);
>>
>> The responsibility of setting brw_blorp_mip_info fields should
>> belong to that method.
>
> I disagree -- you only want this behavior for HiZ ops on depth/stencil,
> not blorp ops in general (like blits with color buffers).

My vote is with Eric on this one.  I wrote an identical patch, and put 
it in brw_hiz_op_params for that very reason.

However, in order for this to not break horribly, don't you need to edit 
the intel/brw_tex_layout code to actually pad the buffer to have that 
extra width?  Otherwise you could access outside the buffer (and I 
could've sworn I actually hit that case while trying this).

I believe Chad has patches to do that...