[Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Bas Nieuwenhuizen]

From: Alex Smith 

CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
through L2. Therefore, to make these writes visible to later accesses
we must invalidate L2 rather than just writing it back, to avoid the
possibility that stale data is read through L2.

Signed-off-by: Alex Smith 
Reviewed-by: Bas Nieuwenhuizen 
Cc: "17.0" 
[Bas: patch is a backport for 17.0 of the cherry-pick below]
(cherry picked from commit bc5d587a80b64fb3e0a5ea8067e6317fbca2bbc5)
---
 src/amd/vulkan/radv_cmd_buffer.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index 628737c75ac..3aa415b152f 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -2580,7 +2580,8 @@ void radv_CmdPipelineBarrier(
flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_DB;
break;
case VK_ACCESS_TRANSFER_WRITE_BIT:
-   flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_CB;
+   flush_bits |= RADV_CMD_FLAG_FLUSH_AND_INV_CB |
+ RADV_CMD_FLAG_INV_GLOBAL_L2;
break;
default:
break;
-- 
2.12.1

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Emil Velikov]

On 28 March 2017 at 19:11, Bas Nieuwenhuizen  wrote:
> On Tue, Mar 28, 2017 at 6:31 PM, Alex Smith  
> wrote:
>> On 28 March 2017 at 17:09, Emil Velikov  wrote:
>>>
>>> On 22 March 2017 at 10:06, Bas Nieuwenhuizen 
>>> wrote:
>>> > On Tue, Mar 21, 2017 at 1:02 PM, Alex Smith
>>> >  wrote:
>>> >> CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
>>> >> through L2. Therefore, to make these writes visible to later accesses
>>> >> we must invalidate L2 rather than just writing it back, to avoid the
>>> >> possibility that stale data is read through L2.
>>> >>
>>> >> Cc: "17.0" 
>>> >> Signed-off-by: Alex Smith 
>>> >> ---
>>> >> It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
>>> >> as far as I can see, and changing things so that they do also solves
>>> >> the problems that this patch fixes.
>>> >>
>>> >> However, I don't know what the exact consequences of doing so are, or
>>> >> whether there are any situations where that shouldn't be done, so I've
>>> >> gone with this fix instead as it seems like a safer option for now.
>>> >
>>> > Yeah we should be able to. I'm more comfortable sending this patch to
>>> > stable though, so this patch is
>>> >
>>> Bas, others,
>>>
>>> Patch addresses radv_{src,dst}_access_flush() which landed with commit
>>> 6dbb0eaccc3, after the 17.0 branchpoint.
>>
>>
>> Oops, my mistake.
>>
>> I think radv_CmdPipelineBarrier on the 17.0 branch still needs
>> RADV_CMD_FLAG_INV_GLOBAL_L2 added for TRANSFER_WRITE barriers at least. Bas,
>> do you think that should be added in a separate patch just for stable, or
>> would you prefer to push those later changes to stable as well? Looks like
>> there's some fixes in those as well.
>
> I'd prefer to backport this patch. The other patches IMO contain too
> much risk for regression and are actually mostly for optimizations.
Amazing, thank you.

Please add a note like below, so that we get some nice and clear references

[Bas: patch is a backport for 17.0 of the cherry-pick below]
(cherry picked from commit bc5d587a80b64fb3e0a5ea8067e6317fbca2bbc5)

-Emil
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Bas Nieuwenhuizen]

On Tue, Mar 28, 2017 at 6:31 PM, Alex Smith  wrote:
> On 28 March 2017 at 17:09, Emil Velikov  wrote:
>>
>> On 22 March 2017 at 10:06, Bas Nieuwenhuizen 
>> wrote:
>> > On Tue, Mar 21, 2017 at 1:02 PM, Alex Smith
>> >  wrote:
>> >> CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
>> >> through L2. Therefore, to make these writes visible to later accesses
>> >> we must invalidate L2 rather than just writing it back, to avoid the
>> >> possibility that stale data is read through L2.
>> >>
>> >> Cc: "17.0" 
>> >> Signed-off-by: Alex Smith 
>> >> ---
>> >> It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
>> >> as far as I can see, and changing things so that they do also solves
>> >> the problems that this patch fixes.
>> >>
>> >> However, I don't know what the exact consequences of doing so are, or
>> >> whether there are any situations where that shouldn't be done, so I've
>> >> gone with this fix instead as it seems like a safer option for now.
>> >
>> > Yeah we should be able to. I'm more comfortable sending this patch to
>> > stable though, so this patch is
>> >
>> Bas, others,
>>
>> Patch addresses radv_{src,dst}_access_flush() which landed with commit
>> 6dbb0eaccc3, after the 17.0 branchpoint.
>
>
> Oops, my mistake.
>
> I think radv_CmdPipelineBarrier on the 17.0 branch still needs
> RADV_CMD_FLAG_INV_GLOBAL_L2 added for TRANSFER_WRITE barriers at least. Bas,
> do you think that should be added in a separate patch just for stable, or
> would you prefer to push those later changes to stable as well? Looks like
> there's some fixes in those as well.

I'd prefer to backport this patch. The other patches IMO contain too
much risk for regression and are actually mostly for optimizations.
>
> Alex
>
>>
>>
>> From a quick look, the functions has been fixed/enhanced with the
>> following 7 commits. Some of which require additional patches.
>> If we want this in stable, please provide a backport or branch based
>> on top of mesa-17.0.2.
>>
>> Thanks
>> Emil
>>
>> 40e0dbf96c4d812be940561f5732b1b0e44b5e1d
>> f3dc318464b786c2696e650e7c69984b5453624b
>> b075eb7d476eb750092f72e7ec65bc41003fa658
>> dd094e4ff9ff0967b515a4330e40feca55247e25
>> 7a600bbc8186fef9475bedfe6d58a54011a8022b
>> 0567ab0407e4058c108443b90b7c23a40c391c3b
>> f92a118434452df201cda6d9ec2405aca669b104
>
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Alex Smith]

On 28 March 2017 at 17:09, Emil Velikov  wrote:

> On 22 March 2017 at 10:06, Bas Nieuwenhuizen 
> wrote:
> > On Tue, Mar 21, 2017 at 1:02 PM, Alex Smith 
> wrote:
> >> CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
> >> through L2. Therefore, to make these writes visible to later accesses
> >> we must invalidate L2 rather than just writing it back, to avoid the
> >> possibility that stale data is read through L2.
> >>
> >> Cc: "17.0" 
> >> Signed-off-by: Alex Smith 
> >> ---
> >> It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
> >> as far as I can see, and changing things so that they do also solves
> >> the problems that this patch fixes.
> >>
> >> However, I don't know what the exact consequences of doing so are, or
> >> whether there are any situations where that shouldn't be done, so I've
> >> gone with this fix instead as it seems like a safer option for now.
> >
> > Yeah we should be able to. I'm more comfortable sending this patch to
> > stable though, so this patch is
> >
> Bas, others,
>
> Patch addresses radv_{src,dst}_access_flush() which landed with commit
> 6dbb0eaccc3, after the 17.0 branchpoint.
>

Oops, my mistake.

I think radv_CmdPipelineBarrier on the 17.0 branch still needs
RADV_CMD_FLAG_INV_GLOBAL_L2
added for TRANSFER_WRITE barriers at least. Bas, do you think that should
be added in a separate patch just for stable, or would you prefer to push
those later changes to stable as well? Looks like there's some fixes in
those as well.

Alex


>
> From a quick look, the functions has been fixed/enhanced with the
> following 7 commits. Some of which require additional patches.
> If we want this in stable, please provide a backport or branch based
> on top of mesa-17.0.2.
>
> Thanks
> Emil
>
> 40e0dbf96c4d812be940561f5732b1b0e44b5e1d
> f3dc318464b786c2696e650e7c69984b5453624b
> b075eb7d476eb750092f72e7ec65bc41003fa658
> dd094e4ff9ff0967b515a4330e40feca55247e25
> 7a600bbc8186fef9475bedfe6d58a54011a8022b
> 0567ab0407e4058c108443b90b7c23a40c391c3b
> f92a118434452df201cda6d9ec2405aca669b104
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Emil Velikov]

On 22 March 2017 at 10:06, Bas Nieuwenhuizen  wrote:
> On Tue, Mar 21, 2017 at 1:02 PM, Alex Smith  
> wrote:
>> CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
>> through L2. Therefore, to make these writes visible to later accesses
>> we must invalidate L2 rather than just writing it back, to avoid the
>> possibility that stale data is read through L2.
>>
>> Cc: "17.0" 
>> Signed-off-by: Alex Smith 
>> ---
>> It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
>> as far as I can see, and changing things so that they do also solves
>> the problems that this patch fixes.
>>
>> However, I don't know what the exact consequences of doing so are, or
>> whether there are any situations where that shouldn't be done, so I've
>> gone with this fix instead as it seems like a safer option for now.
>
> Yeah we should be able to. I'm more comfortable sending this patch to
> stable though, so this patch is
>
Bas, others,

Patch addresses radv_{src,dst}_access_flush() which landed with commit
6dbb0eaccc3, after the 17.0 branchpoint.

From a quick look, the functions has been fixed/enhanced with the
following 7 commits. Some of which require additional patches.
If we want this in stable, please provide a backport or branch based
on top of mesa-17.0.2.

Thanks
Emil

40e0dbf96c4d812be940561f5732b1b0e44b5e1d
f3dc318464b786c2696e650e7c69984b5453624b
b075eb7d476eb750092f72e7ec65bc41003fa658
dd094e4ff9ff0967b515a4330e40feca55247e25
7a600bbc8186fef9475bedfe6d58a54011a8022b
0567ab0407e4058c108443b90b7c23a40c391c3b
f92a118434452df201cda6d9ec2405aca669b104
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Bas Nieuwenhuizen]

On Tue, Mar 21, 2017 at 1:02 PM, Alex Smith  wrote:
> CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
> through L2. Therefore, to make these writes visible to later accesses
> we must invalidate L2 rather than just writing it back, to avoid the
> possibility that stale data is read through L2.
>
> Cc: "17.0" 
> Signed-off-by: Alex Smith 
> ---
> It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
> as far as I can see, and changing things so that they do also solves
> the problems that this patch fixes.
>
> However, I don't know what the exact consequences of doing so are, or
> whether there are any situations where that shouldn't be done, so I've
> gone with this fix instead as it seems like a safer option for now.

Yeah we should be able to. I'm more comfortable sending this patch to
stable though, so this patch is

Reviewed-by: Bas Nieuwenhuizen 


> ---
>  src/amd/vulkan/radv_cmd_buffer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/src/amd/vulkan/radv_cmd_buffer.c 
> b/src/amd/vulkan/radv_cmd_buffer.c
> index ba192f3..d3397a0 100644
> --- a/src/amd/vulkan/radv_cmd_buffer.c
> +++ b/src/amd/vulkan/radv_cmd_buffer.c
> @@ -1477,7 +1477,7 @@ radv_src_access_flush(struct radv_cmd_buffer 
> *cmd_buffer,
>   RADV_CMD_FLAG_FLUSH_AND_INV_CB_META |
>   RADV_CMD_FLAG_FLUSH_AND_INV_DB |
>   RADV_CMD_FLAG_FLUSH_AND_INV_DB_META |
> - RADV_CMD_FLAG_WRITEBACK_GLOBAL_L2;
> + RADV_CMD_FLAG_INV_GLOBAL_L2;
> break;
> default:
> break;
> --
> 2.9.3
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

[Mesa-dev] [PATCH] radv: Invalidate L2 for TRANSFER_WRITE barriers

[Alex Smith]

CP DMA and PKT3_WRITE_DATA (in CmdUpdateBuffer) don't (currently) write
through L2. Therefore, to make these writes visible to later accesses
we must invalidate L2 rather than just writing it back, to avoid the
possibility that stale data is read through L2.

Cc: "17.0" 
Signed-off-by: Alex Smith 
---
It's possible for both CP DMA and PKT3_WRITE_DATA to write through L2
as far as I can see, and changing things so that they do also solves
the problems that this patch fixes.

However, I don't know what the exact consequences of doing so are, or
whether there are any situations where that shouldn't be done, so I've
gone with this fix instead as it seems like a safer option for now.
---
 src/amd/vulkan/radv_cmd_buffer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/amd/vulkan/radv_cmd_buffer.c b/src/amd/vulkan/radv_cmd_buffer.c
index ba192f3..d3397a0 100644
--- a/src/amd/vulkan/radv_cmd_buffer.c
+++ b/src/amd/vulkan/radv_cmd_buffer.c
@@ -1477,7 +1477,7 @@ radv_src_access_flush(struct radv_cmd_buffer *cmd_buffer,
  RADV_CMD_FLAG_FLUSH_AND_INV_CB_META |
  RADV_CMD_FLAG_FLUSH_AND_INV_DB |
  RADV_CMD_FLAG_FLUSH_AND_INV_DB_META |
- RADV_CMD_FLAG_WRITEBACK_GLOBAL_L2;
+ RADV_CMD_FLAG_INV_GLOBAL_L2;
break;
default:
break;
-- 
2.9.3

___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev