Changes how to compute the id of a display.

chromeos/display/output_util.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chromeos/display/output_util.h"
 
 #include <X11/Xlib.h>
 #include <X11/extensions/Xrandr.h>
 #include <X11/Xatom.h>
 
+#include "base/hash.h"
 #include "base/message_loop.h"
 #include "base/string_util.h"
 #include "base/sys_byteorder.h"
 
 namespace chromeos {
 namespace {
 
 // Prefixes for the built-in displays.
 const char kInternal_LVDS[] = "LVDS";
 const char kInternal_eDP[] = "eDP";
 
 // Returns 64-bit persistent ID for the specified manufacturer's ID and
-// product_code, and the index of the output it is connected to.
+// product_name, and the index of the output it is connected to.
 // |output_index| is used to distinguish the displays of the same type. For
 // example, swapping two identical display between two outputs will not be
 // treated as swap. The 'serial number' field in EDID isn't used here because
 // it is not guaranteed to have unique number and it may have the same fixed
 // value (like 0).
 int64 GetID(uint16 manufacturer_id,
-            uint16 product_code,
+            uint32 product_code,
             uint8 output_index) {
-  return ((static_cast<int64>(manufacturer_id) << 24) |
+  return ((static_cast<int64>(manufacturer_id) << 40) |
           (static_cast<int64>(product_code) << 8) | output_index);
 }
 
 bool IsRandRAvailable() {
   int randr_version_major = 0;
   int randr_version_minor = 0;
   static bool is_randr_available = XRRQueryVersion(
       base::MessagePumpAuraX11::GetDefaultXDisplay(),
       &randr_version_major, &randr_version_minor);
   return is_randr_available;
@@ skipping 71 matching lines @@
 
 std::string GetDisplayName(XID output_id) {
   std::string display_name;
   GetOutputDeviceData(output_id, NULL, NULL, &display_name);
   return display_name;
 }
 
 bool GetDisplayId(XID output_id, size_t output_index, int64* display_id_out) {
   uint16 manufacturer_id = 0;
   uint16 product_code = 0;
-  if (GetOutputDeviceData(
-          output_id, &manufacturer_id, &product_code, NULL) &&
-      manufacturer_id != 0) {
+  std::string product_name;
+
+  // GetOutputDeviceData fails if it doesn't have product_name.
+  GetOutputDeviceData(
+      output_id, &manufacturer_id, &product_code, &product_name);
+
+  // Generates product specific value from product_name. See crbug.com/240341
+  // for why we prefer product name string instead of product code in EDID.
+  uint32 product_specific_code = product_name.empty() ?
+      static_cast<uint32>(product_code) : base::Hash(product_name);

[oshima, 2013/05/20 20:29:40]

May be we should mask the product_code with 0xFFFFC?
I think it's better to collide than having different code for the same display,
given that this code path should
be really rare. WDYT?

[marcheu, 2013/05/20 20:54:43]

I think it's fine either way. It's probably also fine (and lower impact) to
replace 9846 with 9845 when we encounter it...

[Jun Mukai, 2013/05/21 01:28:25]

Hmm, I'm not sure what's the mask should be... is 0xFFFFFC good enough?  The
specific issue we've met is 9846/9845, but some other vendors may have some
other code pairs, right?
Indeed this is anyway really rare code path though... (as far as I see, only
internal displays don't have name fields).

[oshima, 2013/05/21 01:40:00]

0xFFFC (sorry it was typo) produces 9844 for 9846/9845. I think
this is something we just need to address case by case
(there is no guarantee that name field doesn't change when a display
switches analog/display mode, for example). Since having
different ID for the same display is worse than having two
displays with the same ID, being conservative is better IMO.

That's being said, I don't feel so strongly about it, so existing
code is okay if you prefer.

+  if (manufacturer_id != 0) {
     // An ID based on display's index will be assigned later if this call
     // fails.
-    *display_id_out = GetID(manufacturer_id, product_code, output_index);
+    *display_id_out = GetID(
+        manufacturer_id, product_specific_code, output_index);
     return true;
   }
   return false;
 }
 
 bool ParseOutputDeviceData(const unsigned char* prop,
                            unsigned long nitems,
                            uint16* manufacturer_id,
                            uint16* product_code,
                            std::string* human_readable_name) {
@@ skipping 170 matching lines @@
   }
 
   return false;
 }
 
 bool IsInternalOutputName(const std::string& name) {
   return name.find(kInternal_LVDS) == 0 || name.find(kInternal_eDP) == 0;
 }
 
 }  // namespace chromeos