Re: [PATCH RFC 9/9] drm/panel: novatek-nt35950: use the atomic state to select the mode

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/panel: atomic_check rejects CRTC disable with MODE_ID=0
- [Low] drm/panel: nt35950: unbalanced regulator disable in atomic_prepare error path
--

commit bb80efea394ba9e6d61de8428c59d0a3f71bf60e
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/panel: novatek-nt35950: use the atomic state to select the mode

The commit subclasses the panel state to store selected mode parameters
and switches callbacks to their atomic variants so the driver can look
up and validate the mode directly in atomic_check().

> diff --git a/drivers/gpu/drm/panel/panel-novatek-nt35950.c b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> index 94aa6489d99fc..f5161f11b1f5e 100644
> --- a/drivers/gpu/drm/panel/panel-novatek-nt35950.c
> +++ b/drivers/gpu/drm/panel/panel-novatek-nt35950.c
[ ... ]
> @@ -354,11 +339,43 @@ static int nt35950_sharp_init_vregs(struct nt35950 *nt, struct device *dev)
>  	return 0;
>  }
>  
> -static int nt35950_prepare(struct drm_panel *panel)
> +static int nt35950_atomic_check(struct drm_panel *panel,
> +				struct drm_atomic_commit *commit)
> +{
> +	struct nt35950 *nt = to_nt35950(panel);
> +	struct drm_panel_state *panel_state;
> +	struct drm_crtc_state *crtc_state;
> +	int i;
> +
> +	panel_state = drm_atomic_get_new_panel_state(commit, panel);
> +	crtc_state = drm_atomic_get_new_crtc_state_for_panel(commit, panel);
> +	if (!panel_state || !crtc_state)
> +		return -EINVAL;
> +
> +	for (i = 0; i < nt->desc->num_modes; i++) {
> +		if (drm_mode_match(&crtc_state->mode,
> +				   &nt->desc->mode_data[i].mode,
> +				   DRM_MODE_MATCH_TIMINGS | DRM_MODE_MATCH_CLOCK)) {
> +			to_nt35950_state(panel_state)->mode_data =
> +				&nt->desc->mode_data[i];
> +			return 0;
> +		}
> +	}
> +
> +	return -EINVAL;
> +}

[Severity: High]
Does this unconditional mode matching loop fail to account for CRTC
disablements?

When a userspace atomic commit sets MODE_ID=0 on the CRTC but leaves the
connector's CRTC_ID unchanged (like DPMS off or blanking), `crtc_state->enable`
is false and `mode` is zeroed. In this scenario, checking the zeroed mode
against valid panel modes will find no match, causing `nt35950_atomic_check()`
to erroneously reject valid state commits with -EINVAL, preventing userspace
from disabling the display using standard KMS atomic updates.

> +
> +static int nt35950_atomic_prepare(struct drm_panel *panel,
> +				  struct drm_atomic_commit *commit)
>  {
>  	struct nt35950 *nt = to_nt35950(panel);
> +	struct drm_panel_state *panel_state;
>  	int ret;
>  
> +	panel_state = drm_atomic_get_new_panel_state(commit, panel);
> +	if (!panel_state || !to_nt35950_state(panel_state)->mode_data)
> +		return -EINVAL;
> +
>  	ret = regulator_enable(nt->vregs[0].consumer);
>  	if (ret)
>  		return ret;
[ ... ]
> @@ -380,7 +397,7 @@ static int nt35950_prepare(struct drm_panel *panel)
>  
>  	nt35950_reset(nt);
>  
> -	ret = nt35950_on(nt);
> +	ret = nt35950_on(nt, to_nt35950_state(panel_state)->mode_data);
>  
>  end:
>  	if (ret < 0) {

[Severity: Low]
Can this error path cause an unbalanced regulator disable?

In `nt35950_atomic_prepare()`, regulators are enabled one by one, and on
failure, it jumps to the error path here:

    end:
        if (ret < 0) {
            regulator_bulk_disable(ARRAY_SIZE(nt->vregs), nt->vregs);
            return ret;
        }

This error path calls `regulator_bulk_disable()` on all 4 regulators, including
the one that just failed and any subsequent regulators that were never enabled,
resulting in unbalanced regulator disables. This triggers warnings and tracking
corruption. Additionally, the error path fails to assert the reset GPIO to
power down the panel properly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-panel-atomic-state-v1-0-b157fddb8de1@oss.qualcomm.com?part=9