Re: [PATCH v4 12/20] drm/crtc: Add new atomic_create_state callback

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The error path in drm_mode_config_crtc_create_state() skips calling drm_crtc_vblank_reset(), leaving vblank state unreset on state allocation failure.
--

commit 5e39f6f785f69a809d5a205ec026340edf6a2281
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/crtc: Add new atomic_create_state callback

This commit introduces the atomic_create_state hook for CRTCs. This moves
away from the overloaded reset() callback to separate the allocation of a
pristine state from hardware resets, facilitating better initialization and
suspend/resume handling.

> diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c

[ ... ]

> @@ -208,6 +208,35 @@ static int drm_mode_config_plane_reset_with_create_state(struct drm_plane *plane
>  	return drm_mode_config_plane_create_state(plane);
>  }
>  
> +static int drm_mode_config_crtc_create_state(struct drm_crtc *crtc)
> +{
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (!crtc->funcs->atomic_create_state)
> +		return 0;
> +
> +	crtc_state = crtc->funcs->atomic_create_state(crtc);
> +	if (IS_ERR(crtc_state))
> +		return PTR_ERR(crtc_state);

If atomic_create_state() fails here (for example, returning -ENOMEM), we
return early and skip calling drm_crtc_vblank_reset().

In the legacy reset() pattern, drm_crtc_vblank_reset() was called
unconditionally even if the state allocation failed, setting
vblank->inmodeset = 1 and bumping the refcount to prevent drm_vblank_get()
from attempting to enable vblank interrupts on an unconfigured CRTC.

Because drm_mode_config_reset() ignores the return value of this function,
initialization will complete successfully but with crtc->state set to NULL
and vblank->inmodeset = 0.

If userspace later requests a vblank event, could the vblank core attempt
to enable hardware interrupts on an uninitialized or powered-off CRTC?

Should drm_crtc_vblank_reset() be called before returning the error here,
or should the error be propagated up to fail the initialization entirely?

> +
> +	if (drm_dev_has_vblank(crtc->dev))
> +		drm_crtc_vblank_reset(crtc);
> +
> +	crtc->state = crtc_state;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512-drm-mode-config-init-v4-0-591dfdcc1bf9@kernel.org?part=12