Re: [PATCH] drm/i915: Release the atomic kmap relocation cache around snb GTT w/a

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 13/02/2018 13:46, Chris Wilson wrote:
> Quoting Chris Wilson (2018-02-13 13:45:33)
>> Quoting Tvrtko Ursulin (2018-02-13 13:42:03)
>>>
>>> On 12/02/2018 21:11, Chris Wilson wrote:
>>>> When we need to rebind the vma into the global GTT for snb, we need to
>>>> drop the current reloc_cache as it will be holding a kmap_atomic() and
>>>> we may need to sleep for i915_vma_bind(). In practice, this is not an
>>>> issue as we already hold an rpm reference for the execbuffer, but with
>>>> tighter error checking around rpm we need to be more careful.
>>>>
>>>> References: 31a39207f04a ("drm/i915: Cache kmap between relocations")
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 ++++++
>>>>    1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> index b15305f2fb76..8c34b1b5a126 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
>>>> @@ -1315,6 +1315,12 @@ eb_relocate_entry(struct i915_execbuffer *eb,
>>>>                 */
>>>>                if (reloc->write_domain == I915_GEM_DOMAIN_INSTRUCTION &&
>>>>                    IS_GEN6(eb->i915)) {
>>>> +                     /*
>>>> +                      * Release the kmap_atomic cache in order to allow the
>>>> +                      * i915_vma_bind() to sleep (if needs be).
>>>> +                      */
>>>> +                     reloc_cache_reset(&eb->reloc_cache);
>>>> +
>>>>                        err = i915_vma_bind(target, target->obj->cache_level,
>>>>                                            PIN_GLOBAL);
>>>>                        if (WARN_ONCE(err,
>>>>
>>>
>>> Hmm yes, very interesting. If you are happy with the performance hit then:
>>>
>>> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> But in general could it also be solved by changing how
>>> intel_runtime_pm_get works - making our wakeref_count top level and only
>>> calling pm_runtime_get_sync on 0 to 1 transition? That would solve the
>>> issue with proposed might_sleep and code paths which know the condition
>>> is actually impossible.
>>
>> I think we should do that anyway to reduce the jitter we see in calling
>> pm_runtime_get_sync().
> 
> I recall the challenge lay in the asserts which also bump our counter to
> hide themselves.

Not so easy then. Plan B - do not merge this patch nor my might sleep, 
but set up a cron job to send the latter for CI once a month. :)

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx