Re: [PATCH] drm/doc: atomic overview, with graph

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: linux-doc@vger.kernel.org,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>,
	Gabriel Krisman Bertazi <krisman@collabora.co.uk>
Subject: Re: [PATCH] drm/doc: atomic overview, with graph
Date: Thu, 02 Mar 2017 16:41:07 +0200	[thread overview]
Message-ID: <1800747.Kjdbf7jb3H@avalon> (raw)
In-Reply-To: <20170302071958.25203-1-daniel.vetter@ffwll.ch>

Hi Daniel,

Thank you for the patch.

On Thursday 02 Mar 2017 08:19:58 Daniel Vetter wrote:
> I want to split up a few more things and document some details better
> (like how exactly to subclass drm_atomic_state). And maybe also split
> up the helpers a bit per-topic, but this should be a ok-ish start for
> better atomic overview.
> 
> v2: Spelling and clarifications (Eric).
> 
> v3: Implement suggestion from Gabriel to fix the graph.
> 
> Cc: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
> Acked-by: Eric Anholt <eric@anholt.net>
> Cc: Eric Anholt <eric@anholt.net>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  Documentation/gpu/drm-kms-helpers.rst |  2 +
>  Documentation/gpu/drm-kms.rst         | 84 +++++++++++++++++++++++++++++++-
>  2 files changed, 85 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst index 050ebe81d256..ac53c0b893f6
> 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -42,6 +42,8 @@ Modeset Helper Reference for Common Vtables
>  .. kernel-doc:: include/drm/drm_modeset_helper_vtables.h
> 
>     :internal:
> +.. _drm_atomic_helper:
> +
>  Atomic Modeset Helper Functions Reference
>  =========================================
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index a504d9ee4d94..eb9d29865c41 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -189,8 +189,90 @@ multiple times to different objects using
> :c:func:`drm_object_attach_property() .. kernel-doc::
> drivers/gpu/drm/drm_mode_object.c
> 
>     :export:
> +Atomic Mode Setting
> +===================
> +
> +
> +.. kernel-render:: DOT
> +   :alt: Mode Objects and Properties
> +   :caption: Mode Objects and Properties
> +
> +   digraph {
> +      node [shape=box]
> +
> +      subgraph cluster_state {
> +          style=dashed
> +          label="Free-standing state"
> +
> +          "drm_atomic_state" -> "duplicated drm_plane_state A"
> +          "drm_atomic_state" -> "duplicated drm_plane_state B"
> +          "drm_atomic_state" -> "duplicated drm_crtc_state"
> +          "drm_atomic_state" -> "duplicated drm_connector_state"
> +          "drm_atomic_state" -> "duplicated driver private state"
> +      }
> +
> +      subgraph cluster_current {
> +          style=dashed
> +          label="Current state"
> +
> +          "drm_device" -> "drm_plane A"
> +          "drm_device" -> "drm_plane B"
> +          "drm_device" -> "drm_crtc"
> +          "drm_device" -> "drm_connector"
> +          "drm_device" -> "driver private object"
> +
> +          "drm_plane A" -> "drm_plane_state A"
> +          "drm_plane B" -> "drm_plane_state B"
> +          "drm_crtc" -> "drm_crtc_state"
> +          "drm_connector" -> "drm_connector_state"
> +          "driver private object" -> "driver private state"
> +      }
> +
> +      "drm_atomic_state" -> "drm_device" [label="atomic_commit"]
> +      "duplicated drm_plane_state A" -> "drm_device"[style=invis]
> +   }
> +
> +Atomic provides transactional modeset (including planes) updates, but a
> +bit differently from the usual transactional approach of try-commit and
> +rollback:
> +
> +- Firstly, no hardware changes are allowed when the commit would fail. This
> +  allows us to implement the DRM_MODE_ATOMIC_TEST_ONLY mode, which allows
> +  userspace to explore whether certain configurations would work or not. +
> +- This would still allow setting and rollback of just the software state,
> +  simplifying conversion of existing drivers. But auditing drivers for
> +  correctness of the atomic_check code becomes really hard with that:
> Rolling
> +  back changes in data structures all over the place is hard to get right.
> +
> +- Lastly, for backwards compatibility and to support all use-cases, atomic

s/backwards/backward/ (see my comment to patch 3/6, and not that the margin is 
now slightly larger :-))

> +  updates need to be incremental and be able to execute in parallel.
> Hardware
> +  doesn't always allow it, but where possible plane updates on
> different CRTCs
> +  should not interfere, and not get stalled due to output routing changing
> on
> +  different CRTCs.
> +
> +Taken all together there's two consequences for the atomic design:
> +
> +- The overall state is split up into per-object state structures:
> +  :c:type:`struct drm_plane_state <drm_plane_state>` for planes,
> :c:type:`struct +  drm_crtc_state <drm_crtc_state>` for CRTCs and
> :c:type:`struct
> +  drm_connector_state <drm_connector_state`

Missing >

> for connectors. These are the
> only +  objects with userspace-visible and settable state. For internal
> state drivers +  can subclass these structures through embeddeding, or add
> entirely new state +  structures for their globally shared hardware
> functions.
> +
> +- An atomic update is assembled and validated as an enterily free-standing

s/enterily/entirely/

With this fixed,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

(this applies to patch 3/6 too, I forgot to mention it there)

> pile +  of structures within the :c:type:`drm_atomic_state
> <drm_atomic_state>` +  container. Again drivers can subclass that container
> for their own state +  structure tracking needs. Only when a state is
> committed is it applied to the +  driver and modeset objects. This way
> rolling back an update boils down to +  releasing memory and unreferencing
> objects like framebuffers.
> +
> +Read on in this chapter, and also in :ref:`drm_atomic_helper` for more
> detailed +coverage of specific topics.
> +
>  Atomic Mode Setting Function Reference
> -======================================
> +--------------------------------------
> 
>  .. kernel-doc:: include/drm/drm_atomic.h
> 
>     :internal:

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel