Re: [PATCH 2/9] drm/doc: Polish kerneldoc for encoders

[Archit Taneja <architt@codeaurora.org>]

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 <daniel.vetter@intel.com>
> ---
>   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 <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..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 &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, 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" },
>   	{ 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. 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.
> @@ -176,19 +200,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..b049748b2514 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,31 @@ 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.

The line above exceeds 80 chars, should be easy to remove this warning.
> +	 *
> +	 * - 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 +131,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.
> +	 */

Same here.

Archit

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel