On Thu, Aug 25, 2016 at 05:53:51PM +0530, Archit Taneja wrote:
>
> 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>
I intentionally don't reformat/touch code when moving it, at all. Ok if I
just fix the typo in the commit message?
-Daniel
>
> 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
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
