[Mesa-dev] [PATCH v3] i965/blorp: Fix hiz ops on MSAA surfaces

[Alejandro Piñeiro]

On 09/02/16 02:29, Jordan Justen wrote:
> On 2016-02-06 10:25:59, Ben Widawsky wrote:
>> On Sat, Feb 06, 2016 at 12:01:50PM +0100, Alejandro Piñeiro wrote:
>>> From: Chris Forbes <chrisf at ijw.co.nz>
>>>
>>> Two things were broken here:
>>> - The depth/stencil surface dimensions were broken for MSAA.
>>> - Sample count was programmed incorrectly.
>>>
>>> Result was the depth resolve didn't work correctly on MSAA surfaces, and
>>> so sampling the surface later produced garbage.
>>>
>>> Fixes the new piglit test arb_texture_multisample-sample-depth, and
>>> various artifacts in 'tesseract' with msaa=4 glineardepth=0.
>>>
>>> Fixes freedesktop bug #76396.
>>>
>>> Not observed any piglit regressions on Haswell.
>>>
>>> v2: Just set brw_hiz_op_params::dst.num_samples rather than adding a
>>>     helper function (Ken).
>>>
>>> v3: moved the alignment needed for hiz+msaa to brw_blorp.cpp, as
>>>     suggested by Chad Versace (Alejandro Piñeiro on behalf of Chris
>>>     Forbes)
>>> ---
>>>
>>> While taking a look to bug #76396, I found that the git commit message
>>> on arb_texture_multisample/sample-depth.c mentions that MSAA+hiz not
>>> working on Haswell was an already know bug. After pinging on IRC Ben
>>> Widawsky pointed me this around one year old review thread.
>>>
>>> It seems that the original patch got not updated after Chad Versace
>>> review. Hoping to not stomp on any toe, I made the update myself (so
>>> this v3 update). Even if this update is not accepted, hopefully this
>>> will resume the review process and the bug will be fixed soon.
>>>
>>> I re-run piglit on Haswell, so "Not observed any piglit regressions on
>>> Haswell." still applies.
>>>
>>> I also slightly changed the commit message to reflect that this patch
>>> fixes bug #76396. Also removed the "Signed-off-by: Chris Forbes
>>> <chrisf at ijw.co.nz>" because is not technically true anymore. Chris can
>>> re-add that again when he push the patch, if accepted.
>> Thanks for doing this Alejandro. Like Ilia said though, the common process is to
>> just add your SoB on top if there was one there to begin with.
>>
> On top? I usually add to the bottom.

That was also what Illia suggested.

> Alejandro,
>
> I ran the patch through jenkins, and it seemed to fix Sandy Bridge,
> Ivy Bridge and Haswell for
> piglit.spec.arb_texture_multisample.arb_texture_multisample-sample-depth
>
> Tested-by: Jordan Justen <jordan.l.justen at intel.com>
> Reviewed-by: Jordan Justen <jordan.l.justen at intel.com>

Two reviews, so I think that it is safe to push without waiting for Chad
Versace.

I have just pushed the commit, adding the Signed-off as you all suggested.

Thanks everybody for the patience and the reviews.

BR