Re: [PATCH v2] drm/i915/hdmi: preserve cached EDID on DDC failure when connector is forced

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/i915/hdmi: DRM_FORCE_OFF treated as forced-on in intel_hdmi_set_edid()
- [Medium] drm/i915/hdmi: CEC invalidation and hotplug churn in intel_hdmi_detect()
--

commit fa3007d46f607179adb22d75054bb3efa254e38b
Author: Aman Sharma <amansharma6122002@gmail.com>

drm/i915/hdmi: preserve cached EDID on DDC failure when connector is forced

This commit preserves the cached detect_edid on forced HDMI connectors when
DDC reads fail during monitor standby, moving intel_hdmi_unset_edid() into
intel_hdmi_set_edid() and skipping EDID teardown on Gen11+ disconnect when
the connector is forced.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b6fa1a1558b82..89ab9a790a419 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2568,6 +2568,19 @@ intel_hdmi_set_edid(struct drm_connector *_connector)
>  		intel_gmbus_force_bit(ddc, false);
>  	}
>  
> +	/*
> +	 * If the connector is forced on (e.g. video=HDMI-A-1:e) and the sink's
> +	 * DDC/I2C bus is powered down during DPMS standby, keep the previously
> +	 * cached EDID rather than wiping it and falling back to 1024x768.
> +	 */
> +	if (!drm_edid && connector->base.force &&
> +	    connector->detect_edid && connector->base.edid_blob_ptr) {
> +		intel_display_power_put(display, POWER_DOMAIN_GMBUS, wakeref);
> +		return true;
> +	}

[Severity: Medium]
What happens here when a connector with a cached EDID is forced off via
DRM_FORCE_OFF?

Because enum drm_connector_force defines DRM_FORCE_OFF as a non-zero value,
connector->base.force evaluates to true when the connector is forced off
(for example via connector_write() in drivers/gpu/drm/drm_debugfs.c, which
sets connector->force = DRM_FORCE_OFF without calling fill_modes() or
intel_hdmi_force()).

When a subsequent probe runs intel_hdmi_detect() -> intel_hdmi_set_edid(),
drm_edid_read_ddc() explicitly returns NULL for DRM_FORCE_OFF:

drivers/gpu/drm/drm_edid.c:drm_edid_read_ddc() {
	if (connector->force == DRM_FORCE_OFF)
		return NULL;
    ...
}

In that case, intel_hdmi_set_edid() will match !drm_edid &&
connector->base.force, skip intel_hdmi_unset_edid() and
drm_edid_connector_update(), retain the stale detect_edid and dp_dual_mode
state, and return true as if the forced-off connector were connected. The
same boolean check in intel_hdmi_detect() also skips
intel_hdmi_unset_edid() on Gen11+ disconnect when force is DRM_FORCE_OFF.

Should these checks explicitly test for >= DRM_FORCE_ON (or != DRM_FORCE_OFF
&& != DRM_FORCE_UNSPECIFIED) instead of a boolean check on
connector->base.force?

> +
> +	intel_hdmi_unset_edid(&connector->base);
> +
>  	/* Below we depend on display info having been updated */
>  	drm_edid_connector_update(&connector->base, drm_edid);
>  
> @@ -2609,10 +2622,11 @@ intel_hdmi_detect(struct drm_connector *_connector, bool force)
>  	wakeref = intel_display_power_get(display, POWER_DOMAIN_GMBUS);
>  
>  	if (DISPLAY_VER(display) >= 11 &&
> -	    !intel_digital_port_connected(encoder))
> +	    !intel_digital_port_connected(encoder)) {
> +		if (!connector->base.force)
> +			intel_hdmi_unset_edid(&connector->base);
>  		goto out;
> -
> -	intel_hdmi_unset_edid(&connector->base);
> +	}

[Severity: Medium]
On DISPLAY_VER(display) >= 11, if the sink drops HPD during standby so
!intel_digital_port_connected(encoder) is true on a forced-on connector,
could jumping to out with status = connector_status_disconnected cause CEC
invalidation and hotplug churn?

In intel_hdmi_detect(), jumping to out leaves status set to
connector_status_disconnected:

drivers/gpu/drm/i915/display/intel_hdmi.c:intel_hdmi_detect() {
    ...
out:
	intel_display_power_put(display, POWER_DOMAIN_GMBUS, wakeref);

	if (status != connector_status_connected)
		cec_notifier_phys_addr_invalidate(intel_hdmi->cec_notifier);

	return status;
}

First, this invalidates cec_notifier even though connector->detect_edid and
display_info are kept, and subsequent calls to intel_hdmi_force() ->
intel_hdmi_set_edid() while DDC is powered down return early before reaching
cec_notifier_set_phys_addr().

Second, returning connector_status_disconnected while connector->base.status
remains forced to connector_status_connected causes
drm_helper_probe_detect_ctx() to see ret != connector->status and increment
connector->epoch_counter on every probe:

drivers/gpu/drm/drm_probe_helper.c:drm_helper_probe_detect_ctx() {
    ...
	if (ret != connector->status)
		connector->epoch_counter += 1;
    ...
}

In intel_hotplug_detect_connector(), connector->base.status is not updated
when connector->base.force is set, so old_epoch_counter !=
connector->base.epoch_counter triggers INTEL_HOTPLUG_CHANGED and sends
hotplug uevents on every HPD or poll while the sink is in standby.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/179284522321.1666644.7085985666852699043@gmail.com?part=1