Re: [PATCH 4/5] drm/nouveau/kms/nv50-: don't drive outputs at 0 bpc

[sashiko-bot@kernel.org]

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 <linuxdev844@thelittleman.net>

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/20261103-nouveau-dp-fixes-v1-0-892f4bd357cd@thelittleman.net?part=4