| Message ID | 20170228171319.1786-4-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Vetter <daniel.vetter@ffwll.ch> writes: > Oh, the shiny and pretties! > Very nice! Reviewed-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk>
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 > =================================
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
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 +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 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. + +For the output routing the first step are Encoders (represented by +: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 userspace. Besides that +they make it unecessarily more complicated for userspace to figure out which +connections between a CRTC and a connector, 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. A CRTC can be connected to multiple +Encoders, but for an 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. 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. + +Internally the output pipeline is a bit more complex and matches todays 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 userspace. + +The second objects are panels, represented by :c:type:`struct drm_panel +<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>`. + +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 =================================
Oh, the shiny and pretties! Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- Documentation/gpu/drm-kms-helpers.rst | 4 ++ Documentation/gpu/drm-kms.rst | 132 ++++++++++++++++++++++++++++++++++ 2 files changed, 136 insertions(+)