Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] drm/nouveau/kms/nv50-: 8 bpc fallback in nv50_outp_atomic_check() breaks 6 bpc eDP and LVDS --
commit 7d97154d2ddf364be90f072cc11c87de1735db87 Author: David R. Hedges <[email protected]> drm/nouveau/kms/nv50-: don't drive outputs at 0 bpc This commit falls back to 8 bpc in nv50_outp_atomic_check() and nv50_msto_atomic_check() when connector->display_info.bpc is 0 so that outputs are not driven at 0 bpc if a modeset happens after a probe clears or resets display_info.bpc before ->get_modes() runs. Link: https://gitlab.freedesktop.org/drm/nouveau/-/work_items/515 > diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c > b/drivers/gpu/drm/nouveau/dispnv50/disp.c > index 2c66e480b5116..a265fd971c99c 100644 > --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c > +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c > @@ -418,8 +418,11 @@ nv50_outp_atomic_check(struct drm_encoder *encoder, > if (ret) > return ret; > > + /* display_info.bpc is reset to 0 when a probe loses the EDID, and > + * isn't given a default again until ->get_modes() runs. > + */ > if (crtc_state->mode_changed || crtc_state->connectors_changed) > - asyh->or.bpc = connector->display_info.bpc; > + asyh->or.bpc = connector->display_info.bpc ?: 8; [Severity: Medium] Could unconditionally defaulting asyh->or.bpc to 8 here break 6 bpc eDP and 18-bit (6 bpc) LVDS panels? Whenever nouveau_connector_detect() or nouveau_connector_detect_lvds() runs without a subsequent ->get_modes() call, drm_connector_update_edid_property() calls drm_edid_connector_update() -> update_display_info() -> drm_reset_display_info(), which resets connector->display_info.bpc to 0. For example, on system or runtime resume, nouveau_display_hpd_resume() schedules nouveau_display_hpd_work() which calls drm_helper_probe_detect() on every connector; when an eDP or LVDS panel stays connected, its epoch_counter does not change, so no hotplug event is sent and ->get_modes() does not run. For EDID 1.3 panels (standard on LVDS and Apple eDP panels), EDID 1.4 panels with undefined depth, or failed EDID reads, display_info.bpc remains 0 after ->detect(). In drivers/gpu/drm/nouveau/nouveau_connector.c, nouveau_connector_detect_depth() defaults eDP and LVDS to 6 bpc (unless LVDS VBIOS flags specify 24-bit): /* EDID 1.4 is *supposed* to be supported on eDP, but, Apple... */ if (nv_connector->type == DCB_CONNECTOR_eDP) { connector->display_info.bpc = 6; return; } /* we're out of options unless we're LVDS, default to 8bpc */ if (nv_encoder->dcb->type != DCB_OUTPUT_LVDS) { connector->display_info.bpc = 8; return; } connector->display_info.bpc = 6; When a modeset is committed while display_info.bpc is 0, setting asyh->or.bpc to 8 in nv50_outp_atomic_check() causes nv50_sor_atomic_enable() in drivers/gpu/drm/nouveau/dispnv50/disp.c to override bios->fp.strapless_is_24bit and force 24-bit LVDS mode on 18-bit LVDS panels: if (lvds_dual) { if (bios->fp.strapless_is_24bit & 2) lvds_8bpc = true; } else { if (bios->fp.strapless_is_24bit & 1) lvds_8bpc = true; } if (asyh->or.bpc == 8) lvds_8bpc = true; } nvif_outp_lvds(&nv_encoder->outp, lvds_dual, lvds_8bpc); It also causes eDP links to be trained and driven at 8 bpc instead of 6 bpc, and prevents nv50_head_atomic_check_dither() in drivers/gpu/drm/nouveau/dispnv50/head.c from enabling 6 bpc dithering on 24-bit framebuffers: if (asyc->dither.mode == DITHERING_MODE_AUTO) { if (asyh->base.depth > asyh->or.bpc * 3) mode = DITHERING_MODE_DYNAMIC2X2; } else { mode = asyc->dither.mode; } if (asyc->dither.depth == DITHERING_DEPTH_AUTO) { if (asyh->or.bpc >= 8) mode |= DITHERING_DEPTH_8BPC; } Should the fallback in nv50_outp_atomic_check() account for eDP and LVDS connectors, or should nouveau_connector_detect_depth() also be invoked once nv_connector->native_mode is available? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=4
