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@xxxxxxxxxxxxxxxx>
> > Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
