[2/9] drm/doc: Polish kerneldoc for encoders
I guess it's too late for review now. But, I want to send this anyway. On Mon, 2016-08-29 at 10:27 +0200, Daniel Vetter wrote: > - Move missing bits into struct drm_encoder docs. > - Explain that encoders are 95% internal and only 5% uapi, and that in > general the uapi part is broken. > - Remove verbose comments for functions not exposed to drivers. > > v2: Review from Archit: > - Appease checkpatch in the moved code. > - Make it clearer that bridges are not exposed to userspace. > > Reviewed-by: Archit Taneja > Signed-off-by: Daniel Vetter > --- > Documentation/gpu/drm-kms.rst | 46 > drivers/gpu/drm/drm_encoder.c | 48 ++--- > include/drm/drm_encoder.h | 70 > +++ > 3 files changed, 101 insertions(+), 63 deletions(-) > > diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst > index 7f788caebea3..47c2835b7c2d 100644 > --- a/Documentation/gpu/drm-kms.rst > +++ b/Documentation/gpu/drm-kms.rst > @@ -128,6 +128,12 @@ Connector Functions Reference > Encoder Abstraction > === > > +.. kernel-doc:: drivers/gpu/drm/drm_encoder.c > + :doc: overview > + > +Encoder Functions Reference > +--- > + > .. kernel-doc:: include/drm/drm_encoder.h > :internal: > > @@ -207,46 +213,6 @@ future); drivers that do not wish to provide special > handling for > primary planes may make use of the helper functions described in ? to > create and register a primary plane with standard capabilities. > > -Encoders (:c:type:`struct drm_encoder `) > -- > - > -An encoder takes pixel data from a CRTC and converts it to a format > -suitable for any attached connectors. On some devices, it may be > -possible to have a CRTC send data to more than one encoder. In that > -case, both encoders would receive data from the same scanout buffer, > -resulting in a "cloned" display configuration across the connectors > -attached to each encoder. > - > -Encoder Initialization > -~~ > - > -As for CRTCs, a KMS driver must create, initialize and register at least > -one :c:type:`struct drm_encoder ` instance. The > -instance is allocated and zeroed by the driver, possibly as part of a > -larger structure. > - > -Drivers must initialize the :c:type:`struct drm_encoder > -` possible_crtcs and possible_clones fields before > -registering the encoder. Both fields are bitmasks of respectively the > -CRTCs that the encoder can be connected to, and sibling encoders > -candidate for cloning. > - > -After being initialized, the encoder must be registered with a call to > -:c:func:`drm_encoder_init()`. The function takes a pointer to the > -encoder functions and an encoder type. Supported types are > - > -- DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A > -- DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort > -- DRM_MODE_ENCODER_LVDS for display panels > -- DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video, > - Component, SCART) > -- DRM_MODE_ENCODER_VIRTUAL for virtual machine displays > - > -Encoders must be attached to a CRTC to be used. DRM drivers leave > -encoders unattached at initialization time. Applications (or the fbdev > -compatibility layer when implemented) are responsible for attaching the > -encoders they want to use to a CRTC. > - > Cleanup > --- > > diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c > index bce781b7bb5f..998a6743a586 100644 > --- a/drivers/gpu/drm/drm_encoder.c > +++ b/drivers/gpu/drm/drm_encoder.c > @@ -26,6 +26,30 @@ > > #include "drm_crtc_internal.h" > > +/** > + * DOC: overview > + * > + * Encoders represent the connecting element between the CRTC (as the overall > + * pixel pipeline, represented by struct _crtc) and the connectors (as > the > + * generic sink entity, represented by struct _connector). Encoders are Doesn't really say what encoders actually do apart being in between crtc and connector. This line could have been useful here - "An encoder takes pixel data from a CRTC and converts it to a format suitable for any attached connectors." > + * objects exposed to userspace, originally to allow userspace to infer > cloning > + * and connector/CRTC restrictions. Unfortunately almost all drivers get this > + * wrong, making the uabi pretty much useless. On top of that the exposed > + * restrictions are too simple for todays hardware, and the recommend way to > + * infer restrictions is by using the DRM_MODE_ATOMIC_TEST_ONLY flag for the > + * atomic IOCTL. > + * > + * Otherwise encoders aren't used in the uapi at all (any modeset request > from > + * userspace directly connects a connector with a CRTC), drivers are > therefore > + * free to use them however they wish. Modeset helper libraries make strong > use > + * of encoders to facilitate code sharing. But for more complex settings it > is
[PATCH 2/9] drm/doc: Polish kerneldoc for encoders
- Move missing bits into struct drm_encoder docs.
- Explain that encoders are 95% internal and only 5% uapi, and that in
general the uapi part is broken.
- Remove verbose comments for functions not exposed to drivers.
v2: Review from Archit:
- Appease checkpatch in the moved code.
- Make it clearer that bridges are not exposed to userspace.
Reviewed-by: Archit Taneja
Signed-off-by: Daniel Vetter
---
Documentation/gpu/drm-kms.rst | 46
drivers/gpu/drm/drm_encoder.c | 48 ++---
include/drm/drm_encoder.h | 70 +++
3 files changed, 101 insertions(+), 63 deletions(-)
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 7f788caebea3..47c2835b7c2d 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -128,6 +128,12 @@ Connector Functions Reference
Encoder Abstraction
===
+.. kernel-doc:: drivers/gpu/drm/drm_encoder.c
+ :doc: overview
+
+Encoder Functions Reference
+---
+
.. kernel-doc:: include/drm/drm_encoder.h
:internal:
@@ -207,46 +213,6 @@ future); drivers that do not wish to provide special
handling for
primary planes may make use of the helper functions described in ? to
create and register a primary plane with standard capabilities.
-Encoders (:c:type:`struct drm_encoder `)
--
-
-An encoder takes pixel data from a CRTC and converts it to a format
-suitable for any attached connectors. On some devices, it may be
-possible to have a CRTC send data to more than one encoder. In that
-case, both encoders would receive data from the same scanout buffer,
-resulting in a "cloned" display configuration across the connectors
-attached to each encoder.
-
-Encoder Initialization
-~~
-
-As for CRTCs, a KMS driver must create, initialize and register at least
-one :c:type:`struct drm_encoder ` instance. The
-instance is allocated and zeroed by the driver, possibly as part of a
-larger structure.
-
-Drivers must initialize the :c:type:`struct drm_encoder
-` possible_crtcs and possible_clones fields before
-registering the encoder. Both fields are bitmasks of respectively the
-CRTCs that the encoder can be connected to, and sibling encoders
-candidate for cloning.
-
-After being initialized, the encoder must be registered with a call to
-:c:func:`drm_encoder_init()`. The function takes a pointer to the
-encoder functions and an encoder type. Supported types are
-
-- DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A
-- DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort
-- DRM_MODE_ENCODER_LVDS for display panels
-- DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video,
- Component, SCART)
-- DRM_MODE_ENCODER_VIRTUAL for virtual machine displays
-
-Encoders must be attached to a CRTC to be used. DRM drivers leave
-encoders unattached at initialization time. Applications (or the fbdev
-compatibility layer when implemented) are responsible for attaching the
-encoders they want to use to a CRTC.
-
Cleanup
---
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index bce781b7bb5f..998a6743a586 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -26,6 +26,30 @@
#include "drm_crtc_internal.h"
+/**
+ * DOC: overview
+ *
+ * Encoders represent the connecting element between the CRTC (as the overall
+ * pixel pipeline, represented by struct _crtc) and the connectors (as the
+ * generic sink entity, represented by struct _connector). Encoders are
+ * objects exposed to userspace, originally to allow userspace to infer cloning
+ * and connector/CRTC restrictions. Unfortunately almost all drivers get this
+ * wrong, making the uabi pretty much useless. On top of that the exposed
+ * restrictions are too simple for todays hardware, and the recommend way to
+ * infer restrictions is by using the DRM_MODE_ATOMIC_TEST_ONLY flag for the
+ * atomic IOCTL.
+ *
+ * Otherwise encoders aren't used in the uapi at all (any modeset request from
+ * userspace directly connects a connector with a CRTC), drivers are therefore
+ * free to use them however they wish. Modeset helper libraries make strong use
+ * of encoders to facilitate code sharing. But for more complex settings it is
+ * usually better to move shared code into a separate _bridge. Compared to
+ * encoders bridges also have the benefit of not being purely an internal
+ * abstraction since they are not exposed to userspace at all.
+ *
+ * Encoders are initialized with drm_encoder_init() and cleaned up using
+ * drm_encoder_cleanup().
+ */
static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
{ DRM_MODE_ENCODER_NONE, "None" },
{ DRM_MODE_ENCODER_DAC, "DAC" },
@@ -71,16 +95,17 @@ void drm_encoder_unregister_all(struct drm_device *dev)
* @encoder_type: user visible type of the encoder
[PATCH 2/9] drm/doc: Polish kerneldoc for encoders
On 08/18/2016 02:25 AM, Daniel Vetter wrote:
> - Move missing bits into struct drm_encoder docs.
> - Explain that encoders are 95% internal and only 5% uapi, and that in
>general the uapi part is broken.
> - Remove verbose comments for functions not exposed to drivers.
>
> Signed-off-by: Daniel Vetter
> ---
> Documentation/gpu/drm-kms.rst | 46 --
> drivers/gpu/drm/drm_encoder.c | 41 +--
> include/drm/drm_encoder.h | 65
> ---
> 3 files changed, 93 insertions(+), 59 deletions(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index 7f788caebea3..47c2835b7c2d 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -128,6 +128,12 @@ Connector Functions Reference
> Encoder Abstraction
> ===
>
> +.. kernel-doc:: drivers/gpu/drm/drm_encoder.c
> + :doc: overview
> +
> +Encoder Functions Reference
> +---
> +
> .. kernel-doc:: include/drm/drm_encoder.h
> :internal:
>
> @@ -207,46 +213,6 @@ future); drivers that do not wish to provide special
> handling for
> primary planes may make use of the helper functions described in ? to
> create and register a primary plane with standard capabilities.
>
> -Encoders (:c:type:`struct drm_encoder `)
> --
> -
> -An encoder takes pixel data from a CRTC and converts it to a format
> -suitable for any attached connectors. On some devices, it may be
> -possible to have a CRTC send data to more than one encoder. In that
> -case, both encoders would receive data from the same scanout buffer,
> -resulting in a "cloned" display configuration across the connectors
> -attached to each encoder.
> -
> -Encoder Initialization
> -~~
> -
> -As for CRTCs, a KMS driver must create, initialize and register at least
> -one :c:type:`struct drm_encoder ` instance. The
> -instance is allocated and zeroed by the driver, possibly as part of a
> -larger structure.
> -
> -Drivers must initialize the :c:type:`struct drm_encoder
> -` possible_crtcs and possible_clones fields before
> -registering the encoder. Both fields are bitmasks of respectively the
> -CRTCs that the encoder can be connected to, and sibling encoders
> -candidate for cloning.
> -
> -After being initialized, the encoder must be registered with a call to
> -:c:func:`drm_encoder_init()`. The function takes a pointer to the
> -encoder functions and an encoder type. Supported types are
> -
> -- DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A
> -- DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort
> -- DRM_MODE_ENCODER_LVDS for display panels
> -- DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video,
> - Component, SCART)
> -- DRM_MODE_ENCODER_VIRTUAL for virtual machine displays
> -
> -Encoders must be attached to a CRTC to be used. DRM drivers leave
> -encoders unattached at initialization time. Applications (or the fbdev
> -compatibility layer when implemented) are responsible for attaching the
> -encoders they want to use to a CRTC.
> -
> Cleanup
> ---
>
> diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
> index bce781b7bb5f..977d8cad9321 100644
> --- a/drivers/gpu/drm/drm_encoder.c
> +++ b/drivers/gpu/drm/drm_encoder.c
> @@ -26,6 +26,29 @@
>
> #include "drm_crtc_internal.h"
>
> +/**
> + * DOC: overview
> + *
> + * Encoders represent the connecting element between the CRTC (as the overall
> + * pixel pipeline, represented by struct _crtc) and the connectors (as
> the
> + * generic sink entity, represented by struct _connector). Encoders are
> + * objects exposed to userspace, originally to allow userspace to infer
> cloning
> + * and connector/CRTC restrictions. Unfortunately almost all drivers get this
> + * wrong, making the uabi pretty much useless. On top of that the exposed
> + * restrictions are too simple for todays hardware, and the recommend way to
> + * infer restrictions is by using the DRM_MODE_ATOMIC_TEST_ONLY flag for the
> + * atomic IOCTL.
> + *
> + * Otherwise encoders aren't used in the uapi at all (any modeset request
> from
> + * userspace directly connects a connector with a CRTC), drivers are
> therefore
> + * free to use them however they wish. Modeset helper libraries make strong
> use
> + * of encoders to facilitate code sharing. But for more complex settings it
> is
> + * usually better to move shared code into a separate _bridge, which also
> + * doesn't have any issues with being exposed to userspace.
I guess the last line could say that the drm_bridge isn't exposed to
userspace at all.
> + *
> + * Encoders are initialized with drm_encoder_init() and cleaned up using
> + * drm_encoder_cleanup().
> + */
> static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
> { DRM_MODE_ENCODER_NONE, "None" },
> {
[PATCH 2/9] drm/doc: Polish kerneldoc for encoders
- Move missing bits into struct drm_encoder docs.
- Explain that encoders are 95% internal and only 5% uapi, and that in
general the uapi part is broken.
- Remove verbose comments for functions not exposed to drivers.
Signed-off-by: Daniel Vetter
---
Documentation/gpu/drm-kms.rst | 46 --
drivers/gpu/drm/drm_encoder.c | 41 +--
include/drm/drm_encoder.h | 65 ---
3 files changed, 93 insertions(+), 59 deletions(-)
diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
index 7f788caebea3..47c2835b7c2d 100644
--- a/Documentation/gpu/drm-kms.rst
+++ b/Documentation/gpu/drm-kms.rst
@@ -128,6 +128,12 @@ Connector Functions Reference
Encoder Abstraction
===
+.. kernel-doc:: drivers/gpu/drm/drm_encoder.c
+ :doc: overview
+
+Encoder Functions Reference
+---
+
.. kernel-doc:: include/drm/drm_encoder.h
:internal:
@@ -207,46 +213,6 @@ future); drivers that do not wish to provide special
handling for
primary planes may make use of the helper functions described in ? to
create and register a primary plane with standard capabilities.
-Encoders (:c:type:`struct drm_encoder `)
--
-
-An encoder takes pixel data from a CRTC and converts it to a format
-suitable for any attached connectors. On some devices, it may be
-possible to have a CRTC send data to more than one encoder. In that
-case, both encoders would receive data from the same scanout buffer,
-resulting in a "cloned" display configuration across the connectors
-attached to each encoder.
-
-Encoder Initialization
-~~
-
-As for CRTCs, a KMS driver must create, initialize and register at least
-one :c:type:`struct drm_encoder ` instance. The
-instance is allocated and zeroed by the driver, possibly as part of a
-larger structure.
-
-Drivers must initialize the :c:type:`struct drm_encoder
-` possible_crtcs and possible_clones fields before
-registering the encoder. Both fields are bitmasks of respectively the
-CRTCs that the encoder can be connected to, and sibling encoders
-candidate for cloning.
-
-After being initialized, the encoder must be registered with a call to
-:c:func:`drm_encoder_init()`. The function takes a pointer to the
-encoder functions and an encoder type. Supported types are
-
-- DRM_MODE_ENCODER_DAC for VGA and analog on DVI-I/DVI-A
-- DRM_MODE_ENCODER_TMDS for DVI, HDMI and (embedded) DisplayPort
-- DRM_MODE_ENCODER_LVDS for display panels
-- DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video,
- Component, SCART)
-- DRM_MODE_ENCODER_VIRTUAL for virtual machine displays
-
-Encoders must be attached to a CRTC to be used. DRM drivers leave
-encoders unattached at initialization time. Applications (or the fbdev
-compatibility layer when implemented) are responsible for attaching the
-encoders they want to use to a CRTC.
-
Cleanup
---
diff --git a/drivers/gpu/drm/drm_encoder.c b/drivers/gpu/drm/drm_encoder.c
index bce781b7bb5f..977d8cad9321 100644
--- a/drivers/gpu/drm/drm_encoder.c
+++ b/drivers/gpu/drm/drm_encoder.c
@@ -26,6 +26,29 @@
#include "drm_crtc_internal.h"
+/**
+ * DOC: overview
+ *
+ * Encoders represent the connecting element between the CRTC (as the overall
+ * pixel pipeline, represented by struct _crtc) and the connectors (as the
+ * generic sink entity, represented by struct _connector). Encoders are
+ * objects exposed to userspace, originally to allow userspace to infer cloning
+ * and connector/CRTC restrictions. Unfortunately almost all drivers get this
+ * wrong, making the uabi pretty much useless. On top of that the exposed
+ * restrictions are too simple for todays hardware, and the recommend way to
+ * infer restrictions is by using the DRM_MODE_ATOMIC_TEST_ONLY flag for the
+ * atomic IOCTL.
+ *
+ * Otherwise encoders aren't used in the uapi at all (any modeset request from
+ * userspace directly connects a connector with a CRTC), drivers are therefore
+ * free to use them however they wish. Modeset helper libraries make strong use
+ * of encoders to facilitate code sharing. But for more complex settings it is
+ * usually better to move shared code into a separate _bridge, which also
+ * doesn't have any issues with being exposed to userspace.
+ *
+ * Encoders are initialized with drm_encoder_init() and cleaned up using
+ * drm_encoder_cleanup().
+ */
static const struct drm_prop_enum_list drm_encoder_enum_list[] = {
{ DRM_MODE_ENCODER_NONE, "None" },
{ DRM_MODE_ENCODER_DAC, "DAC" },
@@ -71,8 +94,9 @@ void drm_encoder_unregister_all(struct drm_device *dev)
* @encoder_type: user visible type of the encoder
* @name: printf style format string for the encoder name, or NULL for default
name
*
- * Initialises a preallocated encoder. Encoder should be
- * subclassed as part of driver encoder objects.
+ * Initialises a preallocated encoder.
