[Mesa-dev] [PATCH] i965/blit: Remove Yf/Ys tiled check under a FIXME at can_fast_copy_blit

[Alejandro Piñeiro]

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 <apinheiro at 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

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