[Intel-gfx] [PATCH] drm/i915: Fix obj->map_and_fenceable across tiling changes

[Jani Nikula]

On Fri, 07 Nov 2014, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Fri, Nov 07, 2014 at 11:05:05AM +0100, Daniel Vetter wrote:
>> On Thu, Nov 06, 2014 at 08:40:35AM +0000, Chris Wilson wrote:
>> > As obj->map_and_fenceable computation has changed to only be set when
>> > the object is bound inside the global GTT (and is suitable aligned to a
>> > fence region) we need to accommodate those changes when the tiling is
>> > adjusted. The easiest solution is to unbind from the global GTT if we
>> > are currently fenceable, but will not be after the tiling change.
>> 
>> QA failed to supply the bisect for this regression, but most likely this
>> has been introduced due to the change in handling obj->map_and_fenceable
>> in
>> 
>> commit e6a844687cf929ec053c7578d5ecc794a8a6c5cf
>> Author: Chris Wilson <chris at chris-wilson.co.uk>
>> Date:   Mon Aug 11 12:00:12 2014 +0200
>> 
>>     drm/i915: Force CPU relocations if not GTT mapped
>
> I think it also took
>
> commit f8fcadba218fe6d23b2e353fea1cf0a4be4c9454
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Fri Oct 31 13:53:52 2014 +0000
>
>     drm/i915: Only mark as map-and-fenceable when bound into the GGTT
>
> to expose the bug in testing.

Valtteri confirmed this as the bisect result, and provided Tested-by on
the fix. Pushed the fix to drm-intel-fixes with the commit message
amended per input from Daniel and the bisect result from Valtteri; blame
me for any mistakes in the commit message.

Thanks for the patch and testing.

BR,
Jani.


>
>> Note that the alignment check is a vestige from our (unsuccessful)
>> attempts to reduce the alignment requirements of tiled but unfenced
>> buffers on gen2/3.
>
> Also, that was when unbinding from the GTT meant UC writes and clflushing,
> so we went to great pains to avoid such.
>  
>> That leaves the actual bug of setting map_and_fenceable to true if we're
>> not bound to ggtt, which violates the change introduced in the above
>> patch. Unbinding in that case really looks like the simplest and safest
>> option, we have to do it anyway.
>> 
>> If Chris agrees, please add the above analysis to the commit message when
>> merging to -fixes. 
>> > 
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=85896
>> > Tested-by: huax.lu at intel.com
>> 
>> Testcase: igt/gem_concurrent_blit
> Testcase: igt/gem_concurrent_blit/gttX*
>
> It was also only triggered by recent additions to
> gem_concurrent_blit (which itself was trying to stress test our
> fence-vs-GPU serialisation for testing requests - so I can claim it was
> intentional!). However, it turns out to be easier to hit in practice
> than in testing. :|
> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre

-- 
Jani Nikula, Intel Open Source Technology Center