Blacklist GLX indirect rendering

Blacklist GLX indirect rendering

Several part to this CL:
1- Implement gathering the direct rendering information. It needs a context, so
it is done through the CollectGraphicsInfoGL hook
2- Implement logic to hook the information to the blacklist
3- In some cases, indirect rendering causes creating surfaces/contexts, needed
to collect the info, to fail. So expose the failure mode (fatal vs non-fatal) to
the caller so that we can fully fail GPU things in that case.

BUG=72373, 339244
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=258721

[Zhenyao Mo, 2014-03-11 17:10:52]

On 2014/03/11 04:36:14, piman wrote:

LGTM

It would be nice to display this piece of information in about:gpu page, but it
can be done separately from this CL.

[Zhenyao Mo, 2014-03-11 19:47:55]

On 2014/03/11 17:10:52, Zhenyao Mo wrote:
> On 2014/03/11 04:36:14, piman wrote:
> 
> LGTM
> 
> It would be nice to display this piece of information in about:gpu page, but
it
> can be done separately from this CL.

Forget to mention: please update
https://code.google.com/p/chromium/codesearch#chromium/src/gpu/config/gpu_con...

[piman, 2014-03-11 19:50:18]

I'm happy to add the code to show that info on about:gpu, but I'm not sure where
to start. I'll take a look around.

+kbr: it seems that this CL fails on the GPU bots. E.g.
http://build.chromium.org/p/tryserver.chromium/builders/linux_gpu_triggered_t...
The logs suggest the bots are indeed using indirect rendering. Is that expected?
Should we fix the bots? Or should we change this CL to somehow white list the
bot configuration?

[Zhenyao Mo, 2014-03-11 19:59:37]

The about:gpu code is in

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/gp...

and

https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[Ken Russell (switch to Gerrit), 2014-03-11 20:50:07]

On 2014/03/11 19:50:18, piman wrote:
> +kbr: it seems that this CL fails on the GPU bots. E.g.
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_gpu_triggered_t...
> The logs suggest the bots are indeed using indirect rendering. Is that
expected?
> Should we fix the bots? Or should we change this CL to somehow white list the
> bot configuration?

It's unexpected that the bots are using indirect rendering. I'll log on to one
of them and take a look.

[Ken Russell (switch to Gerrit), 2014-03-11 20:59:24]

On 2014/03/11 20:50:07, Ken Russell wrote:
> On 2014/03/11 19:50:18, piman wrote:
> > +kbr: it seems that this CL fails on the GPU bots. E.g.
> >
>
http://build.chromium.org/p/tryserver.chromium/builders/linux_gpu_triggered_t...
> > The logs suggest the bots are indeed using indirect rendering. Is that
> expected?
> > Should we fix the bots? Or should we change this CL to somehow white list
the
> > bot configuration?
> 
> It's unexpected that the bots are using indirect rendering. I'll log on to one
> of them and take a look.

I posted glxinfo from that bot to crbug.com/339244 . It claims direct rendering
is being used. I'll try to look at this in Chrome on the same machine.

[Ken Russell (switch to Gerrit), 2014-03-11 21:13:15]

I have a feeling that the GPU sandbox may be interfering with direct GLX
rendering. Compared to running glxinfo on the command line, Chrome always
reports:

NVIDIA: could not open the device file /dev/nvidia0 (Operation not permitted).

https://codereview.chromium.org/194303002/diff/40001/gpu/config/software_rend...
File gpu/config/software_rendering_list_json.cc (right):

https://codereview.chromium.org/194303002/diff/40001/gpu/config/software_rend...
gpu/config/software_rendering_list_json.cc:1046: ]
Could you first land the rest of this CL without the blacklist changes? It would
make it easier to debug what's going on on the Linux bots.

[piman, 2014-03-11 21:18:12]

On Tue, Mar 11, 2014 at 1:50 PM, <kbr@chromium.org> wrote:

> On 2014/03/11 19:50:18, piman wrote:
>
>> +kbr: it seems that this CL fails on the GPU bots. E.g.
>>
>
> http://build.chromium.org/p/tryserver.chromium/builders/
> linux_gpu_triggered_tests/builds/500/steps/maps_pixel_test/logs/stdio
>
>> The logs suggest the bots are indeed using indirect rendering. Is that
>>
> expected?
>
>> Should we fix the bots? Or should we change this CL to somehow white list
>> the
>> bot configuration?
>>
>
> It's unexpected that the bots are using indirect rendering. I'll log on to
> one
> of them and take a look.
>

I found an issue with my CL which would report false always, so maybe
that's it.
I'm just confused about where the line "INFO:root:  direct_rendering    :
False" comes from. I didn't add any code to print that AFAIK. Is there some
magic that generates these from the GPUInfo struct?


>
>
> https://codereview.chromium.org/194303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Ken Russell (switch to Gerrit), 2014-03-11 21:20:29]

On 2014/03/11 21:18:12, piman wrote:
> On Tue, Mar 11, 2014 at 1:50 PM, <mailto:kbr@chromium.org> wrote:
> 
> > On 2014/03/11 19:50:18, piman wrote:
> >
> >> +kbr: it seems that this CL fails on the GPU bots. E.g.
> >>
> >
> > http://build.chromium.org/p/tryserver.chromium/builders/
> > linux_gpu_triggered_tests/builds/500/steps/maps_pixel_test/logs/stdio
> >
> >> The logs suggest the bots are indeed using indirect rendering. Is that
> >>
> > expected?
> >
> >> Should we fix the bots? Or should we change this CL to somehow white list
> >> the
> >> bot configuration?
> >>
> >
> > It's unexpected that the bots are using indirect rendering. I'll log on to
> > one
> > of them and take a look.
> >
> 
> I found an issue with my CL which would report false always, so maybe
> that's it.

Cool.

> I'm just confused about where the line "INFO:root:  direct_rendering    :
> False" comes from. I didn't add any code to print that AFAIK. Is there some
> magic that generates these from the GPUInfo struct?

Yes. Your addition of the new field to the EnumerateFields method made this
happen automatically.

> 
> >
> >
> > https://codereview.chromium.org/194303002/
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[piman, 2014-03-11 21:32:44]

PTAL. I sent this to trybots again, we'll see what happens.

https://codereview.chromium.org/194303002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_internals_ui.cc (right):

https://codereview.chromium.org/194303002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_internals_ui.cc:162: NewDescriptionValuePair("Direct
rendering", direct_rendering));
That was easy.

https://codereview.chromium.org/194303002/diff/60001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/194303002/diff/60001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host.cc:756:
GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info);
This is needed to avoid races.
GpuChildThread sends GpuHostMsg_GraphicsInfoCollected at the end of
GpuChildThread::OnInitialize which does this too, however
GpuHostMsg_GraphicsInfoCollected is picked up by the UI thread, whereas there
could be a channel request on the IO thread. If the UI thread is blocked
(indeed, maybe waiting for the channel to open), the channel request will
proceed before we can process the GpuHostMsg_GraphicsInfoCollected, taking an
out-of-date info.

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_control_l...
File gpu/config/gpu_control_list.h (right):

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_control_l...
gpu/config/gpu_control_list.h:259: explicit BoolInfo(bool value);
I realized that the format natively supports bools, making this simpler.

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_info.cc
File gpu/config/gpu_info.cc (right):

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_info.cc#n...
gpu/config/gpu_info.cc:39: direct_rendering(true),
This is the more reasonable default, only GLX can be indirect.

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_info_coll...
File gpu/config/gpu_info_collector.cc (right):

https://codereview.chromium.org/194303002/diff/60001/gpu/config/gpu_info_coll...
gpu/config/gpu_info_collector.cc:110: gpu_info->direct_rendering =
window_system_binding_info.direct_rendering;
I missed this critical piece... Doh.

[piman, 2014-03-11 21:33:14]

+cevans for the trivial change to gpu_messages.h

[piman, 2014-03-11 21:33:18]

+cevans for the trivial change to gpu_messages.h

[Ken Russell (switch to Gerrit), 2014-03-11 21:53:16]

FYI: I'll remove the bogus mac_gpu_retina entries from the various PRESUBMIT.py
scripts. That configuration doesn't exist any more.

[Ken Russell (switch to Gerrit), 2014-03-11 22:28:40]

https://codereview.chromium.org/194303002/diff/70001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/194303002/diff/70001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host.cc:756:
GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info);
Why was this "else" branch added? Is there a chance that it will cause more GPU
info updates to be propagated to renderers and the GPU process?

[Zhenyao Mo, 2014-03-11 23:05:31]

https://codereview.chromium.org/194303002/diff/70001/content/browser/gpu/gpu_...
File content/browser/gpu/gpu_process_host.cc (right):

https://codereview.chromium.org/194303002/diff/70001/content/browser/gpu/gpu_...
content/browser/gpu/gpu_process_host.cc:756:
GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info);
I don't think this is necessary, as the update will happen through another GPU
message, see here:

https://code.google.com/p/chromium/codesearch#chromium/src/content/gpu/gpu_ch...

[piman, 2014-03-11 23:51:05]

On Tue, Mar 11, 2014 at 3:28 PM, <kbr@chromium.org> wrote:

>
> https://codereview.chromium.org/194303002/diff/70001/
> content/browser/gpu/gpu_process_host.cc
> File content/browser/gpu/gpu_process_host.cc (right):
>
> https://codereview.chromium.org/194303002/diff/70001/
> content/browser/gpu/gpu_process_host.cc#newcode756
>
> content/browser/gpu/gpu_process_host.cc:756:
> GpuDataManagerImpl::GetInstance()->UpdateGpuInfo(gpu_info);
> Why was this "else" branch added? Is there a chance that it will cause
> more GPU info updates to be propagated to renderers and the GPU process?
>

Both of you, please see my comments upthread.

This is needed to avoid races.
GpuChildThread sends GpuHostMsg_GraphicsInfoCollected at the end of
GpuChildThread::OnInitialize which does this too, however
GpuHostMsg_GraphicsInfoCollected is picked up by the UI thread, whereas
there could be a channel request on the IO thread. If the UI thread is
blocked (indeed, maybe waiting for the channel to open), the channel
request will proceed before we can process the
GpuHostMsg_GraphicsInfoCollected, taking an out-of-date info.



>
> https://codereview.chromium.org/194303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2014-03-11 23:53:56]

OK, sorry for missing the comment.

LGTM

[Zhenyao Mo, 2014-03-12 00:02:38]

https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
File content/gpu/gpu_child_thread.cc (right):

https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
content/gpu/gpu_child_thread.cc:161: Send(new
GpuHostMsg_GraphicsInfoCollected(gpu_info_));
By the way, since you update the gpu_info in GpuHostMsg_Initialized message
handler on gpu side, do we still need this?

[piman, 2014-03-12 00:05:43]

On 2014/03/12 00:02:38, Zhenyao Mo wrote:
>
https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
> File content/gpu/gpu_child_thread.cc (right):
> 
>
https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
> content/gpu/gpu_child_thread.cc:161: Send(new
> GpuHostMsg_GraphicsInfoCollected(gpu_info_));
> By the way, since you update the gpu_info in GpuHostMsg_Initialized message
> handler on gpu side, do we still need this?

Currently it sends one extra bit of information, the initialization_time.
That said, I don't think the extra few things it brackets is relevant, so maybe
we could compute it before we send the GpuHostMsg_Initialized, and skip this
message. WDYT?

[Zhenyao Mo, 2014-03-12 00:15:07]

On 2014/03/12 00:05:43, piman wrote:
> On 2014/03/12 00:02:38, Zhenyao Mo wrote:
> >
>
https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
> > File content/gpu/gpu_child_thread.cc (right):
> > 
> >
>
https://codereview.chromium.org/194303002/diff/70001/content/gpu/gpu_child_th...
> > content/gpu/gpu_child_thread.cc:161: Send(new
> > GpuHostMsg_GraphicsInfoCollected(gpu_info_));
> > By the way, since you update the gpu_info in GpuHostMsg_Initialized message
> > handler on gpu side, do we still need this?
> 
> Currently it sends one extra bit of information, the initialization_time.
> That said, I don't think the extra few things it brackets is relevant, so
maybe
> we could compute it before we send the GpuHostMsg_Initialized, and skip this
> message. WDYT?

I agree.

[piman, 2014-03-12 00:32:42]

On Tue, Mar 11, 2014 at 5:15 PM, <zmo@chromium.org> wrote:

> On 2014/03/12 00:05:43, piman wrote:
>
>> On 2014/03/12 00:02:38, Zhenyao Mo wrote:
>> >
>>
>
> https://codereview.chromium.org/194303002/diff/70001/
> content/gpu/gpu_child_thread.cc
>
>> > File content/gpu/gpu_child_thread.cc (right):
>> >
>> >
>>
>
> https://codereview.chromium.org/194303002/diff/70001/
> content/gpu/gpu_child_thread.cc#newcode161
>
>> > content/gpu/gpu_child_thread.cc:161: Send(new
>> > GpuHostMsg_GraphicsInfoCollected(gpu_info_));
>> > By the way, since you update the gpu_info in GpuHostMsg_Initialized
>> message
>> > handler on gpu side, do we still need this?
>>
>
>  Currently it sends one extra bit of information, the initialization_time.
>> That said, I don't think the extra few things it brackets is relevant, so
>>
> maybe
>
>> we could compute it before we send the GpuHostMsg_Initialized, and skip
>> this
>> message. WDYT?
>>
>
> I agree.
>

Done.


>
> https://codereview.chromium.org/194303002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Zhenyao Mo, 2014-03-12 00:51:26]

LGTM again

[Ken Russell (switch to Gerrit), 2014-03-12 01:18:03]

Sorry I missed your earlier comment about the race condition.

LGTM

[piman, 2014-03-12 01:21:54]

The CQ bit was checked by piman@chromium.org

[piman, 2014-03-12 01:25:56]

-cevans +jschuh for gpu_messages.h

[commit-bot: I haz the power, 2014-03-12 01:36:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-12 02:24:05]

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

[commit-bot: I haz the power, 2014-03-12 02:24:06]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[jschuh, 2014-03-12 03:17:27]

ipc security lgtm: simple bool

[jschuh, 2014-03-12 03:17:40]

The CQ bit was checked by jschuh@chromium.org

[commit-bot: I haz the power, 2014-03-12 03:21:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-12 14:58:09]

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

[commit-bot: I haz the power, 2014-03-12 14:58:10]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[piman, 2014-03-12 17:11:38]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-03-12 17:13:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-12 19:22:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-13 11:34:55]

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

[commit-bot: I haz the power, 2014-03-13 11:34:56]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[piman, 2014-03-13 18:10:38]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-03-13 18:11:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-14 00:15:41]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/90001

[commit-bot: I haz the power, 2014-03-14 13:45:26]

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

[commit-bot: I haz the power, 2014-03-14 13:45:27]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[piman, 2014-03-21 03:47:40]

PTAL. The initialization-time GpuInfo coming from the GpuChildThread is bogus
when running in process (including Android) - we previously skipped sending it,
so still skip updating it.

[piman, 2014-03-21 21:14:34]

The CQ bit was checked by piman@chromium.org

[commit-bot: I haz the power, 2014-03-21 21:16:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/piman@chromium.org/194303002/120001

[commit-bot: I haz the power, 2014-03-22 00:37:20]

Change committed as 258721