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 <architt@xxxxxxxxxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> ---
> 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 <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
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
> + * 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.
The last line was not very clear. Did you mean "bridges also have the
benefit of BEING purely an internal abstraction ..."
> + *
> + * 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.
> + *
I think, the last three lines explain the bit-setting part a little too
well. How about adding a very short description about what cloning is
and condense the bit-setting part?
Start with something like, "Cloning involves more than one encoder
connected to the same crtc and this bitmask identifies the possible
sibling encoders for this encoder, including itself..."
> + * 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;
> }
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
