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

[Alejandro Piñeiro]

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.

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