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

[Alejandro Piñeiro]

On 06/02/16 19:25, 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.

Ok, will take that into account for the next time. Thanks both for the
explanation.
> (I actually didn't look at this one too closely, but I did review Chris' patch
> in case nobody bothered to clean it up):
> Reviewed-by: Ben Widawsky <benjamin.widawsky at intel.com>

Thanks for the review. In any case, I think that it would be better to
wait a little, just in case Chad is able to do a new review, as he was
the one that suggested that specific cleaning.

BR

-- 
Alejandro Piñeiro (apinheiro at igalia.com)