Workaround for blacklist KHR_blend_equation_advanced on ARM GPU

Workaround for blacklist KHR_blend_equation_advanced on ARM GPU

ARM driver will check the fragment shader compatiblity for KHR_blend_equation_advanced even if blending is disabled.
Workaround: Set blending equation to any basic equation when we disable blending.
https://code.google.com/p/skia/issues/detail?id=3943

BUG=skia:3943
Committed: https://skia.googlesource.com/skia/+/9764c40cd31c11c82686c8b8dbbeaea9fa4de05d

[Joel.Liang, 2015-06-30 09:44:58]

joel.liang@arm.com changed reviewers:
+ bsalomon@google.com, egdaniel@google.com

[Joel.Liang, 2015-06-30 10:17:44]

I have confirmed that ARM driver will check the fragment shader compatibility
for KHR_blend_equation_advanced extension even if blending is disabled.

glUseProgram(advBlendProgramObj);
...
glEnable(GL_BLEND);
glBlendEquation(GL_HARDLIGHT_KHR); // set advanced blending equation
glDrawArrays(...); // ok to draw with KHR_blend_equation_advanced

glUseProgram(nonBlendProgramObj);
...
glDisable(GL_BLEND);
// Workaround: Set blend equation to one of the basic equation.
// glBlendEquation(GL_FUNC_ADD);
glDrawArrays(...); // ARM driver return GL_INVALID_OPERATION here, because the
advanced blending equation (GL_HARDLIGHT_KHR) is set, and the fragment shader in
nonBlendProgramObj not compatible with KHR_blend_equation_advanced.

It should NOT check the compatibility if blending is disabled.
We have create a ticket for ARM driver team and will fix it later.

For now, I'd like to provide a workaround for this issue, please have a look.

[egdaniel, 2015-07-06 17:53:22]

Just the one nit, and quick comment.

So the artifacts on color burn that you posted in the bug are the result of
numerical instability in the way color burn was original described in the spec.
So that is going to be a driver side change to reorg the math so it is stable.
The strange part though is, color dodge has a very similar numerical issue and
it is surprising that the artifacts don't show up for it (and I am unable to
repro the artifacts on arm myself). Anyways here is a CL that shows how our cpu
code was fixed and you'll want to confirm with the driver guys a fix for burn is
on the way (and that dodge, luckily, was stable from the start)
https://chromiumcodereview.appspot.com/12662006/

https://codereview.chromium.org/1216963004/diff/1/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1216963004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2164: if  (kARM_GrGLVendor == this->ctxInfo().vendor() &&
extra space after if

[egdaniel, 2015-07-06 17:57:33]

By the way, I updated your CL description.
First I removed:
NOTREECHECKS=true
NOTRY=true
NOPRESUBMIT=true

You should not put these in since it causes the CL to bypass our try bots and
similar checks before landing in our tree.

I also added the skia bug number to:
BUG=skia:3943

This will link your CL to the bug, and will auto update the bug when this cl
lands.

[Joel.Liang, 2015-07-07 02:54:27]

On 2015/07/06 17:57:33, egdaniel wrote:
> By the way, I updated your CL description.
> First I removed:
> NOTREECHECKS=true
> NOTRY=true
> NOPRESUBMIT=true
> 
> You should not put these in since it causes the CL to bypass our try bots and
> similar checks before landing in our tree.
> 
> I also added the skia bug number to:
> BUG=skia:3943
> 
> This will link your CL to the bug, and will auto update the bug when this cl
> lands.

The "NOTREECHECKS, NOTTRY and NOTPRESUBMIT" was added for me automatically.
And 'git cl upload' add these again when I submit the patch set 2.

    ** Post Upload Hook Messages **
    Branch changes do not need to rely on the master branch's tree status.
Automatically added 'NOTREECHECKS=true' to the CL's description

    Trybots do not yet work for non-master branches. Automatically added
'NOTRY=true' to the CL's description

    Branch changes do not run the presubmit checks.

I think this because of my local branch is not branch from master.
$ git branch -vv
* arm_blend_equation_advanced_workaround 6800e7c
[gerrit/gatekeeper/blend_equation_advanced_workaround: ahead 2] Remove extra
space

Next time, I will branch it from master.

[Joel.Liang, 2015-07-07 02:56:22]

I have removed the extra space.

https://codereview.chromium.org/1216963004/diff/1/src/gpu/gl/GrGLGpu.cpp
File src/gpu/gl/GrGLGpu.cpp (right):

https://codereview.chromium.org/1216963004/diff/1/src/gpu/gl/GrGLGpu.cpp#newc...
src/gpu/gl/GrGLGpu.cpp:2164: if  (kARM_GrGLVendor == this->ctxInfo().vendor() &&
On 2015/07/06 17:53:22, egdaniel wrote:
> extra space after if

Done.

[rmistry, 2015-07-07 13:22:39]

On 2015/07/07 02:54:27, Joel.Liang wrote:
> On 2015/07/06 17:57:33, egdaniel wrote:
> > By the way, I updated your CL description.
> > First I removed:
> > NOTREECHECKS=true
> > NOTRY=true
> > NOPRESUBMIT=true
> > 
> > You should not put these in since it causes the CL to bypass our try bots
and
> > similar checks before landing in our tree.
> > 
> > I also added the skia bug number to:
> > BUG=skia:3943
> > 
> > This will link your CL to the bug, and will auto update the bug when this cl
> > lands.
> 
> The "NOTREECHECKS, NOTTRY and NOTPRESUBMIT" was added for me automatically.
> And 'git cl upload' add these again when I submit the patch set 2.
> 
>     ** Post Upload Hook Messages **
>     Branch changes do not need to rely on the master branch's tree status.
> Automatically added 'NOTREECHECKS=true' to the CL's description
> 
>     Trybots do not yet work for non-master branches. Automatically added
> 'NOTRY=true' to the CL's description
> 
>     Branch changes do not run the presubmit checks.
> 
> I think this because of my local branch is not branch from master.
> $ git branch -vv
> * arm_blend_equation_advanced_workaround 6800e7c
> [gerrit/gatekeeper/blend_equation_advanced_workaround: ahead 2] Remove extra
> space
> 
> Next time, I will branch it from master.

Yes this happened because it is not a branch from master. Out of curiosity, how
did you create the branch?
Looking at the "Target Ref" of this CL it thinks that you are transitively
tracking the remote branch "gatekeeper/blend_equation_advanced_workaround" which
does not exist.

I suspect that the CQ will fail when trying to submit this CL because of the
target ref.

You should be able to fix this on your branch by fixing the upstream and
re-uploading a patch:
git branch --set-upstream-to origin/master
git cl upload

[Joel.Liang, 2015-07-08 02:14:50]

On 2015/07/07 13:22:39, rmistry wrote:
> Yes this happened because it is not a branch from master. Out of curiosity,
how
> did you create the branch?
> Looking at the "Target Ref" of this CL it thinks that you are transitively
> tracking the remote branch "gatekeeper/blend_equation_advanced_workaround"
which
> does not exist.
> 
> I suspect that the CQ will fail when trying to submit this CL because of the
> target ref.
> 
> You should be able to fix this on your branch by fixing the upstream and
> re-uploading a patch:
> git branch --set-upstream-to origin/master
> git cl upload

The branch is create from a mirror repository in ARM.
And I have done the 'set upstream' and 'git cl upload'.
Now the 'Target Ref' is master.

[egdaniel, 2015-07-08 13:16:11]

lgtm

[Joel.Liang, 2015-07-10 02:33:33]

The CQ bit was checked by joel.liang@arm.com

[Joel.Liang, 2015-07-10 02:33:34]

The CQ bit was unchecked by joel.liang@arm.com

[Joel.Liang, 2015-07-10 02:35:07]

The CQ bit was checked by joel.liang@arm.com to run a CQ dry run

[commit-bot: I haz the power, 2015-07-10 02:35:18]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216963004/40001

[commit-bot: I haz the power, 2015-07-10 02:42:36]

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

[commit-bot: I haz the power, 2015-07-10 02:42:37]

Dry run: This issue passed the CQ dry run.

[Joel.Liang, 2015-07-10 02:45:03]

The CQ bit was checked by joel.liang@arm.com

[Joel.Liang, 2015-07-10 02:45:05]

The CQ bit was unchecked by joel.liang@arm.com

[Joel.Liang, 2015-07-10 02:45:39]

The CQ bit was checked by joel.liang@arm.com

[commit-bot: I haz the power, 2015-07-10 02:45:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216963004/40001

[Joel.Liang, 2015-07-10 02:45:51]

The CQ bit was unchecked by joel.liang@arm.com

[Joel.Liang, 2015-07-10 02:45:55]

The CQ bit was checked by joel.liang@arm.com

[commit-bot: I haz the power, 2015-07-10 02:46:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1216963004/40001

[commit-bot: I haz the power, 2015-07-10 02:46:22]

Committed patchset #3 (id:40001) as
https://skia.googlesource.com/skia/+/9764c40cd31c11c82686c8b8dbbeaea9fa4de05d