Re: [PATCH v3 03/11] drm/atomic: Create function to insert plane state into a commit

[Thomas Zimmermann]

Am 08.09.26 um 16:35 schrieb Maxime Ripard:
drm_atomic_get_plane_state() allocates a new plane state by
duplicating the current one and inserts it into the atomic commit as
a single operation.

However, a later change will need to insert a plane state into a
commit without going through the full allocation and duplication path
in drm_atomic_get_plane_state().

Extract the state insertion logic into a new static
drm_atomic_commit_set_plane_state() helper, and convert
drm_atomic_get_plane_state() to use it.

Signed-off-by: Maxime Ripard <mrip...@kernel.org>

Reviewed-by: Thomas Zimmermann <tzimmerm...@suse.de>

But there's a question below.

---
  drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++------
  1 file changed, 23 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 153d959bd4ff..f62323679f44 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -624,10 +624,27 @@ static int drm_atomic_connector_check(struct 
drm_connector *connector,
        }
  
  	return 0;
  }
  
+static int drm_atomic_commit_set_plane_state(struct drm_atomic_commit *commit,
+                                            struct drm_plane *plane,
+                                            struct drm_plane_state 
*plane_state)
+{
+       int index = drm_plane_index(plane);
+
+       drm_modeset_lock_assert_held(&plane->mutex);
+
+       commit->planes[index].state_to_destroy = plane_state;
+       commit->planes[index].old_state = plane->state;
+       commit->planes[index].new_state = plane_state;
+       commit->planes[index].ptr = plane;
+       plane_state->state = commit;
+
+       return 0;

I've notices that in other commits as well: why does it have a return 
value when the function cannot fail?  If there isn't a need later on, it 
should preferably by removed.

Best regards
Thomas

+}
+
  /**
   * drm_atomic_get_plane_state - get plane state
   * @state: global atomic state object
   * @plane: plane to get state object for
   *
@@ -642,11 +659,11 @@ static int drm_atomic_connector_check(struct 
drm_connector *connector,
   */
  struct drm_plane_state *
  drm_atomic_get_plane_state(struct drm_atomic_commit *state,
                          struct drm_plane *plane)
  {
-       int ret, index = drm_plane_index(plane);
+       int ret;
        struct drm_plane_state *plane_state;
  
  	WARN_ON(!state->acquire_ctx);
        drm_WARN_ON(state->dev, state->checked);
  
@@ -665,15 +682,15 @@ drm_atomic_get_plane_state(struct drm_atomic_commit *state,
  
  	plane_state = plane->funcs->atomic_duplicate_state(plane);
        if (!plane_state)
                return ERR_PTR(-ENOMEM);
  
-	state->planes[index].state_to_destroy = plane_state;
-       state->planes[index].ptr = plane;
-       state->planes[index].old_state = plane->state;
-       state->planes[index].new_state = plane_state;
-       plane_state->state = state;
+       ret = drm_atomic_commit_set_plane_state(state, plane, plane_state);
+       if (ret) {
+               plane->funcs->atomic_destroy_state(plane, plane_state);
+               return ERR_PTR(ret);
+       }
  
  	drm_dbg_atomic(plane->dev, "Added [PLANE:%d:%s] %p state to %p\n",
                       plane->base.id, plane->name, plane_state, state);
  
  	if (plane_state->crtc) {


--
--
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)