Re: [PATCH v10 13/15] drm/gma500: add connector luminance support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/gma500: unreachable atomic luminance support in gma_backlight_init()
- [High] drm/gma500: gma_connector_disable_luminance() permanently disables backlight
- [Medium] drm/gma500: Kconfig unmet dependency and backlight link errors in backlight.c
--

commit 44c343eaada226f65e1091fb750905e2745ab610
Author: Mario Limonciello <mario.limonciello@amd.com>

drm/gma500: add connector luminance support

This commit exposes panel brightness on gma500 through the connector
luminance property when drm luminance support is enabled, routing updates
through existing platform backlight operations.

> diff --git a/drivers/gpu/drm/gma500/Kconfig b/drivers/gpu/drm/gma500/Kconfig
> index dc4c566388192..08871a8f6fa22 100644
> --- a/drivers/gpu/drm/gma500/Kconfig
> +++ b/drivers/gpu/drm/gma500/Kconfig
> @@ -9,7 +9,7 @@ config DRM_GMA500
>  	select I2C_ALGOBIT
>  	# GMA500 depends on ACPI_VIDEO when ACPI is enabled, just like i915
>  	select ACPI_VIDEO if ACPI
> -	select BACKLIGHT_CLASS_DEVICE if ACPI
> +	select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE
>  	select INPUT if ACPI
>  	select ACPI_WMI if ACPI
>  	help
> diff --git a/drivers/gpu/drm/gma500/backlight.c b/drivers/gpu/drm/gma500/backlight.c
> index c8f1716a12d52..4bb39fba04539 100644
> --- a/drivers/gpu/drm/gma500/backlight.c
> +++ b/drivers/gpu/drm/gma500/backlight.c

[ ... ]

> @@ -73,10 +72,9 @@ static const struct backlight_ops gma_backlight_ops __maybe_unused = {
>  	.update_status  = gma_backlight_update_status,
>  };
>  
> -int gma_backlight_init(struct drm_device *dev)
> +int gma_backlight_init(struct drm_device *dev, struct drm_connector *connector)
>  {
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
> -	struct backlight_properties props __maybe_unused = {};
>  	int ret;
>  
>  	dev_priv->backlight_enabled = true;
> @@ -92,28 +90,43 @@ int gma_backlight_init(struct drm_device *dev)
>  		return 0;
>  	}
>  
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
> -	props.brightness = dev_priv->backlight_level;
> -	props.max_brightness = PSB_MAX_BRIGHTNESS;
> -	props.type = BACKLIGHT_RAW;
> +	if (IS_ENABLED(CONFIG_DRM_LUMINANCE))
> +		return drm_connector_attach_luminance_property(connector, 0,
> +							 PSB_MAX_BRIGHTNESS,
> +							 dev_priv->backlight_level);

[Severity: High]
Does attaching the luminance property and returning early in
psb_driver_load() -> gma_backlight_init() leave panel brightness completely
uncontrollable when drm luminance is enabled?

In drivers/gpu/drm/gma500/psb_drv.c, the driver does not support atomic
modesetting:

    static const struct drm_driver driver = {
        .driver_features = DRIVER_MODESET | DRIVER_GEM,

However, drm_connector_attach_luminance_property() creates the property with
the atomic flag:

    property = drm_property_create_range(connector->dev, DRM_MODE_PROP_ATOMIC,
                                         "LUMINANCE", min, max);

Because gma500 does not advertise atomic support, drm_setclientcap() rejects
atomic client capability requests and file_priv->atomic is always false, so
drm_mode_object_get_properties() hides the property from userspace.

In addition, legacy property updates via drm_mode_obj_set_property_ioctl() ->
set_property_legacy() dispatch to psb_intel_lvds_set_property() or
cdv_intel_lvds_set_property(), neither of which handles the luminance
property, and the atomic commit helpers that invoke
gma_connector_set_luminance() and gma_connector_disable_luminance() are never
called by this driver.

Since gma_backlight_init() also skips backlight_device_register() on this
path, is there any way for userspace to adjust panel brightness when drm
luminance is enabled?

> +
> +	{
> +		struct backlight_properties props = {};
>  
> -	dev_priv->backlight_device =
> -		backlight_device_register(dev_priv->ops->backlight_name,
> -					  dev->dev, dev,
> -					  &gma_backlight_ops, &props);
> -	if (IS_ERR(dev_priv->backlight_device))
> -		return PTR_ERR(dev_priv->backlight_device);
> -#endif
> +		props.brightness = dev_priv->backlight_level;
> +		props.max_brightness = PSB_MAX_BRIGHTNESS;
> +		props.type = BACKLIGHT_RAW;
> +
> +		dev_priv->backlight_device =
> +			backlight_device_register(dev_priv->ops->backlight_name,
> +						  dev->dev, dev,
> +						  &gma_backlight_ops, &props);
> +		if (IS_ERR(dev_priv->backlight_device))
> +			return PTR_ERR(dev_priv->backlight_device);
> +	}
>  
>  	return 0;
>  }
>  
> +void gma_connector_set_luminance(const struct drm_connector_state *conn_state)
> +{
> +	gma_backlight_set(conn_state->connector->dev, conn_state->luminance);
> +}
> +
> +void gma_connector_disable_luminance(const struct drm_connector_state *conn_state)
> +{
> +	gma_backlight_disable(conn_state->connector->dev);
> +}

[Severity: High]
Once gma_connector_disable_luminance() has been called, will subsequent calls
to gma_connector_set_luminance() fail to turn the backlight back on?

In drivers/gpu/drm/gma500/backlight.c, gma_connector_disable_luminance()
calls gma_backlight_disable(), which clears dev_priv->backlight_enabled:

    void gma_backlight_disable(struct drm_device *dev)
    {
        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);

        dev_priv->backlight_enabled = false;
        dev_priv->ops->backlight_set(dev, 0);
    }

When gma_connector_set_luminance() is later called to restore or change the
brightness, it calls gma_backlight_set(), which only updates the hardware if
dev_priv->backlight_enabled is true:

    void gma_backlight_set(struct drm_device *dev, int v)
    {
        struct drm_psb_private *dev_priv = to_drm_psb_private(dev);

        dev_priv->backlight_level = v;
        if (dev_priv->backlight_enabled)
            dev_priv->ops->backlight_set(dev, v);
    }

Since nothing in gma_connector_set_luminance() or the encoder enable paths
sets dev_priv->backlight_enabled back to true (or calls
gma_backlight_enable()), won't the panel backlight remain permanently off
after the first disable?

> +
>  void gma_backlight_exit(struct drm_device *dev)
>  {
> -#ifdef CONFIG_BACKLIGHT_CLASS_DEVICE
>  	struct drm_psb_private *dev_priv = to_drm_psb_private(dev);
>  
>  	if (dev_priv->backlight_device)
>  		backlight_device_unregister(dev_priv->backlight_device);
> -#endif
>  }

[Severity: Medium]
Can removing the preprocessor guards in gma_backlight_init() and
gma_backlight_exit() while dropping the backlight class selection in
drivers/gpu/drm/gma500/Kconfig cause unmet dependency warnings and link
failures?

First, in drivers/gpu/drm/gma500/Kconfig:

    select ACPI_VIDEO if ACPI
    select BACKLIGHT_CLASS_DEVICE if ACPI && !DRM_LUMINANCE

Because acpi video in drivers/acpi/Kconfig depends on the backlight class
device, enabling acpi and drm luminance while the backlight class device is
disabled selects acpi video without its direct dependency.

Second, include/linux/backlight.h does not provide inline stubs for
backlight_device_register() or backlight_device_unregister() when the
backlight class device is disabled.

In gma_backlight_exit(), the call to backlight_device_unregister() is only
guarded by a runtime check on dev_priv->backlight_device, so the compiler
still emits an undefined reference to backlight_device_unregister() when the
backlight class device is disabled.

Similarly, if acpi and drm luminance are both disabled (or if gma500 is
built-in while the backlight class device is built as a module),
gma_backlight_init() and gma_backlight_exit() will fail to link against both
backlight_device_register() and backlight_device_unregister().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f24131f5@kernel.org?part=13