[PATCH v2 11/11] drm/todo: Remove the drm_atomic_state todo item

[PATCH v2 11/11] drm/todo: Remove the drm_atomic_state todo item

[Maxime Ripard]

Only planes' prepare_fb and cleanup_fb, and encoders' atomic_check and
atomic_mode_set hooks remain with an object state and not the global
drm_atomic_state.

prepare_fb and cleanup_fb operate by design on a given state and
depending on the calling site can operate on either the old or new
state, so it doesn't really make much sense to convert them.

The encoders' atomic_check and atomic_mode_set operate on the CRTC and
connector state connected to them since encoders don't have a state of
their own. Without those state pointers, we would need to get the CRTC
through the drm_connector_state crtc pointer.

However, in order to get the drm_connector_state pointer, we would need
to get the connector itself and while usually we have a single connector
connected to the encoder, we can't really get it from the encoder at
the moment since it could be behind any number of bridges.

While this could be addressed by (for example) listing all the
connectors and finding the one that has the encoder as its source, it
feels like an unnecessary rework for something that is slowly getting
replaced by bridges.

Since all the users that matter have been converted, let's remove the
TODO item.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---

Changes from v1:
  - New patch
---
 Documentation/gpu/todo.rst | 46 --------------------------------------
 1 file changed, 46 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 009d8e6c7e3c..609794108f5a 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -440,52 +440,6 @@ Contact: Emil Velikov, respective driver maintainers
 
 Level: Intermediate
 
-Plumb drm_atomic_state all over
--------------------------------
-
-Currently various atomic functions take just a single or a handful of
-object states (eg. plane state). While that single object state can
-suffice for some simple cases, we often have to dig out additional
-object states for dealing with various dependencies between the individual
-objects or the hardware they represent. The process of digging out the
-additional states is rather non-intuitive and error prone.
-
-To fix that most functions should rather take the overall
-drm_atomic_state as one of their parameters. The other parameters
-would generally be the object(s) we mainly want to interact with.
-
-For example, instead of
-
-.. code-block:: c
-
-   int (*atomic_check)(struct drm_plane *plane, struct drm_plane_state *state);
-
-we would have something like
-
-.. code-block:: c
-
-   int (*atomic_check)(struct drm_plane *plane, struct drm_atomic_state *state);
-
-The implementation can then trivially gain access to any required object
-state(s) via drm_atomic_get_plane_state(), drm_atomic_get_new_plane_state(),
-drm_atomic_get_old_plane_state(), and their equivalents for
-other object types.
-
-Additionally many drivers currently access the object->state pointer
-directly in their commit functions. That is not going to work if we
-eg. want to allow deeper commit pipelines as those pointers could
-then point to the states corresponding to a future commit instead of
-the current commit we're trying to process. Also non-blocking commits
-execute locklessly so there are serious concerns with dereferencing
-the object->state pointers without holding the locks that protect them.
-Use of drm_atomic_get_new_plane_state(), drm_atomic_get_old_plane_state(),
-etc. avoids these problems as well since they relate to a specific
-commit via the passed in drm_atomic_state.
-
-Contact: Ville Syrjälä, Daniel Vetter
-
-Level: Intermediate
-
 Use struct dma_buf_map throughout codebase
 ------------------------------------------
 
-- 
2.29.2

Re: [PATCH v2 11/11] drm/todo: Remove the drm_atomic_state todo item

[Daniel Vetter]

On Thu, Jan 21, 2021 at 5:36 PM Maxime Ripard <maxime@cerno.tech> wrote:
>
> Only planes' prepare_fb and cleanup_fb, and encoders' atomic_check and
> atomic_mode_set hooks remain with an object state and not the global
> drm_atomic_state.
>
> prepare_fb and cleanup_fb operate by design on a given state and
> depending on the calling site can operate on either the old or new
> state, so it doesn't really make much sense to convert them.
>
> The encoders' atomic_check and atomic_mode_set operate on the CRTC and
> connector state connected to them since encoders don't have a state of
> their own. Without those state pointers, we would need to get the CRTC
> through the drm_connector_state crtc pointer.
>
> However, in order to get the drm_connector_state pointer, we would need
> to get the connector itself and while usually we have a single connector
> connected to the encoder, we can't really get it from the encoder at
> the moment since it could be behind any number of bridges.
>
> While this could be addressed by (for example) listing all the
> connectors and finding the one that has the encoder as its source, it
> feels like an unnecessary rework for something that is slowly getting
> replaced by bridges.
>
> Since all the users that matter have been converted, let's remove the
> TODO item.
>
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>
> ---
>
> Changes from v1:
>   - New patch
> ---
>  Documentation/gpu/todo.rst | 46 --------------------------------------
>  1 file changed, 46 deletions(-)
>
> diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
> index 009d8e6c7e3c..609794108f5a 100644
> --- a/Documentation/gpu/todo.rst
> +++ b/Documentation/gpu/todo.rst
> @@ -440,52 +440,6 @@ Contact: Emil Velikov, respective driver maintainers
>
>  Level: Intermediate
>
> -Plumb drm_atomic_state all over
> --------------------------------
> -
> -Currently various atomic functions take just a single or a handful of
> -object states (eg. plane state). While that single object state can
> -suffice for some simple cases, we often have to dig out additional
> -object states for dealing with various dependencies between the individual
> -objects or the hardware they represent. The process of digging out the
> -additional states is rather non-intuitive and error prone.
> -
> -To fix that most functions should rather take the overall
> -drm_atomic_state as one of their parameters. The other parameters
> -would generally be the object(s) we mainly want to interact with.
> -
> -For example, instead of
> -
> -.. code-block:: c
> -
> -   int (*atomic_check)(struct drm_plane *plane, struct drm_plane_state *state);
> -
> -we would have something like
> -
> -.. code-block:: c
> -
> -   int (*atomic_check)(struct drm_plane *plane, struct drm_atomic_state *state);
> -
> -The implementation can then trivially gain access to any required object
> -state(s) via drm_atomic_get_plane_state(), drm_atomic_get_new_plane_state(),
> -drm_atomic_get_old_plane_state(), and their equivalents for
> -other object types.
> -
> -Additionally many drivers currently access the object->state pointer
> -directly in their commit functions. That is not going to work if we
> -eg. want to allow deeper commit pipelines as those pointers could
> -then point to the states corresponding to a future commit instead of
> -the current commit we're trying to process. Also non-blocking commits
> -execute locklessly so there are serious concerns with dereferencing
> -the object->state pointers without holding the locks that protect them.
> -Use of drm_atomic_get_new_plane_state(), drm_atomic_get_old_plane_state(),
> -etc. avoids these problems as well since they relate to a specific
> -commit via the passed in drm_atomic_state.
> -
> -Contact: Ville Syrjälä, Daniel Vetter
> -
> -Level: Intermediate
> -
>  Use struct dma_buf_map throughout codebase
>  ------------------------------------------
>
> --
> 2.29.2
>


-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch