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 at 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