Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
15719007:
Make GrGLConfigConversionEffect work on Intel GPUs (Closed)
|
Created:
7 years, 6 months ago by Jun Jiang Modified:
7 years, 6 months ago CC:
skia-review_googlegroups.com Base URL:
https://skia.googlecode.com/svn/trunk Visibility:
Public. |
DescriptionMake GrGLConfigConversionEffect work on Intel GPUs
BUG=247664
Committed: http://code.google.com/p/skia/source/detail?r=9672
Patch Set 1 #Patch Set 2 : Simplity the shader #Messages
Total messages: 14 (0 generated)
The GrGLConfigConversionEffect can not work on Intel GPUs(Intel Graphics HD3000 and HD4000),which leads to that all the PM <->UPM conversion and R & B Swap in GPU backend of Canvas2D fall back to CPU side. This patch can make it work on Intel GPUs and won't fall back to CPU side, which was verified on Linux and Windows.
Can you explain *why* the conversion effect as written doesn't work on Intel GPUs? This looks like it adds quite a bit of complexity to the shader on all platforms, and we should be shrinking and simplifying where we can.
On 2013/06/07 14:57:55, Tom Hudson wrote:
> Can you explain *why* the conversion effect as written doesn't work on Intel
> GPUs?
> This looks like it adds quite a bit of complexity to the shader on all
> platforms, and we should be shrinking and simplifying where we can.
Hi, Hudson. Thanks for your comments.
I found On Intel GPUs, floor(output_Stage0.a{r,g,b} * 255.0) is not equal to
integer version of output_Stage0.a{r,g,b} when the integer notation of a, r, g,
b is 2^n(1, 2, 4, 8, 16,32, etc), where output_Stage0 is sampled from a 2D
texture. Specially, it is less than 1 compared to the corresponding a, r, g, b
which is 2^n.
For example, if the integer version of output_Stage0 is (r, g, b, a) = (4, 4, 4,
255), the premultiplied result is (3, 3, 3, 255).
I think there may be some precision difference with the internal int <-> float
conversion with 2^n, but has not been confirmed from the driver team.
Here the idea is to remove the floor operation when it is related with the
internal int<->floor conversion with 2^n. Although more code is added but no
more work is done.
That seems like an expensive change to put in to work around a driver bug. Is there some way we can detect that we're on hardware with the relevant bug, and only emit the more complicated shader in those circumstances? (On some mobile platforms, we need to be targeting 18-20 lines of GLES to be sure to fit in the on-chip cache. I don't think Ganesh is there right now, but I don't want to see us moving away from that, since it's likely to be my job to move us there in upcoming quarters.) And I know Brian will point out that Ganesh has moved away from doing expensive hardware testing at startup; we might need to find a way to plumb it through Chrome asynchronously.
On 2013/06/07 16:19:43, Tom Hudson wrote: > That seems like an expensive change to put in to work around a driver bug. Is > there some way we can detect that we're on hardware with the relevant bug, and > only emit the more complicated shader in those circumstances? > > (On some mobile platforms, we need to be targeting 18-20 lines of GLES to be > sure to fit in the on-chip cache. I don't think Ganesh is there right now, but I > don't want to see us moving away from that, since it's likely to be my job to > move us there in upcoming quarters.) > > And I know Brian will point out that Ganesh has moved away from doing expensive > hardware testing at startup; we might need to find a way to plumb it through > Chrome asynchronously. Hi, Tom. I have uploaded a new patch for review. It is as simple as the original one in code. I choose a compensation value 0.001 to avoid the side effect of the floor operation. I have verified it on Linux and Windows with Intel Graphics Cards. It should work on other Graphics cards as well but I didn't have one in hand.
Brian, do we have a Ganesh benchmark that hits this codepath enough to figure out whether or not there's a change? Or should we just say "close enough" and accept it? I like the comment, but I'm leery of the fudge factor. My point in the previous comment was that when we have a GL context, we could detect that we're on an Intel GPU, and only emit your exact correction from patchset #1 in that case. Then Intel GPUs would be paying the performance penalty of their error, but would at least be correct, and other GPUs wouldn't see any impact.
On 2013/06/10 09:44:30, Tom Hudson wrote: > Brian, do we have a Ganesh benchmark that hits this codepath enough to figure > out whether or not there's a change? Or should we just say "close enough" and > accept it? > I like the comment, but I'm leery of the fudge factor. > > My point in the previous comment was that when we have a GL context, we could > detect that we're on an Intel GPU, and only emit your exact correction from > patchset #1 in that case. > Then Intel GPUs would be paying the performance penalty of their error, but > would at least be correct, and other GPUs wouldn't see any impact. Tom, we don't have a good benchmark. Jun, can you put up your benchmark from https://codereview.chromium.org/15402003/ as a separate CL. Let's land that first so that we can detect any regressions that could be introduced by changes to either the CPU or GPU conversion paths on the wider range of HW that Skia has in its buildbot array. Also, did you test whether this would force us off of the fast path for other GPUs? If we land the benchmark first then we can have baselines to catch any such cases.
On 2013/06/10 13:27:36, bsalomon wrote: > On 2013/06/10 09:44:30, Tom Hudson wrote: > > Brian, do we have a Ganesh benchmark that hits this codepath enough to figure > > out whether or not there's a change? Or should we just say "close enough" and > > accept it? > > I like the comment, but I'm leery of the fudge factor. > > > > My point in the previous comment was that when we have a GL context, we could > > detect that we're on an Intel GPU, and only emit your exact correction from > > patchset #1 in that case. > > Then Intel GPUs would be paying the performance penalty of their error, but > > would at least be correct, and other GPUs wouldn't see any impact. > > Tom, we don't have a good benchmark. > > Jun, can you put up your benchmark from > https://codereview.chromium.org/15402003/ as a separate CL. Let's land that > first so that we can detect any regressions that could be introduced by changes > to either the CPU or GPU conversion paths on the wider range of HW that Skia has > in its buildbot array. > > Also, did you test whether this would force us off of the fast path for other > GPUs? If we land the benchmark first then we can have baselines to catch any > such cases. Hi, Tom and Brian. Thanks for your comments. I will put the benchmark from https://codereview.chromium.org/15402003/ as a separate CL first. And I have not verified this patch on other GPUs since I have no such GPUs in hand. As Brian mentioned, we can try this patch using the benchmark after it landed. And for Tom's concern, if it affect other GPUs, we should either add some code to detect if it is on Intel GPU, or add a new GrConfigConversionEffect::kMulByAlpha_RoundDown_{Compensate}_PMConversion enum and related shaders for the test program and for future use if it works.
On 2013/06/10 13:27:36, bsalomon wrote: > On 2013/06/10 09:44:30, Tom Hudson wrote: > > Brian, do we have a Ganesh benchmark that hits this codepath enough to figure > > out whether or not there's a change? Or should we just say "close enough" and > > accept it? > > I like the comment, but I'm leery of the fudge factor. > > > > My point in the previous comment was that when we have a GL context, we could > > detect that we're on an Intel GPU, and only emit your exact correction from > > patchset #1 in that case. > > Then Intel GPUs would be paying the performance penalty of their error, but > > would at least be correct, and other GPUs wouldn't see any impact. > > Tom, we don't have a good benchmark. > > Jun, can you put up your benchmark from > https://codereview.chromium.org/15402003/ as a separate CL. Let's land that > first so that we can detect any regressions that could be introduced by changes > to either the CPU or GPU conversion paths on the wider range of HW that Skia has > in its buildbot array. > > Also, did you test whether this would force us off of the fast path for other > GPUs? If we land the benchmark first then we can have baselines to catch any > such cases. Hi, Brian and Tom. I have tried this patch on both ATI card and Nvidia Cards and it didn't break them. So this patch can enable Intel Cards without breaking other GPU cards and should be considered to be added into skia upstream code. Any further comments on this patch are welcomed.
On 2013/06/19 07:33:39, Jun Jiang wrote: > On 2013/06/10 13:27:36, bsalomon wrote: > > On 2013/06/10 09:44:30, Tom Hudson wrote: > > > Brian, do we have a Ganesh benchmark that hits this codepath enough to > figure > > > out whether or not there's a change? Or should we just say "close enough" > and > > > accept it? > > > I like the comment, but I'm leery of the fudge factor. > > > > > > My point in the previous comment was that when we have a GL context, we > could > > > detect that we're on an Intel GPU, and only emit your exact correction from > > > patchset #1 in that case. > > > Then Intel GPUs would be paying the performance penalty of their error, but > > > would at least be correct, and other GPUs wouldn't see any impact. > > > > Tom, we don't have a good benchmark. > > > > Jun, can you put up your benchmark from > > https://codereview.chromium.org/15402003/ as a separate CL. Let's land that > > first so that we can detect any regressions that could be introduced by > changes > > to either the CPU or GPU conversion paths on the wider range of HW that Skia > has > > in its buildbot array. > > > > Also, did you test whether this would force us off of the fast path for other > > GPUs? If we land the benchmark first then we can have baselines to catch any > > such cases. > > Hi, Brian and Tom. I have tried this patch on both ATI card and Nvidia Cards and > it didn't break them. So this patch can enable Intel Cards without breaking > other GPU cards and should be considered to be added into skia upstream code. > Any further comments on this patch are welcomed. SGTM. I'll land it and we can watch the bots to make sure there are no regressions on any of the tested platforms.
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM may have been provided, it was from a non-committer or a lowly provisional committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
On 2013/06/19 08:13:59, I haz the power (commit-bot) wrote: > No LGTM from a valid reviewer yet. Only full committers are accepted. > Even if an LGTM may have been provided, it was from a non-committer or > a lowly provisional committer, _not_ a full super star committer. > See http://www.chromium.org/getting-involved/become-a-committer > Note that this has nothing to do with OWNERS files. lgtm
CQ is trying da patch. Follow status at https://skia-tree-status.appspot.com/cq/jun.a.jiang@intel.com/15719007/6001
Message was sent while issue was closed.
Change committed as 9672 |
