Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-25 Thread Aaron Watry 
On Wed, Apr 25, 2018 at 9:03 AM, Jan Vesely  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 
>> Cc: Francisco Jerez 
>> ---
>>  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;
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-25 Thread Jan Vesely 
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 
> Cc: Francisco Jerez 
> ---
>  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;


signature.asc
Description: This is a digitally signed message part
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-19 Thread Francisco Jerez 
Aaron Watry  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 
> Cc: Francisco Jerez 
> ---
>  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 

> + if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
> + (~flags & parent.flags() & host_access_flags))
>  throw error(CL_INVALID_VALUE);
>  
>   return flags;
> -- 
> 2.14.1


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

[Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-19 Thread Aaron Watry 
  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 
Cc: Francisco Jerez 
---
 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;
-- 
2.14.1

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

Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-16 Thread Francisco Jerez 
Aaron Watry  writes:

> On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez  wrote:
>
>> Aaron Watry  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
>> >
>>
>> What combination of flags is the test-case providing for both the
>> parent and sub buffer?
>>
>
> The original motivation for this was a CTS test that was creating a sub
> buffer with flags of:
> CL_MEM_HOST_NO_ACCESS | CL_MEM_READ_WRITE
>
> With a parent buffer created as:
> CL_MEM_HOST_READ_ONLY | CL_MEM_READ_WRITE
>
> Which according to my reading of the spec should be allowed.
>

Right, I see.

>>
>> > Signed-off-by: Aaron Watry 
>> > Cc: Francisco Jerez 
>> > ---
>> >  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..451e8a8c56 100644
>> > --- a/src/gallium/state_trackers/clover/api/memory.cpp
>> > +++ b/src/gallium/state_trackers/clover/api/memory.cpp
>> > @@ -57,10 +57,14 @@ 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);
>> >

I think you want to keep the hunk above and then do something along the
lines of:

+ if (!(flags & CL_MEM_HOST_NO_ACCESS) &&
+ (~flags & parent.flags() & host_access_flags))
+throw error(CL_INVALID_VALUE);

>> > + //Check if new host access flags cause a mismatch between
>> host-read/write-only.
>> > + const cl_mem_flags new_flags = flags & ~(parent.flags()) &
>> ~CL_MEM_HOST_NO_ACCESS;
>> > + if (new_flags & host_access_flags & parent.flags())
>> > +throw error (CL_INVALID_VALUE);
>> > +
>>
>> This doesn't look correct to me, the condition will always evaluate to
>> zero, you're calculating the conjunction of ~parent.flags() and
>> parent.flags() which is zero, so the error will never be emitted.
>>
>
> I'll see what I can do. I agree with a fresh reading that it looks fishy at
> best.
>
> --Aaron
>
>>
>> >   return flags;
>> >
>> >} else {
>> > --
>> > 2.14.1
>>


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

Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-16 Thread Aaron Watry 
On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez  wrote:

> Aaron Watry  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
> >
>
> What combination of flags is the test-case providing for both the
> parent and sub buffer?
>

The original motivation for this was a CTS test that was creating a sub
buffer with flags of:
CL_MEM_HOST_NO_ACCESS | CL_MEM_READ_WRITE

With a parent buffer created as:
CL_MEM_HOST_READ_ONLY | CL_MEM_READ_WRITE

Which according to my reading of the spec should be allowed.

>
> > Signed-off-by: Aaron Watry 
> > Cc: Francisco Jerez 
> > ---
> >  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..451e8a8c56 100644
> > --- a/src/gallium/state_trackers/clover/api/memory.cpp
> > +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> > @@ -57,10 +57,14 @@ 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.
> > + const cl_mem_flags new_flags = flags & ~(parent.flags()) &
> ~CL_MEM_HOST_NO_ACCESS;
> > + if (new_flags & host_access_flags & parent.flags())
> > +throw error (CL_INVALID_VALUE);
> > +
>
> This doesn't look correct to me, the condition will always evaluate to
> zero, you're calculating the conjunction of ~parent.flags() and
> parent.flags() which is zero, so the error will never be emitted.
>

I'll see what I can do. I agree with a fresh reading that it looks fishy at
best.

--Aaron

>
> >   return flags;
> >
> >} else {
> > --
> > 2.14.1
>
___
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Re: [Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-16 Thread Francisco Jerez 
Aaron Watry  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
>

What combination of flags is the test-case providing for both the
parent and sub buffer?

> Signed-off-by: Aaron Watry 
> Cc: Francisco Jerez 
> ---
>  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..451e8a8c56 100644
> --- a/src/gallium/state_trackers/clover/api/memory.cpp
> +++ b/src/gallium/state_trackers/clover/api/memory.cpp
> @@ -57,10 +57,14 @@ 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.
> + const cl_mem_flags new_flags = flags & ~(parent.flags()) & 
> ~CL_MEM_HOST_NO_ACCESS;
> + if (new_flags & host_access_flags & parent.flags())
> +throw error (CL_INVALID_VALUE);
> +

This doesn't look correct to me, the condition will always evaluate to
zero, you're calculating the conjunction of ~parent.flags() and
parent.flags() which is zero, so the error will never be emitted.

>   return flags;
>  
>} else {
> -- 
> 2.14.1


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

[Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation

2018-04-07 Thread Aaron Watry 
  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

Signed-off-by: Aaron Watry 
Cc: Francisco Jerez 
---
 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..451e8a8c56 100644
--- a/src/gallium/state_trackers/clover/api/memory.cpp
+++ b/src/gallium/state_trackers/clover/api/memory.cpp
@@ -57,10 +57,14 @@ 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.
+ const cl_mem_flags new_flags = flags & ~(parent.flags()) & 
~CL_MEM_HOST_NO_ACCESS;
+ if (new_flags & host_access_flags & parent.flags())
+throw error (CL_INVALID_VALUE);
+
  return flags;
 
   } else {
-- 
2.14.1

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