Re: [PATCH v2 01/61] drm/simple-kms: Add create_plane_state hook

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch introduces the `atomic_create_state` pattern for simple-kms but fails to populate the `.atomic_create_state` hooks in the core DRM vtables, rendering the new pattern unusable.
--

--- Patch [1]: [PATCH v2 01/61] drm/simple-kms: Add create_plane_state hook ---
commit 3bbdad5049ec3ee53628f35efd9dd90d236402b0
Author: Maxime Ripard <[email protected]>

drm/simple-kms: Add create_plane_state hook

The simple-kms helper provides a reset_plane hook to let drivers override
the default plane reset, but the hook is overloaded for multiple roles.
The atomic_create_state pattern was introduced to untangle this by providing
a hook that only allocates and initializes a pristine state. This patch adds
a create_plane_state hook to struct drm_simple_display_pipe_funcs.

> diff --git a/include/drm/drm_simple_kms_helper.h b/include/drm/drm_simple_kms_helper.h
> index cb672ce0e856d..b58f6d5328201 100644
> --- a/include/drm/drm_simple_kms_helper.h
> +++ b/include/drm/drm_simple_kms_helper.h
> @@ -44,6 +44,7 @@ struct drm_simple_display_pipe_funcs {
>  	void (*destroy_crtc_state)(struct drm_simple_display_pipe *pipe,
>  				   struct drm_crtc_state *crtc_state);
>  	void (*reset_plane)(struct drm_simple_display_pipe *pipe);
> +	struct drm_plane_state * (*create_plane_state)(struct drm_simple_display_pipe *pipe);

[Severity: Medium]
As the atomic_create_state pattern is being introduced here for simple-kms,
should drm_simple_kms_plane_funcs also have its .atomic_create_state hook
populated?

If a driver converts to this new pattern and uses
drm_mode_config_create_initial_state(), it looks like it will see a NULL
hook in the core DRM vtables and skip creating the plane state:

static const struct drm_plane_funcs drm_simple_kms_plane_funcs = {
	.update_plane		= drm_atomic_helper_update_plane,
	.disable_plane		= drm_atomic_helper_disable_plane,
	.destroy		= drm_plane_cleanup,
	.reset			= drm_simple_kms_plane_reset,
	.atomic_duplicate_state	= drm_simple_kms_plane_duplicate_state,
	.atomic_destroy_state	= drm_simple_kms_plane_destroy_state,
	.format_mod_supported   = drm_simple_kms_format_mod_supported,
};

This would leave plane->state = NULL.

Similarly, is a .atomic_create_state hook also missing for
drm_simple_kms_crtc_funcs (along with a corresponding create_crtc_state hook
in drm_simple_display_pipe_funcs) to ensure the CRTC state is also properly
initialized?

>  	struct drm_plane_state * (*duplicate_plane_state)(struct drm_simple_display_pipe *pipe);
>  	void (*destroy_plane_state)(struct drm_simple_display_pipe *pipe,
>  				    struct drm_plane_state *plane_state);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1