Do not discard existing gpu driver workarounds when called from gpu process

When called from gpu process, gpu_util.cc::ApplyGpuDriverBugWorkarounds
overwrites existing workarounds that have been notified from browser process.

Currently when passing --use_virtualized_gl_contexts to command line,
the workaround id is visible in --gpu-driver-bug-workarounds of gpu process
and in chrome://gpu but not actually taken into account.

Simply adding a trace in content/common/gpu/gpu_command_buffer_stub.cc
shows that the wanted workaround is not enabled.

BUG=553524
R=kbr@chromium.org,jbauman@chromium.org,hendrikw@chromium.org

Review URL: https://codereview.chromium.org/1415603009

TEST= out/Release/chrome --use_virtualized_gl_contexts

Committed: https://crrev.com/75d4db0d5157da23a206e5b516cb78f95bd7331e
Cr-Commit-Position: refs/heads/master@{#359857}

[Julien Isorce Samsung, 2015-11-05 09:12:57]

j.isorce@samsung.com changed reviewers:
+ hendrikw@chromium.org, jbauman@chromium.org

[Julien Isorce Samsung, 2015-11-05 09:12:57]

+hendrikw +jbauman

[hendrikw, 2015-11-05 22:29:36]

On 2015/11/05 09:12:57, j.isorce wrote:
> +hendrikw +jbauman

Out of curiosity, what's the use case for this?

[Julien Isorce Samsung, 2015-11-06 14:52:56]

On 2015/11/05 22:29:36, hendrikw wrote:
> On 2015/11/05 09:12:57, j.isorce wrote:
> > +hendrikw +jbauman
> 
> Out of curiosity, what's the use case for this?

I found it useful to enable/disable any gpu workaround at runtime. It was useful
when investigating https://codereview.chromium.org/1429083002/.

Also the commit message is a bit wrong, in a sense that this patch actually also
allows to pass "--use_virtualized_gl_contexts" manually to the command line or
any gpu workaround.

[hendrikw, 2015-11-06 16:06:34]

On 2015/11/06 14:52:56, j.isorce wrote:
> On 2015/11/05 22:29:36, hendrikw wrote:
> > On 2015/11/05 09:12:57, j.isorce wrote:
> > > +hendrikw +jbauman
> > 
> > Out of curiosity, what's the use case for this?
> 
> I found it useful to enable/disable any gpu workaround at runtime. It was
useful
> when investigating https://codereview.chromium.org/1429083002/.

You can already enable/disable any workaround at runtime (as you mentioned in
the commit msg).

Since we already have a way to do this, I'd prefer not adding a second way.

> 
> Also the commit message is a bit wrong, in a sense that this patch actually
also
> allows to pass "--use_virtualized_gl_contexts" manually to the command line or
> any gpu workaround.

Forgive my confusion, but doesn't --use_virtualized_gl_contexts work?  I've used
both --use_virtualized_gl_contexts and --use_virtualized_gl_contexts=0 in the
past successfully.

I just tried it, it does work.

[Julien Isorce Samsung, 2015-11-09 14:34:19]

On 2015/11/06 16:06:34, hendrikw wrote:
> Forgive my confusion, but doesn't --use_virtualized_gl_contexts work?  I've
used
> both --use_virtualized_gl_contexts and --use_virtualized_gl_contexts=0 in the
> past successfully.
> 
> I just tried it, it does work.

For me it "half" work, in a sense that "ps aux | grep gpu"  shows or not "62" in
--gpu-driver-bug-workarounds upon --use_virtualized_gl_contexts true or false.
But
in content/common/gpu/gpu_command_buffer_stub.cc when calling
"context_group_->feature_info()->workarounds().use_virtualized_gl_contexts" it
does not matter what you passed "--use_virtualized_gl_contexts" or
"--use_virtualized_gl_contexts=0". But works with my patch. I am currently
trying to understand what it does not work without my patch.

[hendrikw, 2015-11-09 16:05:31]

On 2015/11/09 14:34:19, j.isorce wrote:
> On 2015/11/06 16:06:34, hendrikw wrote:
> > Forgive my confusion, but doesn't --use_virtualized_gl_contexts work?  I've
> used
> > both --use_virtualized_gl_contexts and --use_virtualized_gl_contexts=0 in
the
> > past successfully.
> > 
> > I just tried it, it does work.
> 
> For me it "half" work, in a sense that "ps aux | grep gpu"  shows or not "62"
in
> --gpu-driver-bug-workarounds upon --use_virtualized_gl_contexts true or false.
> But
> in content/common/gpu/gpu_command_buffer_stub.cc when calling
> "context_group_->feature_info()->workarounds().use_virtualized_gl_contexts" it
> does not matter what you passed "--use_virtualized_gl_contexts" or
> "--use_virtualized_gl_contexts=0". But works with my patch. I am currently
> trying to understand what it does not work without my patch.

ok, let me know what you find

[Julien Isorce Samsung, 2015-11-09 16:24:11]

On 2015/11/09 16:05:31, hendrikw wrote:
> ok, let me know what you find

I just find the problem. It is not easy to explain.

In gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine
the problem is that when it is called from GPU Process, "if
(command_line.HasSwitch(kFeatureList[i].name)" is always false since
"--use_virtualized_gl_contexts" is not a switches transfered from browser
process to gpu process. It is not transfered directly because the mechanism is
to use "GpuDataManagerImplPrivate::UpdateGpuInfoHelper / NotifyGpuInfoUpdate()".

But it is fine since at this time it has been notified already. So when
gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine is
called from  GpuMain, then "--use_virtualized_gl_contexts" is not present but
"62" is present in "--gpu-driver-bug-workarounds".  (62 is id/type of
USE_VIRTUALIZED_GL_CONTEXTS)

The problem is actually that gpu_util.cc::ApplyGpuDriverBugWorkarounds
overwrites all of them, indeed it loads the workaround list from json and then
calls "command_line->AppendSwitchASCII(switches::kGpuDriverBugWorkarounds,
IntSetToString(workarounds));" which erases "62" because not in json and because
"--use_virtualized_gl_contexts" is not in the cmdlime (because called from gpu
process). I am saying "erased" because "62" IS at this time in the
"--gpu-driver-bug-workarounds" because notified, see above.

So my patch solves this problem but maybe I should add the condition
"isGpuProcess" in the "if
(command_line.HasSwitch(switches::kGpuDriverBugWorkarounds))" of my patch,
if you do not want to have the possibility to manually set
"--gpu-driver-bug-workarounds" from main cmd line (i.e. Browser Process).

[hendrikw, 2015-11-09 18:37:30]

On 2015/11/09 16:24:11, j.isorce wrote:
> On 2015/11/09 16:05:31, hendrikw wrote:
> > ok, let me know what you find
> 
> I just find the problem. It is not easy to explain.
> 
> In gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine
> the problem is that when it is called from GPU Process, "if
> (command_line.HasSwitch(kFeatureList[i].name)" is always false since
> "--use_virtualized_gl_contexts" is not a switches transfered from browser
> process to gpu process. It is not transfered directly because the mechanism is
> to use "GpuDataManagerImplPrivate::UpdateGpuInfoHelper /
NotifyGpuInfoUpdate()".
> 
> But it is fine since at this time it has been notified already. So when
> gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine is
> called from  GpuMain, then "--use_virtualized_gl_contexts" is not present but
> "62" is present in "--gpu-driver-bug-workarounds".  (62 is id/type of
> USE_VIRTUALIZED_GL_CONTEXTS)
> 
> The problem is actually that gpu_util.cc::ApplyGpuDriverBugWorkarounds
> overwrites all of them, indeed it loads the workaround list from json and then
> calls "command_line->AppendSwitchASCII(switches::kGpuDriverBugWorkarounds,
> IntSetToString(workarounds));" which erases "62" because not in json and
because
> "--use_virtualized_gl_contexts" is not in the cmdlime (because called from gpu
> process). I am saying "erased" because "62" IS at this time in the
> "--gpu-driver-bug-workarounds" because notified, see above.
> 
> So my patch solves this problem but maybe I should add the condition
> "isGpuProcess" in the "if
> (command_line.HasSwitch(switches::kGpuDriverBugWorkarounds))" of my patch,
> if you do not want to have the possibility to manually set
> "--gpu-driver-bug-workarounds" from main cmd line (i.e. Browser Process).

Oooooooh, crap.  I wasn't seeing this because I was running on Windows.

@kbr, we broke the command line options with
https://codereview.chromium.org/1406323005/  :/

And this might be broken on android and chromeos too.

This cl does fix it.

[Ken Russell (switch to Gerrit), 2015-11-09 18:53:22]

Description was changed from

==========
Allow to pass ids of gpu driver workarounds to command line

For example with workaround "use_virtualized_gl_contexts",
currently the only way to enable it from the command line
is to pass --use_virtualized_gl_contexts.
(--use_virtualized_gl_contexts=0 to disable it)

This patch allows to enable it providing its id:
--gpu-driver-bug-workarounds=62

Generally the patch allows to provides a set of id:
--gpu-driver-bug-workarounds=i,j,k,l

These ids are append to the auto generated workarounds.
So maybe another gpu switch should be use for that:
kGpuDriverBugWorkaroundsAdd instead of kGpuDriverBugWorkarounds.
Then it would be possible to do:
--gpu-driver-bug-workarounds-add=+i,+j,-k,-l,+m
It already exists switches that interpret +/- so it is consistent
and one could re-use these parsers.

BUG=

R=kbr@chromium.org

TEST=Add --gpu-driver-bug-workarounds=62 and with "ps aux | grep gpu" see 62 is
now appended
==========

to

==========
Allow to pass ids of gpu driver workarounds to command line

For example with workaround "use_virtualized_gl_contexts",
currently the only way to enable it from the command line
is to pass --use_virtualized_gl_contexts.
(--use_virtualized_gl_contexts=0 to disable it)

This patch allows to enable it providing its id:
--gpu-driver-bug-workarounds=62

Generally the patch allows to provides a set of id:
--gpu-driver-bug-workarounds=i,j,k,l

These ids are append to the auto generated workarounds.
So maybe another gpu switch should be use for that:
kGpuDriverBugWorkaroundsAdd instead of kGpuDriverBugWorkarounds.
Then it would be possible to do:
--gpu-driver-bug-workarounds-add=+i,+j,-k,-l,+m
It already exists switches that interpret +/- so it is consistent
and one could re-use these parsers.

BUG=553524

R=kbr@chromium.org

TEST=Add --gpu-driver-bug-workarounds=62 and with "ps aux | grep gpu" see 62 is
now appended
==========

[Ken Russell (switch to Gerrit), 2015-11-09 19:28:01]

On 2015/11/09 18:37:30, hendrikw wrote:
> On 2015/11/09 16:24:11, j.isorce wrote:
> > On 2015/11/09 16:05:31, hendrikw wrote:
> > > ok, let me know what you find
> > 
> > I just find the problem. It is not easy to explain.
> > 
> > In
gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine
> > the problem is that when it is called from GPU Process, "if
> > (command_line.HasSwitch(kFeatureList[i].name)" is always false since
> > "--use_virtualized_gl_contexts" is not a switches transfered from browser
> > process to gpu process. It is not transfered directly because the mechanism
is
> > to use "GpuDataManagerImplPrivate::UpdateGpuInfoHelper /
> NotifyGpuInfoUpdate()".
> > 
> > But it is fine since at this time it has been notified already. So when
> > gpu_driver_bug_list.cc::GpuDriverBugList::AppendWorkaroundsFromCommandLine
is
> > called from  GpuMain, then "--use_virtualized_gl_contexts" is not present
but
> > "62" is present in "--gpu-driver-bug-workarounds".  (62 is id/type of
> > USE_VIRTUALIZED_GL_CONTEXTS)
> > 
> > The problem is actually that gpu_util.cc::ApplyGpuDriverBugWorkarounds
> > overwrites all of them, indeed it loads the workaround list from json and
then
> > calls "command_line->AppendSwitchASCII(switches::kGpuDriverBugWorkarounds,
> > IntSetToString(workarounds));" which erases "62" because not in json and
> because
> > "--use_virtualized_gl_contexts" is not in the cmdlime (because called from
gpu
> > process). I am saying "erased" because "62" IS at this time in the
> > "--gpu-driver-bug-workarounds" because notified, see above.
> > 
> > So my patch solves this problem but maybe I should add the condition
> > "isGpuProcess" in the "if
> > (command_line.HasSwitch(switches::kGpuDriverBugWorkarounds))" of my patch,
> > if you do not want to have the possibility to manually set
> > "--gpu-driver-bug-workarounds" from main cmd line (i.e. Browser Process).
> 
> Oooooooh, crap.  I wasn't seeing this because I was running on Windows.
> 
> @kbr, we broke the command line options with
> https://codereview.chromium.org/1406323005/  :/
> 
> And this might be broken on android and chromeos too.
> 
> This cl does fix it.

Have you verified the fix? It's unfortunate that this breakage wasn't caught by
any existing tests.

Could you please see whether it's feasible to add a test for this in
src/content/test/gpu/gpu_tests/gpu_process.py or
src/content/test/gpu/gpu_tests/hardware_accelerated_feature.py ? Perhaps add a
fake driver bug workaround for testing, enable it on the command line for one of
the tests (see PixelTestsES3SharedPageState in
src/content/test/gpu/page_sets/pixel_tests.py for how to pass a command line
flag for just one of multiple tests), and verify from about:gpu that the
workaround is enabled (see
src/content/test/gpu/gpu_tests/hardware_accelerated_feature.py for an example of
scraping about:gpu from the tests)?

[Julien Isorce Samsung, 2015-11-09 20:35:22]

On 2015/11/09 19:28:01, Ken Russell wrote:
> Could you please see whether it's feasible to add a test for this

Makes sense, I'll try tomorrow. If it turns out it is feasible I guess I should
include the unit test in this patch right ?

[Ken Russell (switch to Gerrit), 2015-11-10 02:21:41]

On 2015/11/09 20:35:22, j.isorce wrote:
> On 2015/11/09 19:28:01, Ken Russell wrote:
> > Could you please see whether it's feasible to add a test for this
> 
> Makes sense, I'll try tomorrow. If it turns out it is feasible I guess I
should
> include the unit test in this patch right ?

Yes, please.

[Julien Isorce Samsung, 2015-11-10 17:08:09]

Description was changed from

==========
Allow to pass ids of gpu driver workarounds to command line

For example with workaround "use_virtualized_gl_contexts",
currently the only way to enable it from the command line
is to pass --use_virtualized_gl_contexts.
(--use_virtualized_gl_contexts=0 to disable it)

This patch allows to enable it providing its id:
--gpu-driver-bug-workarounds=62

Generally the patch allows to provides a set of id:
--gpu-driver-bug-workarounds=i,j,k,l

These ids are append to the auto generated workarounds.
So maybe another gpu switch should be use for that:
kGpuDriverBugWorkaroundsAdd instead of kGpuDriverBugWorkarounds.
Then it would be possible to do:
--gpu-driver-bug-workarounds-add=+i,+j,-k,-l,+m
It already exists switches that interpret +/- so it is consistent
and one could re-use these parsers.

BUG=553524

R=kbr@chromium.org

TEST=Add --gpu-driver-bug-workarounds=62 and with "ps aux | grep gpu" see 62 is
now appended
==========

to

==========
When called from gpu process, gpu_util.cc::ApplyGpuDriverBugWorkarounds
overwrites existing workarounds that have been notified from browser process.

Currently when passing --use_virtualized_gl_contexts to command line,
the workaround id is visible in --gpu-driver-bug-workarounds of gpu process
and in chrome://gpu but not actually taken into account.

Simply adding a trace in content/common/gpu/gpu_command_buffer_stub.cc
shows that the wanted workaround is not enabled.

A new unit test has also been added: driver_bug_workarounds.py

BUG=553524

Review URL: https://codereview.chromium.org/1415603009

R=kbr@chromium.org,jbauman@chromium.org,hendrikw@chromium.org

TEST= out/Release/chrome --use_virtualized_gl_contexts

BUG=553524

R=kbr@chromium.org

TEST=Add --gpu-driver-bug-workarounds=62 and with "ps aux | grep gpu" see 62 is
now appended
==========

[Julien Isorce Samsung, 2015-11-10 17:15:00]

On 2015/11/10 02:21:41, Ken Russell wrote:
> Could you please see whether it's feasible to add a test for this

I added unit test in "patch set 2". Unfortunately I am not sure now if
--use_virtualized_gl_contexts was not visible in chrome://gpu. I need to check.
I mean the bug is there for sure but not convinced the unit test actually checks
this bug.

[Julien Isorce Samsung, 2015-11-11 12:53:16]

On 2015/11/10 17:15:00, j.isorce wrote:
> On 2015/11/10 02:21:41, Ken Russell wrote:
> > Could you please see whether it's feasible to add a test for this
> 
> I added unit test in "patch set 2". Unfortunately I am not sure now if
> --use_virtualized_gl_contexts was not visible in chrome://gpu. I need to
check.
> I mean the bug is there for sure but not convinced the unit test actually
checks
> this bug.

Ok I double checked and I confirm the CL solves the bug but the included unit
test does not check regression for this bug.
In short in current master (so without this CL), "chrome
--use_virtualized_gl_contexts chrome://gpu" shows "use_virtualized_gl_contexts"
even if not actually activated for gpu process which is the actual bug. (adding
a trace in  GpuCommandBufferStub::GpuCommandBufferStub shows that for the GPU
Porcess this workaround is not enabled)

I suppose this chrome://gpu is not querying gpu process  but browser process
instead.

Do you have an idea where I should write a unit test (py or cc) ?

[Ken Russell (switch to Gerrit), 2015-11-11 17:53:30]

On 2015/11/11 12:53:16, j.isorce wrote:
> On 2015/11/10 17:15:00, j.isorce wrote:
> > On 2015/11/10 02:21:41, Ken Russell wrote:
> > > Could you please see whether it's feasible to add a test for this
> > 
> > I added unit test in "patch set 2". Unfortunately I am not sure now if
> > --use_virtualized_gl_contexts was not visible in chrome://gpu. I need to
> check.
> > I mean the bug is there for sure but not convinced the unit test actually
> checks
> > this bug.
> 
> Ok I double checked and I confirm the CL solves the bug but the included unit
> test does not check regression for this bug.
> In short in current master (so without this CL), "chrome
> --use_virtualized_gl_contexts chrome://gpu" shows
"use_virtualized_gl_contexts"
> even if not actually activated for gpu process which is the actual bug.
(adding
> a trace in  GpuCommandBufferStub::GpuCommandBufferStub shows that for the GPU
> Porcess this workaround is not enabled)
> 
> I suppose this chrome://gpu is not querying gpu process  but browser process
> instead.

Thanks for writing the test. It's quite unfortunate that the information
displayed in about:gpu doesn't match reality.
src/content/browser/gpu/gpu_internals_ui.cc uses the GpuDataManager and I would
expect that to contain the ground truth.

Let's fix the underlying problem urgently, and figure out how to write tests for
this afterward. We'd appreciate your help in continuing to figure out how to
regression test this. Please take out the useless test and let's land the
original patch. Hendrik, could you please review the final patch and TBR it to
me?

> Do you have an idea where I should write a unit test (py or cc) ?

Unfortunately it seems at this point it is difficult to make assertions about
the handling of these GPU driver bug workarounds between the browser and GPU
process. We don't have browser tests for these code paths any more because they
were unstable. Making about:gpu reflect exactly what the GPU process received
would be the best way forward given our current testing infrastructure.

[Julien Isorce Samsung, 2015-11-12 10:11:12]

Description was changed from

==========
When called from gpu process, gpu_util.cc::ApplyGpuDriverBugWorkarounds
overwrites existing workarounds that have been notified from browser process.

Currently when passing --use_virtualized_gl_contexts to command line,
the workaround id is visible in --gpu-driver-bug-workarounds of gpu process
and in chrome://gpu but not actually taken into account.

Simply adding a trace in content/common/gpu/gpu_command_buffer_stub.cc
shows that the wanted workaround is not enabled.

A new unit test has also been added: driver_bug_workarounds.py

BUG=553524

Review URL: https://codereview.chromium.org/1415603009

R=kbr@chromium.org,jbauman@chromium.org,hendrikw@chromium.org

TEST= out/Release/chrome --use_virtualized_gl_contexts

BUG=553524

R=kbr@chromium.org

TEST=Add --gpu-driver-bug-workarounds=62 and with "ps aux | grep gpu" see 62 is
now appended
==========

to

==========
When called from gpu process, gpu_util.cc::ApplyGpuDriverBugWorkarounds
overwrites existing workarounds that have been notified from browser process.

Currently when passing --use_virtualized_gl_contexts to command line,
the workaround id is visible in --gpu-driver-bug-workarounds of gpu process
and in chrome://gpu but not actually taken into account.

Simply adding a trace in content/common/gpu/gpu_command_buffer_stub.cc
shows that the wanted workaround is not enabled.

BUG=553524

R=kbr@chromium.org,jbauman@chromium.org,hendrikw@chromium.org

Review URL: https://codereview.chromium.org/1415603009

TEST= out/Release/chrome --use_virtualized_gl_contexts
==========

[Julien Isorce Samsung, 2015-11-12 10:38:46]

On 2015/11/11 17:53:30, Ken Russell wrote:
> Let's fix the underlying problem urgently, and figure out how to write tests
for
> this afterward.

Right.

> Please take out the useless test and let's land the original patch.
> Hendrik, could you please review the final patch and TBR it to me?

Done. Hendrik please review "patch set 3", thx in advance.

> We'd appreciate your help in continuing to figure out how to regression test
this.
> Making about:gpu reflect exactly what the GPU process received 
> would be the best way forward given our current testing infrastructure

I created a new issue (feature):
http://code.google.com/p/chromium/issues/detail?id=554909
Please assigned it to me, I'll try to see what I can do. Thx for the hint about
gpu_internals_ui.cc+GpuDataManager

[hendrikw, 2015-11-12 19:50:03]

On 2015/11/12 10:38:46, j.isorce wrote:
> On 2015/11/11 17:53:30, Ken Russell wrote:
> > Let's fix the underlying problem urgently, and figure out how to write tests
> for
> > this afterward.
> 
> Right.
> 
> > Please take out the useless test and let's land the original patch.
> > Hendrik, could you please review the final patch and TBR it to me?
> 
> Done. Hendrik please review "patch set 3", thx in advance.
> 
> > We'd appreciate your help in continuing to figure out how to regression test
> this.
> > Making about:gpu reflect exactly what the GPU process received 
> > would be the best way forward given our current testing infrastructure
> 
> I created a new issue (feature):
> http://code.google.com/p/chromium/issues/detail?id=554909
> Please assigned it to me, I'll try to see what I can do. Thx for the hint
about
> gpu_internals_ui.cc+GpuDataManager

LGTM

[Ken Russell (switch to Gerrit), 2015-11-12 23:35:28]

lgtm too. Thanks for filing the follow on bug.

[Julien Isorce Samsung, 2015-11-16 14:02:10]

The CQ bit was checked by j.isorce@samsung.com

[commit-bot: I haz the power, 2015-11-16 14:02:30]

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

[commit-bot: I haz the power, 2015-11-16 14:10:01]

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

[commit-bot: I haz the power, 2015-11-16 14:10:02]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Julien Isorce Samsung, 2015-11-16 17:23:35]

The CQ bit was checked by j.isorce@samsung.com

[commit-bot: I haz the power, 2015-11-16 17:26:32]

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

[commit-bot: I haz the power, 2015-11-16 17:36:49]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-11-16 17:37:44]

Patchset 3 (id:??) landed as
https://crrev.com/75d4db0d5157da23a206e5b516cb78f95bd7331e
Cr-Commit-Position: refs/heads/master@{#359857}