On Thu, Mar 02, 2017 at 04:34:18PM +0200, Laurent Pinchart wrote:
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.
Yeah that's an accident from a previous patch, I've removed it.
+Overview +========
+.. kernel-render:: DOT
- :alt: KMS Display Pipeline
- :caption: KMS Display Pipeline Overview
- digraph "KMS" {
node [shape=box]subgraph cluster_static {style=dashedlabel="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=dashedlabel="Userspace-Created"node [shape=oval]"drm_framebuffer 1" -> "drm_plane A""drm_framebuffer 2" -> "drm_plane B"}subgraph cluster_connector {style=dashedlabel="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-con...)
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 ?
For each active connector theres exactly one active encoder. The possible linking is n:m. I've clarified this.
All other suggestions applied, thanks. -Daniel