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

[Eric Anholt]

Chad Versace <chad.versace at linux.intel.com> writes:

> 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.

The padding is not an issue currently, as noted in the comment.
However, if we ever do linear buffers it would become an issue, so it
makes sense to put something in.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 197 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/mesa-dev/attachments/20130228/3f27f4d9/attachment.pgp>