Added SyncToken command buffer trait to help with IPC messages.

Added SyncToken command buffer trait to help with IPC messages.

R=piman@chromium.org
BUG=514815
Committed: https://crrev.com/6a219f8ccdd5148f28458b8184f31859babb9848
Cr-Commit-Position: refs/heads/master@{#353170}

[David Yen, 2015-10-07 23:30:57]

As discussed before (albeit awhile ago), add SyncToken as a command buffer trait
to make IPCing the sync token easier.

Eventually we will probably also have to replace the MailboxHolder to contain
the SyncToken instead of the old 32 bit sync_point, but that will probably have
to be part of the change when we convert all mailboxes to use the new fence
syncs.

[piman, 2015-10-08 22:57:11]

lgtm

[David Yen, 2015-10-08 23:13:07]

The CQ bit was checked by dyen@chromium.org

[commit-bot: I haz the power, 2015-10-08 23:15:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1394543003/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1394543003/1

[commit-bot: I haz the power, 2015-10-08 23:23:50]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-10-08 23:24:28]

Patchset 1 (id:??) landed as
https://crrev.com/6a219f8ccdd5148f28458b8184f31859babb9848
Cr-Commit-Position: refs/heads/master@{#353170}

[jbudorick, 2015-10-10 00:34:45]

A revert of this CL (patchset #1 id:1) has been created in
https://codereview.chromium.org/1386323004/ by jbudorick@chromium.org.

The reason for reverting is: Broke gpu_unittests on Android. e.g. from the
tombstones for this CL's run:

I   42.987s Main  signal 7 (SIGBUS) at 0x7569c97a (code=1), thread 15602
(st:test_process)
I   42.987s Main  
I   42.987s Main  tombstone_06 created on 2015-10-08 01:00:00, about this long
ago: 0:11:29 Device: 03d130ac006adc26
I   42.987s Main  Reading native crash info from stdin
I   42.987s Main  Reading Android symbols from
/b/build/slave/android/build/src/third_party/android_platform/development/scripts/../../../..
I   42.987s Main  Reading Chrome symbols from
/b/build/slave/android/build/src/third_party/android_platform/development/scripts/../../../..
I   42.987s Main  Using toolchain from
:/b/build/slave/android/build/src/third_party/android_platform/development/scripts/../../../../third_party/android_tools/ndk/toolchains/arm-linux-androideabi-4.9/prebuilt/linux-x86_64/bin/arm-linux-androideabi-
I   42.987s Main  pid: 15669, tid: 15669, name: st:test_process  >>>
org.chromium.native_test:test_process <<<
I   42.987s Main  signal 7 (SIGBUS), code 1 (BUS_ADRALN), fault addr 7569c97a
I   42.987s Main      r0 7569c972  r1 00000000  r2 00000000  r3 00000000
I   42.987s Main      r4 7569c972  r5 00000000  r6 0000007b  r7 00000000
I   42.987s Main      r8 00000052  r9 4004e678  sl 00000001  fp 400b2384
I   42.987s Main      ip 00000000  sp bef25480  lr 75441b59  pc 75441b5c
I   42.987s Main  
I   42.987s Main  Stack Trace:
I   42.987s Main    RELADDR   FUNCTION                                          
                                             FILE:LINE
I   42.987s Main    00329b5c 
gpu::SyncToken::SyncToken(gpu::CommandBufferNamespace, unsigned long long,
unsigned long long) 
/data/app-lib/org.chromium.native_test-1/libgpu_unittests.so
I   42.987s Main    00077699  __static_initialization_and_destruction_0(int,
int) [clone .constprop.176]                     
/data/app-lib/org.chromium.native_test-1/libgpu_unittests.so.

[piman, 2015-10-10 00:40:55]

https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
File gpu/command_buffer/common/sync_token.cc (right):

https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
gpu/command_buffer/common/sync_token.cc:66:
reinterpret_cast<SyncTokenInternal*>(data);
I think because SyncToken::data is not guaranteed to be aligned, this
reinterpret_cast is iffy and that's what's causing the problem.

It seems like we should simply list the members in SyncToken (if needed, in the
private section), and maybe provide a data() accessor that does the
reinterpret_cast the opposite way, into char*.

[David Yen, 2015-10-10 00:45:49]

On 2015/10/10 00:40:55, piman (slow to review) wrote:
>
https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
> File gpu/command_buffer/common/sync_token.cc (right):
> 
>
https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
> gpu/command_buffer/common/sync_token.cc:66:
> reinterpret_cast<SyncTokenInternal*>(data);
> I think because SyncToken::data is not guaranteed to be aligned, this
> reinterpret_cast is iffy and that's what's causing the problem.
> 
> It seems like we should simply list the members in SyncToken (if needed, in
the
> private section), and maybe provide a data() accessor that does the
> reinterpret_cast the opposite way, into char*.

Since GenSyncTokenCHROMIUM takes a GLByte* of size GL_SYNC_TOKEN_SIZE_CHROMIUM
we cannot guarantee
that the buffer is aligned even if a SyncToken structure is. I'm thinking we
should make GL_SYNC_TOKEN_SIZE_CHROMIUM big enough to accomodate "SyncToken +
alignof(SyncToken) - 1" and have the SyncToken data start at the correct
alignment. Does that sound reasonable?

Although in practice we will probably have to use sizeof(uint64_t) instead of
alignof since that is c++11 only.

[piman, 2015-10-10 00:51:45]

On Fri, Oct 9, 2015 at 5:45 PM, <dyen@chromium.org> wrote:

> On 2015/10/10 00:40:55, piman (slow to review) wrote:
>
>
>
https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
>
>> File gpu/command_buffer/common/sync_token.cc (right):
>>
>
>
>
>
https://codereview.chromium.org/1394543003/diff/1/gpu/command_buffer/common/s...
>
>> gpu/command_buffer/common/sync_token.cc:66:
>> reinterpret_cast<SyncTokenInternal*>(data);
>> I think because SyncToken::data is not guaranteed to be aligned, this
>> reinterpret_cast is iffy and that's what's causing the problem.
>>
>
> It seems like we should simply list the members in SyncToken (if needed, in
>>
> the
>
>> private section), and maybe provide a data() accessor that does the
>> reinterpret_cast the opposite way, into char*.
>>
>
> Since GenSyncTokenCHROMIUM takes a GLByte* of size
> GL_SYNC_TOKEN_SIZE_CHROMIUM
> we cannot guarantee
> that the buffer is aligned even if a SyncToken structure is. I'm thinking
> we
> should make GL_SYNC_TOKEN_SIZE_CHROMIUM big enough to accomodate
> "SyncToken +
> alignof(SyncToken) - 1" and have the SyncToken data start at the correct
> alignment. Does that sound reasonable?
>

Not really, you don't want to interpret the GLbyte array differently
depending on the alignment.
For GenSyncTokenCHROMIUM, you can simply memcpy the SyncToken into the
users's byte array, then alignment doesn't matter.


> Although in practice we will probably have to use sizeof(uint64_t) instead
> of
> alignof since that is c++11 only.
>
> https://codereview.chromium.org/1394543003/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.