clover: Fix host access validation for sub-buffer creation

Submitted by Aaron Watry on April 20, 2018, 1:39 a.m.

Details

Message ID 20180420013936.21037-1-awatry@gmail.com
State New
Headers show
Series "clover: Fix host access validation for sub-buffer creation" ( rev: 2 ) in Mesa

Not browsing as part of any series.

Commit Message

Aaron Watry April 20, 2018, 1:39 a.m.
From CL 1.2 Section 5.2.1:
    CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
    flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
    CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
    buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
    CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .

Fixes CL 1.2 CTS test/api get_buffer_info

v2: Correct host_access_flags check (Francisco)

Signed-off-by: Aaron Watry <awatry@gmail.com>
Cc: Francisco Jerez <currojerez@riseup.net>
---
 src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Patch hide | download patch | download mbox

diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
index 9b3cd8b1f5..e83be0286a 100644
--- a/src/gallium/state_trackers/clover/api/memory.cpp
+++ b/src/gallium/state_trackers/clover/api/memory.cpp
@@ -57,8 +57,12 @@  namespace {
                                       parent.flags() & host_access_flags) |
                                      (parent.flags() & host_ptr_flags));
 
-         if (~flags & parent.flags() &
-             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
+         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
+            throw error(CL_INVALID_VALUE);
+
+         //Check if new host access flags cause a mismatch between host-read/write-only.
+         if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
+             (~flags & parent.flags() & host_access_flags))
             throw error(CL_INVALID_VALUE);
 
          return flags;

Comments

Aaron Watry <awatry@gmail.com> writes:

>   From CL 1.2 Section 5.2.1:
>     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
>     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
>     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
>     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
>     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
>
> Fixes CL 1.2 CTS test/api get_buffer_info
>
> v2: Correct host_access_flags check (Francisco)
>
> Signed-off-by: Aaron Watry <awatry@gmail.com>
> Cc: Francisco Jerez <currojerez@riseup.net>
> ---
>  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
> index 9b3cd8b1f5..e83be0286a 100644
> --- a/src/gallium/state_trackers/clover/api/memory.cpp
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> @@ -57,8 +57,12 @@ namespace {
>                                        parent.flags() & host_access_flags) |
>                                       (parent.flags() & host_ptr_flags));
>  
> -         if (~flags & parent.flags() &
> -             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
> +         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
> +            throw error(CL_INVALID_VALUE);
> +
> +         //Check if new host access flags cause a mismatch between host-read/write-only.

Space between '//' and comment, and limit to 80 columns.  With that
fixed:

Reviewed-by: Francisco Jerez <currojerez@riseup.net>

> +         if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
> +             (~flags & parent.flags() & host_access_flags))
>              throw error(CL_INVALID_VALUE);
>  
>           return flags;
> -- 
> 2.14.1
On Thu, 2018-04-19 at 20:39 -0500, Aaron Watry wrote:
>   From CL 1.2 Section 5.2.1:
>     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
>     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
>     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
>     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
>     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
> 
> Fixes CL 1.2 CTS test/api get_buffer_info

Hi Aaron,

there are similar failures in test/mem_host_flags:

test_mem_host_write_only_buffer_RW_Mapping
Mapped host pointer difference found
ERROR: test_mem_host_write_only_buffer_RW_Mapping! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:267)
ERROR: test_mem_host_write_only_buffer! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:295)
test_mem_host_write_only_buffer FAILED

test_mem_host_write_only_buffer_RW_Mapping
Mapped host pointer difference found
ERROR: test_mem_host_write_only_buffer_RW_Mapping! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:267)
ERROR: test_mem_host_write_only_subbuffer! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:328)
test_mem_host_write_only_subbuffer FAILED

...
FAILED 2 of 9 tests

Are you looking into those as well?

thanks,
Jan

> 
> v2: Correct host_access_flags check (Francisco)
> 
> Signed-off-by: Aaron Watry <awatry@gmail.com>
> Cc: Francisco Jerez <currojerez@riseup.net>
> ---
>  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
> index 9b3cd8b1f5..e83be0286a 100644
> --- a/src/gallium/state_trackers/clover/api/memory.cpp
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> @@ -57,8 +57,12 @@ namespace {
>                                        parent.flags() & host_access_flags) |
>                                       (parent.flags() & host_ptr_flags));
>  
> -         if (~flags & parent.flags() &
> -             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
> +         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
> +            throw error(CL_INVALID_VALUE);
> +
> +         //Check if new host access flags cause a mismatch between host-read/write-only.
> +         if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
> +             (~flags & parent.flags() & host_access_flags))
>              throw error(CL_INVALID_VALUE);
>  
>           return flags;
On Wed, Apr 25, 2018 at 9:03 AM, Jan Vesely <jan.vesely@rutgers.edu> wrote:
> On Thu, 2018-04-19 at 20:39 -0500, Aaron Watry wrote:
>>   From CL 1.2 Section 5.2.1:
>>     CL_INVALID_VALUE if buffer was created with CL_MEM_HOST_WRITE_ONLY and
>>     flags specify CL_MEM_HOST_READ_ONLY , or if buffer was created with
>>     CL_MEM_HOST_READ_ONLY and flags specify CL_MEM_HOST_WRITE_ONLY , or if
>>     buffer was created with CL_MEM_HOST_NO_ACCESS and flags specify
>>     CL_MEM_HOST_READ_ONLY or CL_MEM_HOST_WRITE_ONLY .
>>
>> Fixes CL 1.2 CTS test/api get_buffer_info
>
> Hi Aaron,
>
> there are similar failures in test/mem_host_flags:
>
> test_mem_host_write_only_buffer_RW_Mapping
> Mapped host pointer difference found
> ERROR: test_mem_host_write_only_buffer_RW_Mapping! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:267)
> ERROR: test_mem_host_write_only_buffer! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:295)
> test_mem_host_write_only_buffer FAILED
>
> test_mem_host_write_only_buffer_RW_Mapping
> Mapped host pointer difference found
> ERROR: test_mem_host_write_only_buffer_RW_Mapping! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:267)
> ERROR: test_mem_host_write_only_subbuffer! ((unknown) from /home/jvesely/OpenCL-CTS/test_conformance/mem_host_flags/mem_host_buffer.cpp:328)
> test_mem_host_write_only_subbuffer FAILED
>
> ...
> FAILED 2 of 9 tests
>
> Are you looking into those as well?

Thanks for making me aware of that one.  I hadn't been looking into it.

The next thing I had been trying to look into was issues with kernel
attributes not being available after compilation for
clGetKernelWorkgroupInfo when running the API test-group.

Your error looks potentially simpler to solve with possibly less
interference with Pierre/Karol's work, so maybe I'll look into that
instead.

For reference, the one I had been looking at was in test_api:

kernel_required_group_size...
Device reported CL_DEVICE_MAX_WORK_ITEM_DIMENSIONS = 3.
The CL_KERNEL_WORK_GROUP_SIZE for the kernel is 256.
For global dimension 64 x 14 x 10, kernel will require local dimension
64 x 2 x 2.
ERROR: Incorrect compile work group size returned for specified size!
(returned 0,0,0, expected 64,2,2)
kernel_required_group_size FAILED

--Aaron

>
> thanks,
> Jan
>
>>
>> v2: Correct host_access_flags check (Francisco)
>>
>> Signed-off-by: Aaron Watry <awatry@gmail.com>
>> Cc: Francisco Jerez <currojerez@riseup.net>
>> ---
>>  src/gallium/state_trackers/clover/api/memory.cpp | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/src/gallium/state_trackers/clover/api/memory.cpp b/src/gallium/state_trackers/clover/api/memory.cpp
>> index 9b3cd8b1f5..e83be0286a 100644
>> --- a/src/gallium/state_trackers/clover/api/memory.cpp
>> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
>> @@ -57,8 +57,12 @@ namespace {
>>                                        parent.flags() & host_access_flags) |
>>                                       (parent.flags() & host_ptr_flags));
>>
>> -         if (~flags & parent.flags() &
>> -             ((dev_access_flags & ~CL_MEM_READ_WRITE) | host_access_flags))
>> +         if (~flags & parent.flags() & (dev_access_flags & ~CL_MEM_READ_WRITE))
>> +            throw error(CL_INVALID_VALUE);
>> +
>> +         //Check if new host access flags cause a mismatch between host-read/write-only.
>> +         if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
>> +             (~flags & parent.flags() & host_access_flags))
>>              throw error(CL_INVALID_VALUE);
>>
>>           return flags;