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

From: Eric Anholt <eric@anholt.net>
To: linux-doc@vger.kernel.org
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 6/6] drm/doc: atomic overview, with graph
Date: Tue, 28 Feb 2017 15:48:47 -0800	[thread overview]
Message-ID: <87d1e13mhs.fsf@eliezer.anholt.net> (raw)
In-Reply-To: <20170228171319.1786-7-daniel.vetter@ffwll.ch>

[-- Attachment #1.1: Type: text/plain, Size: 5855 bytes --]

Daniel Vetter <daniel.vetter@ffwll.ch> writes:

> 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.
>
> One thing I failed at is getting DOT to layout the overview graph how
> I want it. The highlevel structure I want is:
>
> 	Free-standing State
>
> 	Current State
>
> i.e. one over the other. Currently it lays it out side-by-side, but
> not even that really - "Current State" is somewhat offset below. Makes
> the graph look like garbage, and also way too wide for proper
> rendering. Ideas appreciated.
>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: Harry Wentland <harry.wentland@amd.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Thanks for writing these docs.  I wish I had them back when I was
starting vc4's atomic code!  With the two little spelling nits fixed,
3-5 are:

Acked-by: Eric Anholt <eric@anholt.net>

A few copyedits on this one below, but it sounds like you don't want to
push quite yet while you sort out the rendering.

> ---
>  Documentation/gpu/drm-kms-helpers.rst |  2 +
>  Documentation/gpu/drm-kms.rst         | 85 ++++++++++++++++++++++++++++++++++-
>  2 files changed, 86 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 20378881445f..979cee853bb1 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -189,8 +189,91 @@ 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
> +===================
> +
> +
> +.. FIXME: The I want the below graph to be laid out so that the 2 subgraph
> +   clusters are below each another. But I failed.
> +
> +.. 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"]
> +   }
> +
> +Essentially atomic is transactional modeset (including planes) updates, but
> +compared to the usual transactional approach of try-commit and rollback on
> +failure atomic modesetting is a bit different:

Maybe reword:

"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 allows setting and rollback of just the software state,

"allow"

> +  simplifying conversion of existing drivers. But auditing drivers for
> +  correctness of the atomic_check code because really hard with that.

s/because/becomes/?

> +Taken all together there's two consequence for the atomic design:

"consequences"

> +
> +- 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` 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 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 commit is it applied to the

"is committed"

> +  driver and modeset objects. This way rolling back an update boils down to
> +  releasing memory and unreference objects like framebuffers.

"unreferencing"

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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