Re: [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 
>>>
>>> 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
>>> " 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 
> Reviewed-by: Jordan Justen 

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

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Jordan Justen]

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

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 
Reviewed-by: Jordan Justen 

> (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 
> 
> [snip]
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ben Widawsky]

On Sat, Feb 06, 2016 at 12:01:50PM +0100, Alejandro Piñeiro wrote:
> From: Chris Forbes 
> 
> 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
> " 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.

(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 

[snip]


-- 
Ben Widawsky, Intel Open Source Technology Center
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [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 
>>
>> 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
>> " 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 

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 (apinhe...@igalia.com)

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Ilia Mirkin]

On Sat, Feb 6, 2016 at 6:01 AM, Alejandro Piñeiro  wrote:
> I also slightly changed the commit message to reflect that this patch
> fixes bug #76396. Also removed the "Signed-off-by: Chris Forbes
> " because is not technically true anymore. Chris can
> re-add that again when he push the patch, if accepted.

Without commenting on the patch itself, I believe a common process is
to do something like

Signed-off-by: original author
[apinheiro: change x, y, and z]
Signed-off-by: you

But I don't think people are too picky as long as you don't
misrepresent what's going on.

  -ilia
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

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

[Alejandro Piñeiro]

From: Chris Forbes 

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
" because is not technically true anymore. Chris can
re-add that again when he push the patch, if accepted.


 src/mesa/drivers/dri/i965/brw_blorp.cpp | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/src/mesa/drivers/dri/i965/brw_blorp.cpp 
b/src/mesa/drivers/dri/i965/brw_blorp.cpp
index 1bc6d15..4497eab 100644
--- a/src/mesa/drivers/dri/i965/brw_blorp.cpp
+++ b/src/mesa/drivers/dri/i965/brw_blorp.cpp
@@ -319,8 +319,14 @@ brw_hiz_op_params::brw_hiz_op_params(struct 
intel_mipmap_tree *mt,
 * not 8. But commit 1f112cc increased the alignment from 4 to 8, which
 * prevents the clobbering.
 */
-   depth.width = ALIGN(depth.width, 8);
-   depth.height = ALIGN(depth.height, 4);
+   dst.num_samples = mt->num_samples;
+   if (dst.num_samples > 1) {
+  depth.width = ALIGN(mt->logical_width0, 8);
+  depth.height = ALIGN(mt->logical_height0, 4);
+   } else {
+  depth.width = ALIGN(depth.width, 8);
+  depth.height = ALIGN(depth.height, 4);
+   }
 
x1 = depth.width;
y1 = depth.height;
-- 
2.1.4

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev