anv: Pay attention to VK_ACCESS_MEMORY_(READ|WRITE)_BIT

Submitted by Alex Smith on July 20, 2018, 10:44 a.m.

Details

Message ID 20180720104453.7334-1-asmith@feralinteractive.com
State New
Headers show
Series "anv: Pay attention to VK_ACCESS_MEMORY_(READ|WRITE)_BIT" ( rev: 1 ) in Mesa

Not browsing as part of any series.

Commit Message

Alex Smith July 20, 2018, 10:44 a.m.
According to the spec, these should apply to all read/write access
types (so would be equivalent to specifying all other access types
individually). Currently, they were doing nothing.

Signed-off-by: Alex Smith <asmith@feralinteractive.com>
Cc: mesa-stable@lists.freedesktop.org
---
 src/intel/vulkan/anv_private.h | 6 ++++++
 1 file changed, 6 insertions(+)

Patch hide | download patch | download mbox

diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
index cec2842792..775bacaff2 100644
--- a/src/intel/vulkan/anv_private.h
+++ b/src/intel/vulkan/anv_private.h
@@ -1731,6 +1731,9 @@  anv_pipe_flush_bits_for_access_flags(VkAccessFlags flags)
          pipe_bits |= ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
          pipe_bits |= ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
          break;
+      case VK_ACCESS_MEMORY_WRITE_BIT:
+         pipe_bits |= ANV_PIPE_FLUSH_BITS;
+         break;
       default:
          break; /* Nothing to do */
       }
@@ -1761,6 +1764,9 @@  anv_pipe_invalidate_bits_for_access_flags(VkAccessFlags flags)
       case VK_ACCESS_TRANSFER_READ_BIT:
          pipe_bits |= ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT;
          break;
+      case VK_ACCESS_MEMORY_READ_BIT:
+         pipe_bits |= ANV_PIPE_INVALIDATE_BITS;
+         break;
       default:
          break; /* Nothing to do */
       }

Comments

On 20/07/18 11:44, Alex Smith wrote:
> According to the spec, these should apply to all read/write access
> types (so would be equivalent to specifying all other access types
> individually). Currently, they were doing nothing.
>
> Signed-off-by: Alex Smith <asmith@feralinteractive.com>
> Cc: mesa-stable@lists.freedesktop.org
> ---
>   src/intel/vulkan/anv_private.h | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index cec2842792..775bacaff2 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1731,6 +1731,9 @@ anv_pipe_flush_bits_for_access_flags(VkAccessFlags flags)
>            pipe_bits |= ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
>            pipe_bits |= ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
>            break;
> +      case VK_ACCESS_MEMORY_WRITE_BIT:
> +         pipe_bits |= ANV_PIPE_FLUSH_BITS;
> +         break;
>         default:
>            break; /* Nothing to do */
>         }
> @@ -1761,6 +1764,9 @@ anv_pipe_invalidate_bits_for_access_flags(VkAccessFlags flags)
>         case VK_ACCESS_TRANSFER_READ_BIT:
>            pipe_bits |= ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT;
>            break;
> +      case VK_ACCESS_MEMORY_READ_BIT:
> +         pipe_bits |= ANV_PIPE_INVALIDATE_BITS;
> +         break;

I know this function is a bit oddly named for that, but with this part 
of the spec regarding VK_ACCESS_MEMORY_WRITE_BIT :

"

  *

    When included in a destination access mask, makes all available
    writes visible to all future write accesses on entities known to the
    Vulkan device.

"

I would also add :

case VK_ACCESS_MEMORY_WRITE_BIT:
     pipe_bits |= ANV_PIPE_FLUSH_BITS;
     break;

Does that sound fair?

-
Lionel

>         default:
>            break; /* Nothing to do */
>         }
On Fri, Jul 20, 2018 at 8:37 AM Lionel Landwerlin <
lionel.g.landwerlin@intel.com> wrote:

> On 20/07/18 11:44, Alex Smith wrote:
>
> According to the spec, these should apply to all read/write access
> types (so would be equivalent to specifying all other access types
> individually). Currently, they were doing nothing.
>
> Signed-off-by: Alex Smith <asmith@feralinteractive.com> <asmith@feralinteractive.com>
> Cc: mesa-stable@lists.freedesktop.org
> ---
>  src/intel/vulkan/anv_private.h | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
> index cec2842792..775bacaff2 100644
> --- a/src/intel/vulkan/anv_private.h
> +++ b/src/intel/vulkan/anv_private.h
> @@ -1731,6 +1731,9 @@ anv_pipe_flush_bits_for_access_flags(VkAccessFlags flags)
>           pipe_bits |= ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
>           pipe_bits |= ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
>           break;
> +      case VK_ACCESS_MEMORY_WRITE_BIT:
> +         pipe_bits |= ANV_PIPE_FLUSH_BITS;
> +         break;
>        default:
>           break; /* Nothing to do */
>        }
> @@ -1761,6 +1764,9 @@ anv_pipe_invalidate_bits_for_access_flags(VkAccessFlags flags)
>        case VK_ACCESS_TRANSFER_READ_BIT:
>           pipe_bits |= ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT;
>           break;
> +      case VK_ACCESS_MEMORY_READ_BIT:
> +         pipe_bits |= ANV_PIPE_INVALIDATE_BITS;
> +         break;
>
>
> I know this function is a bit oddly named for that, but with this part of
> the spec regarding VK_ACCESS_MEMORY_WRITE_BIT :
>
> "
>
>    -
>
>    When included in a destination access mask, makes all available writes
>    visible to all future write accesses on entities known to the Vulkan device.
>
> "
>
> I would also add :
>
> case VK_ACCESS_MEMORY_WRITE_BIT:
>     pipe_bits |= ANV_PIPE_FLUSH_BITS;
>     break;
>
> Does that sound fair?
>

That's quite the heavy hammer.... But I think it's the right thing to do.
On 20 July 2018 at 19:01, Jason Ekstrand <jason@jlekstrand.net> wrote:

> On Fri, Jul 20, 2018 at 8:37 AM Lionel Landwerlin <
> lionel.g.landwerlin@intel.com> wrote:
>
>> On 20/07/18 11:44, Alex Smith wrote:
>>
>> According to the spec, these should apply to all read/write access
>> types (so would be equivalent to specifying all other access types
>> individually). Currently, they were doing nothing.
>>
>> Signed-off-by: Alex Smith <asmith@feralinteractive.com> <asmith@feralinteractive.com>
>> Cc: mesa-stable@lists.freedesktop.org
>> ---
>>  src/intel/vulkan/anv_private.h | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/src/intel/vulkan/anv_private.h b/src/intel/vulkan/anv_private.h
>> index cec2842792..775bacaff2 100644
>> --- a/src/intel/vulkan/anv_private.h
>> +++ b/src/intel/vulkan/anv_private.h
>> @@ -1731,6 +1731,9 @@ anv_pipe_flush_bits_for_access_flags(VkAccessFlags flags)
>>           pipe_bits |= ANV_PIPE_RENDER_TARGET_CACHE_FLUSH_BIT;
>>           pipe_bits |= ANV_PIPE_DEPTH_CACHE_FLUSH_BIT;
>>           break;
>> +      case VK_ACCESS_MEMORY_WRITE_BIT:
>> +         pipe_bits |= ANV_PIPE_FLUSH_BITS;
>> +         break;
>>        default:
>>           break; /* Nothing to do */
>>        }
>> @@ -1761,6 +1764,9 @@ anv_pipe_invalidate_bits_for_access_flags(VkAccessFlags flags)
>>        case VK_ACCESS_TRANSFER_READ_BIT:
>>           pipe_bits |= ANV_PIPE_TEXTURE_CACHE_INVALIDATE_BIT;
>>           break;
>> +      case VK_ACCESS_MEMORY_READ_BIT:
>> +         pipe_bits |= ANV_PIPE_INVALIDATE_BITS;
>> +         break;
>>
>>
>> I know this function is a bit oddly named for that, but with this part of
>> the spec regarding VK_ACCESS_MEMORY_WRITE_BIT :
>>
>> "
>>
>>    -
>>
>>    When included in a destination access mask, makes all available
>>    writes visible to all future write accesses on entities known to the Vulkan
>>    device.
>>
>> "
>>
>> I would also add :
>>
>> case VK_ACCESS_MEMORY_WRITE_BIT:
>>     pipe_bits |= ANV_PIPE_FLUSH_BITS;
>>     break;
>>
>> Does that sound fair?
>>
>
> That's quite the heavy hammer.... But I think it's the right thing to do.
>

Yes - these bits are supposed to be the heaviest hammer there is. I'll add
that.

Alex