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 <[email protected]> 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-b2a6f2413...@kernel.org?part=13
