From 5f9265658baa1c4c00861445bfaf7846197b0db6 Mon Sep 17 00:00:00 2001 From: Ryan Murray Date: Tue, 13 Oct 2026 13:27:05 +1100 Subject: [PATCH] drm/apple: add guarded RGB8 trial for captured 1080p180 timing A captured external timing has a scored full-range SDR RGB8 color element but lists it as unsafe, so normal policy rejects the mode. Add an explicit boot-time diagnostic that considers that timing-local candidate only when normal selection failed. Match the observed 2080x1106 total and fixed-point 11796583/65536 Hz rate, require progressive nonvirtual metadata, and retain DSC, range, depth, gamut, parser and link restrictions. Keep the option off by default and restrict its caller to external H17P. Diagnostic-added modes never become preferred or supply synthesized EDID preferred timing. Preserve all existing safe selections and other profiles. Document that current-route capture and separate hardware review are required. Validate with 196 production-parser ASan/UBSan cases, metadata/lifetime regressions, the privately retained current-route capture and an isolated W=1 appledrm build using the unchanged R41 config and recorded producer. The capture adds only timing74/color77; all13 normal modes remain identical. No hardware execution or production rate qualification is claimed. Signed-off-by: Ryan Murray Assisted-by: LLM --- drivers/gpu/drm/apple/iomfb_template.c | 12 +++ drivers/gpu/drm/apple/parser.c | 70 ++++++++++--- drivers/gpu/drm/apple/parser.h | 3 + tools/diagnostics/h17p-1080p180-rgb8.md | 37 +++++++ tools/diagnostics/test-apple-external-sdr.py | 100 ++++++++++++++++++- 5 files changed, 205 insertions(+), 17 deletions(-) create mode 100644 tools/diagnostics/h17p-1080p180-rgb8.md diff --git a/drivers/gpu/drm/apple/iomfb_template.c b/drivers/gpu/drm/apple/iomfb_template.c index 34557f7ed1f4e..b29a7a96cbaa2 100644 --- a/drivers/gpu/drm/apple/iomfb_template.c +++ b/drivers/gpu/drm/apple/iomfb_template.c @@ -34,6 +34,13 @@ #include "trace.h" #include "version_utils.h" +#if DCP_FW_VERSION(26, 0, 0) <= DCP_FW_VER +static bool experimental_h17p_1080p180_rgb8; +module_param(experimental_h17p_1080p180_rgb8, bool, 0444); +MODULE_PARM_DESC(experimental_h17p_1080p180_rgb8, + "Diagnose advertised external 1080p180 SDR RGB8 timings marked unsafe"); +#endif + /* Register defines used in bandwidth setup structure */ #define REG_DOORBELL_BIT(idx) (2 + (idx)) @@ -1003,6 +1010,11 @@ static bool dcpep_process_chunks(struct apple_dcp *dcp, } if (!strcmp(req->key, "TimingElements")) { +#if DCP_FW_VERSION(26, 0, 0) <= DCP_FW_VER + ctx.diagnostic_1080p180_rgb8 = experimental_h17p_1080p180_rgb8 && + dcp->hw.iomfb_method_profile == + DCP_IOMFB_METHODS_H17P_EXTERNAL_EXPERIMENTAL; +#endif dcp->modes = enumerate_modes(&ctx, &dcp->nr_modes, dcp->width_mm, dcp->height_mm, dcp->notch_height, diff --git a/drivers/gpu/drm/apple/parser.c b/drivers/gpu/drm/apple/parser.c index 8c2a1069417b6..d7fb1c1955651 100644 --- a/drivers/gpu/drm/apple/parser.c +++ b/drivers/gpu/drm/apple/parser.c @@ -526,10 +526,11 @@ static int parse_timing_color_restrictions(struct dcp_parse_ctx *handle, static int parse_color_modes(struct dcp_parse_ctx *handle, struct dcp_display_mode *out, bool external_sdr, - const struct timing_color_restrictions *limits) + const struct timing_color_restrictions *limits, + struct dcp_color_mode *unsafe_rgb8) { struct iterator outer_it; - unsigned int best_sdr_rank = 0; + unsigned int best_sdr_rank = 0, best_unsafe_rank = 0; int ret = 0; out->sdr_444.score = -1; out->sdr_rgb.score = -1; @@ -601,10 +602,24 @@ static int parse_color_modes(struct dcp_parse_ctx *handle, unsigned int rank; if (cmode.id <= U32_MAX && - (timing_color_contains(&limits->unsafe, cmode.id) || - timing_color_contains(&limits->dsc_required, cmode.id))) + timing_color_contains(&limits->dsc_required, cmode.id)) continue; rank = external_sdr_rank(&cmode); + if (cmode.id <= U32_MAX && + timing_color_contains(&limits->unsafe, cmode.id)) { + /* + * Collect only a timing-local diagnostic candidate. Its + * timing and opt-in scope are checked after the full parse. + */ + if (unsafe_rgb8 && cmode.depth == 8 && rank && + rank >= best_unsafe_rank) { + if (rank > best_unsafe_rank) + unsafe_rgb8->score = -1; + fill_color_mode(unsafe_rgb8, &cmode); + best_unsafe_rank = rank; + } + continue; + } if (!rank || rank < best_sdr_rank) continue; if (rank > best_sdr_rank) @@ -651,6 +666,9 @@ static int parse_mode(struct dcp_parse_ctx *handle, struct iterator it; struct dimension horiz = { .active = -1 }, vert = { .active = -1 }; struct timing_color_restrictions limits = {}; + struct dcp_color_mode unsafe_rgb8 = { .score = -1 }; + bool have_horiz = false, have_vert = false, have_virtual = false; + bool duplicate_scope = false; s64 min_vrr = 0, max_vrr = 0; u64 clock; s64 id = -1; @@ -687,21 +705,29 @@ static int parse_mode(struct dcp_parse_ctx *handle, ret = PTR_ERR(key); else if (is_virtual) ret = skip(it.handle); - else if (!strcmp(key, "HorizontalAttributes")) + else if (!strcmp(key, "HorizontalAttributes")) { + duplicate_scope |= have_horiz; + have_horiz = true; ret = parse_dimension(it.handle, &horiz, external_sdr); - else if (!strcmp(key, "VerticalAttributes")) + } else if (!strcmp(key, "VerticalAttributes")) { + duplicate_scope |= have_vert; + have_vert = true; ret = parse_dimension(it.handle, &vert, external_sdr); - else if (!strcmp(key, "MinimumVariableRefreshRate")) + } else if (!strcmp(key, "MinimumVariableRefreshRate")) ret = parse_int_bound(it.handle, &min_vrr, 0, U32_MAX); else if (!strcmp(key, "MaximumVariableRefreshRate")) ret = parse_int_bound(it.handle, &max_vrr, 0, U32_MAX); else if (!strcmp(key, "ColorModes")) - ret = parse_color_modes(it.handle, out, external_sdr, &limits); + ret = parse_color_modes(it.handle, out, external_sdr, &limits, + handle->diagnostic_1080p180_rgb8 ? + &unsafe_rgb8 : NULL); else if (!strcmp(key, "ID")) ret = parse_int_bound(it.handle, &id, 0, U32_MAX); - else if (!strcmp(key, "IsVirtual")) + else if (!strcmp(key, "IsVirtual")) { + duplicate_scope |= have_virtual; + have_virtual = true; ret = parse_bool(it.handle, &is_virtual); - else if (external_sdr && !strcmp(key, "IsPreferred")) { + } else if (external_sdr && !strcmp(key, "IsPreferred")) { if (have_preferred) ret = -EINVAL; else @@ -726,6 +752,25 @@ static int parse_mode(struct dcp_parse_ctx *handle, return ret; } } + /* + * Never replace a normal candidate or create a timing/color pairing. + * This diagnostic only exposes an otherwise rejected firmware timing; + * it neither repairs link timing nor establishes that "unsafe" is safe. + * The observed 2080x1106 total and 11796583/65536 Hz are deliberate: + * do not widen this to arbitrary nominal-180 Hz blanking or rates. + */ + if (external_sdr && !internal && handle->diagnostic_1080p180_rgb8 && + out->sdr_rgb.score < 0 && unsafe_rgb8.score >= 0 && + !duplicate_scope && have_horiz && have_vert && have_virtual && !is_virtual && + have_interlaced && !interlaced && !notch_height && + horiz.active == 1920 && vert.active == 1080 && + horiz.total == 2080 && vert.total == 1106 && + vert.precise_sync_rate == 11796583 && + horiz.pixel_repetition == 0 && vert.pixel_repetition == 0 && + horiz.sync_polarity >= 0 && vert.sync_polarity >= 0) { + out->sdr_rgb = unsafe_rgb8; + out->unsafe_rgb8_diagnostic = true; + } if (out->sdr_rgb.score >= 0) best_color_mode = out->sdr_rgb.id; else if (out->sdr_444.score >= 0) @@ -802,7 +847,8 @@ static int parse_mode(struct dcp_parse_ctx *handle, if (external_sdr && horiz.sync_polarity >= 0 && vert.sync_polarity >= 0) mode->flags = (horiz.sync_polarity ? DRM_MODE_FLAG_PHSYNC : DRM_MODE_FLAG_NHSYNC) | (vert.sync_polarity ? DRM_MODE_FLAG_PVSYNC : DRM_MODE_FLAG_NVSYNC); - out->metadata_timing_valid = external_sdr && have_preferred && preferred && + out->metadata_timing_valid = external_sdr && !out->unsafe_rgb8_diagnostic && + have_preferred && preferred && have_interlaced && !interlaced && horiz.pixel_repetition == 0 && vert.pixel_repetition == 0 && horiz.sync_polarity >= 0 && vert.sync_polarity >= 0; drm_mode_set_name(mode); @@ -862,7 +908,7 @@ struct dcp_display_mode *enumerate_modes(struct dcp_parse_ctx *handle, /* Process a successful mode */ (*count)++; - if (score > best_score) { + if (!mode->unsafe_rgb8_diagnostic && score > best_score) { best_score = score; best_mode = mode; } diff --git a/drivers/gpu/drm/apple/parser.h b/drivers/gpu/drm/apple/parser.h index 0392b09751ac9..7ba115fe417d6 100644 --- a/drivers/gpu/drm/apple/parser.h +++ b/drivers/gpu/drm/apple/parser.h @@ -15,6 +15,8 @@ struct dcp_parse_ctx { struct apple_dcp *dcp; const void *blob; u32 pos, len; + /* Set only by the opt-in external H17P TimingElements caller. */ + bool diagnostic_1080p180_rgb8; }; enum dcp_color_eotf { @@ -97,6 +99,7 @@ struct dcp_display_mode { struct dcp_color_mode best; bool vrr; bool metadata_timing_valid; + bool unsafe_rgb8_diagnostic; }; struct dimension { diff --git a/tools/diagnostics/h17p-1080p180-rgb8.md b/tools/diagnostics/h17p-1080p180-rgb8.md new file mode 100644 index 0000000000000..a3f87e372b04a --- /dev/null +++ b/tools/diagnostics/h17p-1080p180-rgb8.md @@ -0,0 +1,37 @@ +# H17P external 1080p180 RGB8 diagnostic + +This is a default-off source diagnostic, not a production mode-policy change. +The boot/load-time `experimental_h17p_1080p180_rgb8` appledrm parameter only +reaches the experimental external H17P TimingElements parser. Normal parsing, +other firmware profiles, internal panels and existing valid selections remain +unchanged. The parser resets the opt-in for every new property blob. + +Only an otherwise rejected firmware timing can be exposed: explicit nonvirtual, +progressive 1920x1080 with totals 2080x1106 and the captured fixed-point +rate 11796583/65536 Hz (approximately 180.00157 Hz), with no pixel repetition +and known sync polarities. Its own advertised ColorModes must contain a scored nonvirtual +8-bit full-range SDR RGB candidate in sRGB, BT.709 or Default RGB. The exception +applies only to UnsafeColorElementIDs; DSCRequiredColorElementIDs, malformed +restriction lists, parser bounds and normal link/modeset checks still refuse. +No global color ID is combined with a different timing. The timing fields and +firmware IDs are kept as received, without changing link registers or blanking. + +An added diagnostic timing is never marked preferred and cannot supply the +firmware-derived EDID preferred timing. This does not stop a compositor from +restoring an explicitly saved mode, so use a separately reviewed controlled +session and record the actual selection. Safe modes are not converted to RGB8. + +Before enabling on hardware, capture the current monitor/dock/physical route's +TimingElements and confirm the local candidate, DSC list, full range, depth, +clock and timing. A capture from a different dock/port is not evidence for the +current route. The existing short-vblank firmware warning is not bypassed or +reinterpreted; a successful advertisement alone proves no transport safety. +Physical output, sustained presentation, errors and return to the baseline mode +require separate qualification. Do not infer production safety or relax other +guards from this diagnostic. + +`test-apple-external-sdr.py` exercises the production parser with the option off +and on, malformed/DSC/range/depth/virtual/scope negatives, reversed dictionary +order, timing-local candidates, and non-preferred diagnostic mode handling. +Fixtures are independently generated protocol dictionaries with no private +monitor payloads or identity fields. diff --git a/tools/diagnostics/test-apple-external-sdr.py b/tools/diagnostics/test-apple-external-sdr.py index 695a6059da419..852aae793a935 100644 --- a/tools/diagnostics/test-apple-external-sdr.py +++ b/tools/diagnostics/test-apple-external-sdr.py @@ -74,10 +74,12 @@ def timing(colors, rate=60, identity=203, score=10): cases = [] -def case(name, timings, external=True, count=1, identity=9, depth=8, clock=148500): +def case(name, timings, external=True, count=1, identity=9, depth=8, clock=148500, + diagnostic=False, diagnostic_selected=False, preferred_index=0): out = bytearray(struct.pack(' #include @@ -220,12 +305,14 @@ def restricted(rate=60, unsafe=None, dsc=None, colors=None, before=False): int main(void) { ''' -for i, (name, blob, external, count, identity, depth, clock) in enumerate(cases): +for i, (name, blob, external, count, identity, depth, clock, diagnostic, diagnostic_selected, preferred_index) in enumerate(cases): suffix += f''' {{ - struct dcp_parse_ctx ctx; + struct dcp_parse_ctx ctx = {{ .diagnostic_1080p180_rgb8 = true }}; unsigned int count = 99; assert(parse(blob_{i}, sizeof(blob_{i}), &ctx) == 0); + assert(!ctx.diagnostic_1080p180_rgb8); + ctx.diagnostic_1080p180_rgb8 = {str(diagnostic).lower()}; struct dcp_display_mode *modes = enumerate_modes(&ctx, &count, 600, 340, 0, {str(name.startswith('internal')).lower()}, {str(external).lower()}); if (IS_ERR(modes) || count != {count}) {{ @@ -238,7 +325,10 @@ def restricted(rate=60, unsafe=None, dsc=None, colors=None, before=False): assert(modes[0].color_mode_id == {identity}); assert(modes[0].sdr_rgb.depth == {depth}); assert(modes[0].mode.clock == {clock}); - assert(modes[0].mode.type & DRM_MODE_TYPE_PREFERRED); + assert(modes[0].unsafe_rgb8_diagnostic == {str(diagnostic_selected).lower()}); + assert(!modes[0].unsafe_rgb8_diagnostic || !modes[0].metadata_timing_valid); + for (unsigned int mode = 0; mode < count; mode++) + assert(!!(modes[mode].mode.type & DRM_MODE_TYPE_PREFERRED) == ((int)mode == {preferred_index})); ''' suffix += ' free(modes);\n }\n' suffix += f' puts("PASS: {len(cases)} production binary-parser SDR, malformed-input, legacy and high-refresh cases");\n return 0;\n}}\n'