Ensure memory mapped chunk size is aligned with the FencedAllocator.

CompositorImplAndroid sets a texture size that may not be aligned FencedAllocator allocation size.

This CL ensures chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2623223002
Cr-Commit-Position: refs/heads/master@{#443435}
Committed: https://chromium.googlesource.com/chromium/src/+/e7225c311f5be2450e84dc6f8e0548a699787bb3

[mthiesse, 2017-01-11 16:25:25]

mthiesse@chromium.org changed reviewers:
+ boliu@chromium.org

[mthiesse, 2017-01-11 16:25:25]

PTAL. I'm not at all sure that this is the right approach to fix this.

However, it seems like pure fluke that we haven't hit this CHECK on any
real-world displays, as there really isn't any guarantee that their sizes will
align with the allocator chunk size.

[mthiesse, 2017-01-11 16:26:01]

The CQ bit was checked by mthiesse@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-11 16:26:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 17:05:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 17:05:39]

Dry run: This issue passed the CQ dry run.

[boliu, 2017-01-11 18:12:36]

I'm unable to reproduce this crash. What's the symbolized stack?

If stack goes through here, then this alignment can happen here as well:
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...

Also can it use base::bits::Align?

[mthiesse, 2017-01-11 20:00:49]

On 2017/01/11 18:12:36, boliu wrote:
> I'm unable to reproduce this crash. What's the symbolized stack?
> 
> If stack goes through here, then this alignment can happen here as well:
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...
> 
> Also can it use base::bits::Align?

Symbolized trace is as follows:

signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
[FATAL:mapped_memory.h(142)] Check failed: multiple %
FencedAllocator::kAllocAlignment == 0. 

Stack Trace:
  RELADDR   FUNCTION                                                            
                                                                   FILE:LINE
  0009d5a7  logging::LogMessage::~LogMessage()                                  
                                                                  
~/src/clank/src/base/logging.cc:537
  v------>  gpu::MappedMemoryManager::set_chunk_size_multiple(unsigned int)     
                                                                  
~/src/clank/src/gpu/command_buffer/client/mapped_memory.h:142
  0004b6b3  gpu::gles2::GLES2Implementation::Initialize(unsigned int, unsigned
int, unsigned int, unsigned int)                                    
~/src/clank/src/gpu/command_buffer/client/gles2_implementation.cc:223
  00a436d3  ui::ContextProviderCommandBuffer::BindToCurrentThread()             
                                                                  
~/src/clank/src/services/ui/public/cpp/gpu/context_provider_command_buffer.cc:290
  00a062e7 
content::CompositorImpl::OnGpuChannelEstablished(scoped_refptr<gpu::GpuChannelHost>,
ui::ContextProviderFactory::GpuChannelHostResult) 
~/src/clank/src/content/browser/renderer_host/compositor_impl_android.cc:615

[boliu, 2017-01-11 20:04:46]

On 2017/01/11 20:00:49, mthiesse wrote:
> On 2017/01/11 18:12:36, boliu wrote:
> > I'm unable to reproduce this crash. What's the symbolized stack?
> > 
> > If stack goes through here, then this alignment can happen here as well:
> >
>
https://cs.chromium.org/chromium/src/gpu/command_buffer/client/gles2_implemen...
> > 
> > Also can it use base::bits::Align?
> 
> Symbolized trace is as follows:
> 
> signal 6 (SIGABRT), code -6 (SI_TKILL), fault addr --------
> [FATAL:mapped_memory.h(142)] Check failed: multiple %
> FencedAllocator::kAllocAlignment == 0. 
> 
> Stack Trace:
>   RELADDR   FUNCTION                                                          
 
>                                                                    FILE:LINE
>   0009d5a7  logging::LogMessage::~LogMessage()                                
 
>                                                                   
> ~/src/clank/src/base/logging.cc:537
>   v------>  gpu::MappedMemoryManager::set_chunk_size_multiple(unsigned int)   
 
>                                                                   
> ~/src/clank/src/gpu/command_buffer/client/mapped_memory.h:142
>   0004b6b3  gpu::gles2::GLES2Implementation::Initialize(unsigned int, unsigned
> int, unsigned int, unsigned int)                                    
> ~/src/clank/src/gpu/command_buffer/client/gles2_implementation.cc:223
>   00a436d3  ui::ContextProviderCommandBuffer::BindToCurrentThread()           
 
>                                                                   
>
~/src/clank/src/services/ui/public/cpp/gpu/context_provider_command_buffer.cc:290
>   00a062e7 
>
content::CompositorImpl::OnGpuChannelEstablished(scoped_refptr<gpu::GpuChannelHost>,
> ui::ContextProviderFactory::GpuChannelHostResult) 
> ~/src/clank/src/content/browser/renderer_host/compositor_impl_android.cc:615

yep, put this in GLES2Implementation::Initialize then. afaict there is no
requirement that mapped_memory_limit needs to be aligned or a multiple of
anything.

[mthiesse, 2017-01-11 20:11:53]

Description was changed from

==========
Ensure that CompositorImplAndroid sets a texture size aligned with the
FencedAllocator chunk size.

This CL increases the full screen texture size to align with a FencedAllocator
chunk boundary if it doesn't already.

BUG=680150
==========

to

==========
Ensure that CompositorImplAndroid sets a texture size aligned with the
FencedAllocator chunk size.

This CL increases the full screen texture size to align with a FencedAllocator
chunk boundary if it doesn't already.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[mthiesse, 2017-01-11 20:13:35]

Description was changed from

==========
Ensure that CompositorImplAndroid sets a texture size aligned with the
FencedAllocator chunk size.

This CL increases the full screen texture size to align with a FencedAllocator
chunk boundary if it doesn't already.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
Ensure that CompositorImplAndroid sets a texture size aligned with the
FencedAllocator chunk size.

This CL increases the full screen texture size to align with a FencedAllocator
chunk boundary if it doesn't already.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[mthiesse, 2017-01-11 20:14:23]

mthiesse@chromium.org changed reviewers:
+ jbauman@chromium.org

[mthiesse, 2017-01-11 20:14:23]

jbauman@chromium.org PTAL

[mthiesse, 2017-01-11 20:15:18]

Description was changed from

==========
Ensure that CompositorImplAndroid sets a texture size aligned with the
FencedAllocator chunk size.

This CL increases the full screen texture size to align with a FencedAllocator
chunk boundary if it doesn't already.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
CompositorImplAndroid sets a texture size that may not be aligned
FencedAllocator allocation size.

This CL ensure chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[mthiesse, 2017-01-11 20:15:29]

Description was changed from

==========
CompositorImplAndroid sets a texture size that may not be aligned
FencedAllocator allocation size.

This CL ensure chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
CompositorImplAndroid sets a texture size that may not be aligned
FencedAllocator allocation size.

This CL ensures chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

[mthiesse, 2017-01-12 21:40:02]

On 2017/01/11 20:14:23, mthiesse wrote:
> mailto:jbauman@chromium.org PTAL

Ping jbauman@, speedy review would be greatly appreciated as this fixes a crash
and unblocks development of VrShell. The CL that caused the crash is non-trivial
to revert, and given that the feature is still behind a flag we'd rather just
fix the issue and avoid having to revert the original CL.

[jbauman, 2017-01-12 22:23:11]

lgtm

[mthiesse, 2017-01-12 22:23:50]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-12 22:24:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-12 23:59:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-13 00:00:00]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mthiesse, 2017-01-13 00:06:00]

The CQ bit was checked by mthiesse@chromium.org

[commit-bot: I haz the power, 2017-01-13 00:07:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-13 01:28:40]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1484265960675740,
"parent_rev": "1a5d0781a13fd862a763869bf8852426566d879c", "commit_rev":
"e7225c311f5be2450e84dc6f8e0548a699787bb3"}

[commit-bot: I haz the power, 2017-01-13 01:29:12]

Description was changed from

==========
CompositorImplAndroid sets a texture size that may not be aligned
FencedAllocator allocation size.

This CL ensures chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
==========

to

==========
CompositorImplAndroid sets a texture size that may not be aligned
FencedAllocator allocation size.

This CL ensures chunk size is aligned with the FencedAllocator allocation size.

BUG=680150
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel

Review-Url: https://codereview.chromium.org/2623223002
Cr-Commit-Position: refs/heads/master@{#443435}
Committed:
https://chromium.googlesource.com/chromium/src/+/e7225c311f5be2450e84dc6f8e05...
==========

[commit-bot: I haz the power, 2017-01-13 01:29:16]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/e7225c311f5be2450e84dc6f8e05...