Re: [PATCH v10 06/15] drm/i915/display: add luminance property support

[sashiko-bot@kernel.org]

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 <mario.limonciello@amd.com>

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-b2a6f24131f5@kernel.org?part=6