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

[Anuj Phogat]

On Wed, Dec 7, 2016 at 3:38 AM, Alejandro Piñeiro <apinheiro at 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 <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
>
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
>>>>
>>
>