[RFC,10/14] anv: Add clflush to states.

Submitted by Rafael Antognolli on Dec. 8, 2018, 12:05 a.m.

Details

Message ID 20181208000553.29501-11-rafael.antognolli@intel.com
State New
Headers show
Series "Do not use userptr in anv if softpin is available." ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Rafael Antognolli Dec. 8, 2018, 12:05 a.m.
TODO: This is just flushing the entire dynamic states on every execbuf.
Maybe it's too much. However, in theory we should be already flushing
the states as needed, but I think we didn't hit any bug due to the
coherence implied by userptr.
---
 src/intel/vulkan/anv_batch_chain.c | 4 ++++
 1 file changed, 4 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/anv_batch_chain.c
index 65df28ccb91..99009679435 100644
--- a/src/intel/vulkan/anv_batch_chain.c
+++ b/src/intel/vulkan/anv_batch_chain.c
@@ -1366,6 +1366,10 @@  anv_reloc_list_add_dep(struct anv_cmd_buffer *cmd_buffer,
 
    anv_block_pool_foreach_bo(bo_list, iter, bo) {
       _mesa_set_add(relocs->deps, bo);
+      if (!cmd_buffer->device->info.has_llc) {
+         for (uint32_t i = 0; i < bo->size; i += CACHELINE_SIZE)
+            __builtin_ia32_clflush(bo->map + i);
+      }
    }
 }
 

Comments

This seems very much over-the-top.  It would be better to either find the
specific bug or else just allocate the BOs we use for states as snooped.
See also the anv_gem_set_caching call in genX_query.c.

On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> TODO: This is just flushing the entire dynamic states on every execbuf.
> Maybe it's too much. However, in theory we should be already flushing
> the states as needed, but I think we didn't hit any bug due to the
> coherence implied by userptr.
> ---
>  src/intel/vulkan/anv_batch_chain.c | 4 ++++
>  1 file changed, 4 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_batch_chain.c
> b/src/intel/vulkan/anv_batch_chain.c
> index 65df28ccb91..99009679435 100644
> --- a/src/intel/vulkan/anv_batch_chain.c
> +++ b/src/intel/vulkan/anv_batch_chain.c
> @@ -1366,6 +1366,10 @@ anv_reloc_list_add_dep(struct anv_cmd_buffer
> *cmd_buffer,
>
>     anv_block_pool_foreach_bo(bo_list, iter, bo) {
>        _mesa_set_add(relocs->deps, bo);
> +      if (!cmd_buffer->device->info.has_llc) {
> +         for (uint32_t i = 0; i < bo->size; i += CACHELINE_SIZE)
> +            __builtin_ia32_clflush(bo->map + i);
> +      }
>     }
>  }
>
> --
> 2.17.1
>
> _______________________________________________
> mesa-dev mailing list
> mesa-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Mon, Dec 10, 2018 at 01:52:15PM -0600, Jason Ekstrand wrote:
> This seems very much over-the-top.  It would be better to either find the
> specific bug or else just allocate the BOs we use for states as snooped.  See
> also the anv_gem_set_caching call in genX_query.c.

It seems we were missing some flushes in the states allocated in
anv_shader_bin_create(). I added them there and apparently I don't need
this patch anymore.

> On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <rafael.antognolli@intel.com>
> wrote:
> 
>     TODO: This is just flushing the entire dynamic states on every execbuf.
>     Maybe it's too much. However, in theory we should be already flushing
>     the states as needed, but I think we didn't hit any bug due to the
>     coherence implied by userptr.
>     ---
>      src/intel/vulkan/anv_batch_chain.c | 4 ++++
>      1 file changed, 4 insertions(+)
> 
>     diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/
>     anv_batch_chain.c
>     index 65df28ccb91..99009679435 100644
>     --- a/src/intel/vulkan/anv_batch_chain.c
>     +++ b/src/intel/vulkan/anv_batch_chain.c
>     @@ -1366,6 +1366,10 @@ anv_reloc_list_add_dep(struct anv_cmd_buffer
>     *cmd_buffer,
> 
>         anv_block_pool_foreach_bo(bo_list, iter, bo) {
>            _mesa_set_add(relocs->deps, bo);
>     +      if (!cmd_buffer->device->info.has_llc) {
>     +         for (uint32_t i = 0; i < bo->size; i += CACHELINE_SIZE)
>     +            __builtin_ia32_clflush(bo->map + i);
>     +      }
>         }
>      }
> 
>     --
>     2.17.1
> 
>     _______________________________________________
>     mesa-dev mailing list
>     mesa-dev@lists.freedesktop.org
>     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
>
On Tue, Dec 11, 2018 at 5:32 PM Rafael Antognolli <
rafael.antognolli@intel.com> wrote:

> On Mon, Dec 10, 2018 at 01:52:15PM -0600, Jason Ekstrand wrote:
> > This seems very much over-the-top.  It would be better to either find the
> > specific bug or else just allocate the BOs we use for states as
> snooped.  See
> > also the anv_gem_set_caching call in genX_query.c.
>
> It seems we were missing some flushes in the states allocated in
> anv_shader_bin_create(). I added them there and apparently I don't need
> this patch anymore.
>

Glad you found it!

--Jason


> > On Fri, Dec 7, 2018 at 6:06 PM Rafael Antognolli <
> rafael.antognolli@intel.com>
> > wrote:
> >
> >     TODO: This is just flushing the entire dynamic states on every
> execbuf.
> >     Maybe it's too much. However, in theory we should be already flushing
> >     the states as needed, but I think we didn't hit any bug due to the
> >     coherence implied by userptr.
> >     ---
> >      src/intel/vulkan/anv_batch_chain.c | 4 ++++
> >      1 file changed, 4 insertions(+)
> >
> >     diff --git a/src/intel/vulkan/anv_batch_chain.c b/src/intel/vulkan/
> >     anv_batch_chain.c
> >     index 65df28ccb91..99009679435 100644
> >     --- a/src/intel/vulkan/anv_batch_chain.c
> >     +++ b/src/intel/vulkan/anv_batch_chain.c
> >     @@ -1366,6 +1366,10 @@ anv_reloc_list_add_dep(struct anv_cmd_buffer
> >     *cmd_buffer,
> >
> >         anv_block_pool_foreach_bo(bo_list, iter, bo) {
> >            _mesa_set_add(relocs->deps, bo);
> >     +      if (!cmd_buffer->device->info.has_llc) {
> >     +         for (uint32_t i = 0; i < bo->size; i += CACHELINE_SIZE)
> >     +            __builtin_ia32_clflush(bo->map + i);
> >     +      }
> >         }
> >      }
> >
> >     --
> >     2.17.1
> >
> >     _______________________________________________
> >     mesa-dev mailing list
> >     mesa-dev@lists.freedesktop.org
> >     https://lists.freedesktop.org/mailman/listinfo/mesa-dev
> >
>