Skip to content

Commit

Permalink
ozone: Display configuration events filtering
Browse files Browse the repository at this point in the history
[Why]
b/232845611 brought forth an HDMI-to-HDMI HDCP-specifc issue in which,
due to the successive disable/enable of external displays during retry
[1][2], a CHANGE event is generated upon display disablement [3], which
spirals CrOS into an vicious modeset loop.

In essence, what happens is:
1) HDCP Type1 is enabled
2) A 4k@59.98Hz is detected and display configuration begins
3) The preferred mode fails, so retry logic begins
4) The internal display is modeset with the external disabled
5) This causes HDCP to issues a CHANGE event
6) Retry logic finds a working mode and succeeds modeset
7) CrOS receives the HDCP CHANGE event and triggers a display
   configuration
8) GOTO (3)

The resulting behavior is flashing internal + external displays, which
renders the system inoperable.

While this case in itself might be a bit niche due to the fact that it
requires an HDMI-to-HDMI connection while HDCP type 1 is enabled and
have the external display trigger retry logic, there could be a risk of
a more general issue in which other properties will issue CHANGE events
upon property change, or upon display disablement.

[How]
Direct Rendering Manager (DRM) device events include the ID of the
triggering property, if one exists. It is therefore possible to avoid
display configuration events if specific events can be rejected by the
triggering property's name.

This CL introduces a display CHANGE event filtering system that queries
DRM (via a roundtrip to the GPU process) whether or not an event should
be allowed to trigger a full display configuration task using the
devices' list of properties (which includes its path and the triggering
property's ID).

In this case, by blocking CHANGE events triggered by the "Content
Protection" property, we avoid the vicious modeset loop described above.

[1] https://chromium-review.googlesource.com/c/chromium/src/+/3579646
[2] https://chromium-review.googlesource.com/c/chromium/src/+/3635865
[3] http://cs/chromeos_public/src/third_party/kernel/v5.4/drivers/gpu/drm/drm_hdcp.c?l=454

(cherry picked from commit 90fcfc789ce41447ce4e2c6bb2149978e892e0cf)

Bug: b:232845611, b:208756849, b:231627795
Test: ozone_unittest && display_unittest && manual testing on delbin
Change-Id: Ie08ff9f7c5805f4c8b458e9ff449f57f73eaf5bb
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3675423
Reviewed-by: Daniel Nicoara <dnicoara@chromium.org>
Reviewed-by: Alex Gough <ajgo@chromium.org>
Reviewed-by: Kevin Schoedel <kpschoedel@chromium.org>
Commit-Queue: Gil Dekel <gildekel@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1010374}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3691001
Cr-Commit-Position: refs/branch-heads/5060@{#624}
Cr-Branched-From: b83393d-refs/heads/main@{#1002911}
  • Loading branch information
Gil Dekel authored and Chromium LUCI CQ committed Jun 7, 2022
1 parent e5f6694 commit ee43124
Show file tree
Hide file tree
Showing 14 changed files with 202 additions and 16 deletions.
8 changes: 6 additions & 2 deletions ui/events/ozone/device/device_event.cc
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,13 @@ namespace ui {

DeviceEvent::DeviceEvent(DeviceType type,
ActionType action,
const base::FilePath& path)
const base::FilePath& path,
const PropertyMap& property_map)
: device_type_(type),
action_type_(action),
path_(path) {}
path_(path),
properties_(property_map) {}

DeviceEvent::~DeviceEvent() = default;

} // namespace ui
14 changes: 13 additions & 1 deletion ui/events/ozone/device/device_event.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,15 @@
#define UI_EVENTS_OZONE_DEVICE_DEVICE_EVENT_H_

#include "base/component_export.h"
#include "base/containers/flat_map.h"
#include "base/files/file_path.h"

namespace ui {
namespace {

using PropertyMap = base::flat_map<std::string, std::string>;

} // namespace

class COMPONENT_EXPORT(EVENTS_OZONE) DeviceEvent {
public:
Expand All @@ -23,19 +29,25 @@ class COMPONENT_EXPORT(EVENTS_OZONE) DeviceEvent {
CHANGE,
};

DeviceEvent(DeviceType type, ActionType action, const base::FilePath& path);
DeviceEvent(DeviceType type,
ActionType action,
const base::FilePath& path,
const PropertyMap& property_map = PropertyMap());

DeviceEvent(const DeviceEvent&) = delete;
DeviceEvent& operator=(const DeviceEvent&) = delete;
~DeviceEvent();

DeviceType device_type() const { return device_type_; }
ActionType action_type() const { return action_type_; }
base::FilePath path() const { return path_; }
const PropertyMap properties() const { return properties_; }

private:
DeviceType device_type_;
ActionType action_type_;
base::FilePath path_;
const PropertyMap properties_;
};

} // namespace ui
Expand Down
19 changes: 16 additions & 3 deletions ui/events/ozone/device/udev/device_manager_udev.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
#include "ui/events/ozone/device/udev/device_manager_udev.h"

#include <stddef.h>
#include <string>

#include "base/logging.h"
#include "base/observer_list.h"
#include "base/strings/stringprintf.h"
#include "base/task/current_thread.h"
#include "base/trace_event/trace_event.h"
#include "device/udev_linux/udev.h"
#include "device/udev_linux/udev_loader.h"
#include "ui/events/ozone/device/device_event.h"
#include "ui/events/ozone/device/device_event_observer.h"

Expand Down Expand Up @@ -159,10 +162,8 @@ void DeviceManagerUdev::OnFileCanWriteWithoutBlocking(int fd) {
std::unique_ptr<DeviceEvent> DeviceManagerUdev::ProcessMessage(
udev_device* device) {
const char* path = device::udev_device_get_devnode(device);
const char* action = device::udev_device_get_action(device);
const char* subsystem =
device::udev_device_get_property_value(device, "SUBSYSTEM");

if (!path || !subsystem)
return nullptr;

Expand All @@ -177,6 +178,7 @@ std::unique_ptr<DeviceEvent> DeviceManagerUdev::ProcessMessage(
else
return nullptr;

const char* action = device::udev_device_get_action(device);
DeviceEvent::ActionType action_type;
if (!action || !strcmp(action, "add"))
action_type = DeviceEvent::ADD;
Expand All @@ -187,8 +189,19 @@ std::unique_ptr<DeviceEvent> DeviceManagerUdev::ProcessMessage(
else
return nullptr;

PropertyMap property_map;
udev_list_entry* property_list =
device::udev_device_get_properties_list_entry(device);
udev_list_entry* entry;
udev_list_entry_foreach(entry, property_list) {
const std::string key(device::udev_list_entry_get_name(entry));
const std::string value(
device::udev_device_get_property_value(device, key.c_str()));
property_map.insert({key, value});
}

return std::make_unique<DeviceEvent>(device_type, action_type,
base::FilePath(path));
base::FilePath(path), property_map);
}

} // namespace ui
5 changes: 5 additions & 0 deletions ui/ozone/platform/drm/common/display_types.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#define UI_OZONE_PLATFORM_DRM_COMMON_DISPLAY_TYPES_H_

#include <memory>
#include <string>

#include "base/containers/flat_map.h"

namespace display {
class DisplaySnapshot;
Expand All @@ -16,6 +19,8 @@ namespace ui {
using MovableDisplaySnapshots =
std::vector<std::unique_ptr<display::DisplaySnapshot>>;

using EventPropertyMap = base::flat_map<std::string, std::string>;

} // namespace ui

#endif // UI_OZONE_PLATFORM_DRM_COMMON_DISPLAY_TYPES_H_
65 changes: 65 additions & 0 deletions ui/ozone/platform/drm/gpu/drm_gpu_display_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,15 @@
#include "ui/ozone/platform/drm/gpu/drm_gpu_display_manager.h"

#include <stddef.h>
#include <cstring>
#include <memory>
#include <string>
#include <utility>

#include "base/containers/flat_map.h"
#include "base/logging.h"
#include "base/metrics/histogram_functions.h"
#include "base/strings/string_number_conversions.h"
#include "ui/display/types/display_mode.h"
#include "ui/display/types/display_snapshot.h"
#include "ui/display/types/gamma_ramp_rgb_entry.h"
Expand All @@ -29,6 +32,10 @@ constexpr char kMultipleDisplayIdsCollisionDetected[] =
using MapDisplayIdToIndexAndSnapshotPair =
base::flat_map<int64_t, display::DisplaySnapshot*>;

// A list of property names that are blocked from issuing a full display
// configuration (modeset) via a udev display CHANGE event.
const char* kBlockedEventsByTriggerProperty[] = {"Content Protection"};

class DisplayComparator {
public:
explicit DisplayComparator(const DrmDisplay* display)
Expand Down Expand Up @@ -98,6 +105,15 @@ bool FindModeForDisplay(
return mode_found;
}

std::string GetEventPropertyByKey(const std::string& key,
const EventPropertyMap event_props) {
const auto it = event_props.find(key);
if (it == event_props.end())
return std::string();

return std::string(it->second);
}

} // namespace

DrmGpuDisplayManager::DrmGpuDisplayManager(ScreenManager* screen_manager,
Expand Down Expand Up @@ -200,6 +216,55 @@ void DrmGpuDisplayManager::RelinquishDisplayControl() {
drm->DropMaster();
}

bool DrmGpuDisplayManager::ShouldDisplayEventTriggerConfiguration(
const EventPropertyMap& event_props) {
DCHECK(!event_props.empty());

const std::string event_seq_num =
GetEventPropertyByKey("SEQNUM", event_props);
std::string log_prefix =
"Display event CHANGE" +
(event_seq_num.empty() ? "" : "(SEQNUM:" + event_seq_num + ") ");
std::string trigger_prop_log;

const std::string event_dev_path =
GetEventPropertyByKey("DEVPATH", event_props);
const DrmDeviceVector& devices = drm_device_manager_->GetDrmDevices();
for (const auto& drm : devices) {
if (drm->device_path().value().find(event_dev_path) == std::string::npos)
continue;

// Get the trigger property's ID and convert to an int.
const std::string trigger_prop_id_str =
GetEventPropertyByKey("PROPERTY", event_props);
if (trigger_prop_id_str.empty())
break;

uint32_t trigger_prop_id;
const bool conversion_success =
base::StringToUint(trigger_prop_id_str, &trigger_prop_id);
DCHECK(conversion_success);

// Fetch the name of the property from the device.
ScopedDrmPropertyPtr drm_property(drm->GetProperty(trigger_prop_id));
DCHECK(drm_property);
trigger_prop_log =
"[trigger property: " + std::string(drm_property->name) + "] ";
for (const char* blocked_prop : kBlockedEventsByTriggerProperty) {
if (strcmp(drm_property->name, blocked_prop) == 0) {
VLOG(1) << log_prefix << trigger_prop_log
<< "resolution: blocked; display configuration task "
"rejected.";
return false;
}
}
}

VLOG(1) << log_prefix << trigger_prop_log
<< "resolution: allowed; display configuration task triggered.";
return true;
}

bool DrmGpuDisplayManager::ConfigureDisplays(
const std::vector<display::DisplayConfigurationParams>& config_requests) {
ScreenManager::ControllerConfigsList controllers_to_configure;
Expand Down
5 changes: 5 additions & 0 deletions ui/ozone/platform/drm/gpu/drm_gpu_display_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,11 @@ class DrmGpuDisplayManager {
bool TakeDisplayControl();
void RelinquishDisplayControl();

// Whether or not a udev display change event triggered by a DRM property
// should go through or get blocked.
bool ShouldDisplayEventTriggerConfiguration(
const EventPropertyMap& event_props);

bool ConfigureDisplays(
const std::vector<display::DisplayConfigurationParams>& config_requests);
bool GetHDCPState(int64_t display_id,
Expand Down
10 changes: 10 additions & 0 deletions ui/ozone/platform/drm/gpu/drm_thread.cc
Original file line number Diff line number Diff line change
Expand Up @@ -412,6 +412,16 @@ void DrmThread::RelinquishDisplayControl(
std::move(callback).Run(true);
}

void DrmThread::ShouldDisplayEventTriggerConfiguration(
const EventPropertyMap& event_props,
base::OnceCallback<void(bool)> callback) {
TRACE_EVENT0("drm", "DrmThread::ShouldDisplayEventTriggerConfiguration");
const bool should_trigger =
display_manager_->ShouldDisplayEventTriggerConfiguration(event_props);

std::move(callback).Run(should_trigger);
}

void DrmThread::AddGraphicsDevice(const base::FilePath& path, base::File file) {
TRACE_EVENT0("drm", "DrmThread::AddGraphicsDevice");
device_manager_->AddDrmDevice(path, std::move(file));
Expand Down
3 changes: 3 additions & 0 deletions ui/ozone/platform/drm/gpu/drm_thread.h
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,9 @@ class DrmThread : public base::Thread,
void TakeDisplayControl(base::OnceCallback<void(bool)> callback) override;
void RelinquishDisplayControl(
base::OnceCallback<void(bool)> callback) override;
void ShouldDisplayEventTriggerConfiguration(
const EventPropertyMap& event_props,
base::OnceCallback<void(bool)> callback) override;
void RefreshNativeDisplays(
base::OnceCallback<void(MovableDisplaySnapshots)> callback) override;
void AddGraphicsDevice(const base::FilePath& path, base::File file) override;
Expand Down
41 changes: 34 additions & 7 deletions ui/ozone/platform/drm/host/drm_display_host_manager.cc
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,19 @@ DrmDisplayHostManager::~DrmDisplayHostManager() {
proxy_->RemoveGpuThreadObserver(this);
}

DrmDisplayHostManager::DisplayEvent::DisplayEvent(
DeviceEvent::ActionType action_type,
const base::FilePath& path,
const EventPropertyMap& properties)
: action_type(action_type), path(path), display_event_props(properties) {}

DrmDisplayHostManager::DisplayEvent::DisplayEvent(const DisplayEvent&) =
default;
DrmDisplayHostManager::DisplayEvent&
DrmDisplayHostManager::DisplayEvent::operator=(const DisplayEvent&) = default;

DrmDisplayHostManager::DisplayEvent::~DisplayEvent() = default;

DrmDisplayHost* DrmDisplayHostManager::GetDisplay(int64_t display_id) {
auto it = std::find_if(displays_.begin(), displays_.end(),
FindDrmDisplayHostById(display_id));
Expand Down Expand Up @@ -313,16 +326,21 @@ void DrmDisplayHostManager::OnDeviceEvent(const DeviceEvent& event) {
if (event.device_type() != DeviceEvent::DISPLAY)
return;

event_queue_.push(DisplayEvent(event.action_type(), event.path()));
event_queue_.push(
DisplayEvent(event.action_type(), event.path(), event.properties()));
ProcessEvent();
}

void DrmDisplayHostManager::ProcessEvent() {
while (!event_queue_.empty() && !task_pending_) {
DisplayEvent event = event_queue_.front();
event_queue_.pop();
auto seqnum_it = event.display_event_props.find("SEQNUM");
const std::string seqnum = seqnum_it == event.display_event_props.end()
? ""
: ("(SEQNUM:" + seqnum_it->second + ")");
VLOG(1) << "Got display event " << kDisplayActionString[event.action_type]
<< " for " << event.path.value();
<< seqnum << " for " << event.path.value();
switch (event.action_type) {
case DeviceEvent::ADD:
if (drm_devices_.find(event.path) == drm_devices_.end()) {
Expand All @@ -342,7 +360,8 @@ void DrmDisplayHostManager::ProcessEvent() {
task_pending_ = base::ThreadTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(&DrmDisplayHostManager::OnUpdateGraphicsDevice,
weak_ptr_factory_.GetWeakPtr()));
weak_ptr_factory_.GetWeakPtr(),
event.display_event_props));
break;
case DeviceEvent::REMOVE:
DCHECK(event.path != primary_graphics_card_path_)
Expand Down Expand Up @@ -374,10 +393,9 @@ void DrmDisplayHostManager::OnAddGraphicsDevice(
ProcessEvent();
}

void DrmDisplayHostManager::OnUpdateGraphicsDevice() {
NotifyDisplayDelegate();
task_pending_ = false;
ProcessEvent();
void DrmDisplayHostManager::OnUpdateGraphicsDevice(
const EventPropertyMap& udev_event_props) {
proxy_->GpuShouldDisplayEventTriggerConfiguration(udev_event_props);
}

void DrmDisplayHostManager::OnRemoveGraphicsDevice(
Expand Down Expand Up @@ -533,6 +551,15 @@ void DrmDisplayHostManager::GpuRelinquishedDisplayControl(bool status) {
display_control_change_pending_ = false;
}

void DrmDisplayHostManager::GpuShouldDisplayEventTriggerConfiguration(
bool should_trigger) {
if (should_trigger)
NotifyDisplayDelegate();

task_pending_ = false;
ProcessEvent();
}

void DrmDisplayHostManager::RunUpdateDisplaysCallback(
display::GetDisplaysCallback callback) const {
std::vector<display::DisplaySnapshot*> snapshots;
Expand Down
11 changes: 8 additions & 3 deletions ui/ozone/platform/drm/host/drm_display_host_manager.h
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,20 @@ class DrmDisplayHostManager : public DeviceEventObserver, GpuThreadObserver {
void GpuUpdatedHDCPState(int64_t display_id, bool status);
void GpuTookDisplayControl(bool status);
void GpuRelinquishedDisplayControl(bool status);
void GpuShouldDisplayEventTriggerConfiguration(bool should_trigger);

private:
struct DisplayEvent {
DisplayEvent(DeviceEvent::ActionType action_type,
const base::FilePath& path)
: action_type(action_type), path(path) {}
const base::FilePath& path,
const EventPropertyMap& properties);
DisplayEvent(const DisplayEvent&);
DisplayEvent& operator=(const DisplayEvent&);
~DisplayEvent();

DeviceEvent::ActionType action_type;
base::FilePath path;
EventPropertyMap display_event_props;
};

// Handle hotplug events sequentially.
Expand All @@ -94,7 +99,7 @@ class DrmDisplayHostManager : public DeviceEventObserver, GpuThreadObserver {
void OnAddGraphicsDevice(const base::FilePath& path,
const base::FilePath& sysfs_path,
std::unique_ptr<DrmDeviceHandle> handle);
void OnUpdateGraphicsDevice();
void OnUpdateGraphicsDevice(const EventPropertyMap& udev_event_props);
void OnRemoveGraphicsDevice(const base::FilePath& path);

void RunUpdateDisplaysCallback(display::GetDisplaysCallback callback) const;
Expand Down

0 comments on commit ee43124

Please sign in to comment.