[Mesa-dev] [PATCH] clover: Fix host access validation for sub-buffer creation
Francisco Jerez
currojerez at riseup.net
Tue Apr 17 00:30:01 UTC 2018
Aaron Watry <awatry at gmail.com> writes:
> On Mon, Apr 16, 2018, 5:24 PM Francisco Jerez <currojerez at riseup.net> wrote:
>
>> Aaron Watry <awatry at 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
>> >
>>
>> 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 <awatry at gmail.com>
>> > Cc: Francisco Jerez <currojerez at 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..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
>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 227 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/mesa-dev/attachments/20180416/2b992991/attachment.sig>
More information about the mesa-dev
mailing list