Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
5861007:
Collect GPU information (vendor id and device id) in Linux.... (Closed)
|
Created:
10 years ago by Zhenyao Mo Modified:
9 years, 7 months ago CC:
chromium-reviews, apatrick_chromium, rpetterson Base URL:
svn://chrome-svn/chrome/trunk/src/ Visibility:
Public. |
DescriptionCollect GPU information (vendor id and device id) in Linux.
BUG=49579
TEST=about:gpu page shows correct vendor-id and device-id in linux
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=69836
Patch Set 1 #
Total comments: 16
Patch Set 2 : Resonding to kbr's review #
Total comments: 1
Messages
Total messages: 7 (0 generated)
On 2010/12/16 21:50:44, Zhenyao Mo wrote: Please note that this patch does not set all the fields in GPUInfo as rlp has a CL almost ready to go for the rest of the fields; besides, those fields are not required for the gpu blacklist logic.
This mostly looks good. The major issue is around the use of a potentially uninitialized character buffer. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... File chrome/gpu/gpu_info_collector_linux.cc (right): http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:123: // This creates an offscreen GL context fir gl queries. Returned GLContext fir -> for http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:194: // gl VENDOR and RENDERER info. Out of curiosity did you test this code path on a machine with more than one graphics card? http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:200: scoped_array<char> buffer(new char[buffer_size]); The contents of this array are uninitialized at this point. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:205: access, buffer.get(), buffer_size, 1, gpu->vendor_id); Is this guaranteed to succeed? If not, how does it indicate failure? If it fails and doesn't touch the contents of "buffer" then "buffer" will contain garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:206: std::string vendor_string = buffer.get(); Must ensure that we don't try to construct a std::string out of garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:207: if (!StartsWithASCII(gl_vendor_string, vendor_string, false)) I think it's good you're using a case-insensitive search here. I'd suggest creating a const bool case_sensitive = false; and passing that value down to make the call sites more self-descriptive. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:211: std::string device_string = buffer.get(); Same caveat about constructing a string out of potential garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:218: if (StartsWithASCII(gl_renderer_string, device_string, false)) { Use case_sensitive here too.
http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... File chrome/gpu/gpu_info_collector_linux.cc (right): http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:123: // This creates an offscreen GL context fir gl queries. Returned GLContext On 2010/12/18 03:03:27, kbr wrote: > fir -> for Done. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:194: // gl VENDOR and RENDERER info. I've tested this path by commenting out the condition temporarily and it worked fine (able to identify the exact card through libpci and glGetString name matching). On 2010/12/18 03:03:27, kbr wrote: > Out of curiosity did you test this code path on a machine with more than one > graphics card? http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:200: scoped_array<char> buffer(new char[buffer_size]); I looked into pci_lookup_name source code. It returns buffer.get() if it successfully found the name; returns NULL if no matching; returns an error message (not using the input buffer.get()) if buffer size is not large enough. So I'll check the returned value of pci_lookup_name, and no need to initialize the buffer. On 2010/12/18 03:03:27, kbr wrote: > The contents of this array are uninitialized at this point. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:205: access, buffer.get(), buffer_size, 1, gpu->vendor_id); See comment above. On 2010/12/18 03:03:27, kbr wrote: > Is this guaranteed to succeed? If not, how does it indicate failure? If it fails > and doesn't touch the contents of "buffer" then "buffer" will contain garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:206: std::string vendor_string = buffer.get(); See comment above. On 2010/12/18 03:03:27, kbr wrote: > Must ensure that we don't try to construct a std::string out of garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:207: if (!StartsWithASCII(gl_vendor_string, vendor_string, false)) On 2010/12/18 03:03:27, kbr wrote: > I think it's good you're using a case-insensitive search here. I'd suggest > creating a > > const bool case_sensitive = false; > > and passing that value down to make the call sites more self-descriptive. Done. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:211: std::string device_string = buffer.get(); See comment above. On 2010/12/18 03:03:27, kbr wrote: > Same caveat about constructing a string out of potential garbage. http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... chrome/gpu/gpu_info_collector_linux.cc:218: if (StartsWithASCII(gl_renderer_string, device_string, false)) { On 2010/12/18 03:03:27, kbr wrote: > Use case_sensitive here too. Done.
Lgtm. I'll try to generalize the dxdiag stuff today. On 2010/12/20 17:36:47, Zhenyao Mo wrote: > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > File chrome/gpu/gpu_info_collector_linux.cc (right): > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:123: // This creates an offscreen GL > context fir gl queries. Returned GLContext > On 2010/12/18 03:03:27, kbr wrote: > > fir -> for > > Done. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:194: // gl VENDOR and RENDERER info. > I've tested this path by commenting out the condition temporarily and it worked > fine (able to identify the exact card through libpci and glGetString name > matching). > > On 2010/12/18 03:03:27, kbr wrote: > > Out of curiosity did you test this code path on a machine with more than one > > graphics card? > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:200: scoped_array<char> buffer(new > char[buffer_size]); > I looked into pci_lookup_name source code. It returns buffer.get() if it > successfully found the name; returns NULL if no matching; returns an error > message (not using the input buffer.get()) if buffer size is not large enough. > So I'll check the returned value of pci_lookup_name, and no need to initialize > the buffer. > > On 2010/12/18 03:03:27, kbr wrote: > > The contents of this array are uninitialized at this point. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:205: access, buffer.get(), buffer_size, > 1, gpu->vendor_id); > See comment above. > > On 2010/12/18 03:03:27, kbr wrote: > > Is this guaranteed to succeed? If not, how does it indicate failure? If it > fails > > and doesn't touch the contents of "buffer" then "buffer" will contain garbage. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:206: std::string vendor_string = > buffer.get(); > See comment above. > > On 2010/12/18 03:03:27, kbr wrote: > > Must ensure that we don't try to construct a std::string out of garbage. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:207: if > (!StartsWithASCII(gl_vendor_string, vendor_string, false)) > On 2010/12/18 03:03:27, kbr wrote: > > I think it's good you're using a case-insensitive search here. I'd suggest > > creating a > > > > const bool case_sensitive = false; > > > > and passing that value down to make the call sites more self-descriptive. > > Done. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:211: std::string device_string = > buffer.get(); > See comment above. > > On 2010/12/18 03:03:27, kbr wrote: > > Same caveat about constructing a string out of potential garbage. > > http://codereview.chromium.org/5861007/diff/1/chrome/gpu/gpu_info_collector_l... > chrome/gpu/gpu_info_collector_linux.cc:218: if > (StartsWithASCII(gl_renderer_string, device_string, false)) { > On 2010/12/18 03:03:27, kbr wrote: > > Use case_sensitive here too. > > Done.
LGTM with addition of one comment. No need for me to re-review. http://codereview.chromium.org/5861007/diff/6001/chrome/gpu/gpu_info_collecto... File chrome/gpu/gpu_info_collector_linux.cc (right): http://codereview.chromium.org/5861007/diff/6001/chrome/gpu/gpu_info_collecto... chrome/gpu/gpu_info_collector_linux.cc:204: if ((interface->pci_lookup_name)(access, Add a comment indicating that the current implementation of pci_lookup_name returns the same pointer as that passed in upon success, and a different one (NULL or a pointer to an error message) upon failure. |
