Re: [PATCH 3/6] drm/doc: Add KMS overview graphs

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	linux-doc@vger.kernel.org
Subject: Re: [PATCH 3/6] drm/doc: Add KMS overview graphs
Date: Thu, 02 Mar 2017 16:34:18 +0200	[thread overview]
Message-ID: <2206034.02fWzqRMY3@avalon> (raw)
In-Reply-To: <20170228171319.1786-4-daniel.vetter@ffwll.ch>

Hi Daniel,

Thank you for the patch.

On Tuesday 28 Feb 2017 18:13:16 Daniel Vetter wrote:
> Oh, the shiny and pretties!
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Overall this looks good to me, please see below for a few minor issues.

> ---
>  Documentation/gpu/drm-kms-helpers.rst |   4 ++
>  Documentation/gpu/drm-kms.rst         | 132 +++++++++++++++++++++++++++++++
>  2 files changed, 136 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms-helpers.rst
> b/Documentation/gpu/drm-kms-helpers.rst index 03040aa14fe8..012b6ff3ec3f
> 100644
> --- a/Documentation/gpu/drm-kms-helpers.rst
> +++ b/Documentation/gpu/drm-kms-helpers.rst
> @@ -114,6 +114,8 @@ Framebuffer CMA Helper Functions Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_fb_cma_helper.c
> 
>     :export:
> +.. _drm_bridges:
> +
>  Bridges
>  =======
> 
> @@ -139,6 +141,8 @@ Bridge Helper Reference
>  .. kernel-doc:: drivers/gpu/drm/drm_bridge.c
> 
>     :export:
> +.. _drm_panel_helper:
> +
>  Panel Helper Reference
>  ======================
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 4d4068855ec4..87d8162c9a34 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -17,6 +17,138 @@ be setup by initializing the following fields.
> 
>  Mode Configuration

Not part of this patch, but this line doesn't feel like it's where it shoul 
dbe.

> +Overview
> +========
> +
> +.. kernel-render:: DOT
> +   :alt: KMS Display Pipeline
> +   :caption: KMS Display Pipeline Overview
> +
> +   digraph "KMS" {
> +      node [shape=box]
> +
> +      subgraph cluster_static {
> +          style=dashed
> +          label="Static Objects"
> +
> +          node [bgcolor=grey style=filled]
> +          "drm_plane A" -> "drm_crtc"
> +          "drm_plane B" -> "drm_crtc"
> +          "drm_crtc" -> "drm_encoder A"
> +          "drm_crtc" -> "drm_encoder B"
> +      }
> +
> +      subgraph cluster_user_created {
> +          style=dashed
> +          label="Userspace-Created"
> +
> +          node [shape=oval]
> +          "drm_framebuffer 1" -> "drm_plane A"
> +          "drm_framebuffer 2" -> "drm_plane B"
> +      }
> +
> +      subgraph cluster_connector {
> +          style=dashed
> +          label="Hotpluggable"
> +
> +          "drm_encoder A" -> "drm_connector A"
> +          "drm_encoder B" -> "drm_connector B"
> +      }
> +   }
> +
> +The basic object structure KMS presents to userspace is fairly simple.
> +Framebuffers (represented by :c:type:`struct drm_framebuffer
> <drm_framebuffer>`, +see `Frame Buffer Abstraction`_) feed into planes.
> Multiple (or just one, or +even no) planes

I'd say "One or multiple (or even no) planes", but that's up to you.

> feed their pixel data into a
> CRTC (represented by +:c:type:`struct drm_crtc <drm_crtc>`, see `CRTC
> Abstraction`_) for blending. The +precise blending step is explained in
> more detail in `Plane Composition +Properties`_ and related chapter.

s/chapter/chapters/ ? Or /related chapter/the related chapter/ ?

> +
> +For the output routing the first step are Encoders (represented by

s/are/is/

> +:c:type:`struct drm_encoder <drm_encoder>`, see `Encoder Abstraction`_).
> Those +are really just internal artifacts of the helper libraries used to
> implement KMS +drivers. But unfortunately encoders have been exposed to

s/But u/U/

(http://blog.oxforddictionaries.com/2012/01/can-i-start-a-sentence-with-a-conjunction/)

I'd actually move the sentence towards the end of the paragraph and modify it 
to

"Unfortunately connectors have been exposed to userspace, so we can't remove 
them at this point."

or something similar.

> userspace. Besides that +they make it unecessarily more complicated for
> userspace to figure out which +connections between a CRTC and a connector,

I think you're missing a verb. s/which connections/which connections are 
possible/ ?

> and what kind of cloning is +supported, they serve no purpose in the
> userspace API. Futhermore the exposed +restrictions are often wrongly set
> by drivers, and in many cases not powerful +enough to express the real
> restrictions.

I'd move the "But" sentence here, and possible start a new paragraph.

> A CRTC can be connected to multiple +Encoders, but for an

s/but/and/

> active CRTC there must be at least one encoder.
> +
> +The final, and real, endpoint in the display chain is the connector
> (represented +by :c:type:`struct drm_connector <drm_connector>`, see
> `Connector +Abstraction`_). Connectors can have different possible
> encoders, but the kernel +driver does this selection.

s/but/and/
s/does this selection/selects which encoder to use for each connector/ ?

> The use case is DVI,
> which could switch between an +analog and a digital encoder. There is only
> ever one active connector for any +encoder.

Isn't it the other way around, a single encoder for any connector ?

> +Internally the output pipeline is a bit more complex and matches todays

s/todays/today's/

> hardware +more closely:
> +
> +.. kernel-render:: DOT
> +   :alt: KMS Output Pipeline
> +   :caption: KMS Output Pipeline
> +
> +   digraph "Output Pipeline" {
> +      node [shape=box]
> +
> +      subgraph {
> +          "drm_crtc" [bgcolor=grey style=filled]
> +      }
> +
> +      subgraph cluster_internal {
> +          style=dashed
> +          label="Internal Pipeline"
> +          {
> +              node [bgcolor=grey style=filled]
> +              "drm_encoder A";
> +              "drm_encoder B";
> +              "drm_encoder C";
> +          }
> +
> +          {
> +              node [bgcolor=grey style=filled]
> +              "drm_encoder B" -> "drm_bridge B"
> +              "drm_encoder C" -> "drm_bridge C1"
> +              "drm_bridge C1" -> "drm_bridge C2";
> +          }
> +      }
> +
> +      "drm_crtc" -> "drm_encoder A"
> +      "drm_crtc" -> "drm_encoder B"
> +      "drm_crtc" -> "drm_encoder C"
> +
> +
> +      subgraph cluster_output {
> +          style=dashed
> +          label="Outputs"
> +
> +          "drm_encoder A" -> "drm_connector A";
> +          "drm_bridge B" -> "drm_connector B";
> +          "drm_bridge C2" -> "drm_connector C";
> +
> +          "drm_panel"
> +      }
> +   }
> +
> +Internally two additional helper objects come into play. First, to be able
> to +share code for encoders (sometimes on the same SoC, sometimes off-chip)
> one or +more :ref:`drm_bridges` (represented by :c:type:`struct drm_bridge
> +<drm_bridge>`) can be linked to an encoder. This link is static and cannot
> be +changed, which means the cross-bar (if there is any) needs to be mapped
> between +the CRTC and any encoders. Often for drivers with bridges there's
> no code left +at the encoder level. Atomic drivers can leave out all the
> encoder callbacks to +essentially only leave a dummy routing object behind,
> which is needed for +backwards compatibility since encoders are exposed to

I would have written "backward compatibility" but after checking I'm not too 
sure anymore. Documentation/ contains the same number of occurrences of each 
(at least after this patch, so it's a win for "backward compatibility" with a 
very thin margin in current mainline :-)).

> userspace.
> +
> +The second objects are panels, represented by :c:type:`struct drm_panel

Using a plural here with "second" seems strange to me but I might be wrong.

> +<drm_panel>`, see :ref:`drm_panel_helper`. Panels do not have a fixed
> binding +point, but are generally linked to the driver private structure
> which embeds +:c:type:`struct drm_connector <drm_connector>`.

s/which/that/

> +
> +Note that currently the bridge chaining and interactions with connectors
> and +panels are still in-flux and not really fully sorted out yet.
> +
>  KMS Core Structures and Functions
>  =================================

-- 
Regards,

Laurent Pinchart

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