Re: [Intel-gfx] [PATCH v5 4/6] drm/i915: Use vma resources for async unbinding

Matthew Auld
2022-01-06T12:13:48Z

On 04/01/2022 12:51, Thomas Hellström wrote:
> Implement async (non-blocking) unbinding by not syncing the vma before
> calling unbind on the vma_resource.
> Add the resulting unbind fence to the object's dma_resv from where it is
> picked up by the ttm migration code.
> Ideally these unbind fences should be coalesced with the migration blit
> fence to avoid stalling the migration blit waiting for unbind, as they
> can certainly go on in parallel, but since we don't yet have a
> reasonable data structure to use to coalesce fences and attach the
> resulting fence to a timeline, we defer that for now.
>
> Note that with async unbinding, even while the unbind waits for the
> preceding bind to complete before unbinding, the vma itself might have been
> destroyed in the process, clearing the vma pages. Therefore we can
> only allow async unbinding if we have a refcounted sg-list and keep a
> refcount on that for the vma resource pages to stay intact until
> binding occurs. If this condition is not met, a request for an async
> unbind is diverted to a sync unbind.
>
> v2:
> - Use a separate kmem_cache for vma resources for now to isolate their
> memory allocation and aid debugging.
> - Move the check for vm closed to the actual unbinding thread. Regardless
> of whether the vm is closed, we need the unbind fence to properly wait
> for capture.
> - Clear vma_res::vm on unbind and update its documentation.
> v4:
> - Take cache coloring into account when searching for vma resources
> pending unbind. (Matthew Auld)
> v5:
> - Fix timeout and error check in i915_vma_resource_bind_dep_await().
> - Avoid taking a reference on the object for async binding if
> async unbind capable.
> - Fix braces around a single-line if statement.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> ---

<snip>

> +
> +static void
> +i915_vma_resource_color_adjust_range(struct i915_address_space *vm,
> + unsigned long *start,
> + unsigned long *end)

Make these u64, below also? Just in case this is 32b?

> +{
> + if (i915_vm_has_cache_coloring(vm)) {
> + if (start)
> + start -= I915_GTT_PAGE_SIZE;
> + end += I915_GTT_PAGE_SIZE;

*start *end :)

> + }

else {
WARN_ON_ONCE(vm->color_adjust);
}

?

> +}