Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider: - [High] drm/solomon: vmlinux link failure when BACKLIGHT_CLASS_DEVICE=m - [High] drm/solomon: contrast state desynchronization in ssd130x_set_luminance_value() - [High] drm/solomon: unsynchronized command writes in ssd130x_set_luminance_value() - [Medium] drm/solomon: broken Kconfig dependency for BACKLIGHT_CLASS_DEVICE and DRM_LUMINANCE - [Low] drm/solomon: duplicated contrast dispatch in ssd130xfb_bl_ops --
commit bcc978ec95ba4732042537500fdd7f79b00e0743 Author: Mario Limonciello <[email protected]> drm/solomon: add SSD130x connector luminance support This commit exposes SSD130x display contrast through the DRM connector LUMINANCE property when DRM_LUMINANCE is enabled, while guarding the legacy backlight class registration with BACKLIGHT_CLASS_DEVICE and updating the Kconfig dependencies. > diff --git a/drivers/gpu/drm/solomon/Kconfig b/drivers/gpu/drm/solomon/Kconfig > index 400a6cab3a671..76852246eb9c2 100644 > --- a/drivers/gpu/drm/solomon/Kconfig > +++ b/drivers/gpu/drm/solomon/Kconfig > @@ -1,7 +1,7 @@ > config DRM_SSD130X > tristate "DRM support for Solomon SSD13xx OLED displays" > depends on DRM && MMU > - select BACKLIGHT_CLASS_DEVICE > + depends on DRM_LUMINANCE || BACKLIGHT_CLASS_DEVICE = y || > !BACKLIGHT_CLASS_DEVICE > select DRM_CLIENT_SELECTION > select DRM_GEM_SHMEM_HELPER > select DRM_KMS_HELPER [Severity: High] Can this Kconfig dependency cause a vmlinux link failure when CONFIG_DRM_LUMINANCE=y, CONFIG_BACKLIGHT_CLASS_DEVICE=m, and CONFIG_DRM_SSD130X=y? Because DRM_LUMINANCE is a bool symbol and BACKLIGHT_CLASS_DEVICE is a tristate symbol, when DRM_LUMINANCE=y and BACKLIGHT_CLASS_DEVICE=m, the disjunction evaluates to max(y, n, m) = y, which allows DRM_SSD130X to be built-in (y). However, ssd130x_probe() in drivers/gpu/drm/solomon/ssd130x.c guards the call to devm_backlight_device_register() with #if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE), which evaluates to 1 when CONFIG_BACKLIGHT_CLASS_DEVICE=m. Built-in ssd130x.o will then call devm_backlight_device_register() from the loadable module backlight.ko (which has no inline stub in include/linux/backlight.h), failing to link vmlinux. [Severity: Medium] Does replacing select BACKLIGHT_CLASS_DEVICE with this dependency also cause a regression when both CONFIG_DRM_LUMINANCE=n and CONFIG_BACKLIGHT_CLASS_DEVICE=n? When both options are disabled, !BACKLIGHT_CLASS_DEVICE evaluates to y, allowing DRM_SSD130X to build with neither /sys/class/backlight nor the connector LUMINANCE property enabled, leaving the display with no contrast control interface at all. In addition, on SSD132X_FAMILY panels, ssd132x_init() hardcodes SSD13XX_CONTRAST to 0x80 and previously relied on backlight_enable() in ssd132x_encoder_atomic_enable() to program ssd130x->contrast (127). When both options are disabled, neither backlight_enable() nor drm_atomic_helper_commit_luminance_enables() runs to apply ssd130x->contrast. > diff --git a/drivers/gpu/drm/solomon/ssd130x.c > b/drivers/gpu/drm/solomon/ssd130x.c > index ee43cf0da8792..aaf2b194273ba 100644 > --- a/drivers/gpu/drm/solomon/ssd130x.c > +++ b/drivers/gpu/drm/solomon/ssd130x.c [ ... ] > @@ -1980,8 +1985,44 @@ static int ssd130x_connector_get_modes(struct > drm_connector *connector) > return drm_connector_helper_get_modes_fixed(connector, &ssd130x->mode); > } > > +static int ssd130x_set_luminance_value(struct ssd130x_device *ssd130x, > + unsigned int luminance) > +{ > + int ret; > + > + ssd130x->contrast = luminance; > + > + switch (ssd130x->device_info->family_id) { > + case SSD133X_FAMILY: > + return ssd133x_set_contrast(ssd130x, luminance); > + case SSD135X_FAMILY: > + return ssd135x_set_contrast(ssd130x, luminance); > + default: > + ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST); > + if (ret < 0) > + return ret; > + return ssd130x_write_cmd(ssd130x, 1, luminance); > + } > +} > + > +static void ssd130x_connector_set_luminance(const struct drm_connector_state > *conn_state) > +{ > + struct ssd130x_device *ssd130x = > drm_to_ssd130x(conn_state->connector->dev); > + > + ssd130x_set_luminance_value(ssd130x, conn_state->luminance); > +} > + > +static void ssd130x_connector_disable_luminance(const struct > drm_connector_state *conn_state) > +{ > + struct ssd130x_device *ssd130x = > drm_to_ssd130x(conn_state->connector->dev); > + > + ssd130x_set_luminance_value(ssd130x, 0); > +} [Severity: High] Can concurrent DRM atomic commits and sysfs backlight writes interleave the multi-byte command writes in ssd130x_set_luminance_value()? When both CONFIG_DRM_LUMINANCE and CONFIG_BACKLIGHT_CLASS_DEVICE are enabled, sysfs backlight updates invoke ssd130x_update_bl(), ssd133x_update_bl(), or ssd135x_update_bl() under bdev->update_lock, whereas DRM atomic commits invoke ssd130x_connector_set_luminance() and ssd130x_connector_disable_luminance() from drm_atomic_helper_commit_tail() without holding bdev->update_lock or any shared driver lock. In ssd130x_set_luminance_value(), ssd130x->contrast is updated without locking, and on SSD130X_FAMILY and SSD132X_FAMILY the contrast update is sent as two separate ssd130x_write_cmd() calls (and ssd133x_set_contrast() similarly issues multiple separate regmap writes): ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST); if (ret < 0) return ret; return ssd130x_write_cmd(ssd130x, 1, luminance); If a sysfs backlight write and a DRM atomic luminance update race between those two regmap_write() calls, the second thread will send SSD13XX_CONTRAST (0x81) before the first thread sends its parameter byte. Wouldn't the OLED controller consume 0x81 as the contrast parameter for the first command, and then interpret both threads' subsequent luminance parameter bytes as arbitrary SSD13XX_COMMAND opcodes (such as 0xae SSD13XX_DISPLAY_OFF, 0xa0/0xa1 segment remap, or 0xc0/0xc8 COM scan direction)? [Severity: High] Does exposing both ssd130x->bl_dev and the connector LUMINANCE property at the same time desynchronize contrast state across atomic commits and display disable/enable cycles? In ssd130x_probe() and ssd130x_init_modeset(), the driver registers ssd130x->bl_dev whenever IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) is true and also attaches the LUMINANCE property whenever IS_ENABLED(CONFIG_DRM_LUMINANCE) is true, unlike drivers that only register a legacy backlight device when DRM_LUMINANCE is not used: 1. ssd130x_connector_set_luminance() updates ssd130x->contrast via ssd130x_set_luminance_value() without updating ssd130x->bl_dev->props.brightness, while ssd130x_update_bl(), ssd133x_update_bl(), and ssd135x_update_bl() update ssd130x->contrast from bl_dev->props.brightness without updating connector->state->luminance. If brightness is changed via /sys/class/backlight, any subsequent atomic commit including the active connector runs drm_atomic_helper_commit_luminance_enables() and overwrites the hardware contrast back to conn_state->luminance. 2. When disabling the display, drm_atomic_helper_commit_luminance_disables() calls ssd130x_connector_disable_luminance() -> ssd130x_set_luminance_value(ssd130x, 0), which clobbers ssd130x->contrast to 0, and ssd130x_encoder_atomic_disable() then calls backlight_disable(ssd130x->bl_dev), writing contrast 0 a second time. 3. When re-enabling the display, ssd130x_encoder_atomic_enable() (and its ssd132x, ssd133x, and ssd135x counterparts) runs before drm_atomic_helper_commit_luminance_enables(): ssd130x_init(), ssd133x_init(), and ssd135x_init() first initialize the panel with the clobbered ssd130x->contrast (0) prior to SSD13XX_DISPLAY_ON; next, backlight_enable(ssd130x->bl_dev) programs the stale bl_dev->props.brightness while the display is on; and finally drm_atomic_helper_commit_luminance_enables() calls ssd130x_connector_set_luminance() to program conn_state->luminance. 4. If ssd130x_power_on() or ssd130x_init() fails in ssd130x_encoder_atomic_enable() (or its ssd132x, ssd133x, or ssd135x variants) and powers off vcc_reg, drm_atomic_helper_commit_modeset_enables() still calls drm_atomic_helper_commit_luminance_enables() -> ssd130x_connector_set_luminance(), issuing contrast commands over I2C/SPI to an unpowered controller. [ ... ] > @@ -2015,23 +2056,13 @@ static const struct drm_driver ssd130x_drm_driver = { > .fops = &ssd130x_fops, > }; > > +#if IS_ENABLED(CONFIG_BACKLIGHT_CLASS_DEVICE) > static int ssd130x_update_bl(struct backlight_device *bdev) > { > struct ssd130x_device *ssd130x = bl_get_data(bdev); > - int brightness = backlight_get_brightness(bdev); > - int ret; > - > - ssd130x->contrast = brightness; > - > - ret = ssd130x_write_cmd(ssd130x, 1, SSD13XX_CONTRAST); > - if (ret < 0) > - return ret; > > - ret = ssd130x_write_cmd(ssd130x, 1, ssd130x->contrast); > - if (ret < 0) > - return ret; > - > - return 0; > + return ssd130x_set_luminance_value(ssd130x, > + backlight_get_brightness(bdev)); > } > > static int ssd133x_update_bl(struct backlight_device *bdev) > @@ -2066,6 +2097,7 @@ static const struct backlight_ops ssd130xfb_bl_ops[] = { > .update_status = ssd135x_update_bl, > }, > }; > +#endif [Severity: Low] This isn't a bug, but since ssd130x_update_bl() now delegates to ssd130x_set_luminance_value(), which already switches on ssd130x->device_info->family_id and dispatches to ssd133x_set_contrast() and ssd135x_set_contrast(), could ssd133x_update_bl(), ssd135x_update_bl(), and the per-family ssd130xfb_bl_ops[] array be dropped in favor of a single backlight_ops struct using ssd130x_update_bl()? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/20261102-superm1-backlight-property-v10-0-b2a6f2413...@kernel.org?part=15
