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
