Last Comment Bug 738865 - Use Small tiles should not be android only.
: Use Small tiles should not be android only.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: ARM Linux
: -- normal (vote)
: mozilla14
Assigned To: Oleg Romashin (:romaxa)
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-03-23 17:22 PDT by Oleg Romashin (:romaxa)
Modified: 2012-11-10 11:15 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
More generic sub texture upload detection (3.13 KB, patch)
2012-03-23 17:22 PDT, Oleg Romashin (:romaxa)
chrislord.net: review+
b56girard: review-
pwalton: feedback+
Details | Diff | Splinter Review
Use small tiles not only on android (1.45 KB, patch)
2012-03-24 11:10 PDT, Oleg Romashin (:romaxa)
b56girard: review+
Details | Diff | Splinter Review

Description Oleg Romashin (:romaxa) 2012-03-23 17:22:15 PDT
Created attachment 608928 [details] [diff] [review]
More generic sub texture upload detection

I was playing with b2g on beagle board, and tried to figure out why it is fast on S Galaxy II and slow in my case...
by checking vendors/renderers tricks I found that using small tiles and avoid subTexture upload makes rendering as on beagleboard as fast as on S Galaxy II... because powervr SGX 530 there has similar problem.

I'm wondering can we make that check more generic?

with this fix, I have FPS increase for http://bubblemark.com/dhtml.htm from 10FPS->45FPS
Comment 1 Patrick Walton (:pcwalton) 2012-03-23 17:24:02 PDT
Comment on attachment 608928 [details] [diff] [review]
More generic sub texture upload detection

Looks fine to me, but Chris should review this.
Comment 2 Chris Lord [:cwiiis] 2012-03-24 03:56:01 PDT
Comment on attachment 608928 [details] [diff] [review]
More generic sub texture upload detection

Review of attachment 608928 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the function renamed, but I think BenWa should review this really (sorry for the ping-pong).

::: gfx/gl/GLContext.cpp
@@ +618,5 @@
>          return false;
>  
>      // Don't use small tiles otherwise. (If we implement incremental texture upload,
>      // then we will want to revisit this.)
>      return false;

whoops, we have implemented incremental texture upload, we should probably review this...

::: gfx/gl/GLContext.h
@@ +712,5 @@
>  
>      bool CanUploadSubTextures();
>      bool CanUploadNonPowerOfTwo();
>      bool WantsSmallTiles();
> +    virtual bool SupportRenderToTexture() { return true; }

This doesn't return whether render-to-texture is available, but whether fast texture upload is available. It should be renamed to reflect what it does, and it should return false by default.

Perhaps HasFastDirectUpdate() instead?
Comment 3 Benoit Girard (:BenWa) 2012-03-24 07:07:45 PDT
Comment on attachment 608928 [details] [diff] [review]
More generic sub texture upload detection

Review of attachment 608928 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/gl/GLContext.cpp
@@ +578,5 @@
>  
>      // On PowerVR glTexSubImage does a readback, so it will be slower
>      // than just doing a glTexImage2D() directly. i.e. 26ms vs 10ms
>      if (Renderer() == RendererSGX540 || Renderer() == RendererSGX530)
> +        return SupportRenderToTexture();

I don't want to make a change like until we some performance number, and ideally a small benchmark hack patch and/or some details on how it measure. We've made several assumptions in the pass about performance and checking them after a change is made is very tedious.

I'd be happy to take this patch without this change until we get these numbers.
Comment 4 Oleg Romashin (:romaxa) 2012-03-24 10:40:56 PDT
perf numbers are in first comment, also there are perf numbers in right above that check:
>// than just doing a glTexImage2D() directly. i.e. 26ms vs 10ms
Direct texture rendering with RendererSGX530 supported only on N9... all other devices having same problem as android
Comment 5 Oleg Romashin (:romaxa) 2012-03-24 11:10:23 PDT
Created attachment 609019 [details] [diff] [review]
Use small tiles not only on android
Comment 6 Benoit Girard (:BenWa) 2012-03-24 11:59:14 PDT
Comment on attachment 609019 [details] [diff] [review]
Use small tiles not only on android

As discussed on IRC I was asking for perf numbers for KHRLock, feel free to submit a patch for that if we have evidence that it is faster.
Comment 7 Ryan VanderMeulen [:RyanVM] 2012-03-24 15:09:37 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c14032c09e1
Comment 8 Ed Morley [:emorley] 2012-03-25 06:12:06 PDT
https://hg.mozilla.org/mozilla-central/rev/2c14032c09e1

Note You need to log in before you can comment on or make changes to this bug.