Make SetDisplayPower() take a callback to signal completion

SetDisplayPower() is called by powerd to turn the panel on. After that
powerd will set the backlight level. Since DisplayConfigurator may be
executing the SetDisplayPower() operation asynchronously, the panel
may still be off when powerd tries to set the backlight level. So
powerd's operation would be ignored by the driver.

BUG=chrome-os-partner:35662

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

Cr-Commit-Position: refs/heads/master@{#313983}

chrome/browser/chromeos/dbus/chrome_display_power_service_provider_delegate.cc

@@ -19,7 +19,8 @@ ChromeDisplayPowerServiceProviderDelegate::
 }
 
 void ChromeDisplayPowerServiceProviderDelegate::SetDisplayPower(
-    DisplayPowerState power_state) {
+    DisplayPowerState power_state,
+    const ResponseCallback& callback) {
   // Turning displays off when the device becomes idle or on just before
   // we suspend may trigger a mouse move, which would then be incorrectly
   // reported as user activity.  Let the UserActivityDetector
@@ -27,7 +28,7 @@ void ChromeDisplayPowerServiceProviderDelegate::SetDisplayPower(
   ui::UserActivityDetector::Get()->OnDisplayPowerChanging();
 
   ash::Shell::GetInstance()->display_configurator()->SetDisplayPower(
-      power_state, ui::DisplayConfigurator::kSetDisplayPowerNoFlags);
+      power_state, ui::DisplayConfigurator::kSetDisplayPowerNoFlags, callback);
 }
 
 void ChromeDisplayPowerServiceProviderDelegate::SetDimming(bool dimmed) {

chrome/browser/chromeos/dbus/chrome_display_power_service_provider_delegate.h

@@ -17,7 +17,8 @@ class ChromeDisplayPowerServiceProviderDelegate
   ~ChromeDisplayPowerServiceProviderDelegate() override;
 
   // DisplayPowerServiceProvider::Delegate overrides:
-  void SetDisplayPower(DisplayPowerState power_state) override;
+  void SetDisplayPower(DisplayPowerState power_state,
+                       const ResponseCallback& callback) override;
   void SetDimming(bool dimmed) override;
 
  private:

chromeos/dbus/services/display_power_service_provider.cc

@@ -10,6 +10,17 @@
 
 namespace chromeos {
 
+namespace {
+
+void RunConfigurationCallback(
+    dbus::MethodCall* method_call,
+    dbus::ExportedObject::ResponseSender response_sender,
+    bool status) {
+  response_sender.Run(dbus::Response::FromMethodCall(method_call));
+}
+
+}  // namespace
+
 DisplayPowerServiceProvider::DisplayPowerServiceProvider(
     scoped_ptr<Delegate> delegate)
     : delegate_(delegate.Pass()),
@@ -50,14 +61,15 @@ void DisplayPowerServiceProvider::SetDisplayPower(
     dbus::ExportedObject::ResponseSender response_sender) {
   dbus::MessageReader reader(method_call);
   int int_state = 0;
+  Delegate::ResponseCallback callback =
+      base::Bind(&RunConfigurationCallback, method_call, response_sender);
   if (reader.PopInt32(&int_state)) {
     DisplayPowerState state = static_cast<DisplayPowerState>(int_state);
-    delegate_->SetDisplayPower(state);
+    delegate_->SetDisplayPower(state, callback);
   } else {
     LOG(ERROR) << "Unable to parse " << kSetDisplayPower << " request";
+    callback.Run(false);
   }
-
-  response_sender.Run(dbus::Response::FromMethodCall(method_call));
 }
 
 void DisplayPowerServiceProvider::SetDisplaySoftwareDimming(

chromeos/dbus/services/display_power_service_provider.h

@@ -31,10 +31,14 @@ class CHROMEOS_EXPORT DisplayPowerServiceProvider
  public:
   class Delegate {
    public:
+    typedef base::Callback<void(bool)> ResponseCallback;
+
     virtual ~Delegate() {}
 
-    // Sets the display power state.
-    virtual void SetDisplayPower(DisplayPowerState power_state) = 0;
+    // Sets the display power state. After the display power is set, |callback|
+    // is called with the operation status.
+    virtual void SetDisplayPower(DisplayPowerState power_state,
+                                 const ResponseCallback& callback) = 0;
 
     // Dims or undims the screen.
     virtual void SetDimming(bool dimmed) = 0;

ui/display/chromeos/display_configurator.cc

@@ -32,6 +32,9 @@ const int kConfigureDelayMs = 500;
 // such that we read an up to date state.
 const int kResumeDelayMs = 500;
 
+void DoNothing(bool status) {
+}
+
 }  // namespace
 
 
@@ -460,6 +463,9 @@ DisplayConfigurator::DisplayConfigurator()
 DisplayConfigurator::~DisplayConfigurator() {
   if (native_display_delegate_)
     native_display_delegate_->RemoveObserver(this);
+
+  CallAndClearInProgressCallbacks(false);
+  CallAndClearQueuedCallbacks(false);
 }
 
 void DisplayConfigurator::SetDelegateForTesting(
@@ -759,21 +765,27 @@ void DisplayConfigurator::PrepareForExit() {
 
 void DisplayConfigurator::SetDisplayPower(
     chromeos::DisplayPowerState power_state,
-    int flags) {
-  if (!configure_display_ || display_externally_controlled_)
+    int flags,
+    const ConfigurationCallback& callback) {
+  if (!configure_display_ || display_externally_controlled_) {
+    callback.Run(false);
     return;
+  }
 
   VLOG(1) << "SetDisplayPower: power_state="
           << DisplayPowerStateToString(power_state) << " flags=" << flags
           << ", configure timer="
           << (configure_timer_.IsRunning() ? "Running" : "Stopped");
   if (power_state == requested_power_state_ &&
-      !(flags & kSetDisplayPowerForceProbe))
+      !(flags & kSetDisplayPowerForceProbe)) {
+    callback.Run(true);
     return;
+  }
 
   requested_power_state_ = power_state;
   requested_power_state_change_ = true;
   requested_power_flags_ = flags;
+  queued_configuration_callbacks_.push_back(callback);
 
   RunPendingConfiguration();
 }
@@ -836,7 +848,8 @@ void DisplayConfigurator::SuspendDisplays() {
   // into the "on" state, which greatly reduces resume times.
   if (requested_power_state_ == chromeos::DISPLAY_POWER_ALL_OFF) {
     SetDisplayPower(chromeos::DISPLAY_POWER_ALL_ON,
-                    kSetDisplayPowerOnlyIfSingleInternalDisplay);
+                    kSetDisplayPowerOnlyIfSingleInternalDisplay,
+                    base::Bind(&DoNothing));
 
     // We need to make sure that the monitor configuration we just did actually
     // completes before we return, because otherwise the X message could be
@@ -870,6 +883,7 @@ void DisplayConfigurator::RunPendingConfiguration() {
   if (!ShouldRunConfigurationTask()) {
     LOG(ERROR) << "Called RunPendingConfiguration without any changes"
                   " requested";
+    CallAndClearQueuedCallbacks(true);
     return;
   }
 
@@ -886,6 +900,9 @@ void DisplayConfigurator::RunPendingConfiguration() {
   requested_power_state_change_ = false;
   requested_display_state_ = MULTIPLE_DISPLAY_STATE_INVALID;
 
+  DCHECK(in_progress_configuration_callbacks_.empty());
+  in_progress_configuration_callbacks_.swap(queued_configuration_callbacks_);
+
   configuration_task_->Run();
 }
 
@@ -914,12 +931,18 @@ void DisplayConfigurator::OnConfigured(
 
   configuration_task_.reset();
   NotifyObservers(success, new_display_state);
+  CallAndClearInProgressCallbacks(success);
 
   if (success && !configure_timer_.IsRunning() &&
       ShouldRunConfigurationTask()) {
     configure_timer_.Start(FROM_HERE,
                            base::TimeDelta::FromMilliseconds(kConfigureDelayMs),
                            this, &DisplayConfigurator::RunPendingConfiguration);
+  } else {
+    // If a new configuration task isn't scheduled respond to all queued
+    // callbacks (for example if requested state is current state).
+    if (!configure_timer_.IsRunning())
+      CallAndClearQueuedCallbacks(success);
   }
 }
 
@@ -939,10 +962,25 @@ bool DisplayConfigurator::ShouldRunConfigurationTask() const {
   return false;
 }
 
+void DisplayConfigurator::CallAndClearInProgressCallbacks(bool success) {
+  for (const auto& callback : in_progress_configuration_callbacks_)
+    callback.Run(success);
+
+  in_progress_configuration_callbacks_.clear();
+}
+
+void DisplayConfigurator::CallAndClearQueuedCallbacks(bool success) {
+  for (const auto& callback : queued_configuration_callbacks_)
+    callback.Run(success);
+
+  queued_configuration_callbacks_.clear();
+}
+
 void DisplayConfigurator::RestoreRequestedPowerStateAfterResume() {
   // Force probing to ensure that we pick up any changes that were made while
   // the system was suspended.
-  SetDisplayPower(requested_power_state_, kSetDisplayPowerForceProbe);
+  SetDisplayPower(requested_power_state_, kSetDisplayPowerForceProbe,
+                  base::Bind(&DoNothing));
 }
 
 void DisplayConfigurator::NotifyObservers(

ui/display/chromeos/display_configurator.h

@@ -40,6 +40,8 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   typedef uint64_t ContentProtectionClientId;
   static const ContentProtectionClientId kInvalidClientId = 0;
 
+  typedef base::Callback<void(bool)> ConfigurationCallback;
+
   struct DisplayState {
     DisplayState();
 
@@ -222,9 +224,12 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // Called when powerd notifies us that some set of displays should be turned
   // on or off.  This requires enabling or disabling the CRTC associated with
   // the display(s) in question so that the low power state is engaged.
-  // |flags| contains bitwise-or-ed kSetDisplayPower* values. Returns true if
-  // the system successfully enters (or was already in) |power_state|.
-  void SetDisplayPower(chromeos::DisplayPowerState power_state, int flags);
+  // |flags| contains bitwise-or-ed kSetDisplayPower* values. After the
+  // configuration finishes |callback| is called with the status of the
+  // operation.
+  void SetDisplayPower(chromeos::DisplayPowerState power_state,
+                       int flags,
+                       const ConfigurationCallback& callback);
 
   // Force switching the display mode to |new_state|. Returns false if
   // switching failed (possibly because |new_state| is invalid for the
@@ -334,6 +339,13 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // otherwise.
   bool ShouldRunConfigurationTask() const;
 
+  // Helper functions which will call the callbacks in
+  // |in_progress_configuration_callbacks_| and
+  // |queued_configuration_callbacks_| and clear the lists after. |success| is
+  // the configuration status used when calling the callbacks.
+  void CallAndClearInProgressCallbacks(bool success);
+  void CallAndClearQueuedCallbacks(bool success);
+
   StateController* state_controller_;
   SoftwareMirroringController* mirroring_controller_;
   scoped_ptr<NativeDisplayDelegate> native_display_delegate_;
@@ -367,6 +379,15 @@ class DISPLAY_EXPORT DisplayConfigurator : public NativeDisplayObserver {
   // Bitwise-or value of the |kSetDisplayPower*| flags defined above.
   int requested_power_flags_;
 
+  // List of callbacks from callers waiting for the display configuration to
+  // start/finish. Note these callbacks belong to the pending request, not a
+  // request currently active.
+  std::vector<ConfigurationCallback> queued_configuration_callbacks_;
+
+  // List of callbacks belonging to the currently running display configuration
+  // task.
+  std::vector<ConfigurationCallback> in_progress_configuration_callbacks_;
+
   // True if the caller wants to force the display configuration process.
   bool force_configure_;