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 <architt@codeaurora.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Link: http://patchwork.freedesktop.org/patch/msgid/20160829082757.17913-2-daniel.vetter@ffwll.ch

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 <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 <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
-<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 &drm_crtc) and the connectors (as the
+ * generic sink entity, represented by struct &drm_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 &drm_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
  * @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. Encoder should be subclassed as part of
+ * driver encoder objects. At driver unload time drm_encoder_cleanup() should be
+ * called from the driver's destroy hook in &drm_encoder_funcs.
  *
  * Returns:
  * Zero on success, error code on failure.
  */
 int drm_encoder_init(struct drm_device *dev,
-                      struct drm_encoder *encoder,
-                      const struct drm_encoder_funcs *funcs,
-                      int encoder_type, const char *name, ...)
+                     struct drm_encoder *encoder,
+                     const struct drm_encoder_funcs *funcs,
+                     int encoder_type, const char *name, ...)
 {
         int ret;
 
@@ -176,19 +201,6 @@ static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
         return encoder->crtc;
 }
 
-/**
- * drm_mode_getencoder - get encoder configuration
- * @dev: drm device for the ioctl
- * @data: data pointer for the ioctl
- * @file_priv: drm file for the ioctl call
- *
- * Construct a encoder configuration structure to return to the user.
- *
- * Called by the user via ioctl.
- *
- * Returns:
- * Zero on success, negative errno on failure.
- */
 int drm_mode_getencoder(struct drm_device *dev, void *data,
                         struct drm_file *file_priv)
 {
diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
index 2712fd1a686b..3d7350f1fcc1 100644
--- a/include/drm/drm_encoder.h
+++ b/include/drm/drm_encoder.h
@@ -84,9 +84,6 @@ struct drm_encoder_funcs {
  * @head: list management
  * @base: base KMS object
  * @name: human readable name, can be overwritten by the driver
- * @encoder_type: one of the DRM_MODE_ENCODER_<foo> types in drm_mode.h
- * @possible_crtcs: bitmask of potential CRTC bindings
- * @possible_clones: bitmask of potential sibling encoders for cloning
  * @crtc: currently bound CRTC
  * @bridge: bridge associated to the encoder
  * @funcs: control functions
@@ -101,6 +98,32 @@ struct drm_encoder {
 
         struct drm_mode_object base;
         char *name;
+        /**
+         * @encoder_type:
+         *
+         * One of the DRM_MODE_ENCODER_<foo> types in drm_mode.h. The following
+         * encoder types are defined thus far:
+         *
+         * - 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, or in general any panel
+         *   with a proprietary parallel connector.
+         *
+         * - DRM_MODE_ENCODER_TVDAC for TV output (Composite, S-Video,
+         *   Component, SCART).
+         *
+         * - DRM_MODE_ENCODER_VIRTUAL for virtual machine displays
+         *
+         * - DRM_MODE_ENCODER_DSI for panels connected using the DSI serial bus.
+         *
+         * - DRM_MODE_ENCODER_DPI for panels connected using the DPI parallel
+         *   bus.
+         *
+         * - DRM_MODE_ENCODER_DPMST for special fake encoders used to allow
+         *   mutliple DP MST streams to share one physical encoder.
+         */
         int encoder_type;
 
         /**
@@ -109,7 +132,34 @@ struct drm_encoder {
          */
         unsigned index;
 
+        /**
+         * @possible_crtcs: Bitmask of potential CRTC bindings, using
+         * drm_crtc_index() as the index into the bitfield. The driver must set
+         * the bits for all &drm_crtc objects this encoder can be connected to
+         * before calling drm_encoder_init().
+         *
+         * In reality almost every driver gets this wrong.
+         *
+         * Note that since CRTC objects can't be hotplugged the assigned indices
+         * are stable and hence known before registering all objects.
+         */
         uint32_t possible_crtcs;
+
+        /**
+         * @possible_clones: Bitmask of potential sibling encoders for cloning,
+         * using drm_encoder_index() as the index into the bitfield. The driver
+         * must set the bits for all &drm_encoder objects which can clone a
+         * &drm_crtc together with this encoder before calling
+         * drm_encoder_init(). Drivers should set the bit representing the
+         * encoder itself, too. Cloning bits should be set such that when two
+         * encoders can be used in a cloned configuration, they both should have
+         * each another bits set.
+         *
+         * In reality almost every driver gets this wrong.
+         *
+         * Note that since encoder objects can't be hotplugged the assigned indices
+         * are stable and hence known before registering all objects.
+         */
         uint32_t possible_clones;
 
         struct drm_crtc *crtc;
@@ -146,7 +196,7 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc);
  * @encoder: encoder to test
  * @crtc: crtc to test
  *
- * Return false if @encoder can't be driven by @crtc, true otherwise.
+ * Returns false if @encoder can't be driven by @crtc, true otherwise.
  */
 static inline bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
                                        struct drm_crtc *crtc)
@@ -154,11 +204,21 @@ static inline bool drm_encoder_crtc_ok(struct drm_encoder *encoder,
         return !!(encoder->possible_crtcs & drm_crtc_mask(crtc));
 }
 
+/**
+ * drm_encoder_find - find a &drm_encoder
+ * @dev: DRM device
+ * @id: encoder id
+ *
+ * Returns the encoder with @id, NULL if it doesn't exist. Simple wrapper around
+ * drm_mode_object_find().
+ */
 static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
-        uint32_t id)
+                                                   uint32_t id)
 {
         struct drm_mode_object *mo;
+
         mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_ENCODER);
+
         return mo ? obj_to_encoder(mo) : NULL;
 }