Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] drm/i915/display: missing acpi_video_backlight_use_native() call - [Medium] drm/i915/display: stale backlight level programmed on modeset enable - [Low] drm/i915/display: build failure in intel_backlight_set_luminance() - [Low] drm/i915: unmet BACKLIGHT_CLASS_DEVICE dependency for ACPI_VIDEO --
commit e18daa9a197d727d1dc5e00c9bb74f3e09fc2495 Author: Mario Limonciello <[email protected]> drm/i915/display: add luminance property support This commit adds support for the DRM LUMINANCE connector property on eDP, LVDS, and DSI panels in i915 and Xe when CONFIG_DRM_LUMINANCE is enabled, while retaining the backlight class device when it is disabled. > diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig > index 71426b9c91f00..720191673a1e7 100644 > --- a/drivers/gpu/drm/i915/Kconfig > +++ b/drivers/gpu/drm/i915/Kconfig > @@ -26,7 +26,7 @@ config DRM_I915 > select IRQ_WORK > # i915 depends on ACPI_VIDEO when ACPI is enabled > # but for select to work, need to select ACPI_VIDEO's dependencies, ick > - select BACKLIGHT_CLASS_DEVICE if ACPI > + select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE > select INPUT if ACPI > select ACPI_WMI if ACPI > select ACPI_VIDEO if ACPI [Severity: Low] Does this cause an unmet direct dependency for ACPI_VIDEO when CONFIG_DRM_LUMINANCE=y, CONFIG_ACPI=y, and CONFIG_BACKLIGHT_CLASS_DEVICE=n? In drivers/acpi/Kconfig, ACPI_VIDEO depends directly on BACKLIGHT_CLASS_DEVICE: config ACPI_VIDEO tristate "Video" depends on BACKLIGHT_CLASS_DEVICE Both DRM_I915 here and DRM_XE in drivers/gpu/drm/xe/Kconfig still have: select ACPI_VIDEO if ACPI When DRM_LUMINANCE is enabled and nothing else selects BACKLIGHT_CLASS_DEVICE, Kconfig warns about an unmet direct dependency on BACKLIGHT_CLASS_DEVICE and drivers/acpi/acpi_video.c fails to link due to undefined references to backlight_device_register() and backlight_device_unregister(). [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_backlight.c > b/drivers/gpu/drm/i915/display/intel_backlight.c > index b128896cb1c2d..6d658c7b5d99f 100644 > --- a/drivers/gpu/drm/i915/display/intel_backlight.c > +++ b/drivers/gpu/drm/i915/display/intel_backlight.c [ ... ] > @@ -1712,6 +1723,15 @@ int intel_backlight_setup(struct intel_connector > *connector, enum pipe pipe) > return 0; > } > > +void intel_backlight_set_luminance(const struct drm_connector_state > *conn_state) > +{ > + struct intel_connector *connector = > + to_intel_connector(conn_state->connector); > + > + intel_panel_set_backlight(conn_state, conn_state->luminance, > + connector->panel.backlight.max); > +} [Severity: Low] Will drivers/gpu/drm/i915/display/intel_backlight.c fail to build when CONFIG_BACKLIGHT_CLASS_DEVICE is disabled? In drivers/gpu/drm/i915/display/intel_backlight.c, scale_user_to_hw() and intel_panel_set_backlight() are defined inside an #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) block: #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) static u32 intel_panel_get_backlight(struct intel_connector *connector) ... static void intel_panel_set_backlight(const struct drm_connector_state *conn_state, u32 user_level, u32 user_max) Because intel_backlight_set_luminance() is compiled unconditionally outside that guard, building with CONFIG_BACKLIGHT_CLASS_DEVICE=n (for example when CONFIG_DRM_LUMINANCE=y) fails with an implicit declaration of intel_panel_set_backlight(). [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index d5d4453f283bb..3c81ed630c365 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -7696,6 +7696,8 @@ static void intel_atomic_commit_tail(struct > intel_atomic_state *state) > /* Now enable the clocks, plane, pipe, and connectors that we set up. */ > display->modeset.funcs->commit_modeset_enables(state); > > + drm_atomic_helper_commit_luminance_enables(&state->base); > + > intel_display_power_dc3co_compute(state); [Severity: Medium] When a panel's LUMINANCE property is updated while the CRTC is inactive (or in the same atomic commit that enables the CRTC), doesn't calling drm_atomic_helper_commit_luminance_enables() after commit_modeset_enables() initially turn on the panel at the stale brightness level? While the CRTC is inactive, drm_atomic_helper_commit_luminance_enables() skips calling .set_luminance(), so panel->backlight.level keeps the old hardware brightness level while conn_state->luminance holds the new value. During a subsequent modeset enable in intel_atomic_commit_tail(): display->modeset.funcs->commit_modeset_enables(state) -> intel_edp_backlight_on() / intel_enable_lvds() / gen11_dsi_enable() -> intel_backlight_enable() -> __intel_backlight_enable() -> panel->backlight.funcs->enable(crtc_state, conn_state, panel->backlight.level); This powers on and programs the backlight with the old panel->backlight.level and waits for vblank before drm_atomic_helper_commit_luminance_enables() calls intel_backlight_set_luminance() to update panel->backlight.level and reprogram the hardware a second time. [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_panel.c > b/drivers/gpu/drm/i915/display/intel_panel.c > index 81e638d0c7b33..501573a75b89c 100644 > --- a/drivers/gpu/drm/i915/display/intel_panel.c > +++ b/drivers/gpu/drm/i915/display/intel_panel.c > @@ -610,9 +610,11 @@ int intel_panel_register(struct intel_connector > *connector) > struct intel_panel *panel = &connector->panel; > int ret; > > - ret = intel_backlight_device_register(connector); > - if (ret) > - return ret; > + if (!IS_ENABLED(CONFIG_DRM_LUMINANCE)) { > + ret = intel_backlight_device_register(connector); > + if (ret) > + return ret; > + } [Severity: High] When CONFIG_DRM_LUMINANCE is enabled, how does the driver coordinate with ACPI video backlight detection and BIOS OpRegion ASLE backlight requests now that intel_backlight_device_register() is skipped? Previously, intel_backlight_device_register() called acpi_video_backlight_use_native(), which notifies drivers/acpi/video_detect.c that native GPU backlight control is present (setting native_available = true) and checks whether native backlight control should be used. It also set panel->backlight.device. With intel_backlight_device_register() skipped and intel_backlight_setup() attaching the LUMINANCE property without calling acpi_video_backlight_use_native(): 1. In drivers/gpu/drm/i915/display/intel_acpi.c, intel_acpi_video_register() checks: if (panel->backlight.funcs && !panel->backlight.device) { acpi_video_register_backlight(); break; } Because panel->backlight.device is NULL and acpi_video_backlight_use_native() was never called, intel_acpi_video_register() will register a conflicting acpi_video backlight device if CONFIG_BACKLIGHT_CLASS_DEVICE is enabled. 2. In drivers/gpu/drm/i915/display/intel_opregion.c, asle_set_backlight() checks: if (acpi_video_get_backlight_type() == acpi_backlight_native) { drm_dbg_kms(display->drm, "opregion backlight request ignored\n"); return 0; } Because acpi_video_backlight_use_native() was never called, acpi_video_get_backlight_type() will not return acpi_backlight_native, so BIOS OpRegion ASLE requests will call intel_backlight_set_acpi() and modify panel->backlight.level behind the atomic LUMINANCE property's back. 3. In intel_backlight_setup(), the LUMINANCE property is attached even on systems where acpi_video_backlight_use_native() would return false. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f2413...@kernel.org?part=6
