[Mesa-dev] [PATCH] anv: Stop setting domains to RENDER on EXEC_OBJECT_WRITE

[Jason Ekstrand]

On July 7, 2017 1:52:54 PM Chris Wilson <chris at chris-wilson.co.uk> wrote:

> Quoting Jason Ekstrand (2017-07-07 21:37:29)
>> The reason we were doing this was to ensure that the kernel did the
>> appropriate cross-ring synchronization and flushing.  However, the
>> kernel only looks at EXEC_OBJECT_WRITE to determine whether or not to
>> insert a fence.  It only cares about the domain for determining whether
>> or not it needs to clflush the BO before using it for scanout but the
>> domain automatically gets set to RENDER internally by the kernel if
>> EXEC_OBJECT_WRITE is set.
>
> Once upon a time we also depended upon EXEC_OBJECT_WRITE for correct
> swapout. That was until I saw what you were planning to do for anv. Hmm,
> that puts the oldest kernel that might support anv as
>
> commit 51bc140431e233284660b1d22c47dec9ecdb521e [v4.3]
> Author: Chris Wilson <chris at chris-wilson.co.uk>
> Date:   Mon Aug 31 15:10:39 2015 +0100
>
>     drm/i915: Always mark the object as dirty when used by the GPU

I think we're probably ok there.  We have a hard requirement on memfd which 
I think landed in 4.6 though I could be wrong about that.

>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>> ---
>>  src/intel/vulkan/anv_batch_chain.c | 7 ++-----
>>  1 file changed, 2 insertions(+), 5 deletions(-)
>>
>> diff --git a/src/intel/vulkan/anv_batch_chain.c 
>> b/src/intel/vulkan/anv_batch_chain.c
>> index 9def174..9776a45 100644
>> --- a/src/intel/vulkan/anv_batch_chain.c
>> +++ b/src/intel/vulkan/anv_batch_chain.c
>> @@ -148,9 +148,6 @@ anv_reloc_list_add(struct anv_reloc_list *list,
>>     struct drm_i915_gem_relocation_entry *entry;
>>     int index;
>>
>> -   const uint32_t domain =
>> -      (target_bo->flags & EXEC_OBJECT_WRITE) ? I915_GEM_DOMAIN_RENDER : 0;
>> -
>>     VkResult result = anv_reloc_list_grow(list, alloc, 1);
>>     if (result != VK_SUCCESS)
>>        return result;
>> @@ -163,8 +160,8 @@ anv_reloc_list_add(struct anv_reloc_list *list,
>>     entry->delta = delta;
>>     entry->offset = offset;
>>     entry->presumed_offset = target_bo->offset;
>> -   entry->read_domains = domain;
>> -   entry->write_domain = domain;
>> +   entry->read_domains = 0;
>> +   entry->write_domain = 0;
>
> The first time I saw this I was amazed we let 0 through. It is true that
> the kernel only cares about EXEC_OBJECT_WRITE, and doesn't care whether
> that is from an execobject.flag or from accumulation of
> reloc[].write_domain. (That has been true for all kernels since the
> introduction of NORELOC and the EXEC_OBJECT_WRITE flag)  We don't even
> use the reloc.write_domain information during reloc itself, so
>
> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> -Chris