Don't enable HDCP again if it is active

ui/display/chromeos/display_configurator.cc

 // Copyright 2014 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 "ui/display/chromeos/display_configurator.h"
 
 #include "base/bind.h"
 #include "base/command_line.h"
 #include "base/logging.h"
 #include "base/strings/string_number_conversions.h"
@@ skipping 233 matching lines @@
         all_desired = request_it->second;
     }
 
     switch (it->display->type()) {
       case DISPLAY_CONNECTION_TYPE_UNKNOWN:
         return false;
       // DisplayPort, DVI, and HDMI all support HDCP.
       case DISPLAY_CONNECTION_TYPE_DISPLAYPORT:
       case DISPLAY_CONNECTION_TYPE_DVI:
       case DISPLAY_CONNECTION_TYPE_HDMI: {
-        HDCPState new_desired_state =
-            (all_desired & CONTENT_PROTECTION_METHOD_HDCP) ?
-                HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED;
-        if (!native_display_delegate_->SetHDCPState(*it->display,
-                                                    new_desired_state))
+        HDCPState now_state;

[Daniel Erat, 2014/09/18 14:45:30]

nit: current_state

[kcwu, 2014/09/19 04:39:14]

Done.

+        if (!native_display_delegate_->GetHDCPState(*it->display, &now_state))

[Daniel Erat, 2014/09/18 14:45:30]

does this trigger a slow probe of the display?

[dnicoara, 2014/09/18 15:39:16]

crbug.com/374465 tracked an issue where GetHDCP was slow. Though it should be
fixed.

A couple of follow-up questions:
1) Shouldn't the driver handle this correctly? I'm not particularly familiar
with the API contract and documentation is sparse, but it seems that only
certain drivers disable HDCP before setting the new state.
2) Reading the bug suggests that SetHDCP is called multiple times. Is it done in
a polling manner? It has been suggested that the HDCP state shouldn't be polled.
3) Would it make sense to cache the HDCP state?

It would be nice to expand the interface comments to be more explicit what the
expected behavior of these functions should be.

[kcwu, 2014/09/19 04:39:14]

The driver is fixed. See comment #2 in crbug/412841.

I originally assume the driver can handle this, so I could bypass multiple
enable operation. According the bug, maybe we need to prevent another driver's
fault in the future.
Currently, there is no way to get notification of HDCP state change, so HDCP
state need to be polled in case HDCP is disabled by other program for unknown
reason.
As the reason in 2), we may cache the HDCP state for few seconds for performance
but should not cache longer in user side.

           return false;
+        bool now_desired = (now_state != HDCP_STATE_UNDESIRED);

[Daniel Erat, 2014/09/18 14:45:30]

nit: s/now/current/; "new" and "now" look too similar

[kcwu, 2014/09/19 04:39:15]

Done.

+        bool new_desired = (all_desired & CONTENT_PROTECTION_METHOD_HDCP);
+        // Don't enable again if HDCP is already active.
+        if (now_desired != new_desired) {
+          HDCPState new_state =
+              new_desired ? HDCP_STATE_DESIRED : HDCP_STATE_UNDESIRED;
+          if (!native_display_delegate_->SetHDCPState(*it->display, new_state))
+            return false;
+        }
         break;
       }
       case DISPLAY_CONNECTION_TYPE_INTERNAL:
       case DISPLAY_CONNECTION_TYPE_VGA:
       case DISPLAY_CONNECTION_TYPE_NETWORK:
         // No protections for these types. Do nothing.
         break;
       case DISPLAY_CONNECTION_TYPE_NONE:
         NOTREACHED();
         break;
@@ skipping 679 matching lines @@
         return state_controller_->GetStateForDisplayIds(display_ids);
       }
     }
     default:
       NOTREACHED();
   }
   return MULTIPLE_DISPLAY_STATE_INVALID;
 }
 
 }  // namespace ui