Re: [Mesa-dev] [PATCH v3] i965/blorp: Fix hiz ops on MSAA surfaces
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
Re: [Mesa-dev] [PATCH v3] i965/blorp: Fix hiz ops on MSAA surfaces
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
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
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
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