Hi,
On 08/18/2016 02:25 AM, Daniel Vetter wrote:
> Same treatment as before. Only hiccup is drm_crtc_mask, which
> unfortunately can't be resolved until drm_crtc.h is less of a monster.
> Untangle the header loop with a forward delcaration for that static
s/delcaration/declaration
> inline.
>
> Signed-off-by: Daniel Vetter <daniel.vetter at intel.com>
> ---
> Documentation/gpu/drm-kms.rst | 9 ++
> drivers/gpu/drm/Makefile | 3 +-
> drivers/gpu/drm/drm_crtc.c | 193 -------------------------------
> drivers/gpu/drm/drm_crtc_internal.h | 10 +-
> drivers/gpu/drm/drm_encoder.c | 220
> ++++++++++++++++++++++++++++++++++++
> include/drm/drm_crtc.h | 134 +---------------------
> include/drm/drm_encoder.h | 167 +++++++++++++++++++++++++++
> 7 files changed, 407 insertions(+), 329 deletions(-)
> create mode 100644 drivers/gpu/drm/drm_encoder.c
> create mode 100644 include/drm/drm_encoder.h
>
<snip>
> +
> +/**
> + * drm_encoder_init - Init a preallocated encoder
> + * @dev: drm device
> + * @encoder: the encoder to init
> + * @funcs: callbacks for this encoder
> + * @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.
> + *
> + * 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, ...)
Alignment with the open parentheses is needed here.
> +{
> + int ret;
> +
> + drm_modeset_lock_all(dev);
> +
> + ret = drm_mode_object_get(dev, &encoder->base, DRM_MODE_OBJECT_ENCODER);
> + if (ret)
> + goto out_unlock;
> +
> + encoder->dev = dev;
> + encoder->encoder_type = encoder_type;
> + encoder->funcs = funcs;
> + if (name) {
> + va_list ap;
> +
> + va_start(ap, name);
> + encoder->name = kvasprintf(GFP_KERNEL, name, ap);
> + va_end(ap);
> + } else {
> + encoder->name = kasprintf(GFP_KERNEL, "%s-%d",
> +
> drm_encoder_enum_list[encoder_type].name,
> + encoder->base.id);
> + }
> + if (!encoder->name) {
> + ret = -ENOMEM;
> + goto out_put;
> + }
> +
> + list_add_tail(&encoder->head, &dev->mode_config.encoder_list);
> + encoder->index = dev->mode_config.num_encoder++;
> +
> +out_put:
> + if (ret)
> + drm_mode_object_unregister(dev, &encoder->base);
> +
> +out_unlock:
> + drm_modeset_unlock_all(dev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(drm_encoder_init);
> +
> +/**
> + * drm_encoder_cleanup - cleans up an initialised encoder
> + * @encoder: encoder to cleanup
> + *
> + * Cleans up the encoder but doesn't free the object.
> + */
> +void drm_encoder_cleanup(struct drm_encoder *encoder)
> +{
> + struct drm_device *dev = encoder->dev;
> +
> + /* Note that the encoder_list is considered to be static; should we
> + * remove the drm_encoder at runtime we would have to decrement all
> + * the indices on the drm_encoder after us in the encoder_list.
> + */
> +
> + drm_modeset_lock_all(dev);
> + drm_mode_object_unregister(dev, &encoder->base);
> + kfree(encoder->name);
> + list_del(&encoder->head);
> + dev->mode_config.num_encoder--;
> + drm_modeset_unlock_all(dev);
> +
> + memset(encoder, 0, sizeof(*encoder));
> +}
> +EXPORT_SYMBOL(drm_encoder_cleanup);
> +
> +static struct drm_crtc *drm_encoder_get_crtc(struct drm_encoder *encoder)
> +{
> + struct drm_connector *connector;
> + struct drm_device *dev = encoder->dev;
> + bool uses_atomic = false;
> +
> + /* For atomic drivers only state objects are synchronously updated and
> + * protected by modeset locks, so check those first. */
> + drm_for_each_connector(connector, dev) {
> + if (!connector->state)
> + continue;
> +
> + uses_atomic = true;
> +
> + if (connector->state->best_encoder != encoder)
> + continue;
> +
> + return connector->state->crtc;
> + }
> +
> + /* Don't return stale data (e.g. pending async disable). */
> + if (uses_atomic)
> + return NULL;
> +
> + 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)
> +{
> + struct drm_mode_get_encoder *enc_resp = data;
> + struct drm_encoder *encoder;
> + struct drm_crtc *crtc;
> +
> + if (!drm_core_check_feature(dev, DRIVER_MODESET))
> + return -EINVAL;
> +
> + encoder = drm_encoder_find(dev, enc_resp->encoder_id);
> + if (!encoder)
> + return -ENOENT;
> +
> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> + crtc = drm_encoder_get_crtc(encoder);
> + if (crtc)
> + enc_resp->crtc_id = crtc->base.id;
> + else
> + enc_resp->crtc_id = 0;
> + drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> + enc_resp->encoder_type = encoder->encoder_type;
> + enc_resp->encoder_id = encoder->base.id;
> + enc_resp->possible_crtcs = encoder->possible_crtcs;
> + enc_resp->possible_clones = encoder->possible_clones;
> +
> + return 0;
> +}
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 3fa0275e509f..61d81fb3c8fc 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -40,6 +40,7 @@
> #include <drm/drm_framebuffer.h>
> #include <drm/drm_modes.h>
> #include <drm/drm_connector.h>
> +#include <drm/drm_encoder.h>
>
> struct drm_device;
> struct drm_mode_set;
> @@ -662,97 +663,6 @@ struct drm_crtc {
> };
>
> /**
> - * struct drm_encoder_funcs - encoder controls
> - *
> - * Encoders sit between CRTCs and connectors.
> - */
> -struct drm_encoder_funcs {
> - /**
> - * @reset:
> - *
> - * Reset encoder hardware and software state to off. This function isn't
> - * called by the core directly, only through drm_mode_config_reset().
> - * It's not a helper hook only for historical reasons.
> - */
> - void (*reset)(struct drm_encoder *encoder);
> -
> - /**
> - * @destroy:
> - *
> - * Clean up encoder resources. This is only called at driver unload time
> - * through drm_mode_config_cleanup() since an encoder cannot be
> - * hotplugged in DRM.
> - */
> - void (*destroy)(struct drm_encoder *encoder);
> -
> - /**
> - * @late_register:
> - *
> - * This optional hook can be used to register additional userspace
> - * interfaces attached to the encoder like debugfs interfaces.
> - * It is called late in the driver load sequence from
> drm_dev_register().
> - * Everything added from this callback should be unregistered in
> - * the early_unregister callback.
> - *
> - * Returns:
> - *
> - * 0 on success, or a negative error code on failure.
> - */
> - int (*late_register)(struct drm_encoder *encoder);
> -
> - /**
> - * @early_unregister:
> - *
> - * This optional hook should be used to unregister the additional
> - * userspace interfaces attached to the encoder from
> - * late_unregister(). It is called from drm_dev_unregister(),
> - * early in the driver unload sequence to disable userspace access
> - * before data structures are torndown.
> - */
> - void (*early_unregister)(struct drm_encoder *encoder);
> -};
> -
> -/**
> - * struct drm_encoder - central DRM encoder structure
> - * @dev: parent DRM device
> - * @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
> - * @helper_private: mid-layer private data
> - *
> - * CRTCs drive pixels to encoders, which convert them into signals
> - * appropriate for a given connector or set of connectors.
> - */
> -struct drm_encoder {
> - struct drm_device *dev;
> - struct list_head head;
> -
> - struct drm_mode_object base;
> - char *name;
> - int encoder_type;
> -
> - /**
> - * @index: Position inside the mode_config.list, can be used as an array
> - * index. It is invariant over the lifetime of the encoder.
> - */
> - unsigned index;
> -
> - uint32_t possible_crtcs;
> - uint32_t possible_clones;
> -
> - struct drm_crtc *crtc;
> - struct drm_bridge *bridge;
> - const struct drm_encoder_funcs *funcs;
> - const struct drm_encoder_helper_funcs *helper_private;
> -};
> -
> -/**
> * struct drm_plane_state - mutable plane state
> * @plane: backpointer to the plane
> * @crtc: currently bound CRTC, NULL if disabled
> @@ -2091,7 +2001,6 @@ struct drm_mode_config {
> for_each_if ((encoder_mask) & (1 << drm_encoder_index(encoder)))
>
> #define obj_to_crtc(x) container_of(x, struct drm_crtc, base)
> -#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
> #define obj_to_mode(x) container_of(x, struct drm_display_mode, base)
> #define obj_to_fb(x) container_of(x, struct drm_framebuffer, base)
> #define obj_to_property(x) container_of(x, struct drm_property, base)
> @@ -2136,37 +2045,6 @@ static inline uint32_t drm_crtc_mask(struct drm_crtc
> *crtc)
> return 1 << drm_crtc_index(crtc);
> }
>
> -extern __printf(5, 6)
> -int drm_encoder_init(struct drm_device *dev,
> - struct drm_encoder *encoder,
> - const struct drm_encoder_funcs *funcs,
> - int encoder_type, const char *name, ...);
> -
> -/**
> - * drm_encoder_index - find the index of a registered encoder
> - * @encoder: encoder to find index for
> - *
> - * Given a registered encoder, return the index of that encoder within a DRM
> - * device's list of encoders.
> - */
> -static inline unsigned int drm_encoder_index(struct drm_encoder *encoder)
> -{
> - return encoder->index;
> -}
> -
> -/**
> - * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
> - * @encoder: encoder to test
> - * @crtc: crtc to test
> - *
> - * Return 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)
> -{
> - return !!(encoder->possible_crtcs & drm_crtc_mask(crtc));
> -}
> -
> extern __printf(8, 9)
> int drm_universal_plane_init(struct drm_device *dev,
> struct drm_plane *plane,
> @@ -2202,8 +2080,6 @@ extern void drm_crtc_get_hv_timing(const struct
> drm_display_mode *mode,
> extern int drm_crtc_force_disable(struct drm_crtc *crtc);
> extern int drm_crtc_force_disable_all(struct drm_device *dev);
>
> -extern void drm_encoder_cleanup(struct drm_encoder *encoder);
> -
> extern void drm_mode_config_init(struct drm_device *dev);
> extern void drm_mode_config_reset(struct drm_device *dev);
> extern void drm_mode_config_cleanup(struct drm_device *dev);
> @@ -2315,14 +2191,6 @@ static inline struct drm_crtc *drm_crtc_find(struct
> drm_device *dev,
> return mo ? obj_to_crtc(mo) : NULL;
> }
>
> -static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> - 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;
> -}
> -
> static inline struct drm_property *drm_property_find(struct drm_device *dev,
> uint32_t id)
> {
> diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h
> new file mode 100644
> index 000000000000..2712fd1a686b
> --- /dev/null
> +++ b/include/drm/drm_encoder.h
> @@ -0,0 +1,167 @@
> +/*
> + * Copyright (c) 2016 Intel Corporation
> + *
> + * Permission to use, copy, modify, distribute, and sell this software and
> its
> + * documentation for any purpose is hereby granted without fee, provided that
> + * the above copyright notice appear in all copies and that both that
> copyright
> + * notice and this permission notice appear in supporting documentation, and
> + * that the name of the copyright holders not be used in advertising or
> + * publicity pertaining to distribution of the software without specific,
> + * written prior permission. The copyright holders make no representations
> + * about the suitability of this software for any purpose. It is provided
> "as
> + * is" without express or implied warranty.
> + *
> + * THE COPYRIGHT HOLDERS DISCLAIM ALL WARRANTIES WITH REGARD TO THIS
> SOFTWARE,
> + * INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS, IN NO
> + * EVENT SHALL THE COPYRIGHT HOLDERS BE LIABLE FOR ANY SPECIAL, INDIRECT OR
> + * CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS OF
> USE,
> + * DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
> + * TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR
> PERFORMANCE
> + * OF THIS SOFTWARE.
> + */
> +
> +#ifndef __DRM_ENCODER_H__
> +#define __DRM_ENCODER_H__
> +
> +#include <linux/list.h>
> +#include <linux/ctype.h>
> +#include <drm/drm_modeset.h>
> +
> +/**
> + * struct drm_encoder_funcs - encoder controls
> + *
> + * Encoders sit between CRTCs and connectors.
> + */
> +struct drm_encoder_funcs {
> + /**
> + * @reset:
> + *
> + * Reset encoder hardware and software state to off. This function isn't
> + * called by the core directly, only through drm_mode_config_reset().
> + * It's not a helper hook only for historical reasons.
> + */
> + void (*reset)(struct drm_encoder *encoder);
> +
> + /**
> + * @destroy:
> + *
> + * Clean up encoder resources. This is only called at driver unload time
> + * through drm_mode_config_cleanup() since an encoder cannot be
> + * hotplugged in DRM.
> + */
> + void (*destroy)(struct drm_encoder *encoder);
> +
> + /**
> + * @late_register:
> + *
> + * This optional hook can be used to register additional userspace
> + * interfaces attached to the encoder like debugfs interfaces.
> + * It is called late in the driver load sequence from
> drm_dev_register().
> + * Everything added from this callback should be unregistered in
> + * the early_unregister callback.
> + *
> + * Returns:
> + *
> + * 0 on success, or a negative error code on failure.
> + */
> + int (*late_register)(struct drm_encoder *encoder);
> +
> + /**
> + * @early_unregister:
> + *
> + * This optional hook should be used to unregister the additional
> + * userspace interfaces attached to the encoder from
> + * late_unregister(). It is called from drm_dev_unregister(),
> + * early in the driver unload sequence to disable userspace access
> + * before data structures are torndown.
> + */
> + void (*early_unregister)(struct drm_encoder *encoder);
> +};
> +
> +/**
> + * struct drm_encoder - central DRM encoder structure
> + * @dev: parent DRM device
> + * @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
> + * @helper_private: mid-layer private data
> + *
> + * CRTCs drive pixels to encoders, which convert them into signals
> + * appropriate for a given connector or set of connectors.
> + */
> +struct drm_encoder {
> + struct drm_device *dev;
> + struct list_head head;
> +
> + struct drm_mode_object base;
> + char *name;
> + int encoder_type;
> +
> + /**
> + * @index: Position inside the mode_config.list, can be used as an array
> + * index. It is invariant over the lifetime of the encoder.
> + */
> + unsigned index;
> +
> + uint32_t possible_crtcs;
> + uint32_t possible_clones;
> +
> + struct drm_crtc *crtc;
> + struct drm_bridge *bridge;
> + const struct drm_encoder_funcs *funcs;
> + const struct drm_encoder_helper_funcs *helper_private;
> +};
> +
> +#define obj_to_encoder(x) container_of(x, struct drm_encoder, base)
> +
> +__printf(5, 6)
> +int drm_encoder_init(struct drm_device *dev,
> + struct drm_encoder *encoder,
> + const struct drm_encoder_funcs *funcs,
> + int encoder_type, const char *name, ...);
> +
> +/**
> + * drm_encoder_index - find the index of a registered encoder
> + * @encoder: encoder to find index for
> + *
> + * Given a registered encoder, return the index of that encoder within a DRM
> + * device's list of encoders.
> + */
> +static inline unsigned int drm_encoder_index(struct drm_encoder *encoder)
> +{
> + return encoder->index;
> +}
> +
> +/* FIXME: We have an include file mess still, drm_crtc.h needs untangling. */
> +static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc);
> +
> +/**
> + * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
> + * @encoder: encoder to test
> + * @crtc: crtc to test
> + *
> + * Return 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)
> +{
> + return !!(encoder->possible_crtcs & drm_crtc_mask(crtc));
> +}
> +
> +static inline struct drm_encoder *drm_encoder_find(struct drm_device *dev,
> + uint32_t id)
and here.
> +{
> + struct drm_mode_object *mo;
> + mo = drm_mode_object_find(dev, id, DRM_MODE_OBJECT_ENCODER);
checkpatch --strict asks for a blank line above too. Otherwise:
Reviewed-by: Archit Taneja <architt at codeaurora.org>
Thanks,
Archit
> + return mo ? obj_to_encoder(mo) : NULL;
> +}
> +
> +void drm_encoder_cleanup(struct drm_encoder *encoder);
> +
> +#endif
>
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
