Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
12461002:
android: Add basic support for Broadcom GPUs (Closed)
|
Created:
7 years, 9 months ago by Sami Modified:
7 years, 7 months ago CC:
chromium-reviews, joi+watch-content_chromium.org, darin-cc_chromium.org, jam, apatrick_chromium Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Descriptionandroid: Add basic support for Broadcom GPUs
This patch implements two GPU workarounds to make Chrome work on
devices with a Broadcom GPU:
1. Enable context virtualization on to avoid a EGL_BAD_CONTEXT failure
from eglCreateContext().
2. Disable asynchronous texture uploads to avoid an "Invalid image or
UNSUPPORTED OPERATION" failure from glEGLImageTargetTexture2DOES.
These modifications were tested on a Samsung Galaxy Fame S6810P.
BUG=179815
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=202062
Patch Set 1 #
Total comments: 6
Patch Set 2 : Rebased. #Patch Set 3 : Fix unit test failures. #
Total comments: 1
Patch Set 4 : Clean up ifdef #
Messages
Total messages: 18 (0 generated)
Here's what we need to get going on Broadcom-based devices. The issue about the bogus share handle (see bug) is odd, but I think we generally do want to use virtual contexts everywhere on Android.
lgtm
LGTM https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (left): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:442: context->HasExtension("EGL_KHR_gl_texture_2D_image") && It's pretty annoying that they report these extensions but yet still fail. I'm curious if it could be made to work somehow if they report all these. https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:441: bool is_broadcom = vendor.find("Broadcom") != std::string::npos; Can you explain the reason a bit in the comment?
https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:441: bool is_broadcom = vendor.find("Broadcom") != std::string::npos; You may find that this fails tests due to querying GL_VENDOR. If so, put it in a function called IsBroadcom, and call it below (that way it will never execute in a test since the other extensions aren't exposed in tests).
Thanks guys. Based on the discussion on the bug I'm going to hold off landing this. Seems that later Broadcom models don't have these bugs and turning off async uploads would just penalize performance. How would you feel about flipping the virtual GL context bit into a blacklist instead of whitelist? We basically want it enabled everywhere on Android, right? https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (left): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:442: context->HasExtension("EGL_KHR_gl_texture_2D_image") && On 2013/03/08 22:44:24, epenner wrote: > It's pretty annoying that they report these extensions but yet still fail. I'm > curious if it could be made to work somehow if they report all these. I guess we might just be hitting and unsupported EGLimage format. I think it's technically valid to expose these and support no formats at all :) https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (right): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:441: bool is_broadcom = vendor.find("Broadcom") != std::string::npos; On 2013/03/08 22:47:31, epenner wrote: > You may find that this fails tests due to querying GL_VENDOR. If so, put it in a > function called IsBroadcom, and call it below (that way it will never execute in > a test since the other extensions aren't exposed in tests). Thanks for the tip.
https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... File ui/gl/async_pixel_transfer_delegate_android.cc (left): https://codereview.chromium.org/12461002/diff/1/ui/gl/async_pixel_transfer_de... ui/gl/async_pixel_transfer_delegate_android.cc:442: context->HasExtension("EGL_KHR_gl_texture_2D_image") && On 2013/03/11 10:45:26, Sami wrote: > On 2013/03/08 22:44:24, epenner wrote: > > It's pretty annoying that they report these extensions but yet still fail. I'm > > curious if it could be made to work somehow if they report all these. > > I guess we might just be hitting and unsupported EGLimage format. I think it's > technically valid to expose these and support no formats at all :) That's a good point! If that's the case we could possibly test this generically, by creating a test texture of a typical format(s), and then limit our own usage to those formats that we check.
Since you are resurrecting this, note https://codereview.chromium.org/15231003/, so you'll have to rebase :)
On 2013/05/22 16:10:07, Daniel Sievers wrote: > Since you are resurrecting this, note https://codereview.chromium.org/15231003/, > so you'll have to rebase :) Yes, I noticed things have changed a bit :) Still, this patch is pretty much the same as before.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/12461002/10001
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/12461002/24001
https://chromiumcodereview.appspot.com/12461002/diff/24001/gpu/command_buffer... File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): https://chromiumcodereview.appspot.com/12461002/diff/24001/gpu/command_buffer... gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:239: #if defined(OS_ANDROID) You can get rid of this by making a function IsQualcomm() and calling that after checking it at the end of the "if" statement. Then the function never gets called in tests.
On 2013/05/23 18:13:42, epenner wrote: > https://chromiumcodereview.appspot.com/12461002/diff/24001/gpu/command_buffer... > File gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc (right): > > https://chromiumcodereview.appspot.com/12461002/diff/24001/gpu/command_buffer... > gpu/command_buffer/service/gles2_cmd_decoder_unittest_base.cc:239: #if > defined(OS_ANDROID) > You can get rid of this by making a function IsQualcomm() and calling that after > checking it at the end of the "if" statement. Then the function never gets > called in tests. LGTM. I removed the commit flag to cleanup the #ifdef. You can land anyway though if you want and I'll clean it up when I make async use work-around flags (now that the code is in GPU).
On 2013/05/23 18:17:32, epenner wrote: > LGTM. I removed the commit flag to cleanup the #ifdef. You can land anyway > though if you want and I'll clean it up when I make async use work-around flags > (now that the code is in GPU). Thanks for the suggestion, it's a lot nicer this way.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/12461002/45001
Retried try job too often on win_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/skyostil@chromium.org/12461002/45001
Message was sent while issue was closed.
Change committed as 202062 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
