On Wed, Dec 7, 2016 at 3:38 AM, Alejandro Piñeiro <apinhe...@igalia.com> wrote: > > > On 07/12/16 09:36, Alejandro Piñeiro wrote: >> On 06/12/16 22:26, Anuj Phogat wrote: >>> On Tue, Dec 6, 2016 at 10:58 AM, Alejandro Piñeiro <apinhe...@igalia.com> >>> wrote: >>>> The FIXME suggest that the check should be removed. >>>> >>> Only if we see any performance or feature benefits in doing that. >>> Last I checked I didn't see any performance benefits on Skylake. >> Ok, then I misunderstood the comment. I understood that the code was >> there based on performance data, but that it could be removed in the future. >> >>> I also couldn't figure out the cause of random failure in a piglit test: >>> ./bin/texelFetch fs sampler2DMSArray 4 98x1x9-98x129x9 -auto -fbo >>> >>> I'm still seeing this failure when I tested your patch with Jenkins CI >>> system. >>> Test passes when I run it locally on my system. >> Initially I also thought that failure was a regression of this patch. >> But then I tested back without it, and I got some random failures too. > > FWIW, this is also flaky with and without the patch: > bin/texelFetch fs sampler2DMSArray 4 1x129x9-98x129x9 -auto -fbo > Thanks for the information. It will be useful if we want to upstream this patch sometime later.
>> >>>> This change helps the following test: >>>> GL45-CTS.texture_cube_map_array.color_depth_attachments >>>> >>>> to pass consistently on Skylake GT3e. Without this patch, on >>>> Skylake GT3e that test has a ~76% pass rate, with some random >>>> intel_do_flush_locked errors now and then. >>>> >>> By removing this check you're actually enabling fast copy blit on SKL+ >>> for all the tiling formats. So, now the driver will use XY_FAST_COPY_BLT >>> in place of XY_SRC_COPY_BLT. If this change is fixing a test case for >>> you, that's not because this is the right fix, but that's because you're >>> avoiding to use XY_SRC_COPY_BLT. >>> XY_SRC_COPY_BLT might be causing intel_do_flush_locked errors >>> and the test failure. We need to dig in there to find the real cause. >> Ok, thanks for the hints. Will resume the work based on this paragraph. >> Any help would be appreciated though. >>>> It works fine on Skylake GT2, though. >>>> --- >>>> >>>> I didn't analyze too much the code. It was more git history analysis. >>>> >>>> When I started to work to solve that test, I remembered that there was >>>> a time in the past that worked, so I just did a git bisect. The more >>>> recent bad commit I found was df210f. In any case, that one just fixed >>>> that check, as became broken with commit 0c02d7. The one that added >>>> the check (and the FIXME) was commit 412c8c. >>>> >>>> As the commit message says, that FIXME seems to suggest that is a >>>> provisional change. Although I recognize that the failure is really >>>> specific (for a Skylake model, not failing always), removing the check >>>> gets the test pass consistently, and as far as I see, it didn't >>>> introduce any regression. >>>> >>>> src/mesa/drivers/dri/i965/intel_blit.c | 8 -------- >>>> 1 file changed, 8 deletions(-) >>>> >>>> diff --git a/src/mesa/drivers/dri/i965/intel_blit.c >>>> b/src/mesa/drivers/dri/i965/intel_blit.c >>>> index 03a35ee..9f3b4d1 100644 >>>> --- a/src/mesa/drivers/dri/i965/intel_blit.c >>>> +++ b/src/mesa/drivers/dri/i965/intel_blit.c >>>> @@ -487,14 +487,6 @@ can_fast_copy_blit(struct brw_context *brw, >>>> if (brw->gen < 9) >>>> return false; >>>> >>>> - /* Enable fast copy blit only if the surfaces are Yf/Ys tiled. >>>> - * FIXME: Based on performance data, remove this condition later to >>>> - * enable for all types of surfaces. >>>> - */ >>>> - if (src_tr_mode == INTEL_MIPTREE_TRMODE_NONE && >>>> - dst_tr_mode == INTEL_MIPTREE_TRMODE_NONE) >>>> - return false; >>>> - >>>> if (logic_op != GL_COPY) >>>> return false; >>>> >>>> -- >>>> 2.9.3 >>>> >> > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
