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

chromeos/display/output_configurator.cc

Index: chromeos/display/output_configurator.cc
diff --git a/chromeos/display/output_configurator.cc b/chromeos/display/output_configurator.cc
index dcf82029a7b82f197c571e6189f9205d0bab0b33..1a4a72846866266e47f7753a2aa8d0768e21c0f6 100644
--- a/chromeos/display/output_configurator.cc
+++ b/chromeos/display/output_configurator.cc
@@ -121,6 +121,83 @@ static bool FindMirrorModeForOutputs(Display* display,
   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,
@@ -208,7 +285,8 @@ typedef struct 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;
@@ -246,8 +324,8 @@ static int GetDualOutputs(Display* display,
       }
       // 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.
@@ -274,9 +352,17 @@ static int GetDualOutputs(Display* display,
                                                &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;
+      }
     }
   }
 
@@ -562,11 +648,18 @@ static bool IsProjecting(const OutputSnapshot* outputs, int output_count) {
 
 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);
@@ -578,7 +671,8 @@ OutputConfigurator::OutputConfigurator()
   // 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
@@ -629,7 +723,8 @@ bool OutputConfigurator::CycleDisplayMode() {
 
   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 =
@@ -669,7 +764,8 @@ bool OutputConfigurator::ScreenPowerSet(bool power_on, bool all_displays) {
 
   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_);
 
@@ -729,7 +825,8 @@ bool OutputConfigurator::SetDisplayMode(OutputState new_state) {
 
   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,
@@ -773,7 +870,8 @@ bool OutputConfigurator::Dispatch(const base::NativeEvent& event) {
 
       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,
@@ -816,6 +914,18 @@ 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());
 }