Skip to content

Commit 273d735

Browse files
moooyoYu Leng (from Dev Box)claude
authored
[PowerDisplay] Confirm before enabling module; log EdidId in Phase 0 (#48111)
## Summary of the Pull Request Two crash-correlation aids for the kernel-side DDC/CI BSOD mitigated by #47734: 1. Log EDID hardware ID (manufacturer + product code, e.g. `DELD1A8`) during Phase 0 monitor classification, before any DDC/CI capability fetch enters the BSOD risk window. 2. Show a confirmation dialog before turning the Power Display module on from the Settings page, so the user understands the BSOD risk before the first capability fetch runs. ## PR Checklist - [ ] Closes: #xxx - [x] **Communication:** I've discussed this with core contributors already. If the work hasn't been agreed, this work might be rejected - [x] **Tests:** Added/updated and all pass - [x] **Localization:** All end-user-facing strings can be localized - [ ] **Dev docs:** Added/updated - [ ] **New binaries:** Added on the required places - [ ] [JSON for signing](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ESRPSigning_core.json) for new binaries - [ ] [WXS for installer](https://github.com/microsoft/PowerToys/blob/main/installer/PowerToysSetup/Product.wxs) for new binaries and localization folder - [ ] [YML for CI pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/ci/templates/build-powertoys-steps.yml) for new test projects - [ ] [YML for signed pipeline](https://github.com/microsoft/PowerToys/blob/main/.pipelines/release.yml) - [ ] **Documentation updated:** If checked, please file a pull request on [our docs repo](https://github.com/MicrosoftDocs/windows-uwp/tree/docs/hub/powertoys) and link it here: #xxx ## Detailed Description of the Pull Request / Additional comments ### 1. Phase 0 EdidId logging `MonitorIdentity.EdidIdFromDevicePath` parses the EDID hardware ID segment from a DevicePath of the form ``\?\DISPLAY#DELD1A8#5&abc&0&UID12345#{guid}`` and returns ``DELD1A8``. The 3-letter PNP manufacturer code + 4-hex product code is identical for every physical unit of the same model, so it identifies the *model* without leaking per-unit identifiers. `MonitorManager` logs the EdidId on the existing Phase 0 classification line. Phase 0 uses `QueryDisplayConfig`, which reads OS-cached EDID and cannot BSOD, so this line is guaranteed on disk before the crash-prone Phase 2 capability fetch starts. If a machine crashes during enumeration, the recovered log identifies every attached model (including same-model duplicates), which makes it possible to correlate crash reports to specific monitor models even when the user can't tell us which monitor caused the crash. ### 2. Enable-module confirmation dialog `PowerDisplayViewModel.IsEnabled` setter is refactored to follow the same two-phase pattern already used by `MaxCompatibilityMode`: - `false → true` does not commit immediately; it kicks off `ConfirmAndEnableModuleAsync`, which awaits the existing `DangerousFeatureWarningDialog` (resource prefix `PowerDisplay_EnableModule`) and either commits or reverts the ToggleSwitch via `OnPropertyChanged`. - `true → false` commits unconditionally — we never block a user who wants to turn the module off. - App-startup loads via `InitializeEnabledValue()` / `RefreshEnabledState()` assign the `_isEnabled` field directly, bypassing the setter, so the dialog never fires on settings restore or GPO refresh. - GPO-configured state still short-circuits before any dialog logic. The dialog reuses the existing `DangerousFeatureWarningDialog` injected by `PowerDisplayPage.xaml.cs`. The 5 new `PowerDisplay_EnableModule_*` strings explain that the BSOD is in Windows (not Power Display), that Power Display will auto-disable itself after a detected crash, and that the user has to re-enable + dismiss the warning each time. ## Validation Steps Performed - Built `src/settings-ui` and `src/modules/powerdisplay` locally. - Unit tests: added `EdidIdFromDevicePath_*` cases to `MonitorIdentityTests`, all green. - Settings UI manual: toggling Power Display ON now shows the warning dialog. Pressing Cancel reverts the ToggleSwitch visually; pressing Enable commits and the module starts. Toggling OFF does not prompt. Restarting Settings UI with PowerDisplay enabled does not prompt. GPO-disabled state still locks the toggle. - Log inspection: `MonitorManager` Phase 0 log now shows `EdidId=...` for each path before any capability fetch. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Yu Leng (from Dev Box) <yuleng@microsoft.com> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent bcf0b68 commit 273d735

7 files changed

Lines changed: 208 additions & 49 deletions

File tree

src/modules/powerdisplay/PowerDisplay.Lib.UnitTests/MonitorIdentityTests.cs

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,31 @@ public void EdidIdFromMonitorId_NewFormat_ReturnsMiddleSegment()
120120
Assert.AreEqual("DELD1A8", MonitorIdentity.EdidIdFromMonitorId(input));
121121
}
122122

123+
[TestMethod]
124+
public void EdidIdFromMonitorId_RawDevicePathWithTrailingGuid_ReturnsMiddleSegment()
125+
{
126+
// The Phase 0 classification log calls EdidIdFromMonitorId with a raw
127+
// QueryDisplayConfig DevicePath (still carrying the trailing "#{guid}" segment).
128+
// The helper must extract the EdidId correctly for that form too.
129+
var input = @"\\?\DISPLAY#DELD1A8#5&abc123&0&UID12345#{e6f07b5f-ee97-4a90-b076-33f57bf4eaa7}";
130+
131+
Assert.AreEqual("DELD1A8", MonitorIdentity.EdidIdFromMonitorId(input));
132+
}
133+
134+
[TestMethod]
135+
public void EdidIdFromMonitorId_SameModelMonitorsProduceSameId()
136+
{
137+
// Two Dell U2723QE on different ports share an EdidId but have different UIDs.
138+
// Logging the EdidId identifies the model for crash correlation without leaking
139+
// per-unit identifiers.
140+
var portA = @"\\?\DISPLAY#DELD1A8#5&abc&0&UID111#{guid}";
141+
var portB = @"\\?\DISPLAY#DELD1A8#5&xyz&0&UID222#{guid}";
142+
143+
Assert.AreEqual(
144+
MonitorIdentity.EdidIdFromMonitorId(portA),
145+
MonitorIdentity.EdidIdFromMonitorId(portB));
146+
}
147+
123148
[TestMethod]
124149
public void EdidIdFromMonitorId_NullEmptyOrMalformed_ReturnsEmpty()
125150
{

src/modules/powerdisplay/PowerDisplay.Lib/Models/MonitorIdentity.cs

Lines changed: 8 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -94,13 +94,15 @@ public static string PnpHardwareKeyFromInstanceName(string? instanceName)
9494
}
9595

9696
/// <summary>
97-
/// Extract the EDID PnP identifier from a new-format <c>Monitor.Id</c>. The new format
98-
/// is the DevicePath returned by <c>QueryDisplayConfig</c> with the trailing device-class
99-
/// GUID suffix stripped; the EdidId sits between the leading <c>"\\?\DISPLAY#"</c> and
100-
/// the next <c>#</c>.
97+
/// Extract the EDID PnP identifier (3-letter PNP manufacturer + 4-hex product code,
98+
/// e.g. <c>"DELD1A8"</c>) from either a new-format <c>Monitor.Id</c> or a raw
99+
/// <c>QueryDisplayConfig</c> DevicePath. The EdidId sits between the leading
100+
/// <c>"\\?\DISPLAY#"</c> and the next <c>#</c>, and is identical for every physical
101+
/// monitor of the same model — use it for "which model" crash correlation without
102+
/// leaking per-unit identifiers.
101103
/// </summary>
102-
/// <param name="monitorId">A Monitor.Id in the new DevicePath-based format.</param>
103-
/// <returns>EdidId segment (e.g. <c>"DELD1A8"</c>), or empty string if the input is not a new-format Id.</returns>
104+
/// <param name="monitorId">A Monitor.Id (no trailing <c>#{guid}</c>) or a raw DevicePath (with trailing <c>#{guid}</c>).</param>
105+
/// <returns>EdidId segment (e.g. <c>"DELD1A8"</c>), or empty string if the input is not a recognized form.</returns>
104106
public static string EdidIdFromMonitorId(string? monitorId)
105107
{
106108
if (string.IsNullOrEmpty(monitorId))

src/modules/powerdisplay/PowerDisplay/Helpers/MonitorManager.cs

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -204,8 +204,16 @@ private static void LogClassificationSummary(
204204
: info.OutputTechnology.ToString(CultureInfo.InvariantCulture);
205205
var classification = info.IsInternal ? "Internal" : "External";
206206

207+
// Log EdidId (manufacturer+product code from EDID) up front, before any
208+
// DDC/CI capability fetch runs. QueryDisplayConfig reads OS-cached EDID and
209+
// cannot BSOD, so this line is guaranteed on disk before the crash-prone
210+
// Phase 2 fetch starts — recovered logs identify every attached model
211+
// (and same-model duplicates) for crash correlation.
212+
var edidId = MonitorIdentity.EdidIdFromMonitorId(info.DevicePath);
213+
var edidIdField = string.IsNullOrEmpty(edidId) ? "?" : edidId;
214+
207215
Logger.LogInfo(
208-
$" [Path {info.MonitorNumber}] {info.GdiDeviceName} / \"{info.FriendlyName}\": " +
216+
$" [Path {info.MonitorNumber}] EdidId={edidIdField} {info.GdiDeviceName} / \"{info.FriendlyName}\": " +
209217
$"OutputTechnology={techValue}{classification}");
210218
}
211219

src/settings-ui/Settings.UI/SettingsXAML/Views/DangerousFeatureWarningDialog.xaml

Lines changed: 12 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -15,10 +15,18 @@
1515
Foreground="{ThemeResource SystemFillColorCriticalBrush}"
1616
TextWrapping="Wrap" />
1717
<TextBlock x:Name="WarningDescription" TextWrapping="Wrap" />
18-
<TextBlock
19-
x:Name="WarningList"
20-
Margin="20,0,0,0"
21-
TextWrapping="Wrap" />
18+
<ItemsControl x:Name="WarningList" Margin="20,0,0,0">
19+
<ItemsControl.ItemsPanel>
20+
<ItemsPanelTemplate>
21+
<StackPanel Spacing="4" />
22+
</ItemsPanelTemplate>
23+
</ItemsControl.ItemsPanel>
24+
<ItemsControl.ItemTemplate>
25+
<DataTemplate x:DataType="x:String">
26+
<TextBlock Text="{x:Bind}" TextWrapping="Wrap" />
27+
</DataTemplate>
28+
</ItemsControl.ItemTemplate>
29+
</ItemsControl>
2230
<TextBlock
2331
x:Name="WarningConfirm"
2432
FontWeight="SemiBold"

src/settings-ui/Settings.UI/SettingsXAML/Views/DangerousFeatureWarningDialog.xaml.cs

Lines changed: 35 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -2,30 +2,62 @@
22
// The Microsoft Corporation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5+
using System.Collections.Generic;
56
using Microsoft.PowerToys.Settings.UI.Helpers;
67
using Microsoft.UI.Xaml.Controls;
8+
using Microsoft.Windows.ApplicationModel.Resources;
79

810
namespace Microsoft.PowerToys.Settings.UI.Views
911
{
1012
/// <summary>
1113
/// Confirmation dialog shown when the user enables a feature that can damage the
1214
/// hardware or otherwise leave it in a non-recoverable state. The caller supplies a
13-
/// resource key prefix; the dialog loads "{prefix}_WarningTitle/Header/Description/List/Confirm".
15+
/// resource key prefix; the dialog loads
16+
/// "{prefix}_WarningTitle/Header/Description/WarningList_Item{N}/Confirm".
17+
/// Bullets are prepended in code so translators only see the body text; the
18+
/// item loop probes <c>_WarningList_Item1</c>, <c>_Item2</c>, ... until a missing
19+
/// key is hit, so adding a 4th bullet only requires a new resw entry.
1420
/// </summary>
1521
public sealed partial class DangerousFeatureWarningDialog : ContentDialog
1622
{
23+
// Visual decorations are applied in code so translators only see body text.
24+
private const string WarningHeaderPrefix = "⚠️ ";
25+
private const string BulletPrefix = "• ";
26+
27+
// Hard cap on bullet probes; a real dialog never approaches this.
28+
private const int MaxBulletItems = 10;
29+
30+
// Direct ResourceMap handle so the bullet loop can probe for missing keys with
31+
// TryGetValue (returns null) instead of ResourceLoader.GetString (throws
32+
// "NamedResource Not Found").
33+
private static readonly ResourceMap ResourceMap =
34+
new ResourceManager("PowerToys.Settings.pri").MainResourceMap.GetSubtree("Resources");
35+
1736
public DangerousFeatureWarningDialog(string resourceKeyPrefix)
1837
{
1938
InitializeComponent();
2039

2140
var loader = ResourceLoaderInstance.ResourceLoader;
2241
Title = loader.GetString($"{resourceKeyPrefix}_WarningTitle");
23-
WarningHeader.Text = loader.GetString($"{resourceKeyPrefix}_WarningHeader");
42+
WarningHeader.Text = WarningHeaderPrefix + loader.GetString($"{resourceKeyPrefix}_WarningHeader");
2443
WarningDescription.Text = loader.GetString($"{resourceKeyPrefix}_WarningDescription");
25-
WarningList.Text = loader.GetString($"{resourceKeyPrefix}_WarningList");
2644
WarningConfirm.Text = loader.GetString($"{resourceKeyPrefix}_WarningConfirm");
2745
PrimaryButtonText = loader.GetString("PowerDisplay_Dialog_Enable");
2846
CloseButtonText = loader.GetString("PowerDisplay_Dialog_Cancel");
47+
48+
var items = new List<string>();
49+
for (int i = 1; i <= MaxBulletItems; i++)
50+
{
51+
var candidate = ResourceMap.TryGetValue($"{resourceKeyPrefix}_WarningList_Item{i}");
52+
if (candidate == null || string.IsNullOrEmpty(candidate.ValueAsString))
53+
{
54+
break;
55+
}
56+
57+
items.Add(BulletPrefix + candidate.ValueAsString);
58+
}
59+
60+
WarningList.ItemsSource = items;
2961
}
3062
}
3163
}

src/settings-ui/Settings.UI/Strings/en-us/Resources.resw

Lines changed: 48 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -5579,15 +5579,16 @@ The break timer font matches the text font.</value>
55795579
<value>Confirm Color Temperature Change</value>
55805580
</data>
55815581
<data name="PowerDisplay_ColorTemperature_WarningHeader" xml:space="preserve">
5582-
<value>⚠️ Warning: This is a potentially dangerous operation!</value>
5582+
<value>Warning: This is a potentially dangerous operation!</value>
55835583
</data>
55845584
<data name="PowerDisplay_ColorTemperature_WarningDescription" xml:space="preserve">
55855585
<value>Changing the color temperature setting may cause unpredictable results including:</value>
55865586
</data>
5587-
<data name="PowerDisplay_ColorTemperature_WarningList" xml:space="preserve">
5588-
<value>• Incorrect display colors
5589-
• Display malfunction
5590-
• Settings that cannot be reverted</value>
5587+
<data name="PowerDisplay_ColorTemperature_WarningList_Item1" xml:space="preserve">
5588+
<value>Incorrect display colors or other display malfunction</value>
5589+
</data>
5590+
<data name="PowerDisplay_ColorTemperature_WarningList_Item2" xml:space="preserve">
5591+
<value>Settings that cannot be reverted</value>
55915592
</data>
55925593
<data name="PowerDisplay_ColorTemperature_WarningConfirm" xml:space="preserve">
55935594
<value>Do you want to enable color temperature control for this monitor?</value>
@@ -5608,15 +5609,16 @@ The break timer font matches the text font.</value>
56085609
<value>Confirm power state control</value>
56095610
</data>
56105611
<data name="PowerDisplay_PowerState_WarningHeader" xml:space="preserve">
5611-
<value>⚠️ Warning: This action may be unsafe.</value>
5612+
<value>Warning: This action may be unsafe.</value>
56125613
</data>
56135614
<data name="PowerDisplay_PowerState_WarningDescription" xml:space="preserve">
56145615
<value>Enabling power state control may lead to unexpected behavior, including:</value>
56155616
</data>
5616-
<data name="PowerDisplay_PowerState_WarningList" xml:space="preserve">
5617-
<value>• Monitor may enter standby and not wake via software.
5618-
• You may need to press the monitor’s power button or reconnect the cable to recover.
5619-
• Some monitors may not restore the previous state correctly.</value>
5617+
<data name="PowerDisplay_PowerState_WarningList_Item1" xml:space="preserve">
5618+
<value>Monitor may enter standby and not wake via software — recovery may require the physical power button or reseating the cable.</value>
5619+
</data>
5620+
<data name="PowerDisplay_PowerState_WarningList_Item2" xml:space="preserve">
5621+
<value>Some monitors may not restore the previous state correctly.</value>
56205622
</data>
56215623
<data name="PowerDisplay_PowerState_WarningConfirm" xml:space="preserve">
56225624
<value>Enable power state control for this monitor?</value>
@@ -5625,15 +5627,19 @@ The break timer font matches the text font.</value>
56255627
<value>Confirm input source control</value>
56265628
</data>
56275629
<data name="PowerDisplay_InputSource_WarningHeader" xml:space="preserve">
5628-
<value>⚠️ Warning: This action may be unsafe.</value>
5630+
<value>Warning: This action may be unsafe.</value>
56295631
</data>
56305632
<data name="PowerDisplay_InputSource_WarningDescription" xml:space="preserve">
56315633
<value>Switching input sources may cause unintended results, including:</value>
56325634
</data>
5633-
<data name="PowerDisplay_InputSource_WarningList" xml:space="preserve">
5634-
<value>• Screen may go black if the selected input has no signal.
5635-
• Software control may be lost until you switch back using the monitor’s physical buttons.
5636-
• Some monitors may not reliably expose all input sources.</value>
5635+
<data name="PowerDisplay_InputSource_WarningList_Item1" xml:space="preserve">
5636+
<value>Screen may go black if the selected input has no signal.</value>
5637+
</data>
5638+
<data name="PowerDisplay_InputSource_WarningList_Item2" xml:space="preserve">
5639+
<value>Software control may be lost until you switch back using the monitor’s physical buttons.</value>
5640+
</data>
5641+
<data name="PowerDisplay_InputSource_WarningList_Item3" xml:space="preserve">
5642+
<value>Some monitors may not reliably expose all input sources.</value>
56375643
</data>
56385644
<data name="PowerDisplay_InputSource_WarningConfirm" xml:space="preserve">
56395645
<value>Enable input source control for this monitor?</value>
@@ -5642,19 +5648,41 @@ The break timer font matches the text font.</value>
56425648
<value>Confirm maximum compatibility mode</value>
56435649
</data>
56445650
<data name="PowerDisplay_MaxCompatibility_WarningHeader" xml:space="preserve">
5645-
<value>⚠️ Warning: This may cause unstable monitor behavior.</value>
5651+
<value>Warning: This may cause unstable monitor behavior.</value>
56465652
</data>
56475653
<data name="PowerDisplay_MaxCompatibility_WarningDescription" xml:space="preserve">
56485654
<value>Maximum compatibility mode bypasses your monitor's capabilities string and sends a probe for every supported VCP feature. Some monitors do not handle this well, which may cause:</value>
56495655
</data>
5650-
<data name="PowerDisplay_MaxCompatibility_WarningList" xml:space="preserve">
5651-
<value>• The screen may go black or briefly lose signal during discovery.
5652-
• The monitor may respond unpredictably, including unwanted changes to brightness, contrast, or input source.
5653-
• The monitor may stop responding to DDC/CI until it is power-cycled.</value>
5656+
<data name="PowerDisplay_MaxCompatibility_WarningList_Item1" xml:space="preserve">
5657+
<value>The screen may go black or briefly lose signal during discovery.</value>
5658+
</data>
5659+
<data name="PowerDisplay_MaxCompatibility_WarningList_Item2" xml:space="preserve">
5660+
<value>The monitor may respond unpredictably, including unwanted changes to brightness, contrast, or input source.</value>
5661+
</data>
5662+
<data name="PowerDisplay_MaxCompatibility_WarningList_Item3" xml:space="preserve">
5663+
<value>The monitor may stop responding to DDC/CI until it is power-cycled.</value>
56545664
</data>
56555665
<data name="PowerDisplay_MaxCompatibility_WarningConfirm" xml:space="preserve">
56565666
<value>Only enable this if a monitor is missing from Power Display and you fully understand the side effects listed above. Continue?</value>
56575667
</data>
5668+
<data name="PowerDisplay_EnableModule_WarningTitle" xml:space="preserve">
5669+
<value>Enable Power Display?</value>
5670+
</data>
5671+
<data name="PowerDisplay_EnableModule_WarningHeader" xml:space="preserve">
5672+
<value>Warning: This may cause a system crash on a small number of monitors.</value>
5673+
</data>
5674+
<data name="PowerDisplay_EnableModule_WarningDescription" xml:space="preserve">
5675+
<value>Power Display reads each connected monitor's DDC/CI capabilities at startup. On a small number of monitors with malformed capability strings, this read has been observed to trigger a kernel-side bug that causes Windows to bug-check (BSOD). The underlying issue is in Windows, not in Power Display, but the read is what surfaces it. Possible effects:</value>
5676+
</data>
5677+
<data name="PowerDisplay_EnableModule_WarningList_Item1" xml:space="preserve">
5678+
<value>On affected monitors, the system may crash and restart.</value>
5679+
</data>
5680+
<data name="PowerDisplay_EnableModule_WarningList_Item2" xml:space="preserve">
5681+
<value>If a crash is detected, Power Display will auto-disable itself on next launch; you'll need to re-enable the module and dismiss the warning on the settings page each time.</value>
5682+
</data>
5683+
<data name="PowerDisplay_EnableModule_WarningConfirm" xml:space="preserve">
5684+
<value>Only enable Power Display if you accept the risk above. Continue?</value>
5685+
</data>
56585686
<data name="PowerDisplay_Dialog_Enable" xml:space="preserve">
56595687
<value>Enable</value>
56605688
</data>

0 commit comments

Comments
 (0)