Re: [PATCH 1/9] drm: Extract drm_encoder.[hc]

[Archit Taneja <architt@codeaurora.org>]

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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx