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

[Thomas Zimmermann]

Am 09.07.26 um 13:50 schrieb Maxime Ripard:

The simple-kms helper provides a reset_plane hook to let drivers
override the default plane reset. However, the reset hook is
overloaded: it is used to create the initial software state at probe
time, but also to reset both the hardware and software state during
suspend/resume. These two roles have different expectations, and
reset is not fallible which makes error handling difficult.

The atomic_create_state pattern was introduced to untangle this by
providing a hook that only allocates and initializes a pristine state
without any side effect.

Add a create_plane_state hook to struct
drm_simple_display_pipe_funcs. When provided,
drm_simple_kms_plane_reset() will use it to allocate a fresh state,
falling back to reset_plane and then to the default
drm_atomic_helper_plane_reset().

This will allow simple-kms drivers to be converted to the
atomic_create_state pattern.

Signed-off-by: Maxime Ripard <mrip...@kernel.org>
---
  drivers/gpu/drm/drm_simple_kms_helper.c | 23 +++++++++++++++++++++--
  include/drm/drm_simple_kms_helper.h     |  1 +
  2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_simple_kms_helper.c 
b/drivers/gpu/drm/drm_simple_kms_helper.c
index 8e1d07b9f1e3..efb643ab6448 100644
--- a/drivers/gpu/drm/drm_simple_kms_helper.c
+++ b/drivers/gpu/drm/drm_simple_kms_helper.c
@@ -284,14 +284,33 @@ static const struct drm_plane_helper_funcs 
drm_simple_kms_plane_helper_funcs = {
  static void drm_simple_kms_plane_reset(struct drm_plane *plane)
  {
        struct drm_simple_display_pipe *pipe;

pipe = container_of(plane, struct drm_simple_display_pipe, plane);

-       if (!pipe->funcs || !pipe->funcs->reset_plane)
+       if (!pipe->funcs)
                return drm_atomic_helper_plane_reset(plane);

- return pipe->funcs->reset_plane(pipe);

+       if (pipe->funcs->reset_plane)
+               return pipe->funcs->reset_plane(pipe);
+
+       if (pipe->funcs->create_plane_state) {
+               struct drm_plane_state *state;
+
+               if (plane->state) {
+                       pipe->funcs->destroy_plane_state(pipe, plane->state);

Could be NULL, I think. You might want to duplicate the logic at

https://elixir.bootlin.com/linux/v7.2-rc1/source/drivers/gpu/drm/drm_simple_kms_helper.c#L312

+                       plane->state = NULL;
+               }
+
+               state = pipe->funcs->create_plane_state(pipe);
+               if (WARN_ON(IS_ERR(state)))
+                       return;
+
+               plane->state = state;
+               return;
+       }
+
+       return drm_atomic_helper_plane_reset(plane);
  }

static struct drm_plane_state *drm_simple_kms_plane_duplicate_state(struct drm_plane *plane)

  {
        struct drm_simple_display_pipe *pipe;
diff --git a/include/drm/drm_simple_kms_helper.h 
b/include/drm/drm_simple_kms_helper.h
index cb672ce0e856..b58f6d532820 100644
--- a/include/drm/drm_simple_kms_helper.h
+++ b/include/drm/drm_simple_kms_helper.h
@@ -42,10 +42,11 @@ struct drm_simple_display_pipe_funcs {
        void (*reset_crtc)(struct drm_simple_display_pipe *pipe);
        struct drm_crtc_state * (*duplicate_crtc_state)(struct 
drm_simple_display_pipe *pipe);
        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);
        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);
  };

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)