Re: [PATCH v10 15/15] drm/solomon: add SSD130x connector luminance support

[sashiko-bot@kernel.org]

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

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