Skip to content

GitLab

T
tangled
Commit bbc4035b authored by Gil Dekel's avatar Gil Dekel Committed by Chromium LUCI CQ
Browse files
Options

display/ui: Restrict Internal Displays to Preferred Mode 

Most internal panels expose a single native mode, and for those
internal displays with additional modes, Chrome does not have
use-cases which require those extra modes.

This CL restrict internal displays to expose only their preferred
mode to Chrome.

Test: ozone_unittests && display_unittests
Change-Id: I0f9480846dc7876992110aca0f28e81384a33cb2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595621Reviewed-by: default avatarDaniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840218}
parent 6342ccf3
Hide whitespace changes
Inline Side-by-side
Showing with 126 additions and 39 deletions
ui/display/manager/configure_displays_task.cc
View file @ bbc4035b
......@@ -16,6 +16,7 @@
#include "base/stl_util.h"
#include "ui/display/manager/display_util.h"
#include "ui/display/types/display_configuration_params.h"
#include "ui/display/types/display_constants.h"
#include "ui/display/types/display_snapshot.h"
#include "ui/display/types/native_display_delegate.h"
......@@ -34,6 +35,12 @@ constexpr int kMaxDisplaysCount = 5;
// nullptr.
const DisplayMode* FindNextMode(const DisplaySnapshot& display_state,
const DisplayMode* display_mode) {
// Internal displays are restricted to their native mode. We do not attempt to
// downgrade their modes upon failure.
if (display_state.type() == DISPLAY_CONNECTION_TYPE_INTERNAL) {
return nullptr;
}
if (!display_mode)
return nullptr;
......@@ -51,6 +58,28 @@ const DisplayMode* FindNextMode(const DisplaySnapshot& display_state,
return best_mode;
}
void LogIfInvalidRequestForInternalDisplay(
const DisplayConfigureRequest& request) {
if (request.display->type() != DISPLAY_CONNECTION_TYPE_INTERNAL)
return;
if (request.mode == nullptr)
return;
if (request.mode == request.display->native_mode())
return;
LOG(ERROR) << "A mode other than the preferred mode was requested for the "
"internal display: preferred="
<< request.display->native_mode()->ToString()
<< " vs. requested=" << request.mode->ToString()
<< ". Current mode="
<< (request.display->current_mode()
? request.display->current_mode()->ToString()
: "nullptr (disabled)")
<< ".";
}
// Samples used to define buckets used by DisplayResolution enum.
// The enum is used to record screen resolution statistics.
const int32_t kDisplayResolutionSamples[] = {1024, 1280, 1440, 1920,
......@@ -185,6 +214,8 @@ void ConfigureDisplaysTask::Run() {
std::vector<display::DisplayConfigurationParams> config_requests;
for (const auto& request : requests_) {
LogIfInvalidRequestForInternalDisplay(request);
config_requests.emplace_back(request.display->display_id(), request.origin,
request.mode);
......
ui/display/manager/configure_displays_task_unittest.cc
View file @ bbc4035b
......@@ -14,6 +14,7 @@
#include "ui/display/manager/configure_displays_task.h"
#include "ui/display/manager/test/action_logger_util.h"
#include "ui/display/manager/test/test_native_display_delegate.h"
#include "ui/display/types/display_constants.h"
namespace display {
namespace test {
......@@ -25,20 +26,24 @@ class ConfigureDisplaysTaskTest : public testing::Test {
ConfigureDisplaysTaskTest()
: delegate_(&log_),
small_mode_(gfx::Size(1366, 768), false, 60.0f),
medium_mode_(gfx::Size(1920, 1080), false, 60.0f),
big_mode_(gfx::Size(2560, 1600), false, 60.0f) {}
~ConfigureDisplaysTaskTest() override = default;
void SetUp() override {
displays_.push_back(FakeDisplaySnapshot::Builder()
.SetId(123)
.SetNativeMode(small_mode_.Clone())
.SetCurrentMode(small_mode_.Clone())
.SetNativeMode(medium_mode_.Clone())
.SetCurrentMode(medium_mode_.Clone())
.AddMode(small_mode_.Clone())
.SetType(DISPLAY_CONNECTION_TYPE_INTERNAL)
.Build());
displays_.push_back(FakeDisplaySnapshot::Builder()
.SetId(456)
.SetNativeMode(big_mode_.Clone())
.SetCurrentMode(big_mode_.Clone())
.AddMode(small_mode_.Clone())
.SetType(DISPLAY_CONNECTION_TYPE_DISPLAYPORT)
.Build());
}
......@@ -56,6 +61,7 @@ class ConfigureDisplaysTaskTest : public testing::Test {
ConfigureDisplaysTask::Status status_ = ConfigureDisplaysTask::ERROR;
const DisplayMode small_mode_;
const DisplayMode medium_mode_;
const DisplayMode big_mode_;
std::vector<std::unique_ptr<DisplaySnapshot>> displays_;
......@@ -71,16 +77,16 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithOneDisplay) {
&ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
std::vector<DisplayConfigureRequest> requests(
1,
DisplayConfigureRequest(displays_[0].get(), &small_mode_, gfx::Point()));
1, DisplayConfigureRequest(displays_[0].get(),
displays_[0]->native_mode(), gfx::Point()));
ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
task.Run();
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::SUCCESS, status_);
EXPECT_EQ(
GetCrtcAction({displays_[0]->display_id(), gfx::Point(), &small_mode_}),
log_.GetActionsAndClear());
EXPECT_EQ(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
displays_[0]->native_mode()}),
log_.GetActionsAndClear());
}
TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplay) {
......@@ -98,7 +104,7 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplay) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::SUCCESS, status_);
EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&big_mode_})
......@@ -120,10 +126,26 @@ TEST_F(ConfigureDisplaysTaskTest, DisableDisplayFails) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::ERROR, status_);
EXPECT_EQ(JoinActions(GetCrtcAction(
{displays_[0]->display_id(), gfx::Point(), nullptr})
.c_str(),
nullptr),
EXPECT_EQ(GetCrtcAction({displays_[0]->display_id(), gfx::Point(), nullptr}),
log_.GetActionsAndClear());
}
TEST_F(ConfigureDisplaysTaskTest, NoModeChangeAttemptWhenInternalDisplayFails) {
ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
&ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
delegate_.set_max_configurable_pixels(1);
std::vector<DisplayConfigureRequest> requests(
1, DisplayConfigureRequest(displays_[0].get(),
displays_[0]->native_mode(), gfx::Point()));
ConfigureDisplaysTask task(&delegate_, requests, std::move(callback));
task.Run();
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::ERROR, status_);
EXPECT_EQ(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
displays_[0]->native_mode()}),
log_.GetActionsAndClear());
}
......@@ -167,13 +189,13 @@ TEST_F(ConfigureDisplaysTaskTest, ConfigureWithTwoDisplayFails) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::ERROR, status_);
EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&big_mode_})
.c_str(),
GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&small_mode_})
......@@ -186,7 +208,7 @@ TEST_F(ConfigureDisplaysTaskTest, ReconfigureLastDisplayPartialSuccess) {
ConfigureDisplaysTask::ResponseCallback callback = base::BindOnce(
&ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
delegate_.set_max_configurable_pixels(medium_mode_.size().GetArea());
std::vector<DisplayConfigureRequest> requests;
for (const auto& display : displays_) {
......@@ -199,13 +221,13 @@ TEST_F(ConfigureDisplaysTaskTest, ReconfigureLastDisplayPartialSuccess) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::PARTIAL_SUCCESS, status_);
EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&big_mode_})
.c_str(),
GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&small_mode_})
......@@ -222,9 +244,10 @@ TEST_F(ConfigureDisplaysTaskTest, ReconfigureMiddleDisplayPartialSuccess) {
.SetId(789)
.SetNativeMode(small_mode_.Clone())
.SetCurrentMode(small_mode_.Clone())
.SetType(DISPLAY_CONNECTION_TYPE_HDMI)
.Build());
delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
delegate_.set_max_configurable_pixels(medium_mode_.size().GetArea());
std::vector<DisplayConfigureRequest> requests;
for (const auto& display : displays_) {
......@@ -237,7 +260,7 @@ TEST_F(ConfigureDisplaysTaskTest, ReconfigureMiddleDisplayPartialSuccess) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::PARTIAL_SUCCESS, status_);
EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&big_mode_})
......@@ -246,7 +269,7 @@ TEST_F(ConfigureDisplaysTaskTest, ReconfigureMiddleDisplayPartialSuccess) {
&small_mode_})
.c_str(),
GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&small_mode_})
......@@ -263,7 +286,7 @@ TEST_F(ConfigureDisplaysTaskTest, AsyncConfigureWithTwoDisplaysPartialSuccess) {
&ConfigureDisplaysTaskTest::ConfigureCallback, base::Unretained(this));
delegate_.set_run_async(true);
delegate_.set_max_configurable_pixels(small_mode_.size().GetArea());
delegate_.set_max_configurable_pixels(medium_mode_.size().GetArea());
std::vector<DisplayConfigureRequest> requests;
for (const auto& display : displays_) {
......@@ -279,13 +302,13 @@ TEST_F(ConfigureDisplaysTaskTest, AsyncConfigureWithTwoDisplaysPartialSuccess) {
EXPECT_TRUE(callback_called_);
EXPECT_EQ(ConfigureDisplaysTask::PARTIAL_SUCCESS, status_);
EXPECT_EQ(JoinActions(GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&big_mode_})
.c_str(),
GetCrtcAction({displays_[0]->display_id(), gfx::Point(),
&small_mode_})
displays_[0]->native_mode()})
.c_str(),
GetCrtcAction({displays_[1]->display_id(), gfx::Point(),
&small_mode_})
......
ui/display/manager/display_configurator_unittest.cc
View file @ bbc4035b
......@@ -19,6 +19,7 @@
#include "ui/display/fake/fake_display_snapshot.h"
#include "ui/display/manager/test/action_logger_util.h"
#include "ui/display/manager/test/test_native_display_delegate.h"
#include "ui/display/types/display_constants.h"
#include "ui/display/util/display_util.h"
namespace display {
......@@ -964,14 +965,35 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
.SetIsAspectPerservingScaling(true)
.Build();
// First test simply fails in MULTIPLE_DISPLAY_STATE_SINGLE mode. This is
// probably unrealistic but we want to make sure any assumptions don't creep
// in.
// Since Chrome restricts the internal display to its native mode it should
// not attempt other available modes. The likelihood of an internal display
// failing to pass a modeset test is low, but we cover this case here.
native_display_delegate_->set_max_configurable_pixels(
modes[2]->size().GetArea());
state_controller_.set_state(MULTIPLE_DISPLAY_STATE_SINGLE);
UpdateOutputs(1, true);
EXPECT_EQ(GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
outputs_[0]->native_mode()}),
log_->GetActionsAndClear());
outputs_[0] = FakeDisplaySnapshot::Builder()
.SetId(kDisplayIds[0])
.SetNativeMode(modes[0]->Clone())
.SetCurrentMode(modes[0]->Clone())
.AddMode(modes[1]->Clone())
.AddMode(modes[2]->Clone())
.AddMode(modes[3]->Clone())
.AddMode(modes[4]->Clone())
.SetType(DISPLAY_CONNECTION_TYPE_DISPLAYPORT)
.SetIsAspectPerservingScaling(true)
.Build();
// This test simply fails in MULTIPLE_DISPLAY_STATE_SINGLE mode for an
// external display (assuming the internal display is disabled; e.g. the lid
// is closed).
UpdateOutputs(1, true);
EXPECT_EQ(JoinActions(GetCrtcAction({outputs_[0]->display_id(),
gfx::Point(0, 0), modes[0].get()})
.c_str(),
......@@ -984,6 +1006,18 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
nullptr),
log_->GetActionsAndClear());
outputs_[0] = FakeDisplaySnapshot::Builder()
.SetId(kDisplayIds[0])
.SetNativeMode(modes[0]->Clone())
.SetCurrentMode(modes[0]->Clone())
.AddMode(modes[1]->Clone())
.AddMode(modes[2]->Clone())
.AddMode(modes[3]->Clone())
.AddMode(modes[4]->Clone())
.SetType(DISPLAY_CONNECTION_TYPE_INTERNAL)
.SetIsAspectPerservingScaling(true)
.Build();
outputs_[1] = FakeDisplaySnapshot::Builder()
.SetId(kDisplayIds[1])
.SetNativeMode(modes[0]->Clone())
......@@ -1005,35 +1039,34 @@ TEST_F(DisplayConfiguratorTest, HandleConfigureCrtcFailure) {
EXPECT_EQ(
JoinActions(
GetCrtcAction(
{outputs_[0]->display_id(), gfx::Point(0, 0), modes[0].get()})
GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
outputs_[0]->native_mode()})
.c_str(),
// Then attempt to configure crtc1 with the first mode.
GetCrtcAction(
{outputs_[1]->display_id(), gfx::Point(0, 0), modes[0].get()})
.c_str(),
// First mode tried is expected to fail and it will
// retry wil the 4th mode in the list.
GetCrtcAction(
{outputs_[0]->display_id(), gfx::Point(0, 0), modes[3].get()})
// retry with the 4th mode in the list (for non-internal displays).
GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
outputs_[0]->native_mode()})
.c_str(),
GetCrtcAction(
{outputs_[1]->display_id(), gfx::Point(0, 0), modes[3].get()})
.c_str(),
// Since it was requested to go into mirror mode
// and the configured modes were different, it
// should now try and setup a valid configurable
// extended mode.
GetCrtcAction(
{outputs_[0]->display_id(), gfx::Point(0, 0), modes[0].get()})
// Since it was requested to go into mirror mode and the configured
// modes were different, it should now try and setup a valid
// configurable extended mode.
GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
outputs_[0]->native_mode()})
.c_str(),
GetCrtcAction({outputs_[1]->display_id(),
gfx::Point(0, modes[0]->size().height() +
DisplayConfigurator::kVerticalGap),
modes[0].get()})
.c_str(),
GetCrtcAction(
{outputs_[0]->display_id(), gfx::Point(0, 0), modes[3].get()})
GetCrtcAction({outputs_[0]->display_id(), gfx::Point(0, 0),
outputs_[0]->native_mode()})
.c_str(),
GetCrtcAction({outputs_[1]->display_id(),
gfx::Point(0, modes[0]->size().height() +
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment