[Intel-xe] [PATCH] drm/xe: don't skip rebinds if there's a NULL binding involved

[Matthew Brost]

On Fri, Jul 28, 2023 at 08:30:59PM -0700, Paulo Zanoni wrote:
> Commit a042bb4ca894 ("drm/xe: Avoid doing rebinds") causes
> regressions in numerous Vulkan dEQP sparse-related tests. The errors
> happen in tests that:
> 
>   - check residencyNonResidentStrict=true behavior (use NULL bindings
>     and check that they read/write zero)
>   - and use buffers bigger than 2MB
>   - and write through sparse buffers using compute shaders with ssbos
> 
> So for example, the test:
>   - dEQP-VK.sparse_resources.buffer.ssbo.sparse_residency.buffer_size_2_24
> 
> fails only if residencyNonResidentStrict is true, otherwise it passes
> (becasue otherwise it doesn't check the values of the ranges that were
> supposed to be bound through NULL bindings).
> 
> But the test:
>   - dEQP-VK.sparse_resources.buffer.ssbo.sparse_residency.buffer_size_2_20
>

Thanks for finding this.

> always passes because it uses a smaller buffer, despite doing
> everything else equally. By maninpulating the sizes of the buffers we
> can see that 2MB is the range where the test starts failing.
> 
> On the other hand, some other 2_24 tests were passing, despite having
> 4MB buffers and checking the contents of NULL binding buffers.
> 
> I can't claim I have a deep understanding of the code invovled here,

The problem is if you go from NULL to non-NULL or vice versa so I
believe this fix isn't quite right. Let me try to respin.

Matt

> but this patch seems to be the minimal 'revert' that fixes the issue
> and should be a good point to start the conversation on the mailing
> list.
> 
> Cc: Matthew Brost <matthew.brost at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Testcase: dEQP-VK.sparse_resources.buffer.ssbo.sparse_residency.buffer_size_2_24
> Fixes: a042bb4ca894 ("drm/xe: Avoid doing rebinds")
> Reference: https://gitlab.freedesktop.org/mesa/mesa/-/merge_requests/23045
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 6429d6e5113d..955f664ff1f1 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -2512,7 +2512,8 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_engine *e,
>  					 * Userptr creates a new SG mapping so
>  					 * we must also rebind.
>  					 */
> -					op->remap.skip_prev = !xe_vma_is_userptr(old) &&
> +					op->remap.skip_prev = !is_null &&
> +						!xe_vma_is_userptr(old) &&
>  						IS_ALIGNED(xe_vma_end(vma),
>  							   xe_vma_max_pte_size(old));
>  					if (op->remap.skip_prev) {
> @@ -2547,7 +2548,8 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_engine *e,
>  					 * Userptr creates a new SG mapping so
>  					 * we must also rebind.
>  					 */
> -					op->remap.skip_next = !xe_vma_is_userptr(old) &&
> +					op->remap.skip_next = !is_null &&
> +						!xe_vma_is_userptr(old) &&
>  						IS_ALIGNED(xe_vma_start(vma),
>  							   xe_vma_max_pte_size(old));
>  					if (op->remap.skip_next)
> -- 
> 2.39.2
>