Add "panel_fitting" GPU feature type and use it for mirror mode.

chromeos/display/output_configurator.cc

 // Copyright (c) 2012 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_configurator.h"
 
 #include <X11/Xlib.h>
 #include <X11/extensions/dpms.h>
 #include <X11/extensions/Xrandr.h>
 
@@ skipping 103 matching lines @@
           two_index += 1;
         }
       }
     }
   }
   XRRFreeOutputInfo(primary);
   XRRFreeOutputInfo(secondary);
   return found;
 }
 
+// Should be called if the internal (built-in) output didn't advertise a mode
+// which would be capable to support mirror mode.
+// Relies on hardware panel fitting support,
+// should not be called if it is not available.
+// Tries to add the native mode of the external output to the internal output,
+// assuming panel fitter hardware will take care of scaling and letterboxing.
+// The RROutput IDs |out_one| and |out_two| are used to look up the modes and
+// configure the internal output,
+// |out_one_mode| and |out_two_mode| are the out-parameters for the modes
+// on the two outputs which will have same resolution.
+// Returns false if it fails to configure the internal output appropriately.
+static bool AddMirrorModeToInternalOutput(Display* display,
+                                          XRRScreenResources* screen,
+                                          RROutput out_one,

[cwolfe, 2012/09/18 13:27:47]

Would prefer output_* for these names. I initially interpreted them as output
parameters, which only two of them actually are.

[ynovikov, 2012/09/21 18:48:27]

Done.

+                                          RROutput out_two,
+                                          RRMode* out_one_mode,
+                                          RRMode* out_two_mode) {
+  XRROutputInfo* out_one_info = XRRGetOutputInfo(display, screen, out_one);
+  XRROutputInfo* out_two_info = XRRGetOutputInfo(display, screen, out_two);
+  bool success = false;
+
+  // Both outputs should be connected in mirror mode
+  if (out_one_info->connection == RR_Connected &&
+      out_two_info->connection == RR_Connected) {
+    bool one_is_internal = OutputConfigurator::IsInternalOutputName(

[cwolfe, 2012/09/18 13:27:47]

Make this a member of OutputConfigurator to shorten that? (or alternatively,
does the utility function need to be a member?)

Could also shorten it to IsInternalOutput(output_one_info)

[ynovikov, 2012/09/18 15:16:21]

I like the idea of IsInternalOutput(output_one_info), Oshima, what do you think?

On 2012/09/18 13:27:47, cwolfe wrote:

[oshima, 2012/09/18 21:11:31]

Fine with me.

[ynovikov, 2012/09/21 18:48:27]

Done.

+        std::string(out_one_info->name));
+    bool two_is_internal = OutputConfigurator::IsInternalOutputName(
+        std::string(out_two_info->name));
+
+    XRROutputInfo* internal_info = NULL;
+    XRROutputInfo* external_info = NULL;
+
+    bool there_is_one_internal_output = true;

[cwolfe, 2012/09/18 13:27:47]

found_internal_output with a comment that it will be set if there is exactly one
internal output?

Also, you could check internal_info != NULL to avoid this flag entirely.

[ynovikov, 2012/09/18 15:16:21]

I think your suggestion will make the code more obscure, I'd like to describe
clearly what I am trying to do.

On 2012/09/18 13:27:47, cwolfe wrote:

[cwolfe, 2012/09/18 20:55:50]

Fair enough. I was first going to suggest a bool flag of found_internal_output,
which made internal_info != NULL pretty obvious.

On 2012/09/18 15:16:21, ynovikov wrote:

[ynovikov, 2012/09/21 18:48:27]

Done.

+
+    // Make sure exactly one of the outputs is internal

[cwolfe, 2012/09/18 13:27:47]

Since it would simplify this, I suggest we ignore the two-internal-outputs case
and just prefer the first output.

[oshima, 2012/09/18 14:31:11]

with some log message in case it indeed detects two internal displays
(due to a bug), and probably DCHECK.

[ynovikov, 2012/09/18 15:16:21]

I agree with Oshima that two-internal-outputs case should not be ignored, so it
is not possible to simplify the code - both internal_info and external_info
should be set to correct values.

But, I can think of a case where two internal displays is not a bug - I remember
some cell phones having a display on both sides of the lid, and I think I saw a
computer which also had something like this, behaving as a tablet with closed
lid and as a notebook with opened lid.

So, I'd treat the "else" case as "we didn't decide yet what the correct
behaviour is in this case". Oshima, what is the appropriate thing to do for
this?

On 2012/09/18 14:31:11, oshima wrote:

[cwolfe, 2012/09/18 20:55:50]

To (possibly) clarify, I was leaning toward "if there are two internal inputs,
use the first one" as that simplifies the logic to:

if (one_is_internal) { ... }
else if (two_is_internal) { ... }
else { /* zero internal */ }

If we genuinely have a device with two internal displays, we're probably going
to be back here adding support for *three* displays. :)

[oshima, 2012/09/18 21:11:31]

I don't think we have to support two internal displays case (so agree with
@cwolfe),
but I'd also like to know it if it ever happens, mainly for
troubleshooting/debugging purpose.

Please remember that the person who'll work on new feature or troubleshoot this
may or may
not be the same person, so extra information can be helpful.

[ynovikov, 2012/09/21 18:48:27]

Done.

+    if (one_is_internal && !two_is_internal) {
+      internal_info = out_one_info;
+      external_info = out_two_info;
+    } else if (!one_is_internal && two_is_internal) {
+      internal_info = out_two_info;
+      external_info = out_one_info;
+    } else {
+      there_is_one_internal_output = false;
+    }
+
+    if (there_is_one_internal_output) {
+      RRMode internal_ideal_mode_id =
+          OutputConfigurator::GetOutputNativeMode(internal_info);
+      RRMode external_ideal_mode_id =
+          OutputConfigurator::GetOutputNativeMode(external_info);
+
+      if (internal_ideal_mode_id != None && external_ideal_mode_id != None) {
+        XRRModeInfo* internal_ideal_mode =
+            ModeInfoForID(screen, internal_ideal_mode_id);
+        XRRModeInfo* external_ideal_mode =
+            ModeInfoForID(screen, external_ideal_mode_id);
+
+        // Panel fitting will not work if the internal output maximal resolution
+        // is lower than that of the external output
+        if (internal_ideal_mode->width >= external_ideal_mode->width &&
+            internal_ideal_mode->height >= external_ideal_mode->height) {
+          XRRAddOutputMode(display, one_is_internal ? out_one : out_two,
+              external_ideal_mode_id);
+
+          *out_one_mode = *out_two_mode = external_ideal_mode_id;

[cwolfe, 2012/09/18 13:27:47]

Merge those parameters?

[ynovikov, 2012/09/18 15:16:21]

Their identity is implementation dependent, if in the future we decide that we
should add a new mode which has the same resolution but a different refresh
rate, different values will be returned in these two parameters.

On 2012/09/18 13:27:47, cwolfe wrote:

[cwolfe, 2012/09/18 20:55:50]

Fair enough.

On 2012/09/18 15:16:21, ynovikov wrote:

[ynovikov, 2012/09/21 18:48:27]

Done.

+          success = true;
+        }
+      }
+    }
+  }
+
+  XRRFreeOutputInfo(out_one_info);
+  XRRFreeOutputInfo(out_two_info);
+
+  return success;
+}
+
 // A helper to call XRRSetCrtcConfig with the given options but some of our
 // default output count and rotation arguments.
 static void ConfigureCrtc(Display* display,
                           XRRScreenResources* screen,
                           RRCrtc crtc,
                           int x,
                           int y,
                           RRMode mode,
                           RROutput output) {
   VLOG(1) << "ConfigureCrtc crtc: " << crtc
@@ skipping 67 matching lines @@
   int height;
   int y;
   RRMode native_mode;
   RRMode mirror_mode;
   bool is_internal;
 } OutputSnapshot;
 
 static int GetDualOutputs(Display* display,
                           XRRScreenResources* screen,
                           OutputSnapshot* one,
-                          OutputSnapshot* two) {
+                          OutputSnapshot* two,
+                          bool is_panel_fitting_supported) {

[cwolfe, 2012/09/18 13:27:47]

Would it be clearer to make these functions members of OutputConfigurator to
avoid the extra parameter?

[ynovikov, 2012/09/18 15:16:21]

Perhaps it would, I am not sure why these functions were not members of
OutputConfigurator in the first place (maybe to hide them from the .h file?).
Oshima, what do you think?

On 2012/09/18 13:27:47, cwolfe wrote:

[ynovikov, 2012/09/21 18:48:27]

Done.

   int found_count = 0;
   XRROutputInfo* one_info = NULL;
   XRROutputInfo* two_info = NULL;
 
   for (int i = 0; (i < screen->noutput) && (found_count < 2); ++i) {
     RROutput this_id = screen->outputs[i];
     XRROutputInfo* output_info = XRRGetOutputInfo(display, screen, this_id);
     bool is_connected = (RR_Connected == output_info->connection);
 
     if (is_connected) {
@@ skipping 17 matching lines @@
         to_populate->height = crtc_info->height;
         to_populate->y = crtc_info->y;
         XRRFreeCrtcInfo(crtc_info);
       } else {
         to_populate->current_mode = 0;
         to_populate->height = 0;
         to_populate->y = 0;
       }
       // Find the native_mode and leave the mirror_mode for the pass after the
       // loop.
-      if (output_info->nmode > 0)
-        to_populate->native_mode = output_info->modes[0];
+      to_populate->native_mode =
+          OutputConfigurator::GetOutputNativeMode(output_info);
       to_populate->mirror_mode = 0;
 
       // See if this output refers to an internal display.
       to_populate->is_internal =
           OutputConfigurator::IsInternalOutputName(
               std::string(output_info->name));
 
       VLOG(1) << "Found display #" << found_count
               << " with output " << (int)to_populate->output
               << " crtc " << (int)to_populate->crtc
               << " current mode " << (int)to_populate->current_mode;
       ++found_count;
     } else {
       XRRFreeOutputInfo(output_info);
     }
   }
 
   if (2 == found_count) {
     // Find the mirror modes (if there are any).
     bool can_mirror = FindMirrorModeForOutputs(display,
                                                screen,
                                                one->output,
                                                two->output,
                                                &one->mirror_mode,
                                                &two->mirror_mode);
     if (!can_mirror) {
-      // We can't mirror so set mirror_mode to 0.
-      one->mirror_mode = 0;
-      two->mirror_mode = 0;
+      bool can_add_mirror_mode = false;
+      if (is_panel_fitting_supported) {
+        can_add_mirror_mode =

[cwolfe, 2012/09/18 13:27:47]

added_mirror_mode?

[ynovikov, 2012/09/18 15:16:21]

Sounds good to me, how about mirror_mode_found for the first one and
mirror_mode_added for the second one?

On 2012/09/18 13:27:47, cwolfe wrote:

[cwolfe, 2012/09/18 20:55:50]

SGTM

On 2012/09/18 15:16:21, ynovikov wrote:

[ynovikov, 2012/09/21 18:48:27]

Done.

+            AddMirrorModeToInternalOutput(display, screen,
+                one->output, two->output, &one->mirror_mode, &two->mirror_mode);
+      }
+      if (!can_add_mirror_mode) {
+        // We can't mirror so set mirror_mode to 0.
+        one->mirror_mode = 0;
+        two->mirror_mode = 0;
+      }
     }
   }
 
   XRRFreeOutputInfo(one_info);
   XRRFreeOutputInfo(two_info);
   return found_count;
 }
 
 static OutputState InferCurrentState(Display* display,
                                      XRRScreenResources* screen,
@@ skipping 265 matching lines @@
 
   // "Projecting" is defined as having more than 1 output connected while at
   // least one of them is an internal output.
   return has_internal_output && (connected_output_count > 1);
 }
 
 }  // namespace
 
 OutputConfigurator::OutputConfigurator()
     : is_running_on_chrome_os_(base::chromeos::IsRunningOnChromeOS()),
+      is_panel_fitting_supported_(false),
+      connected_output_count_(0),
       xrandr_event_base_(0),
       output_state_(STATE_INVALID) {
+}
+
+void OutputConfigurator::Init(bool is_panel_fitting_supported) {
   if (!is_running_on_chrome_os_)
     return;
 
+  is_panel_fitting_supported_ = is_panel_fitting_supported;
+
   // Cache the initial output state.
   Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
   CHECK(display != NULL);
   XGrabServer(display);
   Window window = DefaultRootWindow(display);
   XRRScreenResources* screen = GetScreenResourcesAndRecordUMA(display, window);
   CHECK(screen != NULL);
 
   // Detect our initial state.
   OutputSnapshot outputs[2] = { {0}, {0} };
   connected_output_count_ =
-      GetDualOutputs(display, screen, &outputs[0], &outputs[1]);
+      GetDualOutputs(display, screen, &outputs[0], &outputs[1],

[cwolfe, 2012/09/18 13:27:47]

Yeah, I'd prefer making GetDualOutputs a non-static member of
OutputConfigurator.

[ynovikov, 2012/09/21 18:48:27]

Done.

+          is_panel_fitting_supported_);
   output_state_ =
       InferCurrentState(display, screen, outputs, connected_output_count_);
   // Ensure that we are in a supported state with all connected displays powered
   // on.
   OutputState starting_state = GetNextState(display,
                                             screen,
                                             STATE_INVALID,
                                             outputs,
                                             connected_output_count_);
   if (output_state_ != starting_state &&
@@ skipping 30 matching lines @@
   bool did_change = false;
   Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
   CHECK(display != NULL);
   XGrabServer(display);
   Window window = DefaultRootWindow(display);
   XRRScreenResources* screen = GetScreenResourcesAndRecordUMA(display, window);
   CHECK(screen != NULL);
 
   OutputSnapshot outputs[2] = { {0}, {0} };
   connected_output_count_ =
-      GetDualOutputs(display, screen, &outputs[0], &outputs[1]);
+      GetDualOutputs(display, screen, &outputs[0], &outputs[1],
+          is_panel_fitting_supported_);
   OutputState original =
       InferCurrentState(display, screen, outputs, connected_output_count_);
   OutputState next_state =
       GetNextState(display, screen, original, outputs, connected_output_count_);
   if (original != next_state &&
       EnterState(display,
                  screen,
                  window,
                  next_state,
                  outputs,
@@ skipping 19 matching lines @@
   bool success = false;
   Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
   CHECK(display != NULL);
   XGrabServer(display);
   Window window = DefaultRootWindow(display);
   XRRScreenResources* screen = GetScreenResourcesAndRecordUMA(display, window);
   CHECK(screen != NULL);
 
   OutputSnapshot outputs[2] = { {0}, {0} };
   connected_output_count_ =
-      GetDualOutputs(display, screen, &outputs[0], &outputs[1]);
+      GetDualOutputs(display, screen, &outputs[0], &outputs[1],
+          is_panel_fitting_supported_);
   output_state_ =
       InferCurrentState(display, screen, outputs, connected_output_count_);
 
   RRCrtc crtc = None;
   // Set the CRTCs based on whether we want to turn the power on or off and
   // select the outputs to operate on by name or all_displays.
   for (int i = 0; i < connected_output_count_; ++i) {
     if (all_displays || outputs[i].is_internal || power_on) {
       const int x = 0;
       const int y = outputs[i].y;
@@ skipping 39 matching lines @@
 
   Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
   CHECK(display != NULL);
   XGrabServer(display);
   Window window = DefaultRootWindow(display);
   XRRScreenResources* screen = GetScreenResourcesAndRecordUMA(display, window);
   CHECK(screen != NULL);
 
   OutputSnapshot outputs[2] = { {0}, {0} };
   connected_output_count_ =
-      GetDualOutputs(display, screen, &outputs[0], &outputs[1]);
+      GetDualOutputs(display, screen, &outputs[0], &outputs[1],
+          is_panel_fitting_supported_);
   if (EnterState(display,
                  screen,
                  window,
                  new_state,
                  outputs,
                  connected_output_count_)) {
     output_state_ = new_state;
   }
 
   XRRFreeScreenResources(screen);
@@ skipping 23 matching lines @@
       Display* display = base::MessagePumpAuraX11::GetDefaultXDisplay();
       CHECK(display != NULL);
       XGrabServer(display);
       Window window = DefaultRootWindow(display);
       XRRScreenResources* screen =
           GetScreenResourcesAndRecordUMA(display, window);
       CHECK(screen != NULL);
 
       OutputSnapshot outputs[2] = { {0}, {0} };
       int new_output_count =
-          GetDualOutputs(display, screen, &outputs[0], &outputs[1]);
+          GetDualOutputs(display, screen, &outputs[0], &outputs[1],
+              is_panel_fitting_supported_);
       if (new_output_count != connected_output_count_) {
         connected_output_count_ = new_output_count;
         OutputState new_state = GetNextState(display,
                                              screen,
                                              STATE_INVALID,
                                              outputs,
                                              connected_output_count_);
         if (EnterState(display,
                        screen,
                        window,
@@ skipping 22 matching lines @@
 
 void OutputConfigurator::RemoveObserver(Observer* observer) {
   observers_.RemoveObserver(observer);
 }
 
 // static
 bool OutputConfigurator::IsInternalOutputName(const std::string& name) {
   return name.find(kInternal_LVDS) == 0 || name.find(kInternal_eDP) == 0;
 }
 
+// static
+RRMode OutputConfigurator::GetOutputNativeMode(

[cwolfe, 2012/09/18 13:27:47]

Seems like this should be "GetOutputPreferredMode"

Unsure whether we need to check |output_info->npreferred| as well. If that is
regularly non-one, we might want to actually look for the highest-resolution
preferred mode.

[ynovikov, 2012/09/18 15:16:21]

No, what this function should return is the Native mode, not a Preferred mode.
And from what we've seen in Xrandr specification, it is possible that there are
several preferred modes.
This code is just a refactoring, the heuristic to use modes[0] as the native
mode worked before.
If we do find out cases where it is wrong, we can have a more complex algorithm,
like if npreferred is valid, search for highest resolution in modes up to
npreferred, otherwise search for highest resolution in all modes.

On 2012/09/18 13:27:47, cwolfe wrote:

[cwolfe, 2012/09/18 20:55:50]

I dislike the implication that we know anything about the native mode, but don't
feel that strongly. What this returns is a single mode that we figure the user
will prefer.

Your call.

On 2012/09/18 15:16:21, ynovikov wrote:

[ynovikov, 2012/09/21 18:48:27]

Done.

+    const XRROutputInfo* output_info) {
+  RRMode native_mode = None;
+
+  if (output_info->nmode > 0) {

[cwolfe, 2012/09/18 13:27:47]

This is one of the places I actually like early returns. How about:

if (output_info->nmode == 0)
  return None;

return output_info->modes[0];

I assume there is actually a case where nmode *can* be zero?

[oshima, 2012/09/18 14:31:11]

+1 This matches chrome preferred style.

[ynovikov, 2012/09/21 18:48:27]

Done.

+    native_mode = output_info->modes[0];
+  }
+
+  return native_mode;
+}
+
 void OutputConfigurator::NotifyOnDisplayChanged() {
   FOR_EACH_OBSERVER(Observer, observers_, OnDisplayModeChanged());
 }
 
 }  // namespace chromeos