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

Chad Versace chad.versace at linux.intel.com
Thu Feb 28 11:13:34 PST 2013


On 02/28/2013 11:01 AM, Kenneth Graunke wrote:
> 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...

Eric, you're right.

I agree with Ken on the padding issue, and I do have patches for it.
I'm doing a full piglit run now, and will submit them if the results look good.


More information about the mesa-dev mailing list