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

From: Daniel Vetter <daniel@ffwll.ch>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Daniel Vetter <daniel.vetter@intel.com>,
	Daniel Vetter <daniel.vetter@ffwll.ch>,
	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, 2 Mar 2017 16:06:10 +0100	[thread overview]
Message-ID: <20170302150610.h5kduc6ccwrl5yzp@phenom.ffwll.local> (raw)
In-Reply-To: <2206034.02fWzqRMY3@avalon>

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=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 ?

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel